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
+ * 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.
+ */
+void visit_define_alias(Visitor *v, const char *alias, const char **source);
+
+/*
+ * Begins an explicit alias scope.
+ *
+ * Alias definitions after here will only stay valid until the
+ * corresponding visit_end_alias_scope() is called.
+ */
+void visit_start_alias_scope(Visitor *v);
+
+/*
+ * Ends an explicit alias scope.
+ *
+ * Alias definitions between the correspoding visit_start_alias_scope()
+ * call and here go out of scope and won't apply in later code any more.
+ */
+void visit_end_alias_scope(Visitor *v);
+
 /*
  * Visit an enum value.
  *
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 7e5f40e7f0..719a9f5da2 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -135,6 +135,27 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
     return *present;
 }
 
+void visit_define_alias(Visitor *v, const char *alias, const char **source)
+{
+    if (v->define_alias) {
+        v->define_alias(v, alias, source);
+    }
+}
+
+void visit_start_alias_scope(Visitor *v)
+{
+    if (v->start_alias_scope) {
+        v->start_alias_scope(v);
+    }
+}
+
+void visit_end_alias_scope(Visitor *v)
+{
+    if (v->end_alias_scope) {
+        v->end_alias_scope(v);
+    }
+}
+
 bool visit_is_input(Visitor *v)
 {
     return v->type == VISITOR_INPUT;
-- 
2.28.0

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
> + * 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.
> + */
> +void visit_define_alias(Visitor *v, const char *alias, const char **source);
> +
> +/*
> + * Begins an explicit alias scope.
> + *
> + * Alias definitions after here will only stay valid until the
> + * corresponding visit_end_alias_scope() is called.
> + */
> +void visit_start_alias_scope(Visitor *v);
> +
> +/*
> + * Ends an explicit alias scope.
> + *
> + * Alias definitions between the correspoding visit_start_alias_scope()
> + * call and here go out of scope and won't apply in later code any more.
> + */
> +void visit_end_alias_scope(Visitor *v);
> +
>  /*
>   * Visit an enum value.
>   *
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 7e5f40e7f0..719a9f5da2 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -135,6 +135,27 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
>      return *present;
>  }
>  
> +void visit_define_alias(Visitor *v, const char *alias, const char **source)
> +{
> +    if (v->define_alias) {
> +        v->define_alias(v, alias, source);
> +    }
> +}
> +
> +void visit_start_alias_scope(Visitor *v)
> +{
> +    if (v->start_alias_scope) {
> +        v->start_alias_scope(v);
> +    }
> +}
> +
> +void visit_end_alias_scope(Visitor *v)
> +{
> +    if (v->end_alias_scope) {
> +        v->end_alias_scope(v);
> +    }
> +}
> +
>  bool visit_is_input(Visitor *v)
>  {
>      return v->type == VISITOR_INPUT;

The code is trivial and neat, the contracts look fine.  What I'm missing
is "why do we want this", and "how should it be used".  The cover letter
gives me a (somewhat vague) idea on the former, but on the latter, I can
only guess.  I trust it'll become clear later in this series.

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.

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.

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

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

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

> + *
> + * 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.
> + */
> +void visit_define_alias(Visitor *v, const char *alias, const char **source);
> +
> +/*
> + * Begins an explicit alias scope.
> + *
> + * Alias definitions after here will only stay valid until the
> + * corresponding visit_end_alias_scope() is called.
> + */
> +void visit_start_alias_scope(Visitor *v);
> +
> +/*
> + * Ends an explicit alias scope.
> + *
> + * Alias definitions between the correspoding visit_start_alias_scope()
> + * call and here go out of scope and won't apply in later code any more.
> + */
> +void visit_end_alias_scope(Visitor *v);
> +
>  /*
>   * Visit an enum value.
>   *
[...]

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

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.