| Message ID | 20210211183118.422036-5-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: Add support for aliases | expand |
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> Looking just at qobject_input_try_get_object() for now. > --- > 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; > + } > + > + /* > + * Check whether the alias member is present in the input > + * (possibly recursively because aliases are transitive). > + */ > + 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; > +} > + > +/* > + * 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. > + */ > +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); > + 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 (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->name || alias_present(qiv, a, a->name)) { > + *implicit_object = 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 @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. > + */ > + 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) Reminder: * 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. > { > StackObject *tos; > QObject *qobj; QObject *ret; if (QSLIST_EMPTY(&qiv->stack)) { /* Starting at root, name is ignored. */ assert(qiv->root); return qiv->root; } /* We are in a container; find the next element. */ tos = QSLIST_FIRST(&qiv->stack); qobj = tos->obj; > @@ -187,10 +358,30 @@ static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv, > assert(qobj); > > if (qobject_type(qobj) == QTYPE_QDICT) { We're visiting a member of object @qobj, which is tos->obj. > - 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)) { No input found. > + 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. > + */ Cue me scratching head. find_object_member()'s contract explains @implicit_object is 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." I figure this means qiv->stack has a stack object with an alias whose src[] has a prefix that resolves to @tos (the currently visited object). Visiting the members of the currently visited object may or may not visit something where that src[] resolves completely, and that something may or may not have a member for the alias. If it does, there is input, and "something" will happen to route it to the correct place. What exactly? Should become clearer further down. If it doesn't, there is no input. > + if (!qiv->empty_qdict) { > + qiv->empty_qdict = qdict_new(); > + } > + return QOBJECT(qiv->empty_qdict); So far, I have no idea *why* we pretend there was an empty object in the input, why sharing it is safe, and why changing the return value from null to empty dict is okay. Should become clearer further down. > + } else { > + return NULL; There is definitely no input. The old code returns null then (because the qdict_get() above returns null). No change. Good. > + } > + } We get here only when find_object_member() found input. If no aliases applied, @so and @key are unchanged, i.e. @so is @tos, and @key is @name. The code below is the old code with @qobj replaced by so->obj and @name replaced by @key. No change. Good. Else, some alias (or chain of aliases) applied. Let's assume a single alias for now. It is defined for a stack object further down qiv->stack, with an alias name there, and it's src[] resolves to @tos (the currently visited object). find_object_member() found input there, i.e. the alias stack object's input has a member with the alias name. It changed @so to the alias stack object, and @key to the alias name. The code below then gets the input value from member with the alias name in the alias object. This consumes the "alias member" of the outer input object, and uses it as input for the "true member" of the inner object. Good. I figure a chain of aliases works the same; the only difference is we need more steps to find the alias stack object and name. Correct? > + ret = qdict_get(qobject_to(QDict, so->obj), key); Any particular reason why find_object_member() doesn't simply return the input it found? > + if (so->h && consume && ret) { How can @ret be null? > + bool removed = g_hash_table_remove(so->h, key); > assert(removed); > } > } else { Note that we access the parent container via tos->obj, so->obj, and qobj. Too many aliases for my taste. Let's eliminate qobj. Now let me try to figure out how the magic around @implicit_object works. Okay, I came, I saw, I ran out of brain juice. Help! Please walk me through an example that covers all the paths through this function, unless you have better ideas on how to explain it. > @@ -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);
Am 17.02.2021 um 16:32 hat Markus Armbruster geschrieben: > 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> > > Looking just at qobject_input_try_get_object() for now. :-( This patch doesn't even feel that complicated to me. Old: Get the value from the QDict of the current StackObject with the given name. New: First do alias resolution (with find_object_member), which results in a StackObject and a name, and that's the QDict and key where you get the value from. Minor complication: Aliases can refer to members of nested objects that may not be provided in the input. But we want these to work. For example, my chardev series defines aliases to flatten SocketAddressLegacy (old syntax, I haven't rebased it yet): { 'union': 'SocketAddressLegacy', 'data': { 'inet': 'InetSocketAddress', 'unix': 'UnixSocketAddress', 'vsock': 'VsockSocketAddress', 'fd': 'String' }, 'aliases': [ {'source': ['data']}, {'alias': 'fd', 'source': ['data', 'str']} ]} Of course, the idea is that this input should work: { 'type': 'inet', 'hostname': 'localhost', 'port': '1234' } However, without implicit objects, parsing 'data' fails because it expects an object, but none is given (we specified all of its members on the top level through aliases). What we would have to give is: { 'type': 'inet', 'hostname': 'localhost', 'port': '1234', 'data': {} } And _that_ would work. Visiting 'data' succeeds because we now have the object that the visitor expects, and when visiting its members, the aliases fill in all of the mandatory values. So what this patch does is to implicitly assume the 'data': {} instead of erroring out when we know that aliases exist that can still provide values for the content of 'data'. > > --- > > 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; > > + } > > + > > + /* > > + * Check whether the alias member is present in the input > > + * (possibly recursively because aliases are transitive). > > + */ > > + 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; > > +} > > + > > +/* > > + * 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. > > + */ > > +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); > > + 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 (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->name || alias_present(qiv, a, a->name)) { > > + *implicit_object = 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 @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. > > + */ > > + 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) > > Reminder: > > * 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. > > > { > > StackObject *tos; > > QObject *qobj; > QObject *ret; > > if (QSLIST_EMPTY(&qiv->stack)) { > /* Starting at root, name is ignored. */ > assert(qiv->root); > return qiv->root; > } > > /* We are in a container; find the next element. */ > tos = QSLIST_FIRST(&qiv->stack); > qobj = tos->obj; > > @@ -187,10 +358,30 @@ static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv, > > assert(qobj); > > > > if (qobject_type(qobj) == QTYPE_QDICT) { > > We're visiting a member of object @qobj, which is tos->obj. > > > - 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)) { > > No input found. > > > + 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. > > + */ > > Cue me scratching head. I hope the above made it clearer. > find_object_member()'s contract explains @implicit_object is 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." > > I figure this means qiv->stack has a stack object with an alias whose > src[] has a prefix that resolves to @tos (the currently visited object). No. tos->aliases contains an alias where @name is a prefix of alias->src[] (but doesn't fully match src[]). Or in other words, since @name is only a single element, being a prefix means alias->src[0] == name. The second condition ("for which a value may be present in the input") means that if we know that the alias isn't used in the input, then we don't create an implicit object, but use the traditinoal "not found" path. > Visiting the members of the currently visited object may or may not > visit something where that src[] resolves completely, and that something > may or may not have a member for the alias. > > If it does, there is input, and "something" will happen to route it to > the correct place. What exactly? Should become clearer further down. > > If it doesn't, there is no input. Visiting the aliased member will call qobject_input_try_get_object() again, and this is where the alias will be used. We're only making sure that it is even visited instead of erroring out earlier. > > + if (!qiv->empty_qdict) { > > + qiv->empty_qdict = qdict_new(); > > + } > > + return QOBJECT(qiv->empty_qdict); > > So far, I have no idea *why* we pretend there was an empty object in the > input, why sharing it is safe, and why changing the return value from > null to empty dict is okay. Should become clearer further down. > > > + } else { > > + return NULL; > > There is definitely no input. > > The old code returns null then (because the qdict_get() above returns > null). No change. Good. > > > + } > > + } > > We get here only when find_object_member() found input. Yes. The interesting part of the patch ends here. We know the StackObject and name where we find the right value. > If no aliases applied, @so and @key are unchanged, i.e. @so is @tos, and > @key is @name. The code below is the old code with @qobj replaced by > so->obj and @name replaced by @key. No change. Good. > > Else, some alias (or chain of aliases) applied. > > Let's assume a single alias for now. It is defined for a stack object > further down qiv->stack, with an alias name there, and it's src[] > resolves to @tos (the currently visited object). Specifically, to the member @name inside of @tos. > find_object_member() found input there, i.e. the alias stack object's > input has a member with the alias name. It changed @so to the alias > stack object, and @key to the alias name. The code below then gets the > input value from member with the alias name in the alias object. > > This consumes the "alias member" of the outer input object, and uses it > as input for the "true member" of the inner object. > > Good. > > I figure a chain of aliases works the same; the only difference is we > need more steps to find the alias stack object and name. > > Correct? Yes, chains are handled inside find_object_member(), so if necessary, it will recursively resolve aliases. > > + ret = qdict_get(qobject_to(QDict, so->obj), key); > > Any particular reason why find_object_member() doesn't simply return the > input it found? I think it's more managable to have a function that is only responsible for resolving names, and another to actually fetch the values from there. If it did both at once, qobject_input_try_get_object() wouldn't even have anything to do any more. Getting the value from the input was its job before the patch, so I don't see why it should be different afterwards. > > + if (so->h && consume && ret) { > > How can @ret be null? I don't think it can because find_object_member() would return false. This is old code that made sense when calling qdict_get() without checking first if the value is present in the input. > > + bool removed = g_hash_table_remove(so->h, key); > > assert(removed); > > } > > } else { > > Note that we access the parent container via tos->obj, so->obj, and > qobj. Too many aliases for my taste. Let's eliminate qobj. tos->obj and so->obj are different! They can be the same (if no alias was used or the alias source was in the same object, i.e. a simple rename), but they don't have to. qobj exists before this patch and isn't touched at all. I can add another patch to remove it, but it has nothing to do with this one. > Now let me try to figure out how the magic around @implicit_object > works. > > Okay, I came, I saw, I ran out of brain juice. Help! > > Please walk me through an example that covers all the paths through > this function, unless you have better ideas on how to explain it. It is exactly one path that was added, the implicit object. The rest are existing paths. I hope the example above clarified this path. If not, then I'm not sure how to explain. I can understand how the reason for having implicit objects may be unclear (though the comments try to describe that - maybe you didn't read it because you read my story backwards). But ignoring the why, the how feels really obvious to me. The code that you've looked at so far hasn't even changed much. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 17.02.2021 um 16:32 hat Markus Armbruster geschrieben: >> 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> >> >> Looking just at qobject_input_try_get_object() for now. > > :-( > > This patch doesn't even feel that complicated to me. I suspect it's just me having an unusually obtuse week. The code is straightforward enough. What I'm missing is a bit of "how does this accomplish the goal" and "why is this safe" here and there. > Old: Get the value from the QDict of the current StackObject with the > given name. > > New: First do alias resolution (with find_object_member), which results > in a StackObject and a name, and that's the QDict and key where you get > the value from. This part I understand. We simultaneously walk the QAPI type and the input QObject, consuming input as we go. Whenever the walk leaves a QAPI object type, we check the corresponding QObject has been consumed completely. With aliases, we additionally look for input in a certain enclosing input object (i.e. up the recursion stack). If found, consume. > Minor complication: Aliases can refer to members of nested objects that > may not be provided in the input. But we want these to work. > > For example, my chardev series defines aliases to flatten > SocketAddressLegacy (old syntax, I haven't rebased it yet): > > { 'union': 'SocketAddressLegacy', > 'data': { > 'inet': 'InetSocketAddress', > 'unix': 'UnixSocketAddress', > 'vsock': 'VsockSocketAddress', > 'fd': 'String' }, > 'aliases': [ > {'source': ['data']}, > {'alias': 'fd', 'source': ['data', 'str']} > ]} > > Of course, the idea is that this input should work: > > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234' } > > However, without implicit objects, parsing 'data' fails because it > expects an object, but none is given (we specified all of its members on > the top level through aliases). What we would have to give is: > > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234', 'data': {} } > > And _that_ would work. Visiting 'data' succeeds because we now have the > object that the visitor expects, and when visiting its members, the > aliases fill in all of the mandatory values. > > So what this patch does is to implicitly assume the 'data': {} instead > of erroring out when we know that aliases exist that can still provide > values for the content of 'data'. Aliases exist than can still provide, but will they? What if input is {"type": "inet"} ? Your explanation makes me guess this is equivalent to {"type": "inet", "data": {}} which fails the visit, because mandatory members of "data" are missing. Fine. If we make the members optional, it succeeds: qobject_input_optional() checks both the regular and the aliased input, finds neither, and returns false. Still fine. What if "data" is optional, too? Hmmm... Example: { 'struct': 'Outer', 'data': { '*inner': 'Inner' }, { 'struct': 'Inner', 'data': { '*true-name': 'str' } } For input {}, we get an Outer object with .has_inner = false, .inner = NULL Now add 'aliases': [ { 'name': 'alias-name', 'source': [ 'inner', 'true-name' ] } ] } What do we get now? Is it .has_inner = true, .inner = { .has_true_name = false, .true_name = NULL } perhaps? I'll study the rest of your reply next.
Am 18.02.2021 um 14:39 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 17.02.2021 um 16:32 hat Markus Armbruster geschrieben: > >> 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> > >> > >> Looking just at qobject_input_try_get_object() for now. > > > > :-( > > > > This patch doesn't even feel that complicated to me. > > I suspect it's just me having an unusually obtuse week. > > The code is straightforward enough. What I'm missing is a bit of "how > does this accomplish the goal" and "why is this safe" here and there. > > > Old: Get the value from the QDict of the current StackObject with the > > given name. > > > > New: First do alias resolution (with find_object_member), which results > > in a StackObject and a name, and that's the QDict and key where you get > > the value from. > > This part I understand. > > We simultaneously walk the QAPI type and the input QObject, consuming > input as we go. > > Whenever the walk leaves a QAPI object type, we check the corresponding > QObject has been consumed completely. > > With aliases, we additionally look for input in a certain enclosing > input object (i.e. up the recursion stack). If found, consume. > > > Minor complication: Aliases can refer to members of nested objects that > > may not be provided in the input. But we want these to work. > > > > For example, my chardev series defines aliases to flatten > > SocketAddressLegacy (old syntax, I haven't rebased it yet): > > > > { 'union': 'SocketAddressLegacy', > > 'data': { > > 'inet': 'InetSocketAddress', > > 'unix': 'UnixSocketAddress', > > 'vsock': 'VsockSocketAddress', > > 'fd': 'String' }, > > 'aliases': [ > > {'source': ['data']}, > > {'alias': 'fd', 'source': ['data', 'str']} > > ]} > > > > Of course, the idea is that this input should work: > > > > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234' } > > > > However, without implicit objects, parsing 'data' fails because it > > expects an object, but none is given (we specified all of its members on > > the top level through aliases). What we would have to give is: > > > > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234', 'data': {} } > > > > And _that_ would work. Visiting 'data' succeeds because we now have the > > object that the visitor expects, and when visiting its members, the > > aliases fill in all of the mandatory values. > > > > So what this patch does is to implicitly assume the 'data': {} instead > > of erroring out when we know that aliases exist that can still provide > > values for the content of 'data'. > > Aliases exist than can still provide, but will they? What if input is > > {"type": "inet"} > > ? > > Your explanation makes me guess this is equivalent to > > {"type": "inet", "data": {}} > > which fails the visit, because mandatory members of "data" are missing. > Fine. Okay, if you want the gory details, then the answer is yes for this case, but it depends. If we're aliasing a single member, then we can easily check whether the alias is actually specified. If it's not in the input, no implicit object. But in our example, it is a wildcard alias and we don't know yet which aliases it will use. This depends on what the visitor for the implicit object will do (future tense). > If we make the members optional, it succeeds: qobject_input_optional() > checks both the regular and the aliased input, finds neither, and > returns false. Still fine. > > What if "data" is optional, too? Hmmm... Yes, don't use optional objects in the middle of the path of a wildcard alias unless there is no semantic difference between empty object and absent object. This is documented in the code, but it might actually still be missing from qapi-code-gen.txt. > Example: > > { 'struct': 'Outer', > 'data': { '*inner': 'Inner' }, > > { 'struct': 'Inner', > 'data': { '*true-name': 'str' } } > > For input {}, we get an Outer object with > > .has_inner = false, > .inner = NULL > > Now add > > 'aliases': [ { 'name': 'alias-name', > 'source': [ 'inner', 'true-name' ] } ] } > > What do we get now? Is it > > .has_inner = true, > .inner = { .has_true_name = false, > .true_name = NULL } > > perhaps? I think this is the result you would get if you had used a wildcard alias. But since you used a single-member alias, we would see that 'alias-name' is not in the input and actually still return the original result: .has_inner = false, .inner = NULL Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 18.02.2021 um 14:39 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 17.02.2021 um 16:32 hat Markus Armbruster geschrieben: >> >> 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> >> >> >> >> Looking just at qobject_input_try_get_object() for now. >> > >> > :-( >> > >> > This patch doesn't even feel that complicated to me. >> >> I suspect it's just me having an unusually obtuse week. >> >> The code is straightforward enough. What I'm missing is a bit of "how >> does this accomplish the goal" and "why is this safe" here and there. >> >> > Old: Get the value from the QDict of the current StackObject with the >> > given name. >> > >> > New: First do alias resolution (with find_object_member), which results >> > in a StackObject and a name, and that's the QDict and key where you get >> > the value from. >> >> This part I understand. >> >> We simultaneously walk the QAPI type and the input QObject, consuming >> input as we go. >> >> Whenever the walk leaves a QAPI object type, we check the corresponding >> QObject has been consumed completely. >> >> With aliases, we additionally look for input in a certain enclosing >> input object (i.e. up the recursion stack). If found, consume. >> >> > Minor complication: Aliases can refer to members of nested objects that >> > may not be provided in the input. But we want these to work. >> > >> > For example, my chardev series defines aliases to flatten >> > SocketAddressLegacy (old syntax, I haven't rebased it yet): >> > >> > { 'union': 'SocketAddressLegacy', >> > 'data': { >> > 'inet': 'InetSocketAddress', >> > 'unix': 'UnixSocketAddress', >> > 'vsock': 'VsockSocketAddress', >> > 'fd': 'String' }, >> > 'aliases': [ >> > {'source': ['data']}, >> > {'alias': 'fd', 'source': ['data', 'str']} >> > ]} >> > >> > Of course, the idea is that this input should work: >> > >> > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234' } >> > >> > However, without implicit objects, parsing 'data' fails because it >> > expects an object, but none is given (we specified all of its members on >> > the top level through aliases). What we would have to give is: >> > >> > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234', 'data': {} } >> > >> > And _that_ would work. Visiting 'data' succeeds because we now have the >> > object that the visitor expects, and when visiting its members, the >> > aliases fill in all of the mandatory values. >> > >> > So what this patch does is to implicitly assume the 'data': {} instead >> > of erroring out when we know that aliases exist that can still provide >> > values for the content of 'data'. >> >> Aliases exist than can still provide, but will they? What if input is >> >> {"type": "inet"} >> >> ? >> >> Your explanation makes me guess this is equivalent to >> >> {"type": "inet", "data": {}} >> >> which fails the visit, because mandatory members of "data" are missing. >> Fine. > > Okay, if you want the gory details, then the answer is yes for this > case, but it depends. I'm afraid I need the gory details to get over the review hump. > If we're aliasing a single member, then we can easily check whether the > alias is actually specified. If it's not in the input, no implicit > object. I figure this check is in the code parts I haven't gotten to, yet. Not your fault. > But in our example, it is a wildcard alias and we don't know yet which > aliases it will use. This depends on what the visitor for the implicit > object will do (future tense). > >> If we make the members optional, it succeeds: qobject_input_optional() >> checks both the regular and the aliased input, finds neither, and >> returns false. Still fine. >> >> What if "data" is optional, too? Hmmm... > > Yes, don't use optional objects in the middle of the path of a wildcard > alias unless there is no semantic difference between empty object and > absent object. Aha! So my spidey-sense wasn't entirely wrong. > This is documented in the code, but it might actually > still be missing from qapi-code-gen.txt. I can't find it there. Needs fixing, obviously. I guess checking "path of a wildcard alias crosses optional objects" is hard (impractical?) for the same reasons checking "alias can't resolve" is. I'd expect "alias can't resolve" to be caused by typos, incomplete renames, and such. Basic testing should catch at least the typos. Not ideal, but I guess it'll do, at least for now. Relying on testing to catch "crosses optional objects" mistakes feels iffier to me, because it takes more careful tests. Ham-fisted way to make basic tests catch it: *ignore* optional objects when resolving aliases. Is this a bad idea? >> Example: >> >> { 'struct': 'Outer', >> 'data': { '*inner': 'Inner' }, >> >> { 'struct': 'Inner', >> 'data': { '*true-name': 'str' } } >> >> For input {}, we get an Outer object with >> >> .has_inner = false, >> .inner = NULL >> >> Now add >> >> 'aliases': [ { 'name': 'alias-name', >> 'source': [ 'inner', 'true-name' ] } ] } >> >> What do we get now? Is it >> >> .has_inner = true, >> .inner = { .has_true_name = false, >> .true_name = NULL } >> >> perhaps? > > I think this is the result you would get if you had used a wildcard > alias. But since you used a single-member alias, we would see that > 'alias-name' is not in the input and actually still return the original > result: > > .has_inner = false, > .inner = NULL I wasn't aware there's a difference. Now I am. Thanks!
Markus Armbruster <armbru@redhat.com> writes: > Kevin Wolf <kwolf@redhat.com> writes: [...] >> Yes, don't use optional objects in the middle of the path of a wildcard >> alias unless there is no semantic difference between empty object and >> absent object. > > Aha! So my spidey-sense wasn't entirely wrong. Like optional members, union branches get visited only when the input is shaped a certain way. Which makes me wonder: does "don't use optional in the middle" apply to union branches, too? Hmm, I figure it doesn't because * If the union is flat, there is no object: the variant members are the members of the branch struct type. * If the union is simple, there is, but it's always there: 'data'. Hope I'm not speaking in riddles. >> This is documented in the code, but it might actually >> still be missing from qapi-code-gen.txt. > > I can't find it there. Needs fixing, obviously. "there" = qapi-code-gen.txt > I guess checking "path of a wildcard alias crosses optional objects" is > hard (impractical?) for the same reasons checking "alias can't resolve" > is. > > I'd expect "alias can't resolve" to be caused by typos, incomplete > renames, and such. Basic testing should catch at least the typos. Not > ideal, but I guess it'll do, at least for now. > > Relying on testing to catch "crosses optional objects" mistakes feels > iffier to me, because it takes more careful tests. > > Ham-fisted way to make basic tests catch it: *ignore* optional objects > when resolving aliases. Is this a bad idea? [...]
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);
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; > + } > + > + /* > + * Check whether the alias member is present in the input > + * (possibly recursively because aliases are transitive). > + */ > + 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; > +} > + > +/* > + * 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. > + */ > +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); > + 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 (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->name || alias_present(qiv, a, a->name)) { > + *implicit_object = 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 @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) { After looking at the complete loop, I understand: @found is non-null if we have input, either non-alias input found above, or alias input found below. @found_is_wildcard is true if that input is from a wildcard alias. If @found, *so is the alias's outer StackObject, i.e. where it was defined. Else, @so is still the argument value. In either case, @cur_so is the argument value. If found, it is the member name in so->obj. It'll be stored to *name after the loop. Updating *so in the loop and *name after the loop feels a bit awkward. Using a new variable @found_so to go with @found might come out nicer. No need for @cur_so then. See also "*so may already be messed up" below. > + 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. > + */ > + continue; Ignore wildcard alias when we have other input. If multiple different wildcard aliases apply, we silently ignore all but the first one. Can this happen? > + } > + > + if (!alias_source_matches(qiv, cur_so, a, *name, implicit_object)) { The input for this alias is deeper. *implicit_object is now true if a visit of @name needs to be forced to maybe resolve it. See qobject_input_try_get_object() below. > + continue; > + } Cases: 1. This is a wildcard alias propagated to @cur_so, i.e. the member *name of cur_so->obj can serve as input. 2. This is a non-wildcard alias, and the member of cur_so->obj named by it can serve as input, if it exists. The alias_present() below uses *name when a is a wildcard, else a->name. So this checks whether the member we want for input exists. > + > + if (!alias_present(qiv, a, *name)) { > + continue; It doesn't exist; ignore the alias. > + } It exists; the alias applies. > + > + if (found && !found_is_wildcard) { The alias applies, but we already found non-alias input or non-wildcard alias input. This is an error: > + error_setg(errp, "Value for parameter %s was already given " > + "through an alias", > + full_name_so(qiv, *name, false, *so)); > + return false; Note: *so may already be messed up here. I guess (hope?) the callers are fine with that. The contract should spell it out, though. The issue goes away if you store to *so only after the loop. > + } else { The alias applies, and nothing or only wildcard aliases applied before. In the latter case, this alias is not a wildcard, because we skipped those above. Taken together: * More than one input is an error regardless of where it comes from (direct or alias), except * we silently ignore extra wildcard alias input. The error makes sense. Ignoring one wildcard alias when there is non-wildcard input makes sense. When multiple wildcard aliases clash, I think we use the first one, and ignore the others. I'm not sure that's what we want. > + found = a->name ?: *name; > + *so = a->alias_so; > + found_is_wildcard = !a->name; Record the alias applies. Update *so, but not *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); I'm sure there is a reason why we don't need to know whatever @implicit_object can tell us, but it escapes me at the moment. > + } > + > + *name = found; We zap *name when nothing was found. I guess the callers are fine with that. Nevertheless, the contract should spell it out. Storing only if found might be simpler. Oh, the caller right above might actually rely on this behavior. If that's the case, more reason to spell it out in the contract. > + 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);
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; + } + + /* + * Check whether the alias member is present in the input + * (possibly recursively because aliases are transitive). + */ + 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; +} + +/* + * 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. + */ +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); + 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 (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->name || alias_present(qiv, a, a->name)) { + *implicit_object = 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 @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. + */ + 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);
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(-)