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

[Markus Armbruster <armbru@redhat.com>]

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!