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

[Kevin Wolf]

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.

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.

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. This is documented in the code, but it might actually
still be missing from qapi-code-gen.txt.

> 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

Kevin