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>
I had to read this patch mostly backwards to make sense of it. I
recommend you read my review mostly backwards, too: forward within each
function, backwards otherwise.
I sometimes put helpers after their users to avoid that.
> ---
> qapi/qobject-input-visitor.c | 169 +++++++++++++++++++++++++++++++++--
> 1 file changed, 160 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 1415561828..faca5b6b55 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -74,6 +74,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;
> @@ -141,9 +143,139 @@ static const char *full_name(QObjectInputVisitor *qiv,
> const char *name)
> return full_name_so(qiv, name, tos);
> }
>
> +static bool find_object_member(QObjectInputVisitor *qiv,
> + StackObject **so, const char **name,
> + bool *implicit_object, Error **errp);
> +
> +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->alias) {
> + name = a->alias;
> + }
> +
> + 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;
> + }
> +
> + return true;
> +}
> +
> +static bool alias_source_matches(QObjectInputVisitor *qiv,
> + StackObject *so, InputVisitorAlias *a,
> + const char *name, bool *implicit_object)
> +{
> + if (a->src[0] == NULL) {
> + assert(a->alias == NULL);
> + return true;
> + }
> +
> + if (!strcmp(a->src[0], name)) {
> + if (a->alias && a->src[1] == NULL) {
> + /*
> + * We're matching an exact member, the source for this alias is
> + * immediately in @so.
> + */
> + return true;
> + } 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.
> + */
> + if (!a->alias || alias_present(qiv, a, a->alias)) {
> + *implicit_object = true;
> + }
> + }
> + }
> +
> + return false;
> +}
> +
> +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->alias from a->alias_so.
> + */
> + QSLIST_FOREACH(a, &cur_so->aliases, next) {
> + if (a->alias == NULL && found) {
> + /*
> + * Skip wildcard aliases if we already have a match. This is
> + * not a conflict that should result in an error.
> + */
> + 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, *so));
> + return false;
> + } else {
> + found = a->alias ?: *name;
> + *so = a->alias_so;
> + found_is_wildcard = !a->alias;
> + }
> + }
> +
> + /* 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;
> @@ -161,10 +293,24 @@ 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)) {
I guess this changes @so, @key exactly when it follows an alias.
We'll see when @implicit_object is set when we get to the spot that sets
it (remember, reading backwards).
> + if (implicit_object) {
> + if (!qiv->empty_qdict) {
> + qiv->empty_qdict = qdict_new();
> + }
> + return QOBJECT(qiv->empty_qdict);
Returns a soft reference (not to be qobject_unref()'ed), which is
correct.
> + } 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 {
Cases:
* !find_object_member() && implicit_object && no error set
Return a shared empty QDict, no error set.
* !find_object_member() && implicit_object && error set
Must not happen.
* !find_object_member() && !implicit_object && no error set
Return null, no error set.
* !find_object_member() && !implicit_object && error set
Return null, error set.
* find_object_member() && no error set
Return input.
implicit_object is ignored.
* find_object_member() && error set
Must not happen.
I can only guess what these cases mean.
find_object_member() is too complicated to go without a written
contract. Please add one.
I'd prefer to see it before I continue review.
> @@ -190,9 +336,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) {
Likewise (below, not above; we're reading backwards).
> error_setg(errp, QERR_MISSING_PARAMETER, full_name(qiv, name));
> }
> return obj;
> @@ -764,13 +911,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;
> }
>
Awkward.
Before the patch, qobject_input_try_get_object() returns the input for
@name, or else null.
Afterwards, we have three cases:
* Return non-null, no error set: this is the input for name, as before.
* Return null, no error set: there is no input for name.
* Return null, error set: "Value for parameter %s was already given
through an alias". We'll see what that means when we get to the spot
that sets this error (remember, reading backwards).
I can't yet say whether this could be avoided.
> @@ -785,6 +935,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);
