[Qemu-devel] [PATCH] qdev property: cleanup

[Qemu-devel] [PATCH] qdev property: cleanup

[Cao jin]

include:
1. remove unnecessary declaration of static function
2. fix inconsistency between comment and function name, and typo OOM->QOM
2. update comment of function

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
A question about legacy property: I don`t see legacy property is really
used in the code, so, why still add legacy property to object?

since it has no maintainer, so the cc list is by intuition...

 hw/core/qdev.c               | 14 ++++++--------
 include/hw/qdev-properties.h |  4 ++--
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index db41aa1..743b71b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -58,9 +58,6 @@ const char *qdev_fw_name(DeviceState *dev)
     return object_get_typename(OBJECT(dev));
 }
 
-static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
-                                     Error **errp);
-
 static void bus_remove_child(BusState *bus, DeviceState *child)
 {
     BusChild *kid;
@@ -908,12 +905,12 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v,
 }
 
 /**
- * @qdev_add_legacy_property - adds a legacy property
+ * @qdev_property_add_legacy - adds a legacy property
  *
  * Do not use this is new code!  Properties added through this interface will
  * be given names and types in the "legacy" namespace.
  *
- * Legacy properties are string versions of other OOM properties.  The format
+ * Legacy properties are string versions of other QOM properties.  The format
  * of the string depends on the property type.
  */
 static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
@@ -937,10 +934,11 @@ static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
 }
 
 /**
- * @qdev_property_add_static - add a @Property to a device.
+ * @qdev_property_add_static
+ * add a QOM property to @dev via its qdev Property @prop
  *
- * Static properties access data in a struct.  The actual type of the
- * property and the field depends on the property type.
+ * Static properties access data in a struct. The actual type of ObjectProperty
+ * and the struct field depends on the @qtype type.
  */
 void qdev_property_add_static(DeviceState *dev, Property *prop,
                               Error **errp)
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0586cac..8c4481b 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -197,8 +197,8 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
 /**
- * @qdev_property_add_static - add a @Property to a device referencing a
- * field in a struct.
+ * @qdev_property_add_static - add a ObjectProperty to @dev via its qdev
+ * property @prop, referencing a field in the struct.
  */
 void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH] qdev property: cleanup

[Paolo Bonzini]

On 24/03/2016 11:14, Cao jin wrote:
> include:
> 1. remove unnecessary declaration of static function
> 2. fix inconsistency between comment and function name, and typo OOM->QOM
> 2. update comment of function
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> A question about legacy property: I don`t see legacy property is really
> used in the code, so, why still add legacy property to object?

It's used here:

static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
                             int indent)
{
    if (!props)
        return;
    for (; props->name; props++) {
        Error *err = NULL;
        char *value;
        char *legacy_name = g_strdup_printf("legacy-%s", props->name);
        if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
            value = object_property_get_str(OBJECT(dev), legacy_name, &err);
        } else {
            value = object_property_print(OBJECT(dev), props->name, true, &err);
        }
        g_free(legacy_name);

        if (err) {
            error_free(err);
            continue;
        }
        qdev_printf("%s = %s\n", props->name,
                    value && *value ? value : "<null>");
        g_free(value);
    }
}

Paolo

Re: [Qemu-devel] [PATCH] qdev property: cleanup

[Cao jin]

