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

[Anthony Liguori]

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.

Regards,

Anthony Liguori