| Message ID | 1429280557-8887-7-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 17/04/2015 16:22, Daniel P. Berrange wrote: > A QOM property can be parsed as enum using the visit_type_enum() > helper method, but this forces callers to use the more complex > generic object_property_add() method when registering it. It > also requires that users of that object have access to the > string map when they want to read the property value. > > This patch introduces a specialized object_property_add_enum() > method which simplifies the use of enum properties, so the > setters/getters directly get passed the int value. > > typedef enum { > MYDEV_TYPE_FROG, > MYDEV_TYPE_ALLIGATOR, > MYDEV_TYPE_PLATYPUS, > > MYDEV_TYPE_LAST > } MyDevType; > > Then provide a table of enum <-> string mappings > > static const char *const mydevtypemap[MYDEV_TYPE_LAST + 1] = { > [MYDEV_TYPE_FROG] = "frog", > [MYDEV_TYPE_ALLIGATOR] = "alligator", > [MYDEV_TYPE_PLATYPUS] = "platypus", > [MYDEV_TYPE_LAST] = NULL, > }; > > Assuming an object struct of > > typedef struct { > Object parent; > MyDevType devtype; > ...other fields... > } MyDev; > > The property can then be registered as follows: > > static int mydev_prop_get_devtype(Object *obj, > Error **errp G_GNUC_UNUSED) > { > MyDev *dev = MYDEV(obj); > > return dev->devtype; > } > > static void mydev_prop_set_devtype(Object *obj, > int value, > Error **errp G_GNUC_UNUSED) > { > MyDev *dev = MYDEV(obj); > > dev->endpoint = value; > } > > object_property_add_enum(obj, "devtype", > mydevtypemap, > mydev_prop_get_devtype, > mydev_prop_set_devtype, > NULL); > > Note there is no need to check the range of 'value' in > the setter, because the string->enum conversion code will > have already done that and reported an error as required. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > include/qom/object.h | 17 ++++++++++++++++ > qom/object.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 74 insertions(+) > > diff --git a/include/qom/object.h b/include/qom/object.h > index dfdba2f..3462821 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -1262,6 +1262,23 @@ void object_property_add_bool(Object *obj, const char *name, > Error **errp); > > /** > + * object_property_add_enum: > + * @obj: the object to add a property to > + * @name: the name of the property > + * @get: the getter or NULL if the property is write-only. > + * @set: the setter or NULL if the property is read-only > + * @errp: if an error occurs, a pointer to an area to store the error > + * > + * Add a enum property using getters/setters. This function will add a > + * property of type 'enum'. > + */ > +void object_property_add_enum(Object *obj, const char *name, > + const char * const *strings, > + int (*get)(Object *, Error **), > + void (*set)(Object *, int, Error **), > + Error **errp); > + > +/** > * object_property_add_tm: > * @obj: the object to add a property to > * @name: the name of the property > diff --git a/qom/object.c b/qom/object.c > index 2534398..543cc57 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -1607,6 +1607,63 @@ void object_property_add_bool(Object *obj, const char *name, > } > } > > +typedef struct EnumProperty { > + const char * const *strings; > + int (*get)(Object *, Error **); > + void (*set)(Object *, int, Error **); > +} EnumProperty; > + > +static void property_get_enum(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + EnumProperty *prop = opaque; > + int value; > + > + value = prop->get(obj, errp); > + visit_type_enum(v, &value, prop->strings, NULL, name, errp); > +} > + > +static void property_set_enum(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + EnumProperty *prop = opaque; > + int value; > + > + visit_type_enum(v, &value, prop->strings, NULL, name, errp); > + prop->set(obj, value, errp); > +} > + > +static void property_release_enum(Object *obj, const char *name, > + void *opaque) > +{ > + EnumProperty *prop = opaque; > + g_free(prop); > +} > + > +void object_property_add_enum(Object *obj, const char *name, > + const char * const *strings, > + int (*get)(Object *, Error **), > + void (*set)(Object *, int, Error **), > + Error **errp) > +{ > + Error *local_err = NULL; > + EnumProperty *prop = g_malloc0(sizeof(*prop)); > + > + prop->strings = strings; > + prop->get = get; > + prop->set = set; > + > + object_property_add(obj, name, "enum", > + get ? property_get_enum : NULL, > + set ? property_set_enum : NULL, > + property_release_enum, > + prop, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + g_free(prop); > + } > +} > + > typedef struct TMProperty { > void (*get)(Object *, struct tm *, Error **); > } TMProperty; > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
On 17/04/2015 16:56, Paolo Bonzini wrote: > > > On 17/04/2015 16:22, Daniel P. Berrange wrote: >> A QOM property can be parsed as enum using the visit_type_enum() >> helper method, but this forces callers to use the more complex >> generic object_property_add() method when registering it. It >> also requires that users of that object have access to the >> string map when they want to read the property value. >> >> This patch introduces a specialized object_property_add_enum() >> method which simplifies the use of enum properties, so the >> setters/getters directly get passed the int value. >> >> typedef enum { >> MYDEV_TYPE_FROG, >> MYDEV_TYPE_ALLIGATOR, >> MYDEV_TYPE_PLATYPUS, >> >> MYDEV_TYPE_LAST >> } MyDevType; >> >> Then provide a table of enum <-> string mappings >> >> static const char *const mydevtypemap[MYDEV_TYPE_LAST + 1] = { >> [MYDEV_TYPE_FROG] = "frog", >> [MYDEV_TYPE_ALLIGATOR] = "alligator", >> [MYDEV_TYPE_PLATYPUS] = "platypus", >> [MYDEV_TYPE_LAST] = NULL, >> }; >> >> Assuming an object struct of >> >> typedef struct { >> Object parent; >> MyDevType devtype; >> ...other fields... >> } MyDev; >> >> The property can then be registered as follows: >> >> static int mydev_prop_get_devtype(Object *obj, >> Error **errp G_GNUC_UNUSED) >> { >> MyDev *dev = MYDEV(obj); >> >> return dev->devtype; >> } >> >> static void mydev_prop_set_devtype(Object *obj, >> int value, >> Error **errp G_GNUC_UNUSED) >> { >> MyDev *dev = MYDEV(obj); >> >> dev->endpoint = value; >> } >> >> object_property_add_enum(obj, "devtype", >> mydevtypemap, >> mydev_prop_get_devtype, >> mydev_prop_set_devtype, >> NULL); On second thought (after seeing patch 7), please add a property type argument here. We lose introspection of enum property types otherwise. It's possible to use macros so that 'MyEnum' gets translated to two arguments '"MyEnum", MyEnum_lookup', but I don't think that's worth the obfuscation. Paolo >> Note there is no need to check the range of 'value' in >> the setter, because the string->enum conversion code will >> have already done that and reported an error as required. >> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> >> --- >> include/qom/object.h | 17 ++++++++++++++++ >> qom/object.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 74 insertions(+) >> >> diff --git a/include/qom/object.h b/include/qom/object.h >> index dfdba2f..3462821 100644 >> --- a/include/qom/object.h >> +++ b/include/qom/object.h >> @@ -1262,6 +1262,23 @@ void object_property_add_bool(Object *obj, const char *name, >> Error **errp); >> >> /** >> + * object_property_add_enum: >> + * @obj: the object to add a property to >> + * @name: the name of the property >> + * @get: the getter or NULL if the property is write-only. >> + * @set: the setter or NULL if the property is read-only >> + * @errp: if an error occurs, a pointer to an area to store the error >> + * >> + * Add a enum property using getters/setters. This function will add a >> + * property of type 'enum'. >> + */ >> +void object_property_add_enum(Object *obj, const char *name, >> + const char * const *strings, >> + int (*get)(Object *, Error **), >> + void (*set)(Object *, int, Error **), >> + Error **errp); >> + >> +/** >> * object_property_add_tm: >> * @obj: the object to add a property to >> * @name: the name of the property >> diff --git a/qom/object.c b/qom/object.c >> index 2534398..543cc57 100644 >> --- a/qom/object.c >> +++ b/qom/object.c >> @@ -1607,6 +1607,63 @@ void object_property_add_bool(Object *obj, const char *name, >> } >> } >> >> +typedef struct EnumProperty { >> + const char * const *strings; >> + int (*get)(Object *, Error **); >> + void (*set)(Object *, int, Error **); >> +} EnumProperty; >> + >> +static void property_get_enum(Object *obj, Visitor *v, void *opaque, >> + const char *name, Error **errp) >> +{ >> + EnumProperty *prop = opaque; >> + int value; >> + >> + value = prop->get(obj, errp); >> + visit_type_enum(v, &value, prop->strings, NULL, name, errp); >> +} >> + >> +static void property_set_enum(Object *obj, Visitor *v, void *opaque, >> + const char *name, Error **errp) >> +{ >> + EnumProperty *prop = opaque; >> + int value; >> + >> + visit_type_enum(v, &value, prop->strings, NULL, name, errp); >> + prop->set(obj, value, errp); >> +} >> + >> +static void property_release_enum(Object *obj, const char *name, >> + void *opaque) >> +{ >> + EnumProperty *prop = opaque; >> + g_free(prop); >> +} >> + >> +void object_property_add_enum(Object *obj, const char *name, >> + const char * const *strings, >> + int (*get)(Object *, Error **), >> + void (*set)(Object *, int, Error **), >> + Error **errp) >> +{ >> + Error *local_err = NULL; >> + EnumProperty *prop = g_malloc0(sizeof(*prop)); >> + >> + prop->strings = strings; >> + prop->get = get; >> + prop->set = set; >> + >> + object_property_add(obj, name, "enum", >> + get ? property_get_enum : NULL, >> + set ? property_set_enum : NULL, >> + property_release_enum, >> + prop, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + g_free(prop); >> + } >> +} >> + >> typedef struct TMProperty { >> void (*get)(Object *, struct tm *, Error **); >> } TMProperty; >> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > >
On Fri, Apr 17, 2015 at 05:01:24PM +0200, Paolo Bonzini wrote: > > > On 17/04/2015 16:56, Paolo Bonzini wrote: > > > > > > On 17/04/2015 16:22, Daniel P. Berrange wrote: > >> A QOM property can be parsed as enum using the visit_type_enum() > >> helper method, but this forces callers to use the more complex > >> generic object_property_add() method when registering it. It > >> also requires that users of that object have access to the > >> string map when they want to read the property value. > >> > >> This patch introduces a specialized object_property_add_enum() > >> method which simplifies the use of enum properties, so the > >> setters/getters directly get passed the int value. > >> > >> typedef enum { > >> MYDEV_TYPE_FROG, > >> MYDEV_TYPE_ALLIGATOR, > >> MYDEV_TYPE_PLATYPUS, > >> > >> MYDEV_TYPE_LAST > >> } MyDevType; > >> > >> Then provide a table of enum <-> string mappings > >> > >> static const char *const mydevtypemap[MYDEV_TYPE_LAST + 1] = { > >> [MYDEV_TYPE_FROG] = "frog", > >> [MYDEV_TYPE_ALLIGATOR] = "alligator", > >> [MYDEV_TYPE_PLATYPUS] = "platypus", > >> [MYDEV_TYPE_LAST] = NULL, > >> }; > >> > >> Assuming an object struct of > >> > >> typedef struct { > >> Object parent; > >> MyDevType devtype; > >> ...other fields... > >> } MyDev; > >> > >> The property can then be registered as follows: > >> > >> static int mydev_prop_get_devtype(Object *obj, > >> Error **errp G_GNUC_UNUSED) > >> { > >> MyDev *dev = MYDEV(obj); > >> > >> return dev->devtype; > >> } > >> > >> static void mydev_prop_set_devtype(Object *obj, > >> int value, > >> Error **errp G_GNUC_UNUSED) > >> { > >> MyDev *dev = MYDEV(obj); > >> > >> dev->endpoint = value; > >> } > >> > >> object_property_add_enum(obj, "devtype", > >> mydevtypemap, > >> mydev_prop_get_devtype, > >> mydev_prop_set_devtype, > >> NULL); > > On second thought (after seeing patch 7), please add a property type > argument here. We lose introspection of enum property types otherwise. Can you give me a pointer to where we lose introspection ? The hostmem code just registers its 'policy' property with a type name of 'str', which I've just changed to 'enum' in patch 7, so I didn't think I was regressing anything. Is there some QMP command which you think looses the info that I can verify. > It's possible to use macros so that 'MyEnum' gets translated to two > arguments '"MyEnum", MyEnum_lookup', but I don't think that's worth the > obfuscation. Regards, Daniel
On 17/04/2015 17:11, Daniel P. Berrange wrote: > > On second thought (after seeing patch 7), please add a property type > > argument here. We lose introspection of enum property types otherwise. > > Can you give me a pointer to where we lose introspection ? The hostmem > code just registers its 'policy' property with a type name of 'str', That was already a bug. :( It should have been registered as "HostMemPolicy". See for example: $ qemu-system-x86_64 -device ide-hd,help ide-hd.bootindex=int32 ide-hd.unit=uint32 ide-hd.bios-chs-trans=BiosAtaTranslation (Logical CHS translation algorithm, auto/none/lba/large/rechs) ... > which I've just changed to 'enum' in patch 7, so I didn't think I was > regressing anything. Is there some QMP command which you think looses > the info that I can verify. The qom-list command returns a list of property names and types. Paolo
On Fri, Apr 17, 2015 at 05:19:31PM +0200, Paolo Bonzini wrote: > > > On 17/04/2015 17:11, Daniel P. Berrange wrote: > > > On second thought (after seeing patch 7), please add a property type > > > argument here. We lose introspection of enum property types otherwise. > > > > Can you give me a pointer to where we lose introspection ? The hostmem > > code just registers its 'policy' property with a type name of 'str', > > That was already a bug. :( It should have been registered as > "HostMemPolicy". See for example: Ok, next time I'll probably send a patch which fixes that bug independantly and then does the conversion, in case we want the fix on stable too. > $ qemu-system-x86_64 -device ide-hd,help > ide-hd.bootindex=int32 > ide-hd.unit=uint32 > ide-hd.bios-chs-trans=BiosAtaTranslation (Logical CHS translation algorithm, auto/none/lba/large/rechs) > ... > > > which I've just changed to 'enum' in patch 7, so I didn't think I was > > regressing anything. Is there some QMP command which you think looses > > the info that I can verify. > > The qom-list command returns a list of property names and types. Thanks, will look at that. Regards, Daniel
diff --git a/include/qom/object.h b/include/qom/object.h index dfdba2f..3462821 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -1262,6 +1262,23 @@ void object_property_add_bool(Object *obj, const char *name, Error **errp); /** + * object_property_add_enum: + * @obj: the object to add a property to + * @name: the name of the property + * @get: the getter or NULL if the property is write-only. + * @set: the setter or NULL if the property is read-only + * @errp: if an error occurs, a pointer to an area to store the error + * + * Add a enum property using getters/setters. This function will add a + * property of type 'enum'. + */ +void object_property_add_enum(Object *obj, const char *name, + const char * const *strings, + int (*get)(Object *, Error **), + void (*set)(Object *, int, Error **), + Error **errp); + +/** * object_property_add_tm: * @obj: the object to add a property to * @name: the name of the property diff --git a/qom/object.c b/qom/object.c index 2534398..543cc57 100644 --- a/qom/object.c +++ b/qom/object.c @@ -1607,6 +1607,63 @@ void object_property_add_bool(Object *obj, const char *name, } } +typedef struct EnumProperty { + const char * const *strings; + int (*get)(Object *, Error **); + void (*set)(Object *, int, Error **); +} EnumProperty; + +static void property_get_enum(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) +{ + EnumProperty *prop = opaque; + int value; + + value = prop->get(obj, errp); + visit_type_enum(v, &value, prop->strings, NULL, name, errp); +} + +static void property_set_enum(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) +{ + EnumProperty *prop = opaque; + int value; + + visit_type_enum(v, &value, prop->strings, NULL, name, errp); + prop->set(obj, value, errp); +} + +static void property_release_enum(Object *obj, const char *name, + void *opaque) +{ + EnumProperty *prop = opaque; + g_free(prop); +} + +void object_property_add_enum(Object *obj, const char *name, + const char * const *strings, + int (*get)(Object *, Error **), + void (*set)(Object *, int, Error **), + Error **errp) +{ + Error *local_err = NULL; + EnumProperty *prop = g_malloc0(sizeof(*prop)); + + prop->strings = strings; + prop->get = get; + prop->set = set; + + object_property_add(obj, name, "enum", + get ? property_get_enum : NULL, + set ? property_set_enum : NULL, + property_release_enum, + prop, &local_err); + if (local_err) { + error_propagate(errp, local_err); + g_free(prop); + } +} + typedef struct TMProperty { void (*get)(Object *, struct tm *, Error **); } TMProperty;
A QOM property can be parsed as enum using the visit_type_enum() helper method, but this forces callers to use the more complex generic object_property_add() method when registering it. It also requires that users of that object have access to the string map when they want to read the property value. This patch introduces a specialized object_property_add_enum() method which simplifies the use of enum properties, so the setters/getters directly get passed the int value. typedef enum { MYDEV_TYPE_FROG, MYDEV_TYPE_ALLIGATOR, MYDEV_TYPE_PLATYPUS, MYDEV_TYPE_LAST } MyDevType; Then provide a table of enum <-> string mappings static const char *const mydevtypemap[MYDEV_TYPE_LAST + 1] = { [MYDEV_TYPE_FROG] = "frog", [MYDEV_TYPE_ALLIGATOR] = "alligator", [MYDEV_TYPE_PLATYPUS] = "platypus", [MYDEV_TYPE_LAST] = NULL, }; Assuming an object struct of typedef struct { Object parent; MyDevType devtype; ...other fields... } MyDev; The property can then be registered as follows: static int mydev_prop_get_devtype(Object *obj, Error **errp G_GNUC_UNUSED) { MyDev *dev = MYDEV(obj); return dev->devtype; } static void mydev_prop_set_devtype(Object *obj, int value, Error **errp G_GNUC_UNUSED) { MyDev *dev = MYDEV(obj); dev->endpoint = value; } object_property_add_enum(obj, "devtype", mydevtypemap, mydev_prop_get_devtype, mydev_prop_set_devtype, NULL); Note there is no need to check the range of 'value' in the setter, because the string->enum conversion code will have already done that and reported an error as required. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- include/qom/object.h | 17 ++++++++++++++++ qom/object.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+)