Fam Zheng <f...@redhat.com> writes:
> 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!
Okay, I'll try to do this for v2.
[...]
