Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> When looking for an object in a struct in the external representation,
> check not only the currently visited struct, but also whether an alias
> in the current StackObject matches and try to fetch the value from the
> alias then. Providing two values for the same object through different
> aliases is an error.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/qobject-input-visitor.c | 214 +++++++++++++++++++++++++++++++++--
>  1 file changed, 205 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index dd04ef0027..3ea5e5abd6 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -95,6 +95,8 @@ struct QObjectInputVisitor {
>      QObject *root;
>      bool keyval;                /* Assume @root made with keyval_parse() */
>  
> +    QDict *empty_qdict;         /* Used for implicit objects */
> +
>      /* Stack of objects being visited (all entries will be either
>       * QDict or QList). */
>      QSLIST_HEAD(, StackObject) stack;
> @@ -167,9 +169,178 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>      return full_name_so(qiv, name, false, tos);
>  }
>  
> +static bool find_object_member(QObjectInputVisitor *qiv,
> +                               StackObject **so, const char **name,
> +                               bool *implicit_object, Error **errp);
> +
> +/*
> + * Check whether the alias member defined by @a is present in the
> + * input and can be used to obtain the value for the member @name in
> + * the currently visited object.
> + */
> +static bool alias_present(QObjectInputVisitor *qiv,
> +                          InputVisitorAlias *a, const char *name)
> +{
> +    StackObject *so = a->alias_so;
> +
> +    /*
> +     * The passed source @name is only relevant for wildcard aliases which
> +     * don't have a separate name, otherwise we use the alias name.
> +     */
> +    if (a->name) {
> +        name = a->name;
> +    }

The contract promises "for the member @name", but it's actually for
a->name or else name.

Can a->name be non-null and different from name?

Possible way to avoid the complication:

   static bool input_present(QObjectInputVisitor *qiv,
                             StackObject *so, char *name)

From now on: we're looking for input (@so, name), and @so is in
qiv->stack.

> +
> +    /*
> +     * Check whether the alias member is present in the input
> +     * (possibly recursively because aliases are transitive).

Is the recursion guaranteed to terminate?

> +     */
> +    if (!find_object_member(qiv, &so, &name, NULL, NULL)) {
> +        return false;
> +    }
> +
> +    /*
> +     * Every source can be used only once. If a value in the input would end up
> +     * being used twice through aliases, we'll fail the second access.
> +     */
> +    if (!g_hash_table_contains(so->h, name)) {
> +        return false;
> +    }

so->h has the unvisited keys.

qobject_input_try_get_object() removes from so->h when its argument
@consume is true.  It is except in qobject_input_optional() and
qobject_input_start_alternate().  Both normally guard a call that passes
true.

The check here makes us return false when the input has been consumed
already: either by visiting it directly, or visiting something that
successfully aliased to it.

> +
> +    return true;

We return true only when the input exists and has not been consumed
already.  Okay.

If we ask alias_present() again before we get around to visiting (and
removing from so->h), it'll return true again.  Can this happen?

> +}

I just realized:

0. An alias connects a member name in an outer object to one in an inner
object nested in the outer object (or multiple names for wildcards).

1. In visit_define_alias() and InputVisitorAlias, @name is the outer
member name, and source[] the inner member name.

2. We resolve aliases when we visit the inner member.  Now the "source"
is the outer member.

3. Brain hurz!

> +
> +/*
> + * Check whether the member @name in the object visited by @so can be
> + * specified in the input by using the alias described by @a.
> + *
> + * If @name is only a prefix of the alias source, but doesn't match
> + * immediately, false is returned and @implicit_object is set to true
> + * if it is non-NULL.  In all other cases, @implicit_object is left
> + * unchanged.

*implicit_object, to be precise.

> + */

How is @a related to @so?  Peeking ahead... looks like it's a member of
so->aliases.  Recommend to spell that out.

