Re: [Qemu-devel] [PATCH v3 19/24] pci-assign: Clean up "Failed to assign" error messages

[Laszlo Ersek]

On 12/18/15 16:35, Markus Armbruster wrote:
> The arguments of error_setg() & friends should yield a short error
> string without newlines.
> 
> Two 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().
> 
> Cc: Laszlo Ersek <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  hw/i386/kvm/pci-assign.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index 0fd6923..eec1340 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -770,7 +770,7 @@ static char *assign_failed_examine(const AssignedDevice 
> *dev)
>          "*** $ echo \"%04x:%02x:%02x.%x\" > /sys/bus/pci/drivers/"
>          "pci-stub/bind\n"
>          "*** $ echo \"%04x %04x\" > 
> /sys/bus/pci/drivers/pci-stub/remove_id\n"
> -        "***",
> +        "***\n",
>          ns, dev->host.domain, dev->host.bus, dev->host.slot,
>          dev->host.function, vendor_id, device_id,
>          dev->host.domain, dev->host.bus, dev->host.slot, dev->host.function,
> @@ -778,7 +778,7 @@ static char *assign_failed_examine(const AssignedDevice 
> *dev)
>          dev->host.function, vendor_id, device_id);
>  
>  fail:
> -    return g_strdup("Couldn't find out why.");
> +    return g_strdup("Couldn't find out why.\n");
>  }
>  
>  static void assign_device(AssignedDevice *dev, Error **errp)
> @@ -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?\n");
>          return r;
>      }
>  
> 

Reviewed-by: Laszlo Ersek <address@hidden>