| Message ID | 1322687028-29714-2-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 30, 2011 at 03:03:31PM -0600, Anthony Liguori wrote: > +/** > + * @DevicePropertyEtter - called when trying to get/set a property An established term for this is an "accessor". I've never heard "etter" before and it looks like a typo on first sight ;). > +/** > + * @qdev_property_set - writes a property to a device > + * > + * @dev - the device > + * > + * @v - the visitor that will used to write the property value. This should be s/will used/will be used/
On 12/01/2011 02:19 AM, Stefan Hajnoczi wrote: > On Wed, Nov 30, 2011 at 03:03:31PM -0600, Anthony Liguori wrote: >> +/** >> + * @DevicePropertyEtter - called when trying to get/set a property > > An established term for this is an "accessor". I've never heard "etter" > before and it looks like a typo on first sight ;). > >> +/** >> + * @qdev_property_set - writes a property to a device >> + * >> + * @dev - the device >> + * >> + * @v - the visitor that will used to write the property value. This should be > > s/will used/will be used/ Ack. Regards, Anthony Liguori >
Am 30.11.2011 22:03, schrieb Anthony Liguori: > qdev properties are settable only during construction and static to classes. > This isn't flexible enough for QOM. > > This patch introduces a property interface for qdev that provides dynamic > properties that are tied to objects, instead of classes. These properties are > Visitor based instead of string based too. > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > hw/qdev.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/qdev.h | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > qerror.c | 4 ++ > qerror.h | 3 ++ > 4 files changed, 224 insertions(+), 0 deletions(-) > > diff --git a/hw/qdev.c b/hw/qdev.c > index 106407f..ad2d44f 100644 > --- a/hw/qdev.c > +++ b/hw/qdev.c > @@ -390,12 +390,33 @@ void qdev_init_nofail(DeviceState *dev) > } > } > > +static void qdev_property_del_all(DeviceState *dev) > +{ > + while (dev->properties) { > + GSList *i = dev->properties; > + DeviceProperty *prop = i->data; > + > + dev->properties = i->next; > + > + if (prop->release) { > + prop->release(dev, prop->name, prop->opaque); > + } > + > + g_free(prop->name); > + g_free(prop->type); > + g_free(prop); > + g_free(i); > + } > +} > + > /* Unlink device from bus and free the structure. */ > void qdev_free(DeviceState *dev) > { > BusState *bus; > Property *prop; > > + qdev_property_del_all(dev); > + > if (dev->state == DEV_STATE_INITIALIZED) { > while (dev->num_child_bus) { > bus = QLIST_FIRST(&dev->child_bus); > @@ -962,3 +983,81 @@ char* qdev_get_fw_dev_path(DeviceState *dev) > > return strdup(path); > } > + > +void qdev_property_add(DeviceState *dev, const char *name, const char *type, > + DevicePropertyEtter *get, DevicePropertyEtter *set, > + DevicePropertyRelease *release, void *opaque, > + Error **errp) How about letting the caller pass in a DeviceProperty for improved readability and usability? Instead of memorizing the order of currently eight parameters (could probably become more in the future) you can use proper C99 initializers then. > @@ -45,6 +82,7 @@ struct DeviceState { > QTAILQ_ENTRY(DeviceState) sibling; > int instance_id_alias; > int alias_required_for_version; > + GSList *properties; > }; Why GSList instead of qemu-queue.h macros that would provide type safety? I don't think a property can belong to multiple devices, can it? qdev_property_add only refers to a single device, and nothing else adds elements to the list. Kevin
On 12/01/2011 09:52 AM, Kevin Wolf wrote: > Am 30.11.2011 22:03, schrieb Anthony Liguori: >> qdev properties are settable only during construction and static to classes. >> This isn't flexible enough for QOM. >> >> This patch introduces a property interface for qdev that provides dynamic >> properties that are tied to objects, instead of classes. These properties are >> Visitor based instead of string based too. >> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> --- >> hw/qdev.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/qdev.h | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qerror.c | 4 ++ >> qerror.h | 3 ++ >> 4 files changed, 224 insertions(+), 0 deletions(-) >> >> diff --git a/hw/qdev.c b/hw/qdev.c >> index 106407f..ad2d44f 100644 >> --- a/hw/qdev.c >> +++ b/hw/qdev.c >> @@ -390,12 +390,33 @@ void qdev_init_nofail(DeviceState *dev) >> } >> } >> >> +static void qdev_property_del_all(DeviceState *dev) >> +{ >> + while (dev->properties) { >> + GSList *i = dev->properties; >> + DeviceProperty *prop = i->data; >> + >> + dev->properties = i->next; >> + >> + if (prop->release) { >> + prop->release(dev, prop->name, prop->opaque); >> + } >> + >> + g_free(prop->name); >> + g_free(prop->type); >> + g_free(prop); >> + g_free(i); >> + } >> +} >> + >> /* Unlink device from bus and free the structure. */ >> void qdev_free(DeviceState *dev) >> { >> BusState *bus; >> Property *prop; >> >> + qdev_property_del_all(dev); >> + >> if (dev->state == DEV_STATE_INITIALIZED) { >> while (dev->num_child_bus) { >> bus = QLIST_FIRST(&dev->child_bus); >> @@ -962,3 +983,81 @@ char* qdev_get_fw_dev_path(DeviceState *dev) >> >> return strdup(path); >> } >> + >> +void qdev_property_add(DeviceState *dev, const char *name, const char *type, >> + DevicePropertyEtter *get, DevicePropertyEtter *set, >> + DevicePropertyRelease *release, void *opaque, >> + Error **errp) > > How about letting the caller pass in a DeviceProperty for improved > readability and usability? Instead of memorizing the order of currently > eight parameters (could probably become more in the future) you can use > proper C99 initializers then. Yeah, instead of taking a void *opaque, it could then just take the DeviceProperty and use container_of adding a good bit more type safety. I like it, thanks for the suggestion. > >> @@ -45,6 +82,7 @@ struct DeviceState { >> QTAILQ_ENTRY(DeviceState) sibling; >> int instance_id_alias; >> int alias_required_for_version; >> + GSList *properties; >> }; > > Why GSList instead of qemu-queue.h macros that would provide type safety? You're clearly thwarting my attempts at slowly introducing GSList as a replacement for qemu-queue ;-) I really dislike qemu-queue. I think it's a whole lot more difficult to use in practice. The glib data structures are much more rich than qemu-queue. > I don't think a property can belong to multiple devices, can it? > qdev_property_add only refers to a single device, and nothing else adds > elements to the list. Yes, you are correct. Regards, Anthony Liguori > > Kevin >
Am 02.12.2011 02:08, schrieb Anthony Liguori: > On 12/01/2011 09:52 AM, Kevin Wolf wrote: >> Am 30.11.2011 22:03, schrieb Anthony Liguori: >>> qdev properties are settable only during construction and static to classes. >>> This isn't flexible enough for QOM. >>> >>> This patch introduces a property interface for qdev that provides dynamic >>> properties that are tied to objects, instead of classes. These properties are >>> Visitor based instead of string based too. >>> >>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >>> --- >>> hw/qdev.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>> hw/qdev.h | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> qerror.c | 4 ++ >>> qerror.h | 3 ++ >>> 4 files changed, 224 insertions(+), 0 deletions(-) >>> >>> diff --git a/hw/qdev.c b/hw/qdev.c >>> index 106407f..ad2d44f 100644 >>> --- a/hw/qdev.c >>> +++ b/hw/qdev.c >>> @@ -390,12 +390,33 @@ void qdev_init_nofail(DeviceState *dev) >>> } >>> } >>> >>> +static void qdev_property_del_all(DeviceState *dev) >>> +{ >>> + while (dev->properties) { >>> + GSList *i = dev->properties; >>> + DeviceProperty *prop = i->data; >>> + >>> + dev->properties = i->next; >>> + >>> + if (prop->release) { >>> + prop->release(dev, prop->name, prop->opaque); >>> + } >>> + >>> + g_free(prop->name); >>> + g_free(prop->type); >>> + g_free(prop); >>> + g_free(i); >>> + } >>> +} >>> + >>> /* Unlink device from bus and free the structure. */ >>> void qdev_free(DeviceState *dev) >>> { >>> BusState *bus; >>> Property *prop; >>> >>> + qdev_property_del_all(dev); >>> + >>> if (dev->state == DEV_STATE_INITIALIZED) { >>> while (dev->num_child_bus) { >>> bus = QLIST_FIRST(&dev->child_bus); >>> @@ -962,3 +983,81 @@ char* qdev_get_fw_dev_path(DeviceState *dev) >>> >>> return strdup(path); >>> } >>> + >>> +void qdev_property_add(DeviceState *dev, const char *name, const char *type, >>> + DevicePropertyEtter *get, DevicePropertyEtter *set, >>> + DevicePropertyRelease *release, void *opaque, >>> + Error **errp) >> >> How about letting the caller pass in a DeviceProperty for improved >> readability and usability? Instead of memorizing the order of currently >> eight parameters (could probably become more in the future) you can use >> proper C99 initializers then. > > Yeah, instead of taking a void *opaque, it could then just take the > DeviceProperty and use container_of adding a good bit more type safety. I like > it, thanks for the suggestion. > >> >>> @@ -45,6 +82,7 @@ struct DeviceState { >>> QTAILQ_ENTRY(DeviceState) sibling; >>> int instance_id_alias; >>> int alias_required_for_version; >>> + GSList *properties; >>> }; >> >> Why GSList instead of qemu-queue.h macros that would provide type safety? > > You're clearly thwarting my attempts at slowly introducing GSList as a > replacement for qemu-queue ;-) > > I really dislike qemu-queue. I think it's a whole lot more difficult to use in > practice. The glib data structures are much more rich than qemu-queue. qemu-queue.h is type safe, GSList is not. IMO that's a show stopper and I can't understand why we even need to talk about it. If you want to convince me of the opposite, it certainly needs more than vague "easier to use" hand waving. Kevin
On 12/01/2011 09:52 AM, Kevin Wolf wrote: > Am 30.11.2011 22:03, schrieb Anthony Liguori: >> + >> +void qdev_property_add(DeviceState *dev, const char *name, const char *type, >> + DevicePropertyEtter *get, DevicePropertyEtter *set, >> + DevicePropertyRelease *release, void *opaque, >> + Error **errp) > > How about letting the caller pass in a DeviceProperty for improved > readability and usability? Instead of memorizing the order of currently > eight parameters (could probably become more in the future) you can use > proper C99 initializers then. This ends up making the code much more complex for the client if you try to eliminate the opaque and replace it with the structure. It becomes necessary to do a dynamic allocation of the structure and then you also have to add a release function. We could make the structure just contain the function pointers and not the opaque but that doesn't seem very helpful to me. It just adds a few extra lines to the client code without a lot of gain. Regards, Anthony Liguori > >> @@ -45,6 +82,7 @@ struct DeviceState { >> QTAILQ_ENTRY(DeviceState) sibling; >> int instance_id_alias; >> int alias_required_for_version; >> + GSList *properties; >> }; > > Why GSList instead of qemu-queue.h macros that would provide type safety? > > I don't think a property can belong to multiple devices, can it? > qdev_property_add only refers to a single device, and nothing else adds > elements to the list. > > Kevin >
Am 02.12.2011 19:47, schrieb Anthony Liguori: > On 12/01/2011 09:52 AM, Kevin Wolf wrote: >> Am 30.11.2011 22:03, schrieb Anthony Liguori: >>> + >>> +void qdev_property_add(DeviceState *dev, const char *name, const char *type, >>> + DevicePropertyEtter *get, DevicePropertyEtter *set, >>> + DevicePropertyRelease *release, void *opaque, >>> + Error **errp) >> >> How about letting the caller pass in a DeviceProperty for improved >> readability and usability? Instead of memorizing the order of currently >> eight parameters (could probably become more in the future) you can use >> proper C99 initializers then. > > This ends up making the code much more complex for the client if you try to > eliminate the opaque and replace it with the structure. It becomes necessary to > do a dynamic allocation of the structure and then you also have to add a release > function. Hm, why doesn't static allocation work with it? > We could make the structure just contain the function pointers and not the > opaque but that doesn't seem very helpful to me. It just adds a few extra lines > to the client code without a lot of gain. I keep switching back and forth between mails to find out what these parameters are supposed to mean (especially the NULL ones), so yes, I think even passing just the name and function pointers this way would improve readability. Kevin
diff --git a/hw/qdev.c b/hw/qdev.c index 106407f..ad2d44f 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -390,12 +390,33 @@ void qdev_init_nofail(DeviceState *dev) } } +static void qdev_property_del_all(DeviceState *dev) +{ + while (dev->properties) { + GSList *i = dev->properties; + DeviceProperty *prop = i->data; + + dev->properties = i->next; + + if (prop->release) { + prop->release(dev, prop->name, prop->opaque); + } + + g_free(prop->name); + g_free(prop->type); + g_free(prop); + g_free(i); + } +} + /* Unlink device from bus and free the structure. */ void qdev_free(DeviceState *dev) { BusState *bus; Property *prop; + qdev_property_del_all(dev); + if (dev->state == DEV_STATE_INITIALIZED) { while (dev->num_child_bus) { bus = QLIST_FIRST(&dev->child_bus); @@ -962,3 +983,81 @@ char* qdev_get_fw_dev_path(DeviceState *dev) return strdup(path); } + +void qdev_property_add(DeviceState *dev, const char *name, const char *type, + DevicePropertyEtter *get, DevicePropertyEtter *set, + DevicePropertyRelease *release, void *opaque, + Error **errp) +{ + DeviceProperty *prop = g_malloc0(sizeof(*prop)); + + prop->name = g_strdup(name); + prop->type = g_strdup(type); + prop->get = get; + prop->set = set; + prop->release = release; + prop->opaque = opaque; + + dev->properties = g_slist_append(dev->properties, prop); +} + +static DeviceProperty *qdev_property_find(DeviceState *dev, const char *name) +{ + GSList *i; + + for (i = dev->properties; i; i = i->next) { + DeviceProperty *prop = i->data; + + if (strcmp(prop->name, name) == 0) { + return prop; + } + } + + return NULL; +} + +void qdev_property_get(DeviceState *dev, Visitor *v, const char *name, + Error **errp) +{ + DeviceProperty *prop = qdev_property_find(dev, name); + + if (prop == NULL) { + error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name); + return; + } + + if (!prop->get) { + error_set(errp, QERR_PERMISSION_DENIED); + } else { + prop->get(dev, v, prop->opaque, name, errp); + } +} + +void qdev_property_set(DeviceState *dev, Visitor *v, const char *name, + Error **errp) +{ + DeviceProperty *prop = qdev_property_find(dev, name); + + if (prop == NULL) { + error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name); + return; + } + + if (!prop->set) { + error_set(errp, QERR_PERMISSION_DENIED); + } else { + prop->set(dev, prop->opaque, v, name, errp); + } +} + +const char *qdev_property_get_type(DeviceState *dev, const char *name, Error **errp) +{ + DeviceProperty *prop = qdev_property_find(dev, name); + + if (prop == NULL) { + error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name); + return NULL; + } + + return prop->type; +} diff --git a/hw/qdev.h b/hw/qdev.h index 36a4198..0f23677 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -5,6 +5,7 @@ #include "qemu-queue.h" #include "qemu-char.h" #include "qemu-option.h" +#include "qapi/qapi-visit-core.h" typedef struct Property Property; @@ -27,6 +28,42 @@ enum { DEV_NVECTORS_UNSPECIFIED = -1, }; +/** + * @DevicePropertyEtter - called when trying to get/set a property + * + * @dev the device that owns the property + * @v the visitor that contains the property data + * @opaque the opaque registered with the property + * @name the name of the property + * @errp a pointer to an Error that is filled if getting/setting fails. + */ +typedef void (DevicePropertyEtter)(DeviceState *dev, + Visitor *v, + void *opaque, + const char *name, + Error **errp); + +/** + * @DevicePropertyRelease - called when a property is removed from a device + * + * @dev the device that owns the property + * @name the name of the property + * @oapque the opaque registered with the property + */ +typedef void (DevicePropertyRelease)(DeviceState *dev, + const char *name, + void *opaque); + +typedef struct DeviceProperty +{ + gchar *name; + gchar *type; + DevicePropertyEtter *set; + DevicePropertyEtter *get; + DevicePropertyRelease *release; + void *opaque; +} DeviceProperty; + /* This structure should not be accessed directly. We declare it here so that it can be embedded in individual device state structures. */ struct DeviceState { @@ -45,6 +82,7 @@ struct DeviceState { QTAILQ_ENTRY(DeviceState) sibling; int instance_id_alias; int alias_required_for_version; + GSList *properties; }; typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent); @@ -329,4 +367,84 @@ char *qdev_get_fw_dev_path(DeviceState *dev); /* This is a nasty hack to allow passing a NULL bus to qdev_create. */ extern struct BusInfo system_bus_info; +/** + * @qdev_property_add - add a new property to a device + * + * @dev - the device to add a property to + * + * @name - the name of the property. This can contain any character except for + * a forward slash. In general, you should use hyphens '-' instead of + * underscores '_' when naming properties. + * + * @type - the type name of the property. This namespace is pretty loosely + * defined. Sub namespaces are constructed by using a prefix and then + * to angle brackets. For instance, the type 'virtio-net-pci' in the + * 'link' namespace would be 'link<virtio-net-pci>'. + * + * @get - the getter to be called to read a property. If this is NULL, then + * the property cannot be read. + * + * @set - the setter to be called to write a property. If this is NULL, then + * the property cannot be written. + * + * @release - called when the property is removed from the device. This is + * meant to allow a property to free its opaque upon device + * destruction. This may be NULL. + * + * @opaque - this is user data passed to @get, @set, and @release + * + * @errp - returns an error if this function fails + */ +void qdev_property_add(DeviceState *dev, const char *name, const char *type, + DevicePropertyEtter *get, DevicePropertyEtter *set, + DevicePropertyRelease *release, void *opaque, + Error **errp); + + +/** + * @qdev_property_get - reads a property from a device + * + * @dev - the device + * + * @v - the visitor that will receive the property value. This should be an + * Output visitor and the data will be written with @name as the name. + * + * @name - the name of the property + * + * @errp - returns an error if this function fails + */ +void qdev_property_get(DeviceState *dev, Visitor *v, const char *name, + Error **errp); + +/** + * @qdev_property_set - writes a property to a device + * + * @dev - the device + * + * @v - the visitor that will used to write the property value. This should be + * an Input visitor and the data will be first read with @name as the name + * and then written as the property value. + * + * @name - the name of the property + * + * @errp - returns an error if this function fails + */ +void qdev_property_set(DeviceState *dev, Visitor *v, const char *name, + Error **errp); + +/** + * @qdev_property_get_type - returns the type of a property + * + * @dev - the device + * + * @name - the name of the property + * + * @errp - returns an error if this function fails + * + * Returns: + * The type name of the property. + */ +const char *qdev_property_get_type(DeviceState *dev, const char *name, + Error **errp); + #endif diff --git a/qerror.c b/qerror.c index fdf62b9..dd0ee76 100644 --- a/qerror.c +++ b/qerror.c @@ -178,6 +178,10 @@ static const QErrorStringTable qerror_table[] = { .desc = "Could not open '%(filename)'", }, { + .error_fmt = QERR_PERMISSION_DENIED, + .desc = "Insufficient permission to perform this operation", + }, + { .error_fmt = QERR_PROPERTY_NOT_FOUND, .desc = "Property '%(device).%(property)' not found", }, diff --git a/qerror.h b/qerror.h index 2d3d43b..2b1d743 100644 --- a/qerror.h +++ b/qerror.h @@ -150,6 +150,9 @@ QError *qobject_to_qerror(const QObject *obj); #define QERR_OPEN_FILE_FAILED \ "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }" +#define QERR_PERMISSION_DENIED \ + "{ 'class': 'PermissionDenied', 'data': {} }" + #define QERR_PROPERTY_NOT_FOUND \ "{ 'class': 'PropertyNotFound', 'data': { 'device': %s, 'property': %s } }"
qdev properties are settable only during construction and static to classes. This isn't flexible enough for QOM. This patch introduces a property interface for qdev that provides dynamic properties that are tied to objects, instead of classes. These properties are Visitor based instead of string based too. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- hw/qdev.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++ hw/qdev.h | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qerror.c | 4 ++ qerror.h | 3 ++ 4 files changed, 224 insertions(+), 0 deletions(-)