Re: [Qemu-block] [Qemu-devel] [PATCH 2/4] qdev-properties-system: Change set_pointer's parse callback to use Error

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 22 June 2015 at 10:39, Markus Armbruster <address@hidden> wrote:
>> Peter Maydell <address@hidden> writes:
>>
>>> Instead of having set_pointer() call a parse callback which returns
>>> an error number that we then convert to an Error string with
>>> error_set_from_qdev_prop_error(), make the parse callback take an
>>> Error** and set the error itself. This will allow parse routines
>>> to provide more helpful error messages than the generic ones.
>
>> The error messages are faithfully copied from
>> error_set_from_qdev_prop_error().  The next patch will improve
>> parse_drive()'s messages, and that's why you got this patch.
>
> That's right.
>
>> The next patch takes special care of the if=T, T!=none case.  That case
>> only exists for drives, because only for drives did we mix up old-style
>> (one option to configure front- and backend) and new-style (one option
>> to configure backend, -device to configure frontend) configuration.
>>
>> It also changes the if=none case, from
>>
>>     Property 'TYPE.PROP' can't take value 'VAL', it's in use
>>
>> to
>>
>>     "Property 'TYPE.PROP' can't be set to drive ID 'VAL'; that drive
>> has already been connected to another device."
>>
>> More explicit.  Rather long, though.
>
> Do you have a proposed better phrasing? In general I feel that the problem
> with the current error message is that:
>  (1) it is phrased from the point of view of QEMU's internal implementation
>  (2) it is extremely terse
>
> So the lengthiness is in my opinion an improvement. Better not to
> leave the user in the dark about why we just refused to run...

Perhaps

    Drive VAL is already used by another device

Relies on the fact that the context is either blatantly obvious (monitor
command), or the actual message makes it obvious, e.g.

    qemu-system-arm: -device virtio-blk-pci,drive=foo: Drive foo is already 
used by another device

[...]