Patchwork [v2] Add SPICE support to add_client monitor command

login
register
mail settings
Submitter Daniel P. Berrange
Date Feb. 7, 2012, 2:38 p.m.
Message ID <1328625520-19217-1-git-send-email-berrange@redhat.com>
Download mbox | patch
Permalink /patch/139945/
State New
Headers show

Comments

Daniel P. Berrange - Feb. 7, 2012, 2:38 p.m.
From: "Daniel P. Berrange" <berrange@redhat.com>

This is a followup to

  http://patchwork.ozlabs.org/patch/121004/

With the acceptance of some new APIs to libspice-server.so it
is possible to add support for SPICE to the 'add_client'
monitor command, bringing parity with VNC. Since SPICE can
use TLS or plain connections, the command also gains a new
'tls' parameter to specify whether TLS should be attempted
on the injected client sockets.

This new feature is only enabled if building against a
libspice-server >= 0.10.1

* qmp-commands.hx: Add 'tls' parameter & missing doc for
  'skipauth' parameter
* monitor.c: Wire up SPICE for 'add_client' command
* ui/qemu-spice.h, ui/spice-core.c: Add qemu_spice_display_add_client
  API to wire up from monitor

[1] http://cgit.freedesktop.org/spice/spice/commit/server/spice.h?id=d55b68b6b44f2499278fa860fb47ff22f5011faa
    http://cgit.freedesktop.org/spice/spice/commit/server/spice.h?id=bd07dde530d9504e1cfe7ed5837fc00c26f36716

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 monitor.c       |    9 +++++++--
 qmp-commands.hx |    6 ++++--
 ui/qemu-spice.h |    1 +
 ui/spice-core.c |   13 +++++++++++++
 4 files changed, 25 insertions(+), 4 deletions(-)
Gerd Hoffmann - Feb. 10, 2012, 11:30 a.m.
On 02/07/12 15:38, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> This is a followup to
> 
>   http://patchwork.ozlabs.org/patch/121004/
> 
> With the acceptance of some new APIs to libspice-server.so it
> is possible to add support for SPICE to the 'add_client'
> monitor command, bringing parity with VNC. Since SPICE can
> use TLS or plain connections, the command also gains a new
> 'tls' parameter to specify whether TLS should be attempted
> on the injected client sockets.
> 
> This new feature is only enabled if building against a
> libspice-server >= 0.10.1
> 
> * qmp-commands.hx: Add 'tls' parameter & missing doc for
>   'skipauth' parameter
> * monitor.c: Wire up SPICE for 'add_client' command
> * ui/qemu-spice.h, ui/spice-core.c: Add qemu_spice_display_add_client
>   API to wire up from monitor

Spice bits are sane, the monitor bits look good to me too.

Luiz?  Can you have a look at the monitor bits?  If you ack I'll go
queue it up for the next spice update.  Or you can just grab it and
merge via qmp tree.

cheers,
  Gerd
