Re: [Qemu-devel] qapi: What does "'gen': false" actually do, and when should I use it?

[Markus Armbruster]

Updating for future directions explored in "[PATCH RFC v2 00/47] qapi:
QMP introspection".

Markus Armbruster <address@hidden> writes:

> Two parts: 1. what does it do, and 2. when to use it.
>
>
> Part 1: What does it do?
>
> qapi-code-gen.txt explains it thus:
>
>     In rare cases, QAPI cannot express a type-safe representation of a
>     corresponding Client JSON Protocol command.  In these cases, if the
>     command expression includes the key 'gen' with boolean value false,
>     then the 'data' or 'returns' member that intends to bypass generated
>     type-safety and do its own manual validation should use an inline
>     dictionary definition, with a value of '**' rather than a valid type
>     name for the keys that the generated code will not validate.  Please
>     try to avoid adding new commands that rely on this, and instead use
>     type-safe unions.  For an example of bypass usage:
>
>      { 'command': 'netdev_add',
>        'data': {'type': 'str', 'id': 'str', '*props': '**'},
>        'gen': false }

With 'gen': false, 'data' and 'returns' aren't used by anything.  Good,
because this 'data' is actively misleading: we don't have an optional
'props' argument with "a list of properties" (quoting qapi-schema.json),
we have type-dependent property arguments that aren't specified here!

Example:

    { "execute": "netdev_add",
      "arguments": { "type": "user", "id": "netdev1",
                     "dnssearch": "example.org" } }

Incorrect example:

    { "execute": "netdev_add",
      "arguments": { "type": "user", "id": "netdev1",
                     "props": { "dnssearch": "example.org" } } }

"[PATCH RFC v2 42/47] qapi-schema: Fix up misleading specification of
netdev_add" deletes the misleading '*props': '**', and fixes up the
comment.

> I'm afraid that doesn't really answer the "what does it actually do?"
> question, so let's examine an example.  netdev_add isn't the best choice
> because it doesn't return anything, so let's use qom-get and qom-list.
>
> { 'command': 'qom-list',
>   'data': { 'path': 'str' },
>   'returns': [ 'ObjectPropertyInfo' ] }
>
> { 'command': 'qom-get',
>   'data': { 'path': 'str', 'property': 'str' },
>   'returns': '**',
>   'gen': false }
>
> qapi-commands.py generates qmp-marshal.c and qmp-commands.h.  For
> qom-list, it generates qmp_marshal_input_qom_list().  For qom-get, it
> generates nothing.

qom-get uses 'gen': false because it can return anything, and the schema
can't express that, so we have to cheat.  'returns': '**' is merely
documentation to remind us why we cheat.

"[PATCH RFC v2 40/47] qapi: Introduce a first class 'any' type" removes
the need to cheat, and "[PATCH RFC v2 41/47] qom: Don't use 'gen': false
for qom-get, qom-set, object-add" removes the cheating.  Result:

    { 'command': 'qom-get',
      'data': { 'path': 'str', 'property': 'str' },
      'returns': 'any' }

> We still rely on qmp-commands.hx to bind the monitor to the schema.  For
> qom-list, it binds to the generated function:
>
>     {
>         .name       = "qom-list",
>         .args_type  = "path:s",
>         .mhandler.cmd_new = qmp_marshal_input_qom_list,
>     },
>
> For qom-get, it binds to a hand-written one:
>
> {
>         .name       = "qom-get",
>       .args_type  = "path:s,property:s",
>       .mhandler.cmd_new = qmp_qom_get,
>     },
>
> Both are QMP command handlers, i.e their type is
>
>     void (*cmd_new)(QDict *params, QObject **ret_data, Error **errp);
>
> What does the generated handler do for us?  As the filename
> qmp-marshal.c suggests, it's basically a wrapper around a command
> handler with a "native C" interface that unmarshals from QDict to C
> "native C" data types and back.  For qom-list, the native handler is
>
>     ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
>     {
>         // Do stuff with the arguments, create the result
>         return props;
>     }
>
> This is the part you need to implement manually.
>
> For qom-get, you implement this one instead:
>
>     int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
>     {
>         // Unmarshall qdict into arguments
>         const char *path = qdict_get_str(qdict, "path");
>         const char *property = qdict_get_str(qdict, "property");
>         Error *local_err = NULL;
>         Object *obj;
>
>         // Do stuff with the arguments, create the result
>
>         // Marshal result into QObject
>         // nothing to do in this case, because it happens to be one
>         // already
>
>     out:
>         if (local_err) {
>             // Go from Error API to QMP's QError
>             // (QError will go away soon)
>             qerror_report_err(local_err);
>             error_free(local_err);
>             return -1;
>         }
>
>         return 0;
>     }
>
> Note the boilerplate around 'do stuff'.

