| Message ID | 1458814461-5387-1-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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?
[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);
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.
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?
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:)
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
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.
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);
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(-)