Re: [PATCH 00/54] qom/qdev: associate properties with QAPI schema types

[Markus Armbruster via qemu development <qemu-devel@nongnu.org>]

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 <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>