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

[Fam Zheng]

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!

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?