diff mbox

答复： Re: [PATCH v2] object: Add 'help' option for all available backends and properties

Message ID 20160922130136.GO352@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé Sept. 22, 2016, 1:01 p.m. UTC 
On Thu, Sep 22, 2016 at 10:36:45AM +0200, Markus Armbruster wrote:
> "Lin Ma" <lma@suse.com> writes:
> 
> >>>> Markus Armbruster <armbru@redhat.com> 2016/9/20 星期二 上午 1:13 >>>
> >>Andreas Färber <afaerber@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:



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
diff mbox

Patch

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']