| Message ID | 1360602348-4727-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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?
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.
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
[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.
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
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
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. [...]
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
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?
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.
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
Applied. Thanks. Regards, Anthony Liguori
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.
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 #
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qapi-schema.json | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)