On 03/24/2016 07:25 PM, Paolo Bonzini wrote:
>
>
>> A question about legacy property: I don`t see legacy property is really
>> used in the code, so, why still add legacy property to object?
>
> It's used here:
>
> static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
>                               int indent)

Thanks Paolo, I saw here, and then I thought the property is added and 
just be printed out, seems for no real benefit("really used"). Now I 
think I see where I was wrong. Thanks.

BTW, is this patch right?
-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH] qdev property: cleanup

[Markus Armbruster]

[Wasn't delivered correctly by eggs.gnu.org, resending]

The subsystem tag should be just "qdev:".  Suggest "qdev: Clean up
around properties".

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> include:
> 1. remove unnecessary declaration of static function
> 2. fix inconsistency between comment and function name, and typo OOM->QOM
> 2. update comment of function
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> A question about legacy property: I don`t see legacy property is really
> used in the code, so, why still add legacy property to object?
>
> since it has no maintainer, so the cc list is by intuition...
>
>  hw/core/qdev.c               | 14 ++++++--------
>  include/hw/qdev-properties.h |  4 ++--
>  2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index db41aa1..743b71b 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -58,9 +58,6 @@ const char *qdev_fw_name(DeviceState *dev)
>      return object_get_typename(OBJECT(dev));
>  }
>  
> -static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
> -                                     Error **errp);
> -
>  static void bus_remove_child(BusState *bus, DeviceState *child)
>  {
>      BusChild *kid;
> @@ -908,12 +905,12 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v,
>  }
>  
>  /**
> - * @qdev_add_legacy_property - adds a legacy property
> + * @qdev_property_add_legacy - adds a legacy property

Style#1:

    function_name - headline

We generally use the imperative mood: "add a legacy property".

>   *
>   * Do not use this is new code!  Properties added through this interface will
>   * be given names and types in the "legacy" namespace.
>   *
> - * Legacy properties are string versions of other OOM properties.  The format
> + * Legacy properties are string versions of other QOM properties.  The format
>   * of the string depends on the property type.
>   */
>  static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
> @@ -937,10 +934,11 @@ static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
>  }
>  
>  /**
> - * @qdev_property_add_static - add a @Property to a device.
> + * @qdev_property_add_static
> + * add a QOM property to @dev via its qdev Property @prop

Style#2:

    function_name
    headline

>   *
> - * Static properties access data in a struct.  The actual type of the
> - * property and the field depends on the property type.
> + * Static properties access data in a struct. The actual type of ObjectProperty
> + * and the struct field depends on the @qtype type.
>   */

Not sure this part is an improvement.  What's wrong with the current text?

>  void qdev_property_add_static(DeviceState *dev, Property *prop,
>                                Error **errp)
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 0586cac..8c4481b 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -197,8 +197,8 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>                                      Property *prop, const char *value);
>  
>  /**
> - * @qdev_property_add_static - add a @Property to a device referencing a
> - * field in a struct.
> + * @qdev_property_add_static - add a ObjectProperty to @dev via its qdev
> + * property @prop, referencing a field in the struct.
>   */

Style#3:

    function_name - sentence spanning multiple
    lines.

Pick one style and stick to it.

The rest of qdev-properties.h appears to use GTK-Doc style (which I
personally dislike, but that doesn't matter here).  See
https://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html

If both declaration and definition have a comment, as they do here, they
should match exactly.

>  void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);

Re: [Qemu-devel] [PATCH] qdev property: cleanup

[Cao jin]

On 04/08/2016 07:17 PM, Markus Armbruster wrote:

>> - * Static properties access data in a struct.  The actual type of the
>> - * property and the field depends on the property type.
>> + * Static properties access data in a struct. The actual type of ObjectProperty
>> + * and the struct field depends on the @qtype type.
>>    */
>
> Not sure this part is an improvement.  What's wrong with the current text?
>

In a word: little hard for newbies like me to understand. (I think 
comments are for newbies). see my feeling about the comment:

As per my understanding, property has 2 kinds, former qdev property and 
the latest QOM property. For me, the original description is too 
ambiguous about 'property'.

original: *The actual type of the property and the field depends on the 
property type*

Using two same word 'property' is ambiguous and hard for newbie to 
distinguish. The 1st 'property' should mean a QOM property. and the 2nd 
'property', I think the original author`s meaning is: qdev property. 
But, what is the qdev property *type*? cannot find 'type' field in the 
definition except a *qtype*

struct Property {
     const char   *name;
     PropertyInfo *info;
     ptrdiff_t    offset;
     uint8_t      bitnr;
     QType        qtype;
     int64_t      defval;
     int          arrayoffset;
     PropertyInfo *arrayinfo;
     int          arrayfieldsize;
};

And *the actual type of the field* depends on the qtype, take bitmap 
field for example, bitmap field in a structure is always a *int*, but 
when convert to QOM property, it is treated as a *bool*, see 
DEFINE_PROP_BIT, DEFINE_PROP_BIT64, its qtype are QTYPE_QBOOL.

But I am little confused also now, I think my modification isn`t perfect
1. see how qdev_property_add_static invoke object_property_add, it pass 
prop->info->name as its QOM property type

2. when structure field is enum, QOM property will treat it as 
string(not depends on qtype now), see code:

else if (prop->info->enum_table) {
     object_property_set_str(obj, prop->info->enum_table[prop->defval],
                             prop->name, &error_abort);

I will do more analyse before v2.

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH] qdev property: cleanup

