Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2

[Laszlo Ersek <lersek@redhat.com>]

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 <jcody@redhat.com>
> Cc: Fam Zheng <famz@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@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 <lersek@redhat.com>

Thanks
Laszlo