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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> 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.

I managed to confuse myself.  Fortunately, it looks like I failed to
confuse you.

> 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.

I'm too tired / stupid to grasp this in the abstract.  I'll try again
tomorrow, with concrete examples.

>> >> > +
>> >> > +        /*
>> >> > +         * 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.

You're right.

It doesn't accumulate with find_object_member() below.  There, the code
always sets, but the contract doesn't actually promise it.

>> >> > @@ -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