[Markus Armbruster]

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> On 04/08/2016 07:17 PM, Markus Armbruster wrote:
>
>>> - * Static properties access data in a struct.  The actual type of the
>>> - * property and the field depends on the property type.
>>> + * Static properties access data in a struct. The actual type of ObjectProperty
>>> + * and the struct field depends on the @qtype type.
>>>    */
>>
>> Not sure this part is an improvement.  What's wrong with the current text?
>>
>
> In a word: little hard for newbies like me to understand. (I think
> comments are for newbies). see my feeling about the comment:
>
> As per my understanding, property has 2 kinds, former qdev property
> and the latest QOM property.

I'm afraid I don't really understand this stuff anymore myself.  The
code tries to explain itself in comments, but that's too scattered
reading to be of much help when you try to figure out how things fit
together.  Perhaps a usable overview is buried somewhere in one of
Andreas's KVM Forum talks on the subject.  We really need an
introduction and overview of QOM and its major applications such as qdev
in docs/.

Here's my best guess how things work.  Paolo, please correct mistakes.

Before QOM, qdev properties were fully static: defined in an array
member of (static) DeviceInfo.

QOM properties are dynamic: defined in code.

QOM replaced the static DeviceInfo.  The static qdev property arrays
remain, but they're now stored into their DeviceClass's member props
dynamically, by the TypeInfo's class_init() method.

The qdev properties so stored get mapped to QOM properties in
device_initfn().  Two, in fact: a legacy property and a static property.
qdev_property_add_legacy() converts to the former, and
qdev_property_add_static() converts to the latter.

I don't understand the difference between "legacy" and "static" well
enough to explain it.  I also don't really understand what other kinds
of QOM properties exist.

>                              For me, the original description is too
> ambiguous about 'property'.
>
> original: *The actual type of the property and the field depends on
> the property type*
>
> Using two same word 'property' is ambiguous and hard for newbie to
> distinguish. The 1st 'property' should mean a QOM property. and the
> 2nd 'property', I think the original author`s meaning is: qdev
> property.

I agree the wording of the comment is suboptimal.

The thing being added is a static QOM property.  Its type and so forth
are taken from the qdev property @prop.

>           But, what is the qdev property *type*? cannot find 'type'
> field in the definition except a *qtype*
>
> struct Property {
>     const char   *name;
>     PropertyInfo *info;
>     ptrdiff_t    offset;
>     uint8_t      bitnr;
>     QType        qtype;
>     int64_t      defval;
>     int          arrayoffset;
>     PropertyInfo *arrayinfo;
>     int          arrayfieldsize;
> };
>
> And *the actual type of the field* depends on the qtype, take bitmap
> field for example, bitmap field in a structure is always a *int*, but
> when convert to QOM property, it is treated as a *bool*, see
> DEFINE_PROP_BIT, DEFINE_PROP_BIT64, its qtype are QTYPE_QBOOL.
>
> But I am little confused also now, I think my modification isn`t perfect
> 1. see how qdev_property_add_static invoke object_property_add, it
> pass prop->info->name as its QOM property type
>
> 2. when structure field is enum, QOM property will treat it as
> string(not depends on qtype now), see code:
>
> else if (prop->info->enum_table) {
>     object_property_set_str(obj, prop->info->enum_table[prop->defval],
>                             prop->name, &error_abort);
>
> I will do more analyse before v2.

Here's my try rewording the comment (sticking to GTK-Doc conventions
even though I dislike them):

/**
 * @qdev_property_add_static:
 * @dev: Device to add the property to
 * @prop: The qdev property definition
 * @err: location to store error information
 *
 * Add a static QOM property to @dev for qdev property @prop.
 * On error, store error in @errp.
 */

Paolo, is it correct?

Cao jin, is it an improvement?

Re: [Qemu-devel] [PATCH] qdev property: cleanup

[Cao jin]

On 04/12/2016 04:20 PM, Markus Armbruster wrote:

>
> Here's my try rewording the comment (sticking to GTK-Doc conventions
> even though I dislike them):
>
> /**
>   * @qdev_property_add_static:
>   * @dev: Device to add the property to
>   * @prop: The qdev property definition
>   * @err: location to store error information
>   *
>   * Add a static QOM property to @dev for qdev property @prop.
>   * On error, store error in @errp.
>   */
>
> Paolo, is it correct?
>
> Cao jin, is it an improvement?
>

Yes, better, save the unnecessary pains to analyse the property type for 
me:)

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH] qdev property: cleanup

[Paolo Bonzini]

