Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> On Mon, 12/14 10:42, Markus Armbruster wrote:
>> 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>
>
> Yes, it's better in every way!

Okay, I'll try to do this for v2.

[...]