Re: [PATCH 1/6] qapi: Add interfaces for alias support to Visitor

[Kevin Wolf]

Am 27.01.2021 um 13:51 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > This adds functions to the Visitor interface that can be used to define
> > aliases and alias scopes.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  include/qapi/visitor-impl.h | 12 ++++++++++++
> >  include/qapi/visitor.h      | 37 +++++++++++++++++++++++++++++++++++++
> >  qapi/qapi-visit-core.c      | 21 +++++++++++++++++++++
> >  3 files changed, 70 insertions(+)
> >
> > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> > index 7362c043be..e30da2599c 100644
> > --- a/include/qapi/visitor-impl.h
> > +++ b/include/qapi/visitor-impl.h
> > @@ -113,6 +113,18 @@ struct Visitor
> >         The core takes care of the return type in the public interface. */
> >      void (*optional)(Visitor *v, const char *name, bool *present);
> >  
> > +    /*
> > +     * Optional; intended for input visitors. If not given, aliases are
> > +     * ignored.
> > +     */
> > +    void (*define_alias)(Visitor *v, const char *alias, const char 
> > **source);
> > +
> > +    /* Must be set if define_alias is set */
> > +    void (*start_alias_scope)(Visitor *v);
> > +
> > +    /* Must be set if define_alias is set */
> > +    void (*end_alias_scope)(Visitor *v);
> > +
> >      /* Must be set */
> >      VisitorType type;
> >  
> > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> > index ebc19ede7f..9bdc0ee03d 100644
> > --- a/include/qapi/visitor.h
> > +++ b/include/qapi/visitor.h
> > @@ -459,6 +459,43 @@ void visit_end_alternate(Visitor *v, void **obj);
> >   */
> >  bool visit_optional(Visitor *v, const char *name, bool *present);
> >  
> > +/*
> > + * 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
> 
> Terminology: the big comment uses "object".  See also the FIXME in
> visit_start_struct()'s contract.

Ok. Maybe the FIXME should be resolved to avoid this kind of problem.

> > + * 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.
> 
> Define "the current struct".  I believe it's the object being visited.

Yes.

> What happens if we're currently visiting something other than an object,
> i.e. the root of a tree, or a list?

Then you (= the generated code) shouldn't call the function. Aliases
only make sense for objects (because everything else doesn't have keys).

If you call it anyway, it depends on how you visit the elements of the
list. Currently, I think they are always visited with a NULL name. In
this case the alias should just never apply, but it looks like
propagate_aliases() might actually crash because it doesn't check for
NULL names.

We don't have such callers and I don't think we want to have them, so
I'm not sure if we want to fix anything, and if we do, if the fix should
be tolerating and effectively ignoring such alias definitions or if we
should explicitly assert that the name is non-NULL.

> > + *
> > + * @source is a NULL-terminated array of names that describe the path to
> > + * a member, starting from the currently visited struct.
> 
> I'm afraid "describe the path to a member" is too vague.  How?
> 
> I figure this is what you have in mind:
> 
>     cur = the currently visited object
>     for s in source:
>         cur = the member of cur denoted by s
> 
> When @cur is indeed an object, then "the member denoted by @s" makes
> sense: you must pass a name when visiting object members, and whatever
> is visited with name @s is the member denoted by @s.
> 
> "Must pass a name" is documented in the big comment:
> 
>  * The @name parameter of visit_type_FOO() describes the relation
>  * between this QAPI value and its parent container.  When visiting
>  * the root of a tree, @name is ignored; when visiting a member of an
>  * object, @name is the key associated with the value; when visiting a
>  * member of a list, @name is NULL; and when visiting the member of an
>  * alternate, @name should equal the name used for visiting the
>  * alternate.
> 
> But what if @cur is a list?  I guess that makes no sense.  Say so
> explicitly, please.

Yes, everything but the last element in the path must be an object.

Kevin