| Message ID | 20201112172850.401925-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> 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);
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)) { + if (implicit_object) { + 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 { @@ -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) { 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; } @@ -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);
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 | 169 +++++++++++++++++++++++++++++++++-- 1 file changed, 160 insertions(+), 9 deletions(-)