| Message ID | 20191110190310.19799-2-dplotnikov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | allow to set 'drive' property on a realized block device | expand |
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. > { > 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 >
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'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 >>
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.
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 <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. 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. >
Am 26.11.2019 um 07:49 hat Denis Plotnikov geschrieben: > > > 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 <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. > 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 think I agree with Eduardo on this one. This is generic infrastructure and we already see that it may require lots of wrapper functions that call into the same function with different parameters. Checking dev->realized where necessary can do the same. I haven't had a closer look at the specific functions, but maybe they can actually share the same implementation with just some if (dev->realized) blocks. Kevin
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) { 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 = {
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(-)