On Mon, 15 Oct 2012 12:04:52 -0600 Eric Blake <ebl...@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 <kra...@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 >