I started writing this reply right when I saw the series, i.e. an embarrassingly long time ago. Then stuff happened, some of it good, some bad, this review got crowded out, and my memory purged.
I sincerely apologize for the massive delay. Naturally, this needs a rebase now. But let me try to review it as is. Marc-André Lureau <[email protected]> writes: > Hi, > > This series connects QOM and qdev properties to the QAPI type system, so > that management tools can discover the QAPI schema type of each object > property. Sounds lovely! > Today, QOM properties carry a free-form "type" string ("bool", "uint32", > "OnOffAuto", ..) that has no formal link to the QAPI schema exposed by > query-qmp-schema. Management tools must maintain ad-hoc mappings or rely > on naming conventions to figure out how to interpret property values. Correct. Before I dive into individual patches, let me try to work out what the series does as a whole. > This series adds: > - A new QAPITypeInfo struct that pairs a property with its QAPI schema > type name, enum lookup table, and list-element type. Peeking at the code, I see that ObjectProperty gains a member @qapi_type pointing to its QAPITypeInfo. It is null when the ObjectProperty doesn't have a QAPI type. If it's non-null, then ObjectProperty members @name and @type are redundant with qapi_type.name and .type. Correct? Would "every ObjectProperty has a QAPI type" be a reasonable goal for the future? > > - A QAPI code generator (qapi-type-infos) that emits a QAPITypeInfo > instance for every schema-defined type, including the mapping > between internal C names and the schema name visible to clients. Peeking at the code, I find: * The type typedef struct QAPITypeInfo { const char *name; const char *schema_name; const QEnumLookup *lookup; const struct QAPITypeInfo *list; } QAPITypeInfo; * A T_type_info for each QAPI type T, including built-in types. * T_type_info member @name is T's QAPI name, i.e. "T". * T_type_info member @schema_name is T's masked name used in query-qmp-schema output, null when T is elided there. * T_type_info member @list points to TList_type_info when that exists, else it's null. * T_type_info member @lookup points to T_lookup when T is an enum, else it's null. Correct? > - A "qapi-type" field in the ObjectPropertyInfo and > ObjectPropertyValue QMP structs, populated from the QAPITypeInfo > when present giving clients a cross-reference into query-qmp-schema > output. To be precise: when ObjectPropertyInfo member @type is "T", then member @qapi-type is T_type_info.qapi-type. Correct? If .qapi-type is non-null, you can use it to look up precise type information via QAPI introspection, i.e. query-qmp-schema. Correct? Possible problem: query-qmp-schema covers only types that are actually used in QMP. But the above technique additionally wants QOM property types. I haven't checked what your series does about this, if anything. ObjectPropertyInfo is only used with QMP command handlers. It is computed from ObjectProperty. > - Conversion of all PropertyInfo definitions from the old > .type/.enum_table strings to the new .qapi_type pointer. The above is QOM, this is qdev. Like ObjectProperty, PropertyInfo gains a member @qapi_type pointing to its QAPITypeInfo. Howver, this one cannot be null. PropertyInfo members @type and @enum_table are dropped, because they are redundant with qapi_type.type and .lookup. Correct? > - Replacement of the generic qdev_prop_array with typed per-element > array PropertyInfos, removing the arrayinfo/arrayfieldsize > indirection from struct Property. Before the series, an array-valued Property's @info member is @qdev_prop_array. @qdev_prop_array provides no information on the array elements. Instead, Property member @arrayinfo points to the PropertyInfo for the elements, and @arrayfieldsize is the size of an element. Your series makes PropertyInfo array-capable: new members @element_info and @element_size are the elements' PropertyInfo and size. It then adds a proper PropertyInfo for each such property, and drops @qdev_prop_array. Correct? This could perhaps be spun out and merged separately to reduce the size of future respins. Not mentioned: - New QOM property creation functions for creating properties of QAPI type. These take a QAPITypeInfo. - Convert some properties to use them. > - Removal of the deprecated PropertyInfo.type and .enum_table fields, > and of the old object_property_add_enum/add_tm APIs. > > Along the way, a few pre-existing type mismatches in property > definitions are fixed, the "struct tm" RTC property is replaced with a > proper QAPI StructTm type etc. Introducing more specific types or a > "typedef" to QAPI could help provide better associated type informations > than plain "str" in many cases, for example. Examples? > Comments welcome! > > Signed-off-by: Marc-André Lureau <[email protected]>
