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

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 <peter.maydell@linaro.org>
---
 hw/core/qdev-properties-system.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
> ---
>  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".

On 22 June 2015 at 10:12, Markus Armbruster <armbru@redhat.com> wrote:
> I think we should just bite the bullet and extend Error to support
> additional helpful information for humans.

...I thought all of the string was already just helpful information
for humans? I certainly hope we aren't expecting anybody to parse it...

-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On 22 June 2015 at 10:12, Markus Armbruster <armbru@redhat.com> wrote:
>> 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.
[...]
>> I think we should just bite the bullet and extend Error to support
>> additional helpful information for humans.
>
> ...I thought all of the string was already just helpful information
> for humans? I certainly hope we aren't expecting anybody to parse it...

My point is

    qemu-system-arm: -device virtio-blk-pci,drive=foo: Property 'virtio-blk-device can't be set to drive ID 'foo'; that drive has been automatically connected to another device. Use if=none if you do not want that automatic connection.

is considerable less readable than something like

    qemu-system-arm: -device virtio-blk-pci,drive=foo: Property 'virtio-blk-device can't take value 'foo', it's in use
    The drive with ID 'foo' has been automatically connected to another device.
    Use if=none if you do not want that automatic connection.

Error messages should be short and to the point.  We actually enforce a
"no newline in error messages" policy there.

Some errors can use a more verbose explanation, or hints on how to avoid
the error, or hints on how to find out more on what exactly went wrong".
Such explanations should be sensibly formatted, not crammed into the
same line as the error message.

Whether the additional explanation should also be transmitted over QMP
is open for debate.

Note: my example is just for demonstrating layout and style.  No claim
to perfection for the actual text.