Re: [Qemu-devel] [PATCH for-1.4] qapi: Improve chardev-add documentation

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> mdroth <address@hidden> writes:
>
>> On Tue, Feb 12, 2013 at 05:56:00PM +0100, Markus Armbruster wrote:
>>> Gerd Hoffmann <address@hidden> writes:
>>> 
>>> >   Hi,
>>> >
>>> >> But why nested discriminators?
>>> >> 
>>> >>     regular files: type=file
>>> >>     serial       : type=port, data.type=serial
>>> >>     parallel     : type=port, data.type=parallel
>>> >> 
>>> >> Simpler, and closer to existing -chardev:
>>> >> 
>>> >>     regular files: type=file
>>> >>     serial       : type=serial
>>> >>     parallel     : type=parallel
>>> >
>>> > Matter of taste IMHO.
>>> > I can live with that too.
>>> > Feel free to send patches.
>>> >
>>> >> I also dislike the pointless '"data" : {}' required by type pty and
>>> >> null, but I can't figure out how to express 'void' in the schema.
>>> >
>>> > Someone mentioned it is possible to leave out empty data altogether.
>>> > Didn't try whenever our marshaller actually accepts that though.
>>> 
>>> Looks like it doesn't :(
>>> 
>>> Empty objects work fine here:
>>> 
>>>     { 'type': 'ChardevDummy', 'data': { } }
>>> 
>>> Generates the obvious
>>> 
>>>     struct ChardevDummy
>>>     {
>>>     };
>>> 
>>> They don't work here:
>>> 
>>>     { 'union': 'ChardevBackend', 'data': { 'file'   : 'ChardevFile',
>>>                                            'hostdev': 'ChardevHostdev',
>>>                                            'socket' : 'ChardevSocket',
>>>                                            'pty'    : 'ChardevDummy',
>>>                                            'null'   : {} } }
>>> 
>>> Generates
>>> 
>>>     struct ChardevBackend
>>>     {
>>>         ChardevBackendKind kind;
>>>         union {
>>>             void *data;
>>>             ChardevFile * file;
>>>             ChardevHostdev * hostdev;
>>>             ChardevSocket * socket;
>>>             ChardevDummy * pty;
>>>             void null;
>>>         };
>>>     };
>>> 
>>> which is kind of cute, but the compiler doesn't like it.
>>> 
>>> Anthony, Mike, is this a bug?
>>
>> Not exactly, but it's a relic that doesn't seem to be needed anymore.
>> The code that does this is in scripts/qapi.py:
>>
>> def c_type(name):
>>     ...
>>     elif name == None or len(name) == 0:
>>         return 'void'
>>     ...
>>     else:
>>         return '%s *' % name
>>
>> The 'name' param being the value/type of a particular param/key in a
>> QAPI dictionary that defines a schema-defined type.
>>
>> The reason '{}' maps to 'void' is so that in qapi-commands.py, where we 
>> generate
>> stuff like the function signatures for qmp commands, we'd map something like:
>>
>>     { 'command': 'my_cmd',
>>       'data': { 'param1': 'Foo' },
>>       'returns': {} }
>>
>> to:
>>
>>     void my_cmd(Foo *param1);
>>
>> At some point in development we rightly decided that:
>>
>>     { 'command': 'my_cmd',
>>       'data': { 'param1': 'Foo' } }
>>
>> was more efficient, which triggers the 'name == None' case and has the same
>> effect.
>>
>> So, as long as we stay consistent about defining commands in this fashion, we
>> can map 'param_name': {} to something like 'struct {}', and use it in place 
>> of
>> schema-defined dummy types.
>>
>> Though, as I mentioned on IRC, I think just defining a:
>>
>> { 'type': 'Dummy', 'data' {} }
>>
>> Somewhere in the schema and re-using that might actually be cleaner and have
>> the same affect.
>
> Yes.  I don't think we really ought to support inline structures.  It
> keeps the declarations easier to read to make all structs top level
> types.

This argument smells too much of Pascal for my taste.  But it's not
important enough for me to argue.

More important: how things look on the wire.  Here's an instance of
union ChardevBackend:

    { "type" : "null", "data" : {} }

Note that member "data" is mandatory, and must be empty.  Design bug or
feature?

By the way, omitting it results in a mildly bogus error message:

    { "execute" : "chardev-add", "arguments" : { "id" : "foo", "backend" : { 
"type" : "null" } } }
    {"error": {"class": "GenericError", "desc": "Invalid parameter type for 
'data', expected: QDict"}}

'data' doesn't have an "invalid parameter type", it's not there.