From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47444) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TOIkS-0000JW-Hc for qemu-devel@nongnu.org; Tue, 16 Oct 2012 21:48:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TOIkR-00074G-7d for qemu-devel@nongnu.org; Tue, 16 Oct 2012 21:48:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63337) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TOIkQ-000745-Ut for qemu-devel@nongnu.org; Tue, 16 Oct 2012 21:48:39 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q9H1mbP2017339 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 16 Oct 2012 21:48:38 -0400 Date: Tue, 16 Oct 2012 22:49:33 -0300 From: Luiz Capitulino Message-ID: <20121016224933.77c423f7@doriath.home> In-Reply-To: <507C5044.5020403@redhat.com> References: <1350288417-24350-1-git-send-email-kraxel@redhat.com> <1350288417-24350-10-git-send-email-kraxel@redhat.com> <507C5044.5020403@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 9/9] chardev: add hotplug support. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Gerd Hoffmann , qemu-devel@nongnu.org On Mon, 15 Oct 2012 12:04:52 -0600 Eric Blake 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 > > --- > > +++ 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 >