qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlin


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2
Date: Mon, 14 Dec 2015 10:42:41 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Laszlo Ersek <address@hidden> 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 <address@hidden>
>> Cc: Fam Zheng <address@hidden>
>> Cc: Laszlo Ersek <address@hidden>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks!



reply via email to

[Prev in Thread] Current Thread [Next in Thread]