Re: [Qemu-devel] [PATCH v3 9/9] chardev: add hotplug support.

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 15 Oct 2012 12:04:52 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 10/15/2012 02:06 AM, Gerd Hoffmann wrote:
> > This patch adds chardev_add and chardev_del monitor commands.
> > 
> > They work simliar to the netdev_{add,del} commands.  The hmp version of
> 
> s/simliar/similar/
> 
> > chardev_add accepts like the -chardev command line option does.  The qmp
> > version expects the arguments being passed as named parameters.
> > 
> > chardev_del just takes an id argument and zaps the chardev specified.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> > +++ b/qapi-schema.json
> > @@ -2796,3 +2796,42 @@
> >  # Since: 0.14.0
> >  ##
> >  { 'command': 'screendump', 'data': {'filename': 'str'} }
> > +
> > +##
> > +# @chardev_add:
> 
> chardev-add
> 
> > +#
> > +# Add a chardev
> > +#
> > +# @id: the chardev's ID, must be unique
> > +# @backend: the chardev backend: "file", "socket", ...
> 
> Should this be an enum type, instead of an open-coded string?
> 
> > +# @path: file / device / unix socket path
> > +# @name: spice channel name
> > +# @host: host name
> > +# @port: port number
> > +# @server: create socket in server mode
> > +# @wait: wait for connect
> > +# @ipv4: force ipv4-only
> > +# @ipv6: force ipv6-only
> > +# @telnet: telnet negotiation
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 1.3.0
> > +##
> > +{ 'command': 'chardev_add', 'data': {'id'      : 'str',
> > +                                     'backend' : 'str',
> > +                                     '*props'  : '**' },
> 
> Having an open-coded list for props feels awkward; it would be nicer to
> have the schema completely describe everything, even though that may be
> more documentation work.

I agree it's awkward. Besides, this 'props' thing was supposed to be only used
with old commands that already accept QemuOpts style options. The usage of
'props' forces us having a front-end in old qmp style, which we don't want to
get spread.

Having said that, I'm not sure I can offer a good alternative here. Having
all options in the schema would require us to create the QemuOpts object by
hand vs. automatically building it from the qdict that's passed to old
style qmp commands. It's not only documentation work.

There's the work Laszlo did for net clients too. Where we have a
NetClientOptions type which is a union of all possible net clients
options types. All clients get a NetClientOptions instance. But this would
require massive conversion of chardev backends to use a similar type.

Another idea would be to have a different add command for each backend.
This is more work and more "verbose", but has the good effect of a clean
separation & definition of the set of options used by each backend.

But again, as I'm not sure any of my ideas above are good, I'd be fine
with the chardev_add command from this patch.

> 
> > +  'gen': 'no' }
> > +
> > +##
> > +# @chardev_del:
> 
> chardev-del
> 
> 
> > +Arguments:
> > +
> > +- "id": the chardev's ID, must be unique (json-string)
> > +- "backend": the chardev backend: "file", "socket", ... (json-string)
> > +- "path": file / device / unix socket path (json-string, optional)
> > +- "name": spice channel name (json-string, optional)
> > +- "host": host name (json-string, optional)
> > +- "port": port number (json-string, optional)
> > +- "server": create socket in server mode (json-bool, optional)
> 
> Given this line...
> 
> > +- "wait": wait for connect (json-bool, optional)
> > +- "ipv4": force ipv4-only (json-bool, optional)
> > +- "ipv6": force ipv6-only (json-bool, optional)
> > +- "telnet": telnet negotiation (json-bool, optional)
> > +
> > +Example:
> > +
> > +-> { "execute": "chardev_add", "arguments": { "id"      : "foo",
> > +                                              "backend" : "socket",
> > +                                              "path"    : "/tmp/foo",
> > +                                              "server"  : "on",
> 
> ...this line is wrong, since "on" is not a json-bool.  It would have to
> be "server":true
> 
> > +                                              "wait"    : "off" } }
> 
> Similar for "wait":false
>