Re: [PATCH 5/6] qapi: Add support for aliases

From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: jsnow@redhat.com, qemu-devel@nongnu.org
Subject: Re: [PATCH 5/6] qapi: Add support for aliases
Date: Wed, 10 Feb 2021 10:17:21 +0100	[thread overview]
Message-ID: <8735y45ljy.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20201112172850.401925-6-kwolf@redhat.com> (Kevin Wolf's message of "Thu, 12 Nov 2020 18:28:49 +0100")

Kevin Wolf <kwolf@redhat.com> writes:

> Introduce alias definitions for object types (structs and unions). This
> allows using the same QAPI type and visitor for many syntax variations
> that exist in the external representation, like between QMP and the
> command line. It also provides a new tool for evolving the schema while
> maintaining backwards compatibility during a deprecation period.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  docs/devel/qapi-code-gen.txt           | 37 +++++++++++++++++++++++---
>  docs/sphinx/qapidoc.py                 |  2 +-
>  scripts/qapi/expr.py                   | 34 +++++++++++++++++++++--
>  scripts/qapi/schema.py                 | 27 +++++++++++++++----
>  scripts/qapi/types.py                  |  4 ++-
>  scripts/qapi/visit.py                  | 33 ++++++++++++++++++++---
>  tests/qapi-schema/test-qapi.py         |  7 ++++-
>  tests/qapi-schema/double-type.err      |  2 +-
>  tests/qapi-schema/unknown-expr-key.err |  2 +-
>  9 files changed, 130 insertions(+), 18 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 6906a06ad2..6da14d5275 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -231,7 +231,8 @@ Syntax:
>                 'data': MEMBERS,
>                 '*base': STRING,
>                 '*if': COND,
> -               '*features': FEATURES }
> +               '*features': FEATURES,
> +               '*aliases': ALIASES }
>      MEMBERS = { MEMBER, ... }
>      MEMBER = STRING : TYPE-REF
>             | STRING : { 'type': TYPE-REF,

Missing: a forward reference, like we have for 'if' and 'features'.
Here's the obvious one:

   The optional 'if' member specifies a conditional.  See "Configuring
   the schema" below for more on this.

   The optional 'features' member specifies features.  See "Features"
   below for more on this.

  +The optional 'aliases' member specifies aliases.  See "Aliases" below
  +for more on this.

> @@ -286,13 +287,15 @@ Syntax:
>      UNION = { 'union': STRING,
>                'data': BRANCHES,
>                '*if': COND,
> -              '*features': FEATURES }
> +              '*features': FEATURES,
> +              '*aliases': ALIASES }
>            | { 'union': STRING,
>                'data': BRANCHES,
>                'base': ( MEMBERS | STRING ),
>                'discriminator': STRING,
>                '*if': COND,
> -              '*features': FEATURES }
> +              '*features': FEATURES,
> +              '*aliases': ALIASES }
>      BRANCHES = { BRANCH, ... }
>      BRANCH = STRING : TYPE-REF
>             | STRING : { 'type': TYPE-REF, '*if': COND }

Likewise.

> @@ -837,6 +840,34 @@ shows a conditional entity only when the condition is satisfied in
>  this particular build.
>  
>  
> +=== Aliases ===
> +
> +Object types, including structs and unions, can contain alias
> +definitions.
> +
> +Aliases define alternative member names that may be used in the
> +external representation to provide a value for a member in the same
> +object or in a nested object.

"or one if its sub-objects"?  Not sure which is better.

> +
> +Syntax:
> +    ALIAS = { '*alias': STRING,
> +              'source': [ STRING, ... ] }

You used non-terminal ALIASES above.  Please define it here.

I have doubts about the name 'alias'.  The alias is the complete thing,
and 'alias' is just one property of the complete thing.  I think 'name'
would be better.  Further evidence: below, you write "If 'alias' is
present" and "If 'alias' is not present".  I think both read better with
'name' instead of 'alias'.

> +
> +'source' is a list of member names representing the path to an object
> +member, starting from the type where the alias definition is
> +specified.

May 'source' be empty?  More on that below.

"where the definition is specified" feels a bit awkward, and "path"
slightly hand-wavy.  Let me try induction:

   'source' is a list of member names.  The first name is resolved in
   the same object.  Each subsequent member is resolved in the object
   named by the preceding member.

Differently awkward, I guess.

>              It may refer to another alias name.  It is allowed to use
> +a path that doesn't necessarily match an existing member in every
> +variant or even at all; in this case, the alias remains unused.

Aha!  Knowing this would've saved me some trouble in reviewing code.

I wrote on PATCH 1:

    I think updating the big comment in visitor.h for aliases would help.
    Let's postpone it until I've seen the rest of the series.

We can cover unused aliases right there.  Whether they also need to go
into contracts we'll see.

What if only a *prefix* of 'source' matches?  E.g.

    'source': ['eins', 'zwei', 'drei']

and we have an object-valued member 'eins' (match), which has a member
'zwei' (match), which is *not* an object.  Is that an error?  Is it
caught?

> +
> +If 'alias' is present, then the single member referred to by 'source'
> +is made accessible with the name given in 'alias' in the type where
> +the alias definition is specified.

'source' may not be empty here.  Correct?

If yes, please spell it out.

Double-checking I got it...  Say we have

    'alias': 'foo',
    'source': ['bar', 'baz']

where 'bar' is an object with a member 'baz'.

Then input "foo": FOOVAL gets interpreted like "bar": {"baz": FOOVAL}.

If input also contains "bar", we merge.  Duplicates are an error.

This is the case even when 'baz' is an object.  If you want to alias
member 'foo' of 'baz', you have to say

    'alias': 'foo',
    'source': ['bar', 'baz', 'foo']

Correct?

> +
> +If 'alias' is not present, then all members in the object referred to
> +by 'source' are made accessible in the type where the alias definition
> +is specified with the same name as they have in 'source'.

'source' may not be empty here, either.  Correct?

If yes, please spell it out, and make sure the code catches it.

What if it resolve to a non-object?  Is that an error?  Is it caught?

Continuing the double-checking...  Say we have

    # alias missing
    'source': ['gnu']

where 'gnu' is an object with a member 'gnat'.

Input "gnat": GNATVAL gets interpreted like "gnu": {"gnat": GNATVAL}.

Correct?

The document could use examples.  Feel free to steal mine.

I think we should talk about 'alias' first, and only then about
'source'.  It matches their order in the schema, and also matches how I
think about aliases, namely "this name actually means that".  Here,
"this name" is 'alias', and "that" is 'source'.

> +
> +

Don't get deceived by my comments; this is a pretty good start.

I wish I had studied this part before PATCH 1.

>  === Documentation comments ===
>  
>  A multi-line comment that starts and ends with a '##' line is a

I intend to look at the remainder shortly.

[...]