From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47086) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a75KJ-0000wx-Bt for qemu-devel@nongnu.org; Thu, 10 Dec 2015 12:48:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a75KF-0000ct-7g for qemu-devel@nongnu.org; Thu, 10 Dec 2015 12:48:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46568) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a75KE-0000cp-VX for qemu-devel@nongnu.org; Thu, 10 Dec 2015 12:48:19 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 3826AC105B4B for ; Thu, 10 Dec 2015 17:48:18 +0000 (UTC) References: <1449768232-22924-1-git-send-email-armbru@redhat.com> <1449768232-22924-4-git-send-email-armbru@redhat.com> From: Laszlo Ersek Message-ID: <5669BAE0.6090600@redhat.com> Date: Thu, 10 Dec 2015 18:48:16 +0100 MIME-Version: 1.0 In-Reply-To: <1449768232-22924-4-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 , qemu-devel@nongnu.org Cc: Jeff Cody , Fam Zheng 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. > 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. > return -EINVAL; > } > } else if (!strcmp(type, "VMFS")) { > if (matches == 4) { > flat_offset = 0; > } else { > - error_setg(errp, "Invalid extent lines:\n%s", p); > + error_setg(errp, "Invalid extent lines"); > + error_append_hint(errp, "%s", p); > return -EINVAL; > } > } else if (matches != 4) { > - error_setg(errp, "Invalid extent lines:\n%s", p); > + error_setg(errp, "Invalid extent lines"); > + error_append_hint(errp, "%s", p); > return -EINVAL; > } These appear fine as well. > > diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c > index 0fd6923..68622ff 100644 > --- a/hw/i386/kvm/pci-assign.c > +++ b/hw/i386/kvm/pci-assign.c > @@ -812,8 +812,9 @@ static void assign_device(AssignedDevice *dev, Error **errp) > char *cause; > > cause = assign_failed_examine(dev); > - error_setg_errno(errp, -r, "Failed to assign device \"%s\"\n%s", > - dev->dev.qdev.id, cause); > + error_setg_errno(errp, -r, "Failed to assign device \"%s\"", > + dev->dev.qdev.id); > + error_append_hint(errp, "%s", cause); > g_free(cause); > break; > } > @@ -912,11 +913,10 @@ retry: > dev->features |= ASSIGNED_DEVICE_PREFER_MSI_MASK; > goto retry; > } > - error_setg_errno(errp, -r, > - "Failed to assign irq for \"%s\"\n" > - "Perhaps you are assigning a device " > - "that shares an IRQ with another device?", > + error_setg_errno(errp, -r, "Failed to assign irq for \"%s\"", > dev->dev.qdev.id); > + error_append_hint(errp, "Perhaps you are assigning a device " > + "that shares an IRQ with another device?"); > return r; > } > > If you remove the extraneous \n from vhdx_parse_log(), you can add Reviewed-by: Laszlo Ersek Thanks Laszlo