From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40007) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8exJ-0003Pm-E8 for qemu-devel@nongnu.org; Mon, 14 Dec 2015 21:03:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8exG-0002Bd-6r for qemu-devel@nongnu.org; Mon, 14 Dec 2015 21:03:09 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51135) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8exF-0002BW-WF for qemu-devel@nongnu.org; Mon, 14 Dec 2015 21:03:06 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 639BDC000410 for ; Tue, 15 Dec 2015 02:03:04 +0000 (UTC) Date: Tue, 15 Dec 2015 10:03:02 +0800 From: Fam Zheng Message-ID: <20151215020302.GA19034@ad.usersys.redhat.com> References: <1449768232-22924-1-git-send-email-armbru@redhat.com> <1449768232-22924-4-git-send-email-armbru@redhat.com> <5669BAE0.6090600@redhat.com> <87h9jlxt66.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87h9jlxt66.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Jeff Cody , Laszlo Ersek , qemu-devel@nongnu.org On Mon, 12/14 10:42, Markus Armbruster wrote: > Laszlo Ersek 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 > >> Cc: Fam Zheng > >> Cc: Laszlo Ersek > >> Signed-off-by: Markus Armbruster > >> --- > >> 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 > > > > > I figure this would make more sense: > > [TIMESTAMP:][LOCATION: ]Invalid extent line: 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?