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