Laszlo Ersek <ler...@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 <jc...@redhat.com>
>> Cc: Fam Zheng <f...@redhat.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Signed-off-by: Markus Armbruster <arm...@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 <ler...@redhat.com>
Thanks!
