| Message ID | 1335896294-9530-4-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 01.05.2012 20:18, schrieb Anthony Liguori: > This allows a base class to easily add properties. > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> Implementation looks okay but /me not so happy with it: This conflicts with the move of the qdev static property infrastructure from DeviceState to Object. Consider rebasing this onto part of Paolo's series and call it object_add_properties? Andreas > --- > hw/qdev.c | 25 ++++++++++++------------- > hw/qdev.h | 2 ++ > 2 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/hw/qdev.c b/hw/qdev.c > index 6a8f6bd..e17a9ab 100644 > --- a/hw/qdev.c > +++ b/hw/qdev.c > @@ -76,22 +76,26 @@ bool qdev_exists(const char *name) > static void qdev_property_add_legacy(DeviceState *dev, Property *prop, > Error **errp); > > -void qdev_set_parent_bus(DeviceState *dev, BusState *bus) > +void qdev_add_properties(DeviceState *dev, Property *props) > { > Property *prop; > > + for (prop = props; prop && prop->name; prop++) { > + qdev_property_add_legacy(dev, prop, NULL); > + qdev_property_add_static(dev, prop, NULL); > + } > + qdev_prop_set_defaults(dev, props); > +} > + > +void qdev_set_parent_bus(DeviceState *dev, BusState *bus) > +{ > if (qdev_hotplug) { > assert(bus->allow_hotplug); > } > > dev->parent_bus = bus; > QTAILQ_INSERT_HEAD(&bus->children, dev, sibling); > - > - for (prop = qdev_get_bus_info(dev)->props; prop && prop->name; prop++) { > - qdev_property_add_legacy(dev, prop, NULL); > - qdev_property_add_static(dev, prop, NULL); > - } > - qdev_prop_set_defaults(dev, dev->parent_bus->info->props); > + qdev_add_properties(dev, dev->parent_bus->info->props); > } > > /* Create a new device. This only initializes the device state structure > @@ -633,13 +637,8 @@ static void device_initfn(Object *obj) > dev->instance_id_alias = -1; > dev->state = DEV_STATE_CREATED; > > - for (prop = qdev_get_props(dev); prop && prop->name; prop++) { > - qdev_property_add_legacy(dev, prop, NULL); > - qdev_property_add_static(dev, prop, NULL); > - } > - > + qdev_add_properties(dev, qdev_get_props(dev)); > object_property_add_str(OBJECT(dev), "type", qdev_get_type, NULL, NULL); > - qdev_prop_set_defaults(dev, qdev_get_props(dev)); > } > > /* Unlink device from bus and free the structure. */ > diff --git a/hw/qdev.h b/hw/qdev.h > index 4e90119..ca8386a 100644 > --- a/hw/qdev.h > +++ b/hw/qdev.h > @@ -360,4 +360,6 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus); > > extern int qdev_hotplug; > > +void qdev_add_properties(DeviceState *dev, Property *props); > + > #endif
On 05/01/2012 02:05 PM, Andreas Färber wrote: > Am 01.05.2012 20:18, schrieb Anthony Liguori: >> This allows a base class to easily add properties. >> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> > > Implementation looks okay but /me not so happy with it: This conflicts > with the move of the qdev static property infrastructure from > DeviceState to Object. > > Consider rebasing this onto part of Paolo's series and call it > object_add_properties? Eh? There's nothing object_ about these properties and there's no way I'm willing to put legacy properties in object... So I'm not quite sure what you're suggesting. Regards, Anthony Liguori > > Andreas > >> --- >> hw/qdev.c | 25 ++++++++++++------------- >> hw/qdev.h | 2 ++ >> 2 files changed, 14 insertions(+), 13 deletions(-) >> >> diff --git a/hw/qdev.c b/hw/qdev.c >> index 6a8f6bd..e17a9ab 100644 >> --- a/hw/qdev.c >> +++ b/hw/qdev.c >> @@ -76,22 +76,26 @@ bool qdev_exists(const char *name) >> static void qdev_property_add_legacy(DeviceState *dev, Property *prop, >> Error **errp); >> >> -void qdev_set_parent_bus(DeviceState *dev, BusState *bus) >> +void qdev_add_properties(DeviceState *dev, Property *props) >> { >> Property *prop; >> >> + for (prop = props; prop&& prop->name; prop++) { >> + qdev_property_add_legacy(dev, prop, NULL); >> + qdev_property_add_static(dev, prop, NULL); >> + } >> + qdev_prop_set_defaults(dev, props); >> +} >> + >> +void qdev_set_parent_bus(DeviceState *dev, BusState *bus) >> +{ >> if (qdev_hotplug) { >> assert(bus->allow_hotplug); >> } >> >> dev->parent_bus = bus; >> QTAILQ_INSERT_HEAD(&bus->children, dev, sibling); >> - >> - for (prop = qdev_get_bus_info(dev)->props; prop&& prop->name; prop++) { >> - qdev_property_add_legacy(dev, prop, NULL); >> - qdev_property_add_static(dev, prop, NULL); >> - } >> - qdev_prop_set_defaults(dev, dev->parent_bus->info->props); >> + qdev_add_properties(dev, dev->parent_bus->info->props); >> } >> >> /* Create a new device. This only initializes the device state structure >> @@ -633,13 +637,8 @@ static void device_initfn(Object *obj) >> dev->instance_id_alias = -1; >> dev->state = DEV_STATE_CREATED; >> >> - for (prop = qdev_get_props(dev); prop&& prop->name; prop++) { >> - qdev_property_add_legacy(dev, prop, NULL); >> - qdev_property_add_static(dev, prop, NULL); >> - } >> - >> + qdev_add_properties(dev, qdev_get_props(dev)); >> object_property_add_str(OBJECT(dev), "type", qdev_get_type, NULL, NULL); >> - qdev_prop_set_defaults(dev, qdev_get_props(dev)); >> } >> >> /* Unlink device from bus and free the structure. */ >> diff --git a/hw/qdev.h b/hw/qdev.h >> index 4e90119..ca8386a 100644 >> --- a/hw/qdev.h >> +++ b/hw/qdev.h >> @@ -360,4 +360,6 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus); >> >> extern int qdev_hotplug; >> >> +void qdev_add_properties(DeviceState *dev, Property *props); >> + >> #endif > >
Am 01.05.2012 22:37, schrieb Anthony Liguori: > On 05/01/2012 02:05 PM, Andreas Färber wrote: >> Am 01.05.2012 20:18, schrieb Anthony Liguori: >>> This allows a base class to easily add properties. >>> >>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> >> Implementation looks okay but /me not so happy with it: This conflicts >> with the move of the qdev static property infrastructure from >> DeviceState to Object. >> >> Consider rebasing this onto part of Paolo's series and call it >> object_add_properties? > > Eh? There's nothing object_ about these properties and there's no way > I'm willing to put legacy properties in object... > > So I'm not quite sure what you're suggesting. You just suggested to Peter using qdev_add_properties() in new QOM ARM classes of his. I'd rather not propagate using qdev_* functions in new QOM code because either it remains forever or renaming becomes another touch-all series. In Paolo's series the Property* concept is moved from qdev to QOM; thus if it's in Object we usually use an object_ prefix. Not too long ago you were willing to merge the large part of Paolo's series which included this code movement, so if you don't want that after all you should communicate that openly as a reply there. :) Andreas
On 05/01/2012 03:43 PM, Andreas Färber wrote: > Am 01.05.2012 22:37, schrieb Anthony Liguori: >> On 05/01/2012 02:05 PM, Andreas Färber wrote: >>> Am 01.05.2012 20:18, schrieb Anthony Liguori: >>>> This allows a base class to easily add properties. >>>> >>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >>> >>> Implementation looks okay but /me not so happy with it: This conflicts >>> with the move of the qdev static property infrastructure from >>> DeviceState to Object. >>> >>> Consider rebasing this onto part of Paolo's series and call it >>> object_add_properties? >> >> Eh? There's nothing object_ about these properties and there's no way >> I'm willing to put legacy properties in object... >> >> So I'm not quite sure what you're suggesting. > > You just suggested to Peter using qdev_add_properties() in new QOM ARM > classes of his. > > I'd rather not propagate using qdev_* functions in new QOM code because > either it remains forever or renaming becomes another touch-all series. > > In Paolo's series the Property* concept is moved from qdev to QOM; thus > if it's in Object we usually use an object_ prefix. > Not too long ago you were willing to merge the large part of Paolo's > series which included this code movement, so if you don't want that > after all you should communicate that openly as a reply there. :) Legacy properties != static properties. qdev_add_properties adds both legacy and static properties. I'm happy to put static properties into object but not legacy properties. So qdev_add_properties is going to stick around even after Paolo's changes. Base classes are free to call object_property_add_static directly and avoid qdev_add_properties entirely but we need the later for backwards compat. Regards, Anthony Liguori > > Andreas >
On 1 May 2012 21:48, Anthony Liguori <aliguori@us.ibm.com> wrote: > Legacy properties != static properties. > > qdev_add_properties adds both legacy and static properties. I'm happy to > put static properties into object but not legacy properties. So, er, how are you defining legacy and static properties? I would have thought 'legacy property' was 'anything we happen to already have'... > Base classes are free to call object_property_add_static directly and avoid > qdev_add_properties entirely but we need the later for backwards compat. If qdev_add_properties is purely for back-compat it should be clearly labelled so we don't use it in new code. -- PMM
On 05/01/2012 03:57 PM, Peter Maydell wrote: > On 1 May 2012 21:48, Anthony Liguori<aliguori@us.ibm.com> wrote: >> Legacy properties != static properties. >> >> qdev_add_properties adds both legacy and static properties. I'm happy to >> put static properties into object but not legacy properties. > > So, er, how are you defining legacy and static properties? > I would have thought 'legacy property' was 'anything we happen > to already have'... No, per the naming convention Paolo introduced, it's roughly: 1) legacy properties can be get/set as strings using the qdev parsing methods. Via qdev, they're exposed as -device <type>,<prop-name>=<string>. In QOM, they're exposed as: /path/to/device/legacy-<prop-name> = <string> 2) static properties are well-behaved QOM properties that just happen to be defined by the DEFINE_PROP_XXX macros. They use the get/set methods of the property types. New code can (and probably should) make use of static properties. There's magic in the qdev layer now to decide whether a Property in the array of properties becomes a legacy or static property (it's only ever exposed as one type). When we move static properties to Object, I'd like to keep legacy strictly as a qdev concept so we don't accidentally introduce more legacy properties. >> Base classes are free to call object_property_add_static directly and avoid >> qdev_add_properties entirely but we need the later for backwards compat. > > If qdev_add_properties is purely for back-compat it should be > clearly labelled so we don't use it in new code. Yes, I expect it would be once we have Paolo's property series in. For now, it's the only real interface we have for adding bulk properties. Regards, Anthony Liguori > > -- PMM >
Il 02/05/2012 00:01, Anthony Liguori ha scritto: > > There's magic in the qdev layer now to decide whether a Property in the > array of properties becomes a legacy or static property (it's only ever > exposed as one type). I don't think this is true: a legacy property always has a static counterpart. The magic (really just a fallback) is to decide whether -device <type>,<prop>=<value> will use the legacy property or a string visitor + a static property. Paolo
On 05/01/2012 05:12 PM, Paolo Bonzini wrote: > Il 02/05/2012 00:01, Anthony Liguori ha scritto: >> >> There's magic in the qdev layer now to decide whether a Property in the >> array of properties becomes a legacy or static property (it's only ever >> exposed as one type). > > I don't think this is true: a legacy property always has a static > counterpart. The magic (really just a fallback) is to decide whether > -device<type>,<prop>=<value> will use the legacy property or a string > visitor + a static property. Yes, I mispoke. All legacy properties are static properties but not all static properties are legacy (IIUC). The exception is PROP_PTR which is always a legacy but never a static. So there is quite a bit of magic. Regards, Anthony Liguori > > Paolo >
diff --git a/hw/qdev.c b/hw/qdev.c index 6a8f6bd..e17a9ab 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -76,22 +76,26 @@ bool qdev_exists(const char *name) static void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp); -void qdev_set_parent_bus(DeviceState *dev, BusState *bus) +void qdev_add_properties(DeviceState *dev, Property *props) { Property *prop; + for (prop = props; prop && prop->name; prop++) { + qdev_property_add_legacy(dev, prop, NULL); + qdev_property_add_static(dev, prop, NULL); + } + qdev_prop_set_defaults(dev, props); +} + +void qdev_set_parent_bus(DeviceState *dev, BusState *bus) +{ if (qdev_hotplug) { assert(bus->allow_hotplug); } dev->parent_bus = bus; QTAILQ_INSERT_HEAD(&bus->children, dev, sibling); - - for (prop = qdev_get_bus_info(dev)->props; prop && prop->name; prop++) { - qdev_property_add_legacy(dev, prop, NULL); - qdev_property_add_static(dev, prop, NULL); - } - qdev_prop_set_defaults(dev, dev->parent_bus->info->props); + qdev_add_properties(dev, dev->parent_bus->info->props); } /* Create a new device. This only initializes the device state structure @@ -633,13 +637,8 @@ static void device_initfn(Object *obj) dev->instance_id_alias = -1; dev->state = DEV_STATE_CREATED; - for (prop = qdev_get_props(dev); prop && prop->name; prop++) { - qdev_property_add_legacy(dev, prop, NULL); - qdev_property_add_static(dev, prop, NULL); - } - + qdev_add_properties(dev, qdev_get_props(dev)); object_property_add_str(OBJECT(dev), "type", qdev_get_type, NULL, NULL); - qdev_prop_set_defaults(dev, qdev_get_props(dev)); } /* Unlink device from bus and free the structure. */ diff --git a/hw/qdev.h b/hw/qdev.h index 4e90119..ca8386a 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -360,4 +360,6 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus); extern int qdev_hotplug; +void qdev_add_properties(DeviceState *dev, Property *props); + #endif
This allows a base class to easily add properties. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- hw/qdev.c | 25 ++++++++++++------------- hw/qdev.h | 2 ++ 2 files changed, 14 insertions(+), 13 deletions(-)