On 12/04/2016 10:20, Markus Armbruster wrote:
> QOM replaced the static DeviceInfo.  The static qdev property arrays
> remain, but they're now stored into their DeviceClass's member props
> dynamically, by the TypeInfo's class_init() method.
> 
> The qdev properties so stored get mapped to QOM properties in
> device_initfn().  Two, in fact: a legacy property and a static property.
> qdev_property_add_legacy() converts to the former, and
> qdev_property_add_static() converts to the latter.

So far so good. :)

> I don't understand the difference between "legacy" and "static" well
> enough to explain it.  I also don't really understand what other kinds
> of QOM properties exist.

The "legacy" properties are needed for "info qtree".  For example, a PCI
address "01.0" is the number 8 in the normal property, and the string
"01.0" in the legacy property.

The normal property does accept a string, and converts it into the
number, but if you want the "human-readable" string back you need to use
the legacy properties.

>> original: *The actual type of the property and the field depends on
>> the property type*
>>
>> Using two same word 'property' is ambiguous and hard for newbie to
>> distinguish. The 1st 'property' should mean a QOM property. and the
>> 2nd 'property', I think the original author`s meaning is: qdev
>> property.

The type of the QOM property depends on the PropertyInfo and it's stored
in prop->info->name.

QType is, generally speaking, a JSON type name (int, bool, string, list,
dictionary).  Instead, the QOM property type is more complex; it can be:
1) a QAPI type name 2) link<QOM-TYPE> 3) child<QOM-TYPE>.

QAPI type names in turns are a superset of JSON type names.  For example
the QType for an enumeration could be QTYPE_QSTRING (yes, it has
prop->qtype equal to QTYPE_QINT now, but that's an accident).  The QAPI
type name instead could be "LostTickPolicy".  In the future, the latter
would also be introspectable through the "query-qmp-schema".

There is no reason for prop->qtype to be QTYPE_QINT in the case of a
string.  It would be easy to fix.  For example you could remove
prop->qtype and add a new prop->info->qtype that contains one of
QTYPE_QINT, QTYPE_QBOOL or, for enumerations, QTYPE_QSTRING.

> I agree the wording of the comment is suboptimal.
> 
> The thing being added is a static QOM property.  Its type and so forth
> are taken from the qdev property @prop.
> 
>>           But, what is the qdev property *type*? cannot find 'type'
>> field in the definition except a *qtype*

It is prop->info->name: PropertyInfo is a description of the property,
and the name of the PropertyInfo becomes the type of the QOM property.

>> struct Property {
>>     const char   *name;
>>     PropertyInfo *info;
>>     ptrdiff_t    offset;
>>     uint8_t      bitnr;
>>     QType        qtype;
>>     int64_t      defval;
>>     int          arrayoffset;
>>     PropertyInfo *arrayinfo;
>>     int          arrayfieldsize;
>> };
>>
>> And *the actual type of the field* depends on the qtype

It doesn't depend on the qtype.  It is stored there for convenience.

> Here's my try rewording the comment (sticking to GTK-Doc conventions
> even though I dislike them):
> 
> /**
>  * @qdev_property_add_static:
>  * @dev: Device to add the property to
>  * @prop: The qdev property definition
>  * @err: location to store error information
>  *
>  * Add a static QOM property to @dev for qdev property @prop.
>  * On error, store error in @errp.
>  */
> 
> Paolo, is it correct?

Yes, it is.  You can add also "The type of the QOM property is derived
from prop->info", which is what Cao Jin really wanted to convey in his
patch.

Paolo

Re: [Qemu-devel] [PATCH] qdev property: cleanup

[Cao jin]

On 04/15/2016 06:06 PM, Paolo Bonzini wrote:
>

>> Here's my try rewording the comment (sticking to GTK-Doc conventions
>> even though I dislike them):
>>
>> /**
>>   * @qdev_property_add_static:
>>   * @dev: Device to add the property to
>>   * @prop: The qdev property definition
>>   * @err: location to store error information
>>   *
>>   * Add a static QOM property to @dev for qdev property @prop.
>>   * On error, store error in @errp.
>>   */
>>
>> Paolo, is it correct?
>
> Yes, it is.  You can add also "The type of the QOM property is derived
> from prop->info", which is what Cao Jin really wanted to convey in his
> patch.
>
> Paolo
>

Thanks Paolo for your explanation, much useful info for me:)

I will collect Markus`s rewording and your suggestion into v2 later.

-- 
Yours Sincerely,

Cao jin