Re: [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices

[Denis Plotnikov]

On 25.11.2019 18:30, Eduardo Habkost wrote:
> On Fri, Nov 22, 2019 at 11:36:30AM +0000, Denis Plotnikov wrote:
>>
>> On 18.11.2019 21:54, Eduardo Habkost wrote:
>>> On Sun, Nov 10, 2019 at 10:03:09PM +0300, Denis Plotnikov wrote:
>>>> Some device's property can be changed if the device has been already
>>>> realized. For example, it could be "drive" property of a scsi disk device.
>>>>
>>>> So far, set_pointer could operate only on a relized device. The patch
>>>> extends its interface for operation on an unrealized device.
>>>>
>>>> Signed-off-by: Denis Plotnikov <address@hidden>
>>>> ---
>>>>    hw/core/qdev-properties-system.c | 32 +++++++++++++++++++++-----------
>>>>    1 file changed, 21 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/core/qdev-properties-system.c 
>>>> b/hw/core/qdev-properties-system.c
>>>> index ba412dd2ca..c534590dcd 100644
>>>> --- a/hw/core/qdev-properties-system.c
>>>> +++ b/hw/core/qdev-properties-system.c
>>>> @@ -38,9 +38,14 @@ static void get_pointer(Object *obj, Visitor *v, 
>>>> Property *prop,
>>>>    }
>>>>    
>>>>    static void set_pointer(Object *obj, Visitor *v, Property *prop,
>>>> -                        void (*parse)(DeviceState *dev, const char *str,
>>>> -                                      void **ptr, const char *propname,
>>>> -                                      Error **errp),
>>>> +                        void (*parse_realized)(DeviceState *dev,
>>>> +                                               const char *str, void 
>>>> **ptr,
>>>> +                                               const char *propname,
>>>> +                                               Error **errp),
>>>> +                        void (*parse_unrealized)(DeviceState *dev,
>>>> +                                                 const char *str, void 
>>>> **ptr,
>>>> +                                                 const char *propname,
>>>> +                                                 Error **errp),
>>>>                            const char *name, Error **errp)
>>> Wouldn't it be simpler to just add a PropertyInfo::allow_set_after_realize
>>> bool field, and call the same setter function?  Then you can
>>> simply change do_parse_drive() to check if realized is true.
>> May be, but I thought It would be more clear to have a separate callback
>> for all the devices supporting the property setting when realized.
>> Also the "drive" property setting on realized and non-realized device a
>> little bit different: in the realized case the setter function expects
>> to get
>> BlockDriverState only, when in the unrealized case the setter can accept
>> both BlockBackend and BlockDriverState. Also, in the unrealized case the
>> setter function doesn't expect to have a device with an empty BlockBackend.
>> I decided that extending do_parse_drive would make it more complex for
>> understanding. That's why I made two separate functions for both cases.
> I understand you might want two separate functions in the
> specific case of drive.  You can still call different
> functions after checking dev->realized inside do_parse_drive().
>
> My point was that you don't need to make set_pointer() require
> two separate function pointers just to propagate 1 bit of
> information that is already available in DeviceState.  In patch
> 2/2 you had to create 4 different copies of parse_drive*()
> because of this.
Yes, that's true. I wanted to suggest a more general way to deal with a 
device on realized and non-realized state.
I may be too much and not necessary. May be we should wait for a 
feedback from the block maintainers?
>
>
>> I'd like to mention that I have a few concerns about
>> do_parse_drive_realized (please see the next patch from the series) and
>> I'd like them to be reviewed as well. After that, may be it would be
>> better to go the way you suggested.
> In the case if your questions in patch 2/2, I'm afraid I don't
> know the answers and we need help from the block maintainers.
Anyway, thanks for taking a glance.
>