Re: [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor

From: Kevin Wolf <kwolf@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: jsnow@redhat.com, qemu-devel@nongnu.org
Subject: Re: [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor
Date: Wed, 27 Jan 2021 21:59:51 +0100	[thread overview]
Message-ID: <20210127205951.GF6090@merkur.fritz.box> (raw)
In-Reply-To: <87tur2plzm.fsf@dusky.pond.sub.org>

Am 27.01.2021 um 14:06 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > This makes qobject-input-visitor remember the currently valid aliases in
> > each StackObject. It doesn't actually allow using the aliases yet.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 115 insertions(+)
> >
> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> > index 23843b242e..a00ac32682 100644
> > --- a/qapi/qobject-input-visitor.c
> > +++ b/qapi/qobject-input-visitor.c
> > @@ -29,6 +29,29 @@
> >  #include "qemu/cutils.h"
> >  #include "qemu/option.h"
> >  
> > +typedef struct InputVisitorAlias {
> > +   /* Alias name. NULL for any (wildcard alias). */
> > +    const char *alias;
> > +
> > +    /*
> > +     * NULL terminated array representing a path.
> > +     * Must contain at least one non-NULL element if alias is not NULL.
> 
> What does .alias = NULL, .src[] empty mean?
> 
> The previous patch's contract for visit_define_alias():
> 
>    /*
>     * Defines a new alias rule.
>     *
>     * If @alias is non-NULL, the member named @alias in the external
>     * representation of the current struct is defined as an alias for the
>     * member described by @source.
>     *
>     * If @alias is NULL, all members of the struct described by @source are
>     * considered to have alias members with the same key in the current
>     * struct.
>     *
>     * @source is a NULL-terminated array of names that describe the path to
>     * a member, starting from the currently visited struct.
>     *
>     * The alias stays valid until the current alias scope ends.
>     * visit_start/end_struct() implicitly start/end an alias scope.
>     * Additionally, visit_start/end_alias_scope() can be used to explicitly
>     * create a nested alias scope.
>     */
> 
> If I read this correctly, then empty .src[] denotes "the current
> struct", and therefore .alias = NULL, .src[] empty means "all members of
> the current struct are considered to have alias members with the same
> key in the current struct".  Which is be a complicated way to accomplish
> nothing.
> 
> Am I confused?

Indeed, this doesn't make sense when @alias_so is the currently
processed StackObject. I guess qobject_input_define_alias() should just
forbid this case.

But see below for the relevant case.

> > +     */
> > +    const char **src;
> > +
> > +    /* StackObject in which the alias should be looked for */
> > +    struct StackObject *alias_so;
> 
> Clear as mud.  Is it "the current struct"?  If not, what else?  Perhaps
> an example would help me understand.

It is the object where the alias was defined.

.alias = NULL, .src[] empty happens after propagating the alias to a
child object, i.e. alias_so is different from the current StackObject.

Let's take a simple SocketAddressLegacy object as an example. Without
aliases, it might look like this:

{ 'type': 'inet',
  'data': { 'host': 'localhost', 'port': 1234 } }

We want to eliminate 'data' from the external representation with an
alias, so we map everything in 'data' to the top level. So the actual
external representation that we want to parse in the example is this:

{ 'type': 'inet', 'host': 'localhost', 'port': 1234 }

For the implementation this alias means that while visiting the top
level SocketAddressLegacy object (with StackObject A) we define an
InputVisitorAlias like this:

    {
        .alias = NULL,
        .src = ['data', NULL],
        .alias_so = A,
    }

When we proceed to visit the 'data' member, we call start_struct, which
creates a new StackObject B. The alias is propagated, stripping 'data'
from the start of .src:

    {
        .alias = NULL,
        .src = [NULL],
        .alias_so = A, /* This still refers to A, not B! */
    }

Next we're parsing the members of InetSocketAddress (the type of
'data'). The visitor wants to visit 'host' now, but it's not present in
the QDict to parse (StackObject.obj, which is actually an empty QDict in
this case because 'data' wasn't given at all).

So what happens is that it looks at aliases that could provide a value
for this key. Its alias list contains the alias=NULL,src=[NULL] item,
which makes it check if 'host' exists in .alias_so (which is the
StackObject that belongs to the top level SocketAddressLegacy) instead,
and indeed we have a 'host' key there, so we can take the value from
there.

Does this make the mechanism a bit clearer?

> > +
> > +    /*
> > +     * The alias remains valid as long as the containing StackObject has
> 
> What's "the containing StackObject"?  I guess it's the one that has this
> InputVisitorAlias in .aliases.

Yes.

> > +     * StackObject.alias_scope_nesting >= InputVisitorAlias.scope_nesting
> > +     * or until the whole StackObject is removed.
> > +     */
> > +    int scope_nesting;
> > +
> > +    QSLIST_ENTRY(InputVisitorAlias) next;
> > +} InputVisitorAlias;
> > +
> >  typedef struct StackObject {
> >      const char *name;            /* Name of @obj in its parent, if any */
> >      QObject *obj;                /* QDict or QList being visited */
> > @@ -38,6 +61,9 @@ typedef struct StackObject {
> >      const QListEntry *entry;    /* If @obj is QList: unvisited tail */
> >      unsigned index;             /* If @obj is QList: list index of @entry */
> >  
> > +    QSLIST_HEAD(, InputVisitorAlias) aliases;
> > +    int alias_scope_nesting;    /* Increase on scope start, decrease on end */
> 
> I get what the comment means, but imperative mood is odd for a variable.
> "Number of open scopes", perhaps?

Works for me.

> > +
> >      QSLIST_ENTRY(StackObject) node; /* parent */
> >  } StackObject;
> >  
> 
> I'm afraid I'm too confused about the interface (previous patch) and the
> data structures to continue review with reasonable efficiency.  I don't
> mean to imply either is bad!  I'm just confused, that's all.

I hope the above helps to resolve the confusion. Feel free to come back
with more questions or ping me on IRC if necessary.

Kevin