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

[Markus Armbruster]

Cao jin <address@hidden> 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?