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

From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: jsnow@redhat.com, qemu-devel@nongnu.org
Subject: Re: [PATCH 1/6] qapi: Add interfaces for alias support to Visitor
Date: Tue, 02 Feb 2021 14:51:25 +0100	[thread overview]
Message-ID: <87czxi8tnm.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20210127203136.GE6090@merkur.fritz.box> (Kevin Wolf's message of "Wed, 27 Jan 2021 21:31:36 +0100")

Kevin Wolf <kwolf@redhat.com> writes:

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

True.  Checking... the churn outside tests/ looks quite tolerable.  Feel
free to stick a suitable patch into v2.

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

I'm like putting "no nonsense" into the contract, then enforce it with
an assertion if practical.

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

Got it, thanks.