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

[Peter Maydell]

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...

> If this change is deemed an improvement for drive=, then it surely is
> one for the other backend references, isn't it?  These are parse_chr()
> for chardev= (shown above), and set_netdev() for netdev= (not shown).
>
>     Property 'TYPE.PROP' can't take value 'VAL', it's in use
>
> vs.
>
>     Property 'TYPE.PROP' can't take value 'VAL'; that chardev has already 
> been connected to another device.
>
> Same as above, except s/drive/chardev/.  Likewise for netdev.
>
> Other users of error_set_from_qdev_prop_error() remain: set_vlan(),
> set_mac(), set_pci_devfn(), set_pci_host_devaddr().  Okay.  I figure the
> error message duplication could be avoided by calling
> error_set_from_qdev_prop_error() here.  No biggie either way.

Yes. My feeling is that error_set_from_qdev_prop_error() should
probably be removed entirely; but with 2.4 looming I didn't want
to bulk this patchset out with that change.

-- PMM