On Mon, 12/14 10:42, Markus Armbruster wrote:
> 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>
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?
