Kevin Wolf <kw...@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 <kw...@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);
