Re: [PATCH v3 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 | 227 +++++++++++++++++++++++++++++++++--
>  1 file changed, 218 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 16a75442ff..6193df28a5 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -97,6 +97,8 @@ struct QObjectInputVisitor {
>      QObject *root;
>      bool keyval;                /* Assume @root made with keyval_parse() */
>  
> +    QDict *empty_qdict;         /* Used for implicit objects */

Would

       /* For visiting objects where all members are from aliases */

be clearer?

> +
>      /* Stack of objects being visited (all entries will be either
>       * QDict or QList). */
>      QSLIST_HEAD(, StackObject) stack;
> @@ -169,9 +171,190 @@ 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 *is_alias_prefix, Error **errp);

According to the function's contract below, three cases:

* Input present: update *so, *name, return true.

* Input absent: zap *so, *name, set *is_alias_prefix, return false.

* Error: set *errp, leave *is_alias_prefix undefined, return false.

> +
> +/*
> + * Check whether the member @name in @so, or an alias for it, is
> + * present in the input and can be used to obtain the value.
> + */
> +static bool input_present(QObjectInputVisitor *qiv, StackObject *so,
> +                          const char *name)
> +{
> +    /*
> +     * Check whether the alias member is present in the input
> +     * (possibly recursively because aliases are transitive).
> +     * The QAPI generator makes sure that alises cannot form loops, so
> +     * the recursion guaranteed to terminate.
> +     */
> +    if (!find_object_member(qiv, &so, &name, NULL, NULL)) {

* Input absent: zap @so and @name.

* Error: don't zap.

Since @so and @name aren't used anymore, the difference doesn't matter.
Okay.

> +        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;
> +    }
> +
> +    return true;
> +}
> +
> +/*
> + * Check whether the member @name in the object visited by @so can be
> + * specified in the input by using the alias described by @a (which
> + * must be an alias contained in so->aliases).
> + *
> + * If @name is only a prefix of the alias source, but doesn't match
> + * immediately, false is returned and *is_alias_prefix is set to true
> + * if it is non-NULL.  In all other cases, *is_alias_prefix is left
> + * unchanged.
> + */
> +static bool alias_source_matches(QObjectInputVisitor *qiv,
> +                                 StackObject *so, InputVisitorAlias *a,
> +                                 const char *name, bool *is_alias_prefix)
> +{
> +    if (a->src[0] == NULL) {
> +        assert(a->name == NULL);
> +        return true;
> +    }
> +
> +    if (!strcmp(a->src[0], name)) {
> +        if (a->name && a->src[1] == NULL) {
> +            /*
> +             * We're matching an exact member, the source for this alias is
> +             * immediately in @so.
> +             */
> +            return true;
> +        } else if (is_alias_prefix) {
> +            /*
> +             * 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. The QAPI generator checks this condition.
> +             */

Double-checking: this actually ensures that we only ever create the
implicit object when it will not remain empty.  Correct?

> +            if (!a->name || input_present(qiv, a->alias_so, a->name)) {
> +                *is_alias_prefix = true;
> +            }
> +        }
> +    }
> +
> +    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 @is_alias_prefix on
> + * return is undefined in this case.
> + *
> + * If no value could be found in the input, false is returned and @so
> + * and @name are set to NULL.  This is not an error and @errp remains
> + * unchanged.  If @is_alias_prefix 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 @is_alias_prefix on return is undefined in this case.
> + */
> +static bool find_object_member(QObjectInputVisitor *qiv,
> +                               StackObject **so, const char **name,
> +                               bool *is_alias_prefix, Error **errp)
> +{
> +    QDict *qdict = qobject_to(QDict, (*so)->obj);
> +    const char *found_name = NULL;
> +    StackObject *found_so = NULL;
> +    bool found_is_wildcard = false;
> +    InputVisitorAlias *a;
> +
> +    if (is_alias_prefix) {
> +        *is_alias_prefix = false;
> +    }
> +
> +    /* Directly present in the container */
> +    if (qdict_haskey(qdict, *name)) {
> +        found_name = *name;
> +        found_so = *so;
> +    }
> +
> +    /*
> +     * 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, &(*so)->aliases, next) {
> +        if (a->name == NULL && found_name && !found_is_wildcard) {
> +            /*
> +             * Skip wildcard aliases if we already have a match. This is
> +             * not a conflict that should result in an error.
> +             *
> +             * However, multiple wildcard aliases matching is an error
> +             * and will be caught below.
> +             */
> +            continue;
> +        }
> +
> +        if (!alias_source_matches(qiv, *so, a, *name, is_alias_prefix)) {
> +            continue;
> +        }

According to the contract of alias_source_matches() above, three cases:

* No match: try next alias

* Partial match: set *is_alias_prefix = true if non-null

* Full match: leave it alone

> +
> +        /*
> +         * For single-member aliases, an alias name is specified in the
> +         * alias definition. For wildcard aliases, the alias has the same
> +         * name as the member in the source object, i.e. *name.
> +         */
> +        if (!input_present(qiv, a->alias_so, a->name ?: *name)) {
> +            continue;

What if alias_source_matches() already set *is_alias_prefix = true?

I figure this can't happen, because it guards the assignment with the
exact same call of input_present().  In other words, we can get here
only for "full match".  Correct?

Such repeated calls of helpers can be a sign of awkward interfaces.
Let's not worry about that now.

> +        }
> +
> +        /*
> +         * A non-wildcard alias simply overrides a wildcard alias, but
> +         * two matching non-wildcard aliases or two matching wildcard
> +         * aliases conflict with each other.
> +         */
> +        if (found_name && (!found_is_wildcard || a->name == NULL)) {
> +            error_setg(errp, "Value for parameter %s was already given "
> +                       "through an alias",
> +                       full_name_so(qiv, *name, false, *so));
> +            return false;
> +        } else {
> +            found_name = a->name ?: *name;
> +            found_so = a->alias_so;
> +            found_is_wildcard = !a->name;
> +        }
> +    }
> +
> +    /*
> +     * Chained aliases: *found_so/found_name might be the source of
> +     * another alias.
> +     */
> +    if (found_name && (found_so != *so || found_name != *name)) {
> +        find_object_member(qiv, &found_so, &found_name, NULL, errp);

* Input present: update @found_so, @found_name.

* Input absent: zap @found_name, @found_name.

* Error: set *errp.

  Can @found_name be non-null?  If yes, we can set *errp and return
  true, which would be bad.

> +    }
> +
> +    *so = found_so;
> +    *name = found_name;
> +
> +    return found_name != NULL;
> +}
> +
>  static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
>                                               const char *name,
> -                                             bool consume)
> +                                             bool consume, Error **errp)

