On Thu, Sep 22, 2016 at 10:36:45AM +0200, Markus Armbruster wrote: > "Lin Ma" <l...@suse.com> writes: > > >>>> Markus Armbruster <arm...@redhat.com> 2016/9/20 星期二 上午 1:13 >>> > >>Andreas Färber <afaer...@suse.de> writes: > > Saving acceptable values of enumeration types into member description > > of ObjectProperty is a good idea. > > > > The member description of ObjectProperty instance of any user-creatable > > object is NULL so far, > > It's null until set with object_property_set_description(). We do that > for a few properties, and may do it more. > > > If I use object_property_set_description() to fill > > the > > acceptable values of enumeration type into the description in function > > object_property_add_enum and object_class_property_add_enum, Then I > > can use this description to judge whether a ObjectProperty instance' type > > is enumeration or not in function user_creatable_help_func. In this case, If > > member description is not NULL, it means this ObjectProperty instance is > > an enumeration. > > No. If you need to decide in user_creatable_help_func() whether a > property has an enumeration type, something's wrong at the > infrastructure level. > > > Any suggestions? > > Yes: > > >>When it's null we could still fall back to a description of the type. > >>Does such a thing exist? Enumeration types could provide one listing > >>their values. > > Don't make up a description in user_creatable_help_func(), improve the > description infrastructure and its use so you get more useful ones > there. > > The existing description infrastructure is just Property member > description and object_property_set_description(). Rarely used, so > description is generally null. > > Calling object_property_set_description() more often could be helpful, > but to come up with a sensible description string, you need to know what > the property does. Needs to be left to people actually familiar with > the objects. > > Aside: historically, we add properties to *instances*. All the property > meta-data gets duplicated for every instance, including property > descriptions. This is more flexible than adding the meta-data to the > class. The flexibility is rarely needed, but the price in wasted memory > is always paid. Only since commit 16bf7f5, we can add it to classes. > Adding lots of helpful property descriptions would increase the cost of > instance properties further. > > What you could perhaps do is adding a *type* description. For enums, > that would show the set of acceptable values. Then if the property has > no description, fall back to the description of its type.
I don't think we need to invent anything new. We can use the existing property description facility and auto-generate the string containing the permitted values thus: diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index dabc42e..0446839 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -202,9 +202,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self._btin += gen_enum(name, values, prefix) if do_builtins: self.defn += gen_enum_lookup(name, values, prefix) + self._btin += gen_enum_value_str(name, values) else: self._fwdecl += gen_enum(name, values, prefix) self.defn += gen_enum_lookup(name, values, prefix) + self._fwdecl += gen_enum_value_str(name, values) def visit_array_type(self, name, info, element_type): if isinstance(element_type, QAPISchemaBuiltinType): diff --git a/scripts/qapi.py b/scripts/qapi.py index 21bc32f..d11c414 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1649,6 +1649,15 @@ const char *const %(c_name)s_lookup[] = { return ret +def gen_enum_value_str(name, values): + return mcgen(''' + +#define %(c_name)s_value_str "%(value_str)s" +''', + c_name=c_name(name), + value_str=", ".join(["'%s'" % c for c in values])) + + def gen_enum(name, values, prefix=None): # append automatically generated _MAX value enum_values = values + ['_MAX'] Now, when registering an enum property do something like this: object_class_property_add_enum(oc, "format", "QCryptoSecretFormat", QCryptoSecretFormat_lookup, qcrypto_secret_prop_get_format, qcrypto_secret_prop_set_format, NULL); object_class_property_set_description(oc, "format", "Data format, one of " QCryptoSecretFormat_value_str, &error_abort); So that description ends up being "Data format, one of 'base64', 'plain'" It would be nicer if the object_property_add_* methods just accepted a 'const char *description' too. As well as being more efficient that a second method call which has to search for the ObjectProperty struct, potentially reporting errors, it will also encourage people to actually provide descriptions Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|