Re: [PATCH v3 6/6] tests/qapi-schema: Test cases for aliases

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.09.2021 um 10:59 hat Markus Armbruster geschrieben:
>> >> > +    /* You can't use more than one option at the same time */
>> >> > +    v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'foo': 
>> >> > 42 } }");
>> >> > +    visit_type_AliasStruct3(v, NULL, &tmp, &err);
>> >> > +    error_free_or_abort(&err);
>> >> 
>> >> "Parameter 'foo' is unexpected".  Say what?  It *is* expected, it just
>> >> clashes with 'nested.foo'.
>> >> 
>> >> I figure this is what happens:
>> >> 
>> >> * visit_type_AliasStruct3()
>> >> 
>> >>   - visit_start_struct()
>> >> 
>> >>   - visit_type_AliasStruct3_members()
>> >> 
>> >>     • visit_type_AliasStruct1() for member @nested.
>> >> 
>> >>       This consumes consumes input nested.foo.
>> >> 
>> >>   - visit_check_struct()
>> >> 
>> >>     Error: input foo has not been consumed.
>> >> 
>> >> Any ideas on how to report this error more clearly?
>> >
>> > It's a result of the logic that wildcard aliases are silently ignored
>> > when they aren't needed. The reason why I included this is that it would
>> > allow you to have two members with the same name in the object
>> > containing the alias and in the aliased object without conflicting as
>> > long as both are given.
>> 
>> *brain cramp*
>> 
>> Example?
>
> Let's use the real-world example I mentioned below:
>
> { 'union': 'ChardevBackend',
>   'data': { ...,
>             'socket': 'ChardevSocket',
>             ... },
>   'aliases': [ { 'source': ['data'] } ] }

To pretend the simple union was flat, i.e. peel off its 'data', because
that nesting doesn't exist in the CLI you want to QAPIfy.

>
> { 'struct': 'ChardevSocket',
>   'data': { 'addr': 'SocketAddressLegacy',
>             ... },
>   'base': 'ChardevCommon',
>   'aliases': [ { 'source': ['addr'] } ] }

To unbox struct SocketAddressLegacy, i.e. peel off its 'addr', for the
same reason.

>
> { 'union': 'SocketAddressLegacy',
>   'data': {
>     'inet': 'InetSocketAddress',
>     'unix': 'UnixSocketAddress',
>     'vsock': 'VsockSocketAddress',
>     'fd': 'String' },
>   'aliases': [
>     { 'source': ['data'] },

To pretend the simple union was flat, i.e. peel off its 'data',

>     { 'name': 'fd', 'source': ['data', 'str'] }

To unbox struct String, i.e. peel off its 'data'.

>   ] }

Okay, I understand what you're trying to do.  However:

> We have two simple unions there, and wildcard aliases all the way
> through, so that you can have things like "hostname" on the top level.
> However, two simple unions mean that "type" could refer to either
> ChardevBackend.type or to SocketAddressLegacy.type.

Yup.  In ChardevBackend, we have both a (common) member @type, and a
chain of aliases that resolves @type to data.addr.data.type.

> Even though strictly speaking 'type=socket' is ambiguous, you don't want
> to error out, but interpret it as a value for the outer one.

I'm not sure.

When exactly are collisions an error?  How exactly are non-erroneous
collisions resolved?  qapi-code-gen.rst needs to answer this.

Back to the example.  If 'type' resolves to ChardevBackend's member, how
should I specify SocketAddressLegacy's member?  'addr.type'?

Aside: existing -chardev infers SocketAddressLegacy's tag addr.type from
the presence of variant members, but that's a separate QAPIfication
problem.

I figure aliases let me refer to these guys at any level I like:
'data.addr.data.FOO', 'data.addr.FOO', 'addr.data.FOO', 'addr.FOO', or
just 'FOO'.  Except for 'type', where just 'type' means something else.
Bizarre...

We actually require much less: for QMP chardev-add, we need
'data.addr.data.FOO' and nothing else, and for CLI -chardev, we need
'FOO' and nothing else (I think).  The unneeded ones become accidental
parts of the external interface.  If experience is any guide, we'll have
plenty of opportunity to regret such accidents :)

Can we somehow restrict external interfaces to what we actually need?

>> > Never skipping wildcard aliases makes the code simpler and results in
>> > the expected error message here. So I'll do that for v4.
>> 
>> Trusting your judgement.
>> 
>> > Note that parsing something like '--chardev type=socket,addr.type=unix,
>> > path=/tmp/sock,id=c' now depends on the order in the generated code. If
>> > the top level 'type' weren't parsed and removed from the input first,
>> > visiting 'addr.type' would now detect a conflict. For union types, we
>> > know that 'type' is always parsed first, so it's not a problem, but in
>> > the general case you need to be careful with the order.
>> 
>> Uff!  I think I'd like to understand this better.  No need to delay v4
>> for that.
>> 
>> Can't yet say whether we need to spell out the limitation in commit
>> message and/or documentation.
>
> The point where we could error out is when parsing SocketAddressLegacy,
> because there can be two possible providers for "type".
>
> The idea in the current code of this series was that we'll just ignore
> wildcard aliases if we already have a value, because then the value must
> be meant for somewhere else. So it doesn't error out and leaves the
> value in the QDict for someone else to pick it up. If nobody picks it
> up, it's an error "'foo' is unexpected".
>
> If we change it and do error out when there are multiple possible values
> through wildcard aliases, then the only thing that makes it work is that
> ChardevBackend.type is parsed first and is therefore not a possible
> value for SocketAddressLegacy.type any more.

You wrote you're picking the alternative with the simpler code for v4.
Fine with me, as long as it's reasonably easy to explain, in
qapi-code-gen.rst.