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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> 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.

Yes, please.

> 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,

Make all members of ...

>         .src = ['data', NULL],

...  the object .data also available in ...

>         .alias_so = A,

the object corresponding to A, which is the object we're visiting right
now (A is on top of the stack).

Correct?

>     }
>
> 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,

Make all members of ...

>         .src = [NULL],

... the current object also available in ...

>         .alias_so = A, /* This still refers to A, not B! */

the object corresponding to A, which is the object containing the one
we're visiting right now (A right below the top of the stack).

Correct?

>     }
>
> 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).

Wait a sec!  Doesn't qobject_input_start_struct(v, "data", ...) *fail*
when "data" is absent?  Oh, looks like you're messing with that in PATCH
4.  Forward reference only in your explanation, not in this patch, which
"doesn't actually allow using the aliases yet".

> 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.

When src[0] isn't null, this InputVisitorAlias is to be applied deeper
in the visit, and we ignore it here.

Correct?

> Does this make the mechanism a bit clearer?

Feels like it :)

>> > +
>> > +    /*
>> > +     * 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;

We need a list because any number of aliases may apply here (src[0]
null) or deeper in the stack (not null).

Correct?

>> > +    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.

Thanks!