Re: [Qemu-devel] [PATCH v5 21/28] qapi: Require valid names

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

[...]
> I had a second look.  I think the generator accepting '**' in exactly
> the right places relies on:
>
> (1) check_name() accepts only proper names, not '**'.
>
> (2) All names get checked with check_name().
>
> (3) Except check_type() accepts special type name '**' when allow_star.
>
> (4) allow_star is set for exactly the right places.
>
> With check_name()'s superfluous / incorrect '**' check gone, (1)
> obviously holds.  (2) isn't obvious, thus food for code review.  (3) is
> trivial.  (4) is trivial except for "exactly the right places".
> qapi-code-gen.txt:
>
>     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 '**' rather
>     than a valid type name.
>
> Unfortunately, "the 'data' or 'returns' member that intends to bypass
> [...] should use '**' rather than a valid type name" can be read in (at
> least) two ways:
>
> 1. It applies to the 'data', 'returns' members of the command object.
>
> 2. It applies to members of 'data', 'returns' members of the command
> object.
>
> The schema uses both readings: qom-get has 'returns': '**', and qom-set,
> netdev_add, object_add have 'data' members of the form KEY: '**'.
>
> Note that neither code nor tests have 'data': '**' or KEY: '**' in
> 'returns'.

Type '**' generally means "any JSON value".  However, a command's 'data'
must be a JSON object (see docs/qmp/qmp-spec.txt).  Ways to deal with
this:

A. Ignore.  Conforming to the schema is necessary but not sufficient for
   a command being accepted anyway.

B. The meaning of type '**' depends on context: it's "any JSON object"
   for a command's 'data', else "any JSON value".

C. Outlaw 'data': '**' for now.

I prefer C, I can accept A, I dislike B.

> qapi.py appears to implement a third way: '**' may appear as type name
> anywhere within 'data' or 'returns'.  May well make sense, and may well
> work, but we have no test coverage for it.
>
> We can extend tests to cover what the generator accepts (separate series
> recommended), or restrict the generator to what we actually use and
> test.  I'm leaning towards the latter.
>
> Further note that '**' can only be used right in a command declaration.
> Factoring out the right hand side of 'data' or 'returns' into a separate
> struct type declaration isn't possible when it contains '**'.

We could treat '**' as top type rather than as hack for a few commands.
Conceptually clean, but hard to justify without users.

[...]