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

[Denis Plotnikov]

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

Thanks for reviewing!
Denis

>
>>   {
>>       DeviceState *dev = DEVICE(obj);
>> @@ -48,11 +53,6 @@ static void set_pointer(Object *obj, Visitor *v, Property 
>> *prop,
>>       void **ptr = qdev_get_prop_ptr(dev, prop);
>>       char *str;
>>   
>> -    if (dev->realized) {
>> -        qdev_prop_set_after_realize(dev, name, errp);
>> -        return;
>> -    }
>> -
>>       visit_type_str(v, name, &str, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>> @@ -63,7 +63,17 @@ static void set_pointer(Object *obj, Visitor *v, Property 
>> *prop,
>>           *ptr = NULL;
>>           return;
>>       }
>> -    parse(dev, str, ptr, prop->name, errp);
>> +
>> +    if (dev->realized) {
>> +        if (parse_realized) {
>> +            parse_realized(dev, str, ptr, prop->name, errp);
>> +        } else {
>> +            qdev_prop_set_after_realize(dev, name, errp);
>> +        }
>> +    } else {
>> +        parse_unrealized(dev, str, ptr, prop->name, errp);
>> +    }
>> +
>>       g_free(str);
>>   }
>>   
>> @@ -178,13 +188,13 @@ static void get_drive(Object *obj, Visitor *v, const 
>> char *name, void *opaque,
>>   static void set_drive(Object *obj, Visitor *v, const char *name, void 
>> *opaque,
>>                         Error **errp)
>>   {
>> -    set_pointer(obj, v, opaque, parse_drive, name, errp);
>> +    set_pointer(obj, v, opaque, NULL, parse_drive, name, errp);
>>   }
>>   
>>   static void set_drive_iothread(Object *obj, Visitor *v, const char *name,
>>                                  void *opaque, Error **errp)
>>   {
>> -    set_pointer(obj, v, opaque, parse_drive_iothread, name, errp);
>> +    set_pointer(obj, v, opaque, NULL, parse_drive_iothread, name, errp);
>>   }
>>   
>>   const PropertyInfo qdev_prop_drive = {
>> -- 
>> 2.17.0
>>