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

[Markus Armbruster]

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.