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

[Eduardo Habkost <ehabkost@redhat.com>]

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 <dplotnikov@virtuozzo.com>
> >> ---
> >>   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.


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

-- 
Eduardo