This becomes:

    QObject *qmp_qom_get(const char *path, const char *property, Error **errp)
    {
        Object *obj;
    
        obj = object_resolve_path(path, NULL);
        if (!obj) {
            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                      "Device '%s' not found", path);
            return NULL;
        }
    
        return object_property_get_qobject(obj, property, errp);
    }

> Notably absent from the boilerplate: rejecting unknown parameters,
> checking mandatory parameters are present, checking types.  How come?
>
> For historical reasons, part of the unmarshalling job is done by QMP
> core, using args_type information from qmp-commands.hx.
>
> For qom-list, .args_type = "path:s".  The core checks argument "path" is
> present and is a JSON string, and no other argument is present.
>
> For qom-get, .args_type = "path:s,property:s".  I trust you can guess
> what the core checks there.
>
> When we eliminate qmp-commands.hx, the core's checking goes away, and
> needs to be replaced.
>
> Generated handlers like qmp_marshal_input_qom_list() already do full
> type checking of known parameters, so we just rely on that.  Handwritten
> ones like qmp_qom_get() generally don't.  They'll need to be updated to
> do it.

Either that, or lose their need for 'gen': false.

> Part 2: When to use it?
>
> We use 'gen': false when we can't (be bothered to) specify the exact
> type of an argument or result.
>
> Bad example: netdev_add
>
>     We have arguments 'type': 'str' and '*props': '**'.
>
>     We should have a union tagged by network backend type.  For each
>     type, the union holds the type's properties (if any).
>
> Better example: device_add (but that's not even QAPIfied, yet)
>
>     If QAPIfied, we'd have arguments like 'driver': 'str' and '*props':
>     '**'.

No, we won't have '*props'.  We'll have additional driver-dependent
arguments.

>     Looks just like netdev_add.  The difference is that network backends
>     and their properties are defined in one place, but device models and
>     their properties aren't.  They get collected at run time.  As long
>     as the schema is fixed at compile-time, it can't express the
>     resulting tagged union.
>
> Another good example: qom-get
>
>     We have a return value '**'.
>
>     The run time type is the type of the property identified by the
>     arguments.  Therefore, the compile time type can only be the union
>     of all property types, which effectively boils down to "anything".
>
>     The only way to say "anything" right now is '**'.  Requires 'gen':
>     false.  I figure we could extend the generators to support '**' in a
>     few places, which may let us avoid 'gen': false here.

s/ in a few places//

> Drawback of '**': introspection knows nothing.
>
> Introspection knowing nothing about netdev_add's and device_add's
> acceptable properties is a big, painful gap.
>
> Please don't invent new reasons for 'gen': false without a very
> compelling use case.  If you think you have one, we need to talk to make
> sure use of 'gen': false really beats the alternatives.  Alternatives
> may include extending the generators.

I added one in "[PATCH RFC v2 45/47] qapi: New QMP command query-schema
for QMP schema introspection":

    { 'command': 'query-schema',
      'returns': [ 'SchemaInfo' ],
      'gen': false }                # just to simplify qmp_query_json()

Partly because the simpler qmp_query_json() is more efficient, and
mostly because I'm lazy.  I'd gladly do without 'gen': false here if we
can then get rid of 'gen': false entirely.