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

[mdroth]

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.