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.
> 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 <ler...@redhat.com>
Thanks
Laszlo
