Re: [PATCH v3 4/6] qapi: Apply aliases in qobject-input-visitor

[Kevin Wolf <kwolf@redhat.com>]

Am 14.09.2021 um 08:58 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 06.09.2021 um 17:16 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>

> >> > +/*
> >> > + * Check whether the member @name in the object visited by @so can be
> >> > + * specified in the input by using the alias described by @a (which
> >> > + * must be an alias contained in so->aliases).
> >> > + *
> >> > + * If @name is only a prefix of the alias source, but doesn't match
> >> > + * immediately, false is returned and *is_alias_prefix is set to true
> >> > + * if it is non-NULL.  In all other cases, *is_alias_prefix is left
> >> > + * unchanged.
> >> > + */
> >> > +static bool alias_source_matches(QObjectInputVisitor *qiv,
> >> > +                                 StackObject *so, InputVisitorAlias *a,
> >> > +                                 const char *name, bool *is_alias_prefix)
> >> > +{
> >> > +    if (a->src[0] == NULL) {
> >> > +        assert(a->name == NULL);
> >> > +        return true;
> >> > +    }
> >> > +
> >> > +    if (!strcmp(a->src[0], name)) {
> >> > +        if (a->name && a->src[1] == NULL) {
> >> > +            /*
> >> > +             * We're matching an exact member, the source for this alias is
> >> > +             * immediately in @so.
> >> > +             */
> >> > +            return true;
> >> > +        } else if (is_alias_prefix) {
> >> > +            /*
> >> > +             * We're only looking at a prefix of the source path for the alias.
> >> > +             * If the input contains no object of the requested name, we will
> >> > +             * implicitly create an empty one so that the alias can still be
> >> > +             * used.
> >> > +             *
> >> > +             * We want to create the implicit object only if the alias is
> >> > +             * actually used, but we can't tell here for wildcard aliases (only
> >> > +             * a later visitor call will determine this). This means that
> >> > +             * wildcard aliases must never have optional keys in their source
> >> > +             * path. The QAPI generator checks this condition.
> >> > +             */
> >> 
> >> Double-checking: this actually ensures that we only ever create the
> >> implicit object when it will not remain empty.  Correct?
> >
> > For wildcard aliases, we still can't know which keys will be visited
> > later. Checking that we don't have optional keys only avoids the
> > confusion between absent and present, but empty objects that you would
> > get from the implicit objects. So it means that creating an implicit
> > object is never wrong, either the nested object can be visited (which
> > means we needed the implicit object) or it errors out.
> 
> What I'm trying to understand is whether aliases may make up an empty
> object, and if yes, under what conditions.  Can you help me?
> 
> "Make up an empty object" = have an empty QDict in the result where the
> JSON input doesn't have a {}.

Well, the result is a C type, not a QDict. We never build a single QDict
for the object including values resolved from aliases, we just fetch the
values from different QDicts if necessary.

But for what I think you're trying to get at: Yes, it can happen that we
start visiting a struct which was not present in the JSON, and for which
no members will match. This is if you have a wildcard alias for the
members of this object because we must assume that the alias might
provide the necessary values - but it might as well not have them.

There are two cases here:

1. The nested object contains non-optional members. This is an error
   case. The error message will change from missing struct to missing
   member, but this isn't too bad because the missing member does in
   fact exist on the outer level, too, as an alias. So I think the error
   message is still good.

2. The nested object only contains optional members. Then the alias
   allows just not specifying the empty nested object, all of the zero
   required members are taken from the outer object.

   This would be a problem if the nested object were optional itself
   because it would turn absent into present, but empty. So this is the
   reason why we check in the generator that you don't have optional
   members in the path of wildcard aliases.

> >> > +
> >> > +        /*
> >> > +         * For single-member aliases, an alias name is specified in the
> >> > +         * alias definition. For wildcard aliases, the alias has the same
> >> > +         * name as the member in the source object, i.e. *name.
> >> > +         */
> >> > +        if (!input_present(qiv, a->alias_so, a->name ?: *name)) {
> >> > +            continue;
> >> 
> >> What if alias_source_matches() already set *is_alias_prefix = true?
> >> 
> >> I figure this can't happen, because it guards the assignment with the
> >> exact same call of input_present().  In other words, we can get here
> >> only for "full match".  Correct?
> >
> > Probably, but my reasoning is much simpler: If alias_source_matches()
> > sets *is_alias_prefix, it also returns false, so we would already have
> > taken a different path above.
> 
> I see.  The contract even says so: "false is returned and
> *is_alias_prefix is set to true".  It's actually the only way for
> *is_alias_prefix to become true.
> 
> Output parameters that are set only sometimes require the caller to
> initialize the receiving variable, or use it only under the same
> condition it is set.  The former is easy to forget, and the latter is
> easy to get wrong.
> 
> "Set sometimes" can be useful, say when you have several calls where the
> output should "accumulate".  When you don't, I prefer to set always,
> because it makes the function harder to misuse.  Would you mind?

It does "accumulate" here. We want to return true if any of the aliases
make it true.

> >> > @@ -189,10 +372,31 @@ static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
> >> >      assert(qobj);
> >> >  
> >> >      if (qobject_type(qobj) == QTYPE_QDICT) {
> >> > -        assert(name);
> >> > -        ret = qdict_get(qobject_to(QDict, qobj), name);
> >> > -        if (tos->h && consume && ret) {
> >> > -            bool removed = g_hash_table_remove(tos->h, name);
> >> > +        StackObject *so = tos;
> >> > +        const char *key = name;
> >> > +        bool is_alias_prefix;
> >> > +
> >> > +        assert(key);
> >> > +        if (!find_object_member(qiv, &so, &key, &is_alias_prefix, errp)) {
> >> 
> >> * Input absent: zap @so, @key, set @is_alias_prefix.
> >> 
> >> * Error: set *errp, leave @is_alias_prefix undefined.
> >> 
> >> > +            if (is_alias_prefix) {
> >> 
> >> Use of undefined @is_alias_prefix in case "Error".  Bug in code or in
> >> contract?
> >
> > We should probably use ERRP_GUARD() and check for !*errp here.
> 
> A need to use ERRP_GUARD() often signals "awkward interface".
> 
> What about always setting is_alias_prefix?  Then it's false on error.

Ok, I can define it as false for error cases if you prefer.

I'm not sure if I find it more readable than !*errp && ..., though. It
makes is_alias_prefix carry more information than its name suggests.

Kevin