> +static bool alias_source_matches(QObjectInputVisitor *qiv,
> +                                 StackObject *so, InputVisitorAlias *a,
> +                                 const char *name, bool *implicit_object)
> +{
> +    if (a->src[0] == NULL) {
> +        assert(a->name == NULL);

This is a wildcard alias propagated to @so.  Since there's nothing left
in ->src[], the alias's inner members are the members of QDict so->obj.

> +        return true;
> +    }
> +
> +    if (!strcmp(a->src[0], name)) {
> +        if (a->name && a->src[1] == NULL) {

This is a non-wildcard alias (possibly propagated, doesn't matter).
Since ->src[] contains just @name, the alias's inner member is the
member @name of so->obj.

> +            /*
> +             * We're matching an exact member, the source for this alias is
> +             * immediately in @so.
> +             */
> +            return true;

Else, its inner member must be deeper still in so->obj.

> +        } else if (implicit_object) {
> +            /*
> +             * We're only looking at a prefix of the source path for the alias.
> +             * If the input contains no object of the requested name, we will
> +             * implicitly create an empty one so that the alias can still be
> +             * used.
> +             *
> +             * We want to create the implicit object only if the alias is
> +             * actually used, but we can't tell here for wildcard aliases (only
> +             * a later visitor call will determine this). This means that
> +             * wildcard aliases must never have optional keys in their source
> +             * path.

This is the clue I missed yesterday.

An alias can only be used if we visit its inner member.  The caller
wants to fake input as needed to force the visit, but it needs help to
decide when to fake.  Passing non-null @implicit_object asks for this
help.

> +             */
> +            if (!a->name || alias_present(qiv, a, a->name)) {
> +                *implicit_object = true;
> +            }


Cases:

0. Wildcard, we don't know about input

   Set to true.  We may be able to use the alias, so we better force a
   visit.

1. Non-wildcard, have unconsumed input

   Set to true.  We will use the alias.  But why do we need to force a
   visit?  I'm probably confused again/

2. Non-wildcard, don't have unconsumed input

   Do nothing.  We definitely can't use the alias, so there is no need
   to force a visit.

> +        }
> +    }
> +
> +    return false;
> +}
> +
> +/*
> + * Find the place in the input where the value for the object member
> + * @name in @so is specified, considering applicable aliases.
> + *
> + * If a value could be found, true is returned and @so and @name are
> + * updated to identify the key name and StackObject where the value
> + * can be found in the input.  (This is either unchanged or the
> + * alias_so/name of an alias.)  The value of @implicit_object on
> + * return is undefined in this case.
> + *
> + * If no value could be found in the input, false is returned.  This
> + * is not an error and @errp remains unchanged.  If @implicit_object
> + * is non-NULL, it is set to true if the given name is a prefix of the
> + * source path of an alias for which a value may be present in the
> + * input.  It is set to false otherwise.
> + *
> + * If an error occurs (e.g. two values are specified for the member
> + * through different names), false is returned and @errp is set.  The
> + * value of @implicit_object on return is undefined in this case.
> + */
> +static bool find_object_member(QObjectInputVisitor *qiv,
> +                               StackObject **so, const char **name,
> +                               bool *implicit_object, Error **errp)
> +{
> +    StackObject *cur_so = *so;
> +    QDict *qdict = qobject_to(QDict, cur_so->obj);
> +    const char *found = NULL;
> +    bool found_is_wildcard = false;
> +    InputVisitorAlias *a;
> +
> +    if (implicit_object) {
> +        *implicit_object = false;
> +    }
> +
> +    /* Directly present in the container */
> +    if (qdict_haskey(qdict, *name)) {
> +        found = *name;
> +    }
> +
> +    /*
> +     * Find aliases whose source path matches @name in this StackObject. We can
> +     * then get the value with the key a->name from a->alias_so.
> +     */
> +    QSLIST_FOREACH(a, &cur_so->aliases, next) {
> +        if (a->name == NULL && found) {
> +            /*
> +             * Skip wildcard aliases if we already have a match. This is
> +             * not a conflict that should result in an error.
> +             */

What if multiple wildcard aliases match?

Out of steam with the finishing line in sight.  So it goes.

> +            continue;
> +        }
> +
> +        if (!alias_source_matches(qiv, cur_so, a, *name, implicit_object)) {
> +            continue;
> +        }
> +
> +        if (!alias_present(qiv, a, *name)) {
> +            continue;
> +        }
> +
> +        if (found && !found_is_wildcard) {
> +            error_setg(errp, "Value for parameter %s was already given "
> +                       "through an alias",
> +                       full_name_so(qiv, *name, false, *so));
> +            return false;
> +        } else {
> +            found = a->name ?: *name;
> +            *so = a->alias_so;
> +            found_is_wildcard = !a->name;
> +        }
> +    }
> +
> +    /* Chained aliases: *so/found might be the source of another alias */
> +    if (found && (*so != cur_so || found != *name)) {
> +        find_object_member(qiv, so, &found, NULL, errp);
> +    }
> +
> +    *name = found;
> +    return found;
> +}
> +
>  static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
>                                               const char *name,
> -                                             bool consume)
> +                                             bool consume, Error **errp)
>  {
>      StackObject *tos;
>      QObject *qobj;
> @@ -187,10 +358,30 @@ static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
>      assert(qobj);
>  
>      if (qobject_type(qobj) == QTYPE_QDICT) {
> -        assert(name);
> -        ret = qdict_get(qobject_to(QDict, qobj), name);
> -        if (tos->h && consume && ret) {
> -            bool removed = g_hash_table_remove(tos->h, name);
> +        StackObject *so = tos;
> +        const char *key = name;
> +        bool implicit_object;
> +
> +        assert(key);
> +        if (!find_object_member(qiv, &so, &key, &implicit_object, errp)) {
> +            if (implicit_object) {
> +                /*
> +                 * The member is not present in the input, but
> +                 * something inside of it might still be given through
> +                 * an alias. Pretend there was an empty object in the
> +                 * input.
> +                 */
> +                if (!qiv->empty_qdict) {
> +                    qiv->empty_qdict = qdict_new();
> +                }
> +                return QOBJECT(qiv->empty_qdict);
> +            } else {
> +                return NULL;
> +            }
> +        }
> +        ret = qdict_get(qobject_to(QDict, so->obj), key);
> +        if (so->h && consume && ret) {
> +            bool removed = g_hash_table_remove(so->h, key);
>              assert(removed);
>          }
>      } else {
> @@ -216,9 +407,10 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
>                                           const char *name,
>                                           bool consume, Error **errp)
>  {
> -    QObject *obj = qobject_input_try_get_object(qiv, name, consume);
> +    ERRP_GUARD();
> +    QObject *obj = qobject_input_try_get_object(qiv, name, consume, errp);
>  
> -    if (!obj) {
> +    if (!obj && !*errp) {
>          error_setg(errp, QERR_MISSING_PARAMETER, full_name(qiv, name));
>      }
>      return obj;
> @@ -799,13 +991,16 @@ static bool qobject_input_type_size_keyval(Visitor *v, const char *name,
>  static void qobject_input_optional(Visitor *v, const char *name, bool *present)
>  {
>      QObjectInputVisitor *qiv = to_qiv(v);
> -    QObject *qobj = qobject_input_try_get_object(qiv, name, false);
> +    Error *local_err = NULL;
> +    QObject *qobj = qobject_input_try_get_object(qiv, name, false, &local_err);
>  
> -    if (!qobj) {
> +    /* If there was an error, let the caller try and run into the error */
> +    if (!qobj && !local_err) {
>          *present = false;
>          return;
>      }
>  
> +    error_free(local_err);
>      *present = true;
>  }
>  
> @@ -820,6 +1015,7 @@ static void qobject_input_free(Visitor *v)
>          qobject_input_stack_object_free(tos);
>      }
>  
> +    qobject_unref(qiv->empty_qdict);
>      qobject_unref(qiv->root);
>      if (qiv->errname) {
>          g_string_free(qiv->errname, TRUE);