Before the patch, two cases:

* Input present: consume input if @consume, return the input

* Input absent: return null

The patch adds

* Other error: set *errp, return null

Slightly awkward to use, as we shall see below at [1] and [2].
Observation, not demand.

>  {
>      StackObject *tos;
>      QObject *qobj;
> @@ -189,10 +372,31 @@ 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 is_alias_prefix;
> +
> +        assert(key);
> +        if (!find_object_member(qiv, &so, &key, &is_alias_prefix, errp)) {

* Input absent: zap @so, @key, set @is_alias_prefix.

* Error: set *errp, leave @is_alias_prefix undefined.

> +            if (is_alias_prefix) {

Use of undefined @is_alias_prefix in case "Error".  Bug in code or in
contract?

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

We pretend there was an object to make the calling visitor enter the
object and visit its members.  Visiting a member first looks for input
in the (empty) object, then follows aliases to look for it elsewhere.

Is "might" still correct?  The comment in alias_source_matches() makes
me hope it's actually "will".


> +                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);
> +        assert(ret != NULL);
> +        if (so->h && consume) {
> +            bool removed = g_hash_table_remove(so->h, key);
>              assert(removed);
>          }
>      } else {
> @@ -218,9 +422,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));
>      }

[1] Squash case "Input absent" into case "Error".

>      return obj;
> @@ -803,13 +1008,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) {

[2] Case "Input absent", and ...

>          *present = false;
>          return;
>      }

... cases "Input present" and "Error".

>  
> +    error_free(local_err);
>      *present = true;
>  }
>  
> @@ -842,6 +1050,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);