Re: [Qemu-devel] [PATCH 3/4] qdev-properties-system: Improve error message for drive assignment conflict

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> If the user forgot if=none on their drive specification they're likely
> to get an error message because the drive is assigned once automatically
> by QEMU and once by the manual id=/drive= user command line specification.
> Improve the error message produced in this case to explicitly guide the
> user towards if=none.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  hw/core/qdev-properties-system.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/qdev-properties-system.c 
> b/hw/core/qdev-properties-system.c
> index 56954b4..bde9e12 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -78,8 +78,20 @@ static void parse_drive(DeviceState *dev, const char *str, 
> void **ptr,
>          return;
>      }
>      if (blk_attach_dev(blk, dev) < 0) {
> -        error_setg(errp, "Property '%s.%s' can't take value '%s', it's in 
> use",
> -                  object_get_typename(OBJECT(dev)), propname, str);
> +        DriveInfo *dinfo = blk_legacy_dinfo(blk);
> +
> +        if (dinfo->auto_claimed) {
> +            error_setg(errp, "Property '%s.%s' can't be set to drive ID 
> '%s'; "
> +                       "that drive has been automatically connected to 
> another "
> +                       "device. Use if=none if you do not want that 
> automatic "
> +                       "connection.",
> +                       object_get_typename(OBJECT(dev)), propname, str);
> +        } else {
> +            error_setg(errp, "Property '%s.%s' can't be set to drive ID 
> '%s'; "
> +                       "that drive has already been connected to another "
> +                       "device.",
> +                       object_get_typename(OBJECT(dev)), propname, str);
> +        }
>          return;
>      }
>      *ptr = blk;

We generally do not end error messages with a period.

The message for auto_claimed drives is of the form

    LOCATION: This went wrong.  Advice on how to fix it.

All in one awfully long line.  A friendler format is

    LOCATION: This went wrong
    Advice on how to fix it.

Unfortunately, the Error API doesn't support that.

Easy with error_report():

    error_report("This went wrong");
    error_printf("Advice on how to fix it.");

With soon-to-be-gone qerror_report():

    qerror_report(ERROR_CLASS_GENERIC_ERROR, "This went wrong");
    error_printf_unless_qmp("Advice on how to fix it.");

I think we should just bite the bullet and extend Error to support
additional helpful information for humans.  See also
http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg02482.html
and commit 7216ae3 "qemu-option: Disable two helpful messages that got
broken recently".