Patchwork [for-1.4] qapi: Improve chardev-add documentation

login
register
mail settings
Submitter Markus Armbruster
Date Feb. 11, 2013, 5:05 p.m.
Message ID <1360602348-4727-1-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/219646/
State New
Headers show

Comments

Markus Armbruster - Feb. 11, 2013, 5:05 p.m.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi-schema.json | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Eric Blake - Feb. 11, 2013, 5:13 p.m.
On 02/11/2013 10:05 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi-schema.json | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster - Feb. 11, 2013, 5:16 p.m.
Apropos chardev-add schema.  I know I gave up whatever bikeshedding
privileges I might have by not reviewing the series before it went in,
but here goes anyway: I don't like the name ChardevPort.  'Port' tells
me nothing.  It's really host character device passthrough, isn't it,
Gerd?

Is it too late to find a more telling name?  Eric?
Eric Blake - Feb. 11, 2013, 5:19 p.m.
On 02/11/2013 10:16 AM, Markus Armbruster wrote:
> Apropos chardev-add schema.  I know I gave up whatever bikeshedding
> privileges I might have by not reviewing the series before it went in,
> but here goes anyway: I don't like the name ChardevPort.  'Port' tells
> me nothing.  It's really host character device passthrough, isn't it,
> Gerd?
> 
> Is it too late to find a more telling name?  Eric?

I don't know that I have a suggestion for a better name, but libvirt
hasn't coded to the new interface yet, so as long as we lock in a
reasonable name before 1.4 is cut, I don't see a problem with changing
the name.  It's just that time is not on our side.
Gerd Hoffmann - Feb. 12, 2013, 8:32 a.m.
On 02/11/13 18:16, Markus Armbruster wrote:
> Apropos chardev-add schema.  I know I gave up whatever bikeshedding
> privileges I might have by not reviewing the series before it went in,
> but here goes anyway: I don't like the name ChardevPort.  'Port' tells
> me nothing.  It's really host character device passthrough, isn't it,
> Gerd?

ChardevPort is more or less taken from Paolos qapi interface draft, and
I found it intuitive enough to go with it.  This is about passing
through serial (and parallel) *ports* to the guest.  And, yes, it works
for other tty devices too, so you don't always have a physical port.

So, if you have a better name suggestion in time for 1.4 ...

cheers,
  Gerd
