qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 13/16] qdev: Make qdev_prop_set_drive() match the other helpe


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH 13/16] qdev: Make qdev_prop_set_drive() match the other helpers
Date: Mon, 8 Jun 2020 07:52:39 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

On 6/8/20 7:20 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> On 6/5/20 4:56 PM, Markus Armbruster wrote:
>>> qdev_prop_set_drive() can fail.  None of the other qdev_prop_set_FOO()
>>> can; they abort on error.
>>>
>>> To clean up this inconsistency, rename qdev_prop_set_drive() to
>>> qdev_prop_set_drive_err(), and create a qdev_prop_set_drive() that
>>> aborts on error.
>>>
>>> Coccinelle script to update callers:
>>>
>>>     @ depends on !(file in "hw/core/qdev-properties-system.c")@
>>>     expression dev, name, value;
>>>     symbol error_abort;
>>>     @@
>>>     -    qdev_prop_set_drive(dev, name, value, &error_abort);
>>>     +    qdev_prop_set_drive(dev, name, value);
>>
>> Why not open-code qdev_prop_set_drive_err(..., &error_abort)?
> 
> Consistency with qdev_prop_set_chr() and qdev_prop_set_netdev().
> 
> My starting point was "what makes block backends so different that they
> need error handling where nothing else does?"
> 
> After a considerable amount of digging, my answer is "nothing".
> qdev_prop_set_drive(), qdev_prop_set_chr() and qdev_prop_set_netdev()
> can all run into errors.  On closer examination, all programming errors
> (thus &error_abort), except for "backend is already in use", and to
> trigger that one, you have to get creative and steal the backend for
> another purpose, e.g. with -global.  This is the abridged version of a
> longwinded argument I didn't want to make in this series, so I left the
> error handling alone.
> 
> In the longer run, I want qdev_prop_set_drive_err() to die.

I agree with the longer run. I naively thought this could be done
in the same patch.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>>
>>>
>>>     @@
>>>     expression dev, name, value, errp;
>>>     @@
>>>     -    qdev_prop_set_drive(dev, name, value, errp);
>>>     +    qdev_prop_set_drive_err(dev, name, value, errp);
>>>
>> [...]
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]