Luiz Capitulino - Feb. 10, 2012, 5:38 p.m.
On Tue,  7 Feb 2012 14:38:40 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> This is a followup to
> 
>   http://patchwork.ozlabs.org/patch/121004/
> 
> With the acceptance of some new APIs to libspice-server.so it
> is possible to add support for SPICE to the 'add_client'
> monitor command, bringing parity with VNC. Since SPICE can
> use TLS or plain connections, the command also gains a new
> 'tls' parameter to specify whether TLS should be attempted
> on the injected client sockets.
> 
> This new feature is only enabled if building against a
> libspice-server >= 0.10.1
> 
> * qmp-commands.hx: Add 'tls' parameter & missing doc for
>   'skipauth' parameter
> * monitor.c: Wire up SPICE for 'add_client' command
> * ui/qemu-spice.h, ui/spice-core.c: Add qemu_spice_display_add_client
>   API to wire up from monitor
> 
> [1] http://cgit.freedesktop.org/spice/spice/commit/server/spice.h?id=d55b68b6b44f2499278fa860fb47ff22f5011faa
>     http://cgit.freedesktop.org/spice/spice/commit/server/spice.h?id=bd07dde530d9504e1cfe7ed5837fc00c26f36716
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  monitor.c       |    9 +++++++--
>  qmp-commands.hx |    6 ++++--
>  ui/qemu-spice.h |    1 +
>  ui/spice-core.c |   13 +++++++++++++
>  4 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index aadbdcb..0d4daad 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -823,13 +823,18 @@ static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_d
>      CharDriverState *s;
>  
>      if (strcmp(protocol, "spice") == 0) {
> +        int fd = monitor_get_fd(mon, fdname);
> +        int skipauth = qdict_get_try_bool(qdict, "skipauth", 0);
> +        int tls = qdict_get_try_bool(qdict, "tls", 0);
>          if (!using_spice) {
>              /* correct one? spice isn't a device ,,, */
>              qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
>              return -1;
>          }
> -	qerror_report(QERR_ADD_CLIENT_FAILED);
> -	return -1;
> +        if (qemu_spice_display_add_client(fd, skipauth, tls) < 0) {
> +            close(fd);
> +        }
> +        return 0;
>  #ifdef CONFIG_VNC
>      } else if (strcmp(protocol, "vnc") == 0) {
>  	int fd = monitor_get_fd(mon, fdname);
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b5e2ab8..82625c8 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -910,8 +910,8 @@ EQMP
>  
>      {
>          .name       = "add_client",
> -        .args_type  = "protocol:s,fdname:s,skipauth:b?",
> -        .params     = "protocol fdname skipauth",
> +        .args_type  = "protocol:s,fdname:s,skipauth:b?,tls:b?",
> +        .params     = "protocol fdname skipauth tls",
>          .help       = "add a graphics client",
>          .user_print = monitor_user_noop,
>          .mhandler.cmd_new = add_graphics_client,
> @@ -927,6 +927,8 @@ Arguments:
>  
>  - "protocol": protocol name (json-string)
>  - "fdname": file descriptor name (json-string)
> +- "skipauth": whether to skip authentication (json-bool)
> +- "tls": whether to perform TLS (json-bool)

Please, mark them as optionals "(json-bool, optional)"

>  
>  Example:
>  
> diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
> index c35b29c..8932771 100644
> --- a/ui/qemu-spice.h
> +++ b/ui/qemu-spice.h
> @@ -33,6 +33,7 @@ void qemu_spice_init(void);
>  void qemu_spice_input_init(void);
>  void qemu_spice_audio_init(void);
>  void qemu_spice_display_init(DisplayState *ds);
> +int qemu_spice_display_add_client(int csock, int skipauth, int tls);

You also have to add this to the #else /* CONFIG_SPICE */ clause otherwise
it breaks the build when not building spice support.

>  int qemu_spice_add_interface(SpiceBaseInstance *sin);
>  int qemu_spice_set_passwd(const char *passwd,
>                            bool fail_if_connected, bool disconnect_if_connected);
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 5639c6f..d98863e 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -747,6 +747,19 @@ int qemu_spice_set_pw_expire(time_t expires)
>      return qemu_spice_set_ticket(false, false);
>  }
>  
> +int qemu_spice_display_add_client(int csock, int skipauth, int tls)
> +{
> +#if SPICE_SERVER_VERSION >= 0x000a01
> +    if (tls) {
> +        return spice_server_add_ssl_client(spice_server, csock, skipauth);
> +    } else {
> +        return spice_server_add_client(spice_server, csock, skipauth);
> +    }
> +#else
> +    return -1;
> +#endif
> +}
> +
>  static void spice_register_config(void)
>  {
>      qemu_add_opts(&qemu_spice_opts);
Luiz Capitulino - Feb. 10, 2012, 5:40 p.m.
On Fri, 10 Feb 2012 12:30:21 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On 02/07/12 15:38, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> > 
> > This is a followup to
> > 
> >   http://patchwork.ozlabs.org/patch/121004/
> > 
> > With the acceptance of some new APIs to libspice-server.so it
> > is possible to add support for SPICE to the 'add_client'
> > monitor command, bringing parity with VNC. Since SPICE can
> > use TLS or plain connections, the command also gains a new
> > 'tls' parameter to specify whether TLS should be attempted
> > on the injected client sockets.
> > 
> > This new feature is only enabled if building against a
> > libspice-server >= 0.10.1
> > 
> > * qmp-commands.hx: Add 'tls' parameter & missing doc for
> >   'skipauth' parameter
> > * monitor.c: Wire up SPICE for 'add_client' command
> > * ui/qemu-spice.h, ui/spice-core.c: Add qemu_spice_display_add_client
> >   API to wire up from monitor
> 
> Spice bits are sane, the monitor bits look good to me too.
> 
> Luiz?  Can you have a look at the monitor bits?  If you ack I'll go
> queue it up for the next spice update.  Or you can just grab it and
> merge via qmp tree.

Reviewed. Will need a respin. Will ack v3 and you merge it through the
spice queue.

Patch

diff --git a/monitor.c b/monitor.c
index aadbdcb..0d4daad 100644
--- a/monitor.c
+++ b/monitor.c
@@ -823,13 +823,18 @@  static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_d
     CharDriverState *s;
 
     if (strcmp(protocol, "spice") == 0) {
+        int fd = monitor_get_fd(mon, fdname);
+        int skipauth = qdict_get_try_bool(qdict, "skipauth", 0);
+        int tls = qdict_get_try_bool(qdict, "tls", 0);
         if (!using_spice) {
             /* correct one? spice isn't a device ,,, */
             qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
             return -1;
         }
-	qerror_report(QERR_ADD_CLIENT_FAILED);
-	return -1;
+        if (qemu_spice_display_add_client(fd, skipauth, tls) < 0) {
+            close(fd);
+        }
+        return 0;
 #ifdef CONFIG_VNC
     } else if (strcmp(protocol, "vnc") == 0) {
 	int fd = monitor_get_fd(mon, fdname);
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b5e2ab8..82625c8 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -910,8 +910,8 @@  EQMP
 
     {
         .name       = "add_client",
-        .args_type  = "protocol:s,fdname:s,skipauth:b?",
-        .params     = "protocol fdname skipauth",
+        .args_type  = "protocol:s,fdname:s,skipauth:b?,tls:b?",
+        .params     = "protocol fdname skipauth tls",
         .help       = "add a graphics client",
         .user_print = monitor_user_noop,
         .mhandler.cmd_new = add_graphics_client,
@@ -927,6 +927,8 @@  Arguments:
 
 - "protocol": protocol name (json-string)
 - "fdname": file descriptor name (json-string)
+- "skipauth": whether to skip authentication (json-bool)
+- "tls": whether to perform TLS (json-bool)
 
 Example:
 
diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index c35b29c..8932771 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -33,6 +33,7 @@  void qemu_spice_init(void);
 void qemu_spice_input_init(void);
 void qemu_spice_audio_init(void);
 void qemu_spice_display_init(DisplayState *ds);
+int qemu_spice_display_add_client(int csock, int skipauth, int tls);
 int qemu_spice_add_interface(SpiceBaseInstance *sin);
 int qemu_spice_set_passwd(const char *passwd,
                           bool fail_if_connected, bool disconnect_if_connected);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 5639c6f..d98863e 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -747,6 +747,19 @@  int qemu_spice_set_pw_expire(time_t expires)
     return qemu_spice_set_ticket(false, false);
 }
 
+int qemu_spice_display_add_client(int csock, int skipauth, int tls)
+{
+#if SPICE_SERVER_VERSION >= 0x000a01
+    if (tls) {
+        return spice_server_add_ssl_client(spice_server, csock, skipauth);
+    } else {
+        return spice_server_add_client(spice_server, csock, skipauth);
+    }
+#else
+    return -1;
+#endif
+}
+
 static void spice_register_config(void)
 {
     qemu_add_opts(&qemu_spice_opts);