Markus Armbruster - Feb. 12, 2013, 9:44 a.m.
[cc'ing Paolo]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On 02/11/13 18:16, Markus Armbruster wrote:
>> Apropos chardev-add schema.  I know I gave up whatever bikeshedding
>> privileges I might have by not reviewing the series before it went in,
>> but here goes anyway: I don't like the name ChardevPort.  'Port' tells
>> me nothing.  It's really host character device passthrough, isn't it,
>> Gerd?
>
> ChardevPort is more or less taken from Paolos qapi interface draft, and
> I found it intuitive enough to go with it.  This is about passing
> through serial (and parallel) *ports* to the guest.  And, yes, it works
> for other tty devices too, so you don't always have a physical port.

What's the distinctive property of ChardevPort things?

Same question worded differently: what other things would we want to
stuff into this (type, device) object rather than elsewhere?

I'd say the distinctive bit is "'device' names a host character device".
If we find another host character device we want to pass through, we'd
almost certainly put it into ChardevPort, wouldn't we?

Aside: 'type' is only there because we can't be bothered to figure out
the device type ourselves.  Lame.

Exception: pseudoterminals.  They don't fit, because you don't open them
by name.  But they're so special they already got their own
ChardevBackend type.

Now let's take another step back: a character device is just a file.
Why can't we use plain ChardevFile for it?

File type could be determined automatically.  If we don't want to do
that, we have to make the user specify it via parameter.  The parameter
could be optional and default to "regular file".

> So, if you have a better name suggestion in time for 1.4 ...

I believe the cautious choice is disabling type port for 1.4, so we can
figure out the design we want without undue time pressure.

Disabling it for 1.4 shouldn't be a big deal, because the QMP command is
not yet feature complete anyway.

If we decide to keep it for 1.4, I'd suggest to rename from 'port' to
'hostdev' or similar.
Paolo Bonzini - Feb. 12, 2013, 10:25 a.m.
Il 12/02/2013 10:44, Markus Armbruster ha scritto:
> Aside: 'type' is only there because we can't be bothered to figure out
> the device type ourselves.  Lame.

Is there any API for that, apart from shooting out random ioctls?

> Now let's take another step back: a character device is just a file.
> Why can't we use plain ChardevFile for it?

Because it doesn't make sense in general to use separate in/out for them.

> File type could be determined automatically.  If we don't want to do
> that, we have to make the user specify it via parameter.  The parameter
> could be optional and default to "regular file".

It also would have to be separate for in and out; also, I'm not sure how
you would handle a serial port that is used only for input or only for
output.  Right now ChardevFile uses a simple string for the input and
output sides.

{ 'type': 'ChardevFile', 'data': { '*in' : 'str',
                                   'out' : 'str' } }

>> > So, if you have a better name suggestion in time for 1.4 ...
> I believe the cautious choice is disabling type port for 1.4, so we can
> figure out the design we want without undue time pressure.
> 
> Disabling it for 1.4 shouldn't be a big deal, because the QMP command is
> not yet feature complete anyway.

I think it's overkill.

> If we decide to keep it for 1.4, I'd suggest to rename from 'port' to
> 'hostdev' or similar.

Fine by me.

Paolo
Gerd Hoffmann - Feb. 12, 2013, 10:35 a.m.
Hi,

> Now let's take another step back: a character device is just a file.
> Why can't we use plain ChardevFile for it?

It's not.

First, the file backend allows input and output being different files,
and the input file is optional.  That doesn't make sense for the
parallel/serial port case.

Second, it's actually more than a file.  The guest can change device
parameters such as the baudrate on the virtual device and qemu will
apply those changes to the host device.

IMO it is different enough to have its own type.

> File type could be determined automatically.  If we don't want to do
> that, we have to make the user specify it via parameter.  The parameter
> could be optional and default to "regular file".

Well, not really.

I don't feel like doing guesswork based on the filename.  If we have one
in the first place, we can also get passed in a file handle (via
/dev/fdset/name).

And I also don't feel like invoking random ioctls on the file handle we
got, trying to figure whenever the device at hand is a serial port or
parallel port or something else.

>> So, if you have a better name suggestion in time for 1.4 ...
> 
> I believe the cautious choice is disabling type port for 1.4, so we can
> figure out the design we want without undue time pressure.

I fail to see what is to terribly bad on the design.

> If we decide to keep it for 1.4, I'd suggest to rename from 'port' to
> 'hostdev' or similar.

I don't care that much what the actual name of the beast is.
Feel free to submit a patch changing the name.

cheers,
  Gerd
Markus Armbruster - Feb. 12, 2013, 3:34 p.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> Now let's take another step back: a character device is just a file.
>> Why can't we use plain ChardevFile for it?
>
> It's not.

A Unix character device is a special file.

> First, the file backend allows input and output being different files,
> and the input file is optional.  That doesn't make sense for the
> parallel/serial port case.
>
> Second, it's actually more than a file.  The guest can change device
> parameters such as the baudrate on the virtual device and qemu will
> apply those changes to the host device.
>
> IMO it is different enough to have its own type.

Special files support some special operations.  I didn't propose to drop
support for the special operations and treat special files exactly like
regular files.  I asked whether we really need a completely separate way
to configure them.

The argument for separate configuration is file's capability to split
input and output.  Okay.

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

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.

[...]
Gerd Hoffmann - Feb. 12, 2013, 3:55 p.m.
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.

cheers,
  Gerd
Markus Armbruster - Feb. 12, 2013, 4:56 p.m.
Gerd Hoffmann <kraxel@redhat.com> 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?
Michael Roth - Feb. 12, 2013, 9:40 p.m.
On Tue, Feb 12, 2013 at 05:56:00PM +0100, Markus Armbruster wrote:
> Gerd Hoffmann <kraxel@redhat.com> 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.
Anthony Liguori - Feb. 12, 2013, 10:20 p.m.
mdroth <mdroth@linux.vnet.ibm.com> writes:

> On Tue, Feb 12, 2013 at 05:56:00PM +0100, Markus Armbruster wrote:
>> Gerd Hoffmann <kraxel@redhat.com> 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
Anthony Liguori - Feb. 12, 2013, 11:04 p.m.
Applied.  Thanks.

Regards,

Anthony Liguori
Markus Armbruster - Feb. 13, 2013, 1:34 p.m.
Anthony Liguori <aliguori@us.ibm.com> writes:

> mdroth <mdroth@linux.vnet.ibm.com> writes:
>
>> On Tue, Feb 12, 2013 at 05:56:00PM +0100, Markus Armbruster wrote:
>>> Gerd Hoffmann <kraxel@redhat.com> 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.

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index 736f881..bd289ae 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3152,6 +3152,9 @@ 
 #
 # Return info about the chardev backend just created.
 #
+# @pty: #optional name of the slave pseudoterminal device, present if
+#       and only if a chardev of type 'pty' was created
+#
 # Since: 1.4
 ##
 { 'type' : 'ChardevReturn', 'data': { '*pty' : 'str' } }
@@ -3159,12 +3162,12 @@ 
 ##
 # @chardev-add:
 #
-# Add a file chardev
+# Add a character device backend
 #
 # @id: the chardev's ID, must be unique
 # @backend: backend type and parameters
 #
-# Returns: chardev info.
+# Returns: ChardevReturn.
 #
 # Since: 1.4
 ##
@@ -3175,7 +3178,7 @@ 
 ##
 # @chardev-remove:
 #
-# Remove a chardev
+# Remove a character device backend
 #
 # @id: the chardev's ID, must exist and not be in use
 #