| Message ID | 1449768232-22924-4-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/10/15 18:23, Markus Armbruster wrote: > The arguments of error_setg() & friends should yield a short error > string without newlines. > > A few places try to append additional help to the error message by > embedding newlines in the error string. That's nice, but let's do it > the right way, with error_append_hint(). Offenders tracked down with > the Coccinelle semantic patch from commit 312fd5f. > > Cc: Jeff Cody <jcody@redhat.com> > Cc: Fam Zheng <famz@redhat.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/vhdx-log.c | 9 +++++---- > block/vmdk.c | 9 ++++++--- > hw/i386/kvm/pci-assign.c | 12 ++++++------ > 3 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/block/vhdx-log.c b/block/vhdx-log.c > index 47ae4b1..2ac8693 100644 > --- a/block/vhdx-log.c > +++ b/block/vhdx-log.c > @@ -786,10 +786,11 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed, > ret = -EPERM; > error_setg_errno(errp, EPERM, > "VHDX image file '%s' opened read-only, but " > - "contains a log that needs to be replayed. To " > - "replay the log, execute:\n qemu-img check -r " > - "all '%s'", > - bs->filename, bs->filename); > + "contains a log that needs to be replayed", > + bs->filename); > + error_append_hint(errp, "To replay the log, run:\n" > + "qemu-img check -r all '%s'\n", > + bs->filename); This doesn't seem right. In error_report_err(), the hint is printed ("unless QMP") with an additional \n: void error_report_err(Error *err) { error_report("%s", error_get_pretty(err)); if (err->hint) { error_printf_unless_qmp("%s\n", err->hint->str); } error_free(err); } Hence we shouldn't add the final \n to the hint. > goto exit; > } > /* now flush the log */ > diff --git a/block/vmdk.c b/block/vmdk.c > index b4a224e..3a4c4ed 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -794,18 +794,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, > goto next_line; > } else if (!strcmp(type, "FLAT")) { > if (matches != 5 || flat_offset < 0) { > - error_setg(errp, "Invalid extent lines: \n%s", p); > + error_setg(errp, "Invalid extent lines"); > + error_append_hint(errp, "%s", p); Looks good. > return -EINVAL; > } > } else if (!strcmp(type, "VMFS")) { > if (matches == 4) { > flat_offset = 0; > } else { > - error_setg(errp, "Invalid extent lines:\n%s", p); > + error_setg(errp, "Invalid extent lines"); > + error_append_hint(errp, "%s", p); > return -EINVAL; > } > } else if (matches != 4) { > - error_setg(errp, "Invalid extent lines:\n%s", p); > + error_setg(errp, "Invalid extent lines"); > + error_append_hint(errp, "%s", p); > return -EINVAL; > } These appear fine as well. > > diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c > index 0fd6923..68622ff 100644 > --- a/hw/i386/kvm/pci-assign.c > +++ b/hw/i386/kvm/pci-assign.c > @@ -812,8 +812,9 @@ static void assign_device(AssignedDevice *dev, Error **errp) > char *cause; > > cause = assign_failed_examine(dev); > - error_setg_errno(errp, -r, "Failed to assign device \"%s\"\n%s", > - dev->dev.qdev.id, cause); > + error_setg_errno(errp, -r, "Failed to assign device \"%s\"", > + dev->dev.qdev.id); > + error_append_hint(errp, "%s", cause); > g_free(cause); > break; > } > @@ -912,11 +913,10 @@ retry: > dev->features |= ASSIGNED_DEVICE_PREFER_MSI_MASK; > goto retry; > } > - error_setg_errno(errp, -r, > - "Failed to assign irq for \"%s\"\n" > - "Perhaps you are assigning a device " > - "that shares an IRQ with another device?", > + error_setg_errno(errp, -r, "Failed to assign irq for \"%s\"", > dev->dev.qdev.id); > + error_append_hint(errp, "Perhaps you are assigning a device " > + "that shares an IRQ with another device?"); > return r; > } > > If you remove the extraneous \n from vhdx_parse_log(), you can add Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo
Laszlo Ersek <lersek@redhat.com> writes: > On 12/10/15 18:23, Markus Armbruster wrote: >> The arguments of error_setg() & friends should yield a short error >> string without newlines. >> >> A few places try to append additional help to the error message by >> embedding newlines in the error string. That's nice, but let's do it >> the right way, with error_append_hint(). Offenders tracked down with >> the Coccinelle semantic patch from commit 312fd5f. >> >> Cc: Jeff Cody <jcody@redhat.com> >> Cc: Fam Zheng <famz@redhat.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/vhdx-log.c | 9 +++++---- >> block/vmdk.c | 9 ++++++--- >> hw/i386/kvm/pci-assign.c | 12 ++++++------ >> 3 files changed, 17 insertions(+), 13 deletions(-) >> >> diff --git a/block/vhdx-log.c b/block/vhdx-log.c >> index 47ae4b1..2ac8693 100644 >> --- a/block/vhdx-log.c >> +++ b/block/vhdx-log.c >> @@ -786,10 +786,11 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed, >> ret = -EPERM; >> error_setg_errno(errp, EPERM, >> "VHDX image file '%s' opened read-only, but " >> - "contains a log that needs to be replayed. To " >> - "replay the log, execute:\n qemu-img check -r " >> - "all '%s'", >> - bs->filename, bs->filename); >> + "contains a log that needs to be replayed", >> + bs->filename); >> + error_append_hint(errp, "To replay the log, run:\n" >> + "qemu-img check -r all '%s'\n", >> + bs->filename); > > This doesn't seem right. In error_report_err(), the hint is printed > ("unless QMP") with an additional \n: > > void error_report_err(Error *err) > { > error_report("%s", error_get_pretty(err)); > if (err->hint) { > error_printf_unless_qmp("%s\n", err->hint->str); > } > error_free(err); > } > > Hence we shouldn't add the final \n to the hint. You're right. > >> goto exit; >> } >> /* now flush the log */ >> diff --git a/block/vmdk.c b/block/vmdk.c >> index b4a224e..3a4c4ed 100644 >> --- a/block/vmdk.c >> +++ b/block/vmdk.c >> @@ -794,18 +794,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, >> goto next_line; >> } else if (!strcmp(type, "FLAT")) { >> if (matches != 5 || flat_offset < 0) { >> - error_setg(errp, "Invalid extent lines: \n%s", p); >> + error_setg(errp, "Invalid extent lines"); >> + error_append_hint(errp, "%s", p); > > Looks good. Unless @p ends with a newline. error_report_err() would report this error as [TIMESTAMP:][LOCATION: ]Invalid extent lines <first line that doesn't parse> <remaining text that may or may not parse, if any> <newline> I figure this would make more sense: [TIMESTAMP:][LOCATION: ]Invalid extent line: <first line that doesn't parse> Regardless, error_report_err()'s contract isn't clear on whether the caller is supposed to end the hint with a newline or not. It could be made more tolerant and append a newline only when there isn't one already. What do you think? >> return -EINVAL; >> } >> } else if (!strcmp(type, "VMFS")) { >> if (matches == 4) { >> flat_offset = 0; >> } else { >> - error_setg(errp, "Invalid extent lines:\n%s", p); >> + error_setg(errp, "Invalid extent lines"); >> + error_append_hint(errp, "%s", p); >> return -EINVAL; >> } >> } else if (matches != 4) { >> - error_setg(errp, "Invalid extent lines:\n%s", p); >> + error_setg(errp, "Invalid extent lines"); >> + error_append_hint(errp, "%s", p); >> return -EINVAL; >> } > > These appear fine as well. > > >> >> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c >> index 0fd6923..68622ff 100644 >> --- a/hw/i386/kvm/pci-assign.c >> +++ b/hw/i386/kvm/pci-assign.c >> @@ -812,8 +812,9 @@ static void assign_device(AssignedDevice *dev, Error **errp) >> char *cause; >> >> cause = assign_failed_examine(dev); >> - error_setg_errno(errp, -r, "Failed to assign device \"%s\"\n%s", >> - dev->dev.qdev.id, cause); >> + error_setg_errno(errp, -r, "Failed to assign device \"%s\"", >> + dev->dev.qdev.id); >> + error_append_hint(errp, "%s", cause); >> g_free(cause); >> break; >> } >> @@ -912,11 +913,10 @@ retry: >> dev->features |= ASSIGNED_DEVICE_PREFER_MSI_MASK; >> goto retry; >> } >> - error_setg_errno(errp, -r, >> - "Failed to assign irq for \"%s\"\n" >> - "Perhaps you are assigning a device " >> - "that shares an IRQ with another device?", >> + error_setg_errno(errp, -r, "Failed to assign irq for \"%s\"", >> dev->dev.qdev.id); >> + error_append_hint(errp, "Perhaps you are assigning a device " >> + "that shares an IRQ with another device?"); >> return r; >> } >> >> > > If you remove the extraneous \n from vhdx_parse_log(), you can add > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks!
On Mon, 12/14 10:42, Markus Armbruster wrote: > Laszlo Ersek <lersek@redhat.com> writes: > > > On 12/10/15 18:23, Markus Armbruster wrote: > >> The arguments of error_setg() & friends should yield a short error > >> string without newlines. > >> > >> A few places try to append additional help to the error message by > >> embedding newlines in the error string. That's nice, but let's do it > >> the right way, with error_append_hint(). Offenders tracked down with > >> the Coccinelle semantic patch from commit 312fd5f. > >> > >> Cc: Jeff Cody <jcody@redhat.com> > >> Cc: Fam Zheng <famz@redhat.com> > >> Cc: Laszlo Ersek <lersek@redhat.com> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> --- > >> block/vhdx-log.c | 9 +++++---- > >> block/vmdk.c | 9 ++++++--- > >> hw/i386/kvm/pci-assign.c | 12 ++++++------ > >> 3 files changed, 17 insertions(+), 13 deletions(-) > >> > >> diff --git a/block/vhdx-log.c b/block/vhdx-log.c > >> index 47ae4b1..2ac8693 100644 > >> --- a/block/vhdx-log.c > >> +++ b/block/vhdx-log.c > >> @@ -786,10 +786,11 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed, > >> ret = -EPERM; > >> error_setg_errno(errp, EPERM, > >> "VHDX image file '%s' opened read-only, but " > >> - "contains a log that needs to be replayed. To " > >> - "replay the log, execute:\n qemu-img check -r " > >> - "all '%s'", > >> - bs->filename, bs->filename); > >> + "contains a log that needs to be replayed", > >> + bs->filename); > >> + error_append_hint(errp, "To replay the log, run:\n" > >> + "qemu-img check -r all '%s'\n", > >> + bs->filename); > > > > This doesn't seem right. In error_report_err(), the hint is printed > > ("unless QMP") with an additional \n: > > > > void error_report_err(Error *err) > > { > > error_report("%s", error_get_pretty(err)); > > if (err->hint) { > > error_printf_unless_qmp("%s\n", err->hint->str); > > } > > error_free(err); > > } > > > > Hence we shouldn't add the final \n to the hint. > > You're right. > > > > >> goto exit; > >> } > >> /* now flush the log */ > >> diff --git a/block/vmdk.c b/block/vmdk.c > >> index b4a224e..3a4c4ed 100644 > >> --- a/block/vmdk.c > >> +++ b/block/vmdk.c > >> @@ -794,18 +794,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, > >> goto next_line; > >> } else if (!strcmp(type, "FLAT")) { > >> if (matches != 5 || flat_offset < 0) { > >> - error_setg(errp, "Invalid extent lines: \n%s", p); > >> + error_setg(errp, "Invalid extent lines"); > >> + error_append_hint(errp, "%s", p); > > > > Looks good. > > Unless @p ends with a newline. > > error_report_err() would report this error as > > [TIMESTAMP:][LOCATION: ]Invalid extent lines > <first line that doesn't parse> > <remaining text that may or may not parse, if any> > <newline> > > I figure this would make more sense: > > [TIMESTAMP:][LOCATION: ]Invalid extent line: <first line that doesn't parse> Yes, it's better in every way! Fam > > Regardless, error_report_err()'s contract isn't clear on whether the > caller is supposed to end the hint with a newline or not. It could be > made more tolerant and append a newline only when there isn't one > already. > > What do you think?
Fam Zheng <famz@redhat.com> writes: > On Mon, 12/14 10:42, Markus Armbruster wrote: >> Laszlo Ersek <lersek@redhat.com> writes: >> >> > On 12/10/15 18:23, Markus Armbruster wrote: >> >> The arguments of error_setg() & friends should yield a short error >> >> string without newlines. >> >> >> >> A few places try to append additional help to the error message by >> >> embedding newlines in the error string. That's nice, but let's do it >> >> the right way, with error_append_hint(). Offenders tracked down with >> >> the Coccinelle semantic patch from commit 312fd5f. >> >> >> >> Cc: Jeff Cody <jcody@redhat.com> >> >> Cc: Fam Zheng <famz@redhat.com> >> >> Cc: Laszlo Ersek <lersek@redhat.com> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> --- >> >> block/vhdx-log.c | 9 +++++---- >> >> block/vmdk.c | 9 ++++++--- >> >> hw/i386/kvm/pci-assign.c | 12 ++++++------ >> >> 3 files changed, 17 insertions(+), 13 deletions(-) >> >> >> >> diff --git a/block/vhdx-log.c b/block/vhdx-log.c >> >> index 47ae4b1..2ac8693 100644 >> >> --- a/block/vhdx-log.c >> >> +++ b/block/vhdx-log.c >> >> @@ -786,10 +786,11 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed, >> >> ret = -EPERM; >> >> error_setg_errno(errp, EPERM, >> >> "VHDX image file '%s' opened read-only, but " >> >> - "contains a log that needs to be replayed. To " >> >> - "replay the log, execute:\n qemu-img check -r " >> >> - "all '%s'", >> >> - bs->filename, bs->filename); >> >> + "contains a log that needs to be replayed", >> >> + bs->filename); >> >> + error_append_hint(errp, "To replay the log, run:\n" >> >> + "qemu-img check -r all '%s'\n", >> >> + bs->filename); >> > >> > This doesn't seem right. In error_report_err(), the hint is printed >> > ("unless QMP") with an additional \n: >> > >> > void error_report_err(Error *err) >> > { >> > error_report("%s", error_get_pretty(err)); >> > if (err->hint) { >> > error_printf_unless_qmp("%s\n", err->hint->str); >> > } >> > error_free(err); >> > } >> > >> > Hence we shouldn't add the final \n to the hint. >> >> You're right. >> >> > >> >> goto exit; >> >> } >> >> /* now flush the log */ >> >> diff --git a/block/vmdk.c b/block/vmdk.c >> >> index b4a224e..3a4c4ed 100644 >> >> --- a/block/vmdk.c >> >> +++ b/block/vmdk.c >> >> @@ -794,18 +794,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, >> >> goto next_line; >> >> } else if (!strcmp(type, "FLAT")) { >> >> if (matches != 5 || flat_offset < 0) { >> >> - error_setg(errp, "Invalid extent lines: \n%s", p); >> >> + error_setg(errp, "Invalid extent lines"); >> >> + error_append_hint(errp, "%s", p); >> > >> > Looks good. >> >> Unless @p ends with a newline. >> >> error_report_err() would report this error as >> >> [TIMESTAMP:][LOCATION: ]Invalid extent lines >> <first line that doesn't parse> >> <remaining text that may or may not parse, if any> >> <newline> >> >> I figure this would make more sense: >> >> [TIMESTAMP:][LOCATION: ]Invalid extent line: <first line that >> doesn't parse> > > Yes, it's better in every way! Okay, I'll try to do this for v2. [...]
diff --git a/block/vhdx-log.c b/block/vhdx-log.c index 47ae4b1..2ac8693 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -786,10 +786,11 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed, ret = -EPERM; error_setg_errno(errp, EPERM, "VHDX image file '%s' opened read-only, but " - "contains a log that needs to be replayed. To " - "replay the log, execute:\n qemu-img check -r " - "all '%s'", - bs->filename, bs->filename); + "contains a log that needs to be replayed", + bs->filename); + error_append_hint(errp, "To replay the log, run:\n" + "qemu-img check -r all '%s'\n", + bs->filename); goto exit; } /* now flush the log */ diff --git a/block/vmdk.c b/block/vmdk.c index b4a224e..3a4c4ed 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -794,18 +794,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, goto next_line; } else if (!strcmp(type, "FLAT")) { if (matches != 5 || flat_offset < 0) { - error_setg(errp, "Invalid extent lines: \n%s", p); + error_setg(errp, "Invalid extent lines"); + error_append_hint(errp, "%s", p); return -EINVAL; } } else if (!strcmp(type, "VMFS")) { if (matches == 4) { flat_offset = 0; } else { - error_setg(errp, "Invalid extent lines:\n%s", p); + error_setg(errp, "Invalid extent lines"); + error_append_hint(errp, "%s", p); return -EINVAL; } } else if (matches != 4) { - error_setg(errp, "Invalid extent lines:\n%s", p); + error_setg(errp, "Invalid extent lines"); + error_append_hint(errp, "%s", p); return -EINVAL; } diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c index 0fd6923..68622ff 100644 --- a/hw/i386/kvm/pci-assign.c +++ b/hw/i386/kvm/pci-assign.c @@ -812,8 +812,9 @@ static void assign_device(AssignedDevice *dev, Error **errp) char *cause; cause = assign_failed_examine(dev); - error_setg_errno(errp, -r, "Failed to assign device \"%s\"\n%s", - dev->dev.qdev.id, cause); + error_setg_errno(errp, -r, "Failed to assign device \"%s\"", + dev->dev.qdev.id); + error_append_hint(errp, "%s", cause); g_free(cause); break; } @@ -912,11 +913,10 @@ retry: dev->features |= ASSIGNED_DEVICE_PREFER_MSI_MASK; goto retry; } - error_setg_errno(errp, -r, - "Failed to assign irq for \"%s\"\n" - "Perhaps you are assigning a device " - "that shares an IRQ with another device?", + error_setg_errno(errp, -r, "Failed to assign irq for \"%s\"", dev->dev.qdev.id); + error_append_hint(errp, "Perhaps you are assigning a device " + "that shares an IRQ with another device?"); return r; }
The arguments of error_setg() & friends should yield a short error string without newlines. A few places try to append additional help to the error message by embedding newlines in the error string. That's nice, but let's do it the right way, with error_append_hint(). Offenders tracked down with the Coccinelle semantic patch from commit 312fd5f. Cc: Jeff Cody <jcody@redhat.com> Cc: Fam Zheng <famz@redhat.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/vhdx-log.c | 9 +++++---- block/vmdk.c | 9 ++++++--- hw/i386/kvm/pci-assign.c | 12 ++++++------ 3 files changed, 17 insertions(+), 13 deletions(-)