[PATCH 0/6] Fix xl vncviewer

[PATCH 0/6] Fix xl vncviewer

[Ian Jackson]

xl vncviewer does not work at all.  In this series I fix it.

Sadly the protocol spoken by xend and qemu is very bad.  xl makes some
assumptions about the protocol spoken by qemu which are not currently
true.  In patch 1 I make some interim changes to qemu have it do what
xl already expects, including storing the password in xenstore.  (This
is no worse than xend, which stores the password in two places
already, one of which is overwritten by qemu at startup...)

Patches 2-6 are fairly straightforward bugfixes to xl.

[PATCH 1/1] vnc, xen: write vnc address and password to xenstore

[Ian Jackson]

The xend protocol as actually implemented is:
  * xend writes:
     /vm/UUID/vncpasswd = "PASS"   (n0,rDOMID)
     /local/domain/0/backend/vfb/DOMID/0/vncunused = "0"   (n0,rDOMID)
     /local/domain/0/backend/vfb/DOMID/0/vnc = "1"   (n0,rDOMID)
     /local/domain/0/backend/vfb/DOMID/0/vnclisten = "ADDR"   (n0,rDOMID)
     /local/domain/0/backend/vfb/DOMID/0/vncdisplay = "PORT"   (n0,rDOMID)
     /local/domain/0/backend/vfb/DOMID/0/vncpasswd = "PASS"   (n0,rDOMID)
  * qemu reads /vm/UUID/vncpasswd and overwrites it with "\0"
  * qemu writes
     /local/domain/DOMID/console/vnc-port = "PORT"   (n0,rDOMID)
  * xm vncviewer reads entries from backend/vfb,
    as well as console/vnc-port.
Much of this is insane.

xl quite properly does not create anything in backend/vfb for an HVM
domain with no vfb.  But xl vncviewer needs to know the port number
and the address and the password.

So, for now, have qemu write these nodes too:
     /local/domain/DOMID/console/vnc-listen = "ADDR"   (n0,rDOMID)
     /local/domain/DOMID/console/vnc-pass = "PASS"   (n0,rDOMID)
This corresponds to the protocol actually currently implemented in
libxl.

We will revisit this after the 4.1 release and invent a non-insane
protocol.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 qemu-common.h |    6 ++++++
 qemu-xen.h    |    1 -
 vl.c          |    1 -
 vnc.c         |    3 +++
 xenstore.c    |   57 ++++++++++++++++++++++++++++++++++++++++-----------------
 5 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/qemu-common.h b/qemu-common.h
index 50dfb6b..02d4cc4 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -209,4 +209,10 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count);
 
 #endif /* dyngen-exec.h hack */
 
+#include "qemu_socket.h"
+
+void xenstore_write_vncinfo(int port,
+                            const struct sockaddr *addr,
+                            socklen_t addrlen,
+                            const char *password);
 #endif
diff --git a/qemu-xen.h b/qemu-xen.h
index 0e70dbe..d50c89f 100644
--- a/qemu-xen.h
+++ b/qemu-xen.h
@@ -71,7 +71,6 @@ void xenstore_process_event(void *opaque);
 void xenstore_record_dm(const char *subpath, const char *state);
 void xenstore_record_dm_state(const char *state);
 void xenstore_check_new_media_present(int timeout);
-void xenstore_write_vncport(int vnc_display);
 void xenstore_read_vncpasswd(int domid, char *pwbuf, size_t pwbuflen);
 void xenstore_write_vslots(char *vslots);
 
diff --git a/vl.c b/vl.c
index 5f48e1f..f07a659 100644
--- a/vl.c
+++ b/vl.c
@@ -6003,7 +6003,6 @@ int main(int argc, char **argv, char **envp)
                     vnc_display_port = vnc_display_open(ds, vnc_display, vncunused);
 		    if (vnc_display_port < 0)
                         exit(1);
-		    xenstore_write_vncport(vnc_display_port);
                 }
 #if defined(CONFIG_SDL)
                 if (sdl || !vnc_display)
diff --git a/vnc.c b/vnc.c
index ba26f9e..7629dfa 100644
--- a/vnc.c
+++ b/vnc.c
@@ -2768,6 +2768,9 @@ int vnc_display_open(DisplayState *ds, const char *display, int find_unused)
 	return -1;
     }
 
+    xenstore_write_vncinfo(ntohs(iaddr.sin_port), addr, addrlen,
+                           vs->password);
+
     if (qemu_set_fd_handler2(vs->lsock, vnc_listen_poll, vnc_listen_read, NULL, vs) < 0)
 	return -1;
 
diff --git a/xenstore.c b/xenstore.c
index d364a5e..173a7c0 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -1149,32 +1149,55 @@ void xenstore_process_event(void *opaque)
     free(vec);
 }
 
-void xenstore_write_vncport(int display)
+static void xenstore_write_domain_console_item
+    (const char *item, const char *val)
 {
-    char *buf = NULL, *path;
-    char *portstr = NULL;
+    char *dompath;
+    char *path = NULL;
 
     if (xsh == NULL)
         return;
 
-    path = xs_get_domain_path(xsh, domid);
-    if (path == NULL) {
-        fprintf(logfile, "xs_get_domain_path() error\n");
-        goto out;
-    }
+    dompath = xs_get_domain_path(xsh, domid);
+    if (dompath == NULL) goto out_err;
 
-    if (pasprintf(&buf, "%s/console/vnc-port", path) == -1)
-        goto out;
-
-    if (pasprintf(&portstr, "%d", display) == -1)
-        goto out;
+    if (pasprintf(&path, "%s/console/%s", dompath, item) == -1) goto out_err;
 
-    if (xs_write(xsh, XBT_NULL, buf, portstr, strlen(portstr)) == 0)
-        fprintf(logfile, "xs_write() vncport failed\n");
+    if (xs_write(xsh, XBT_NULL, path, val, strlen(val)) == 0)
+        goto out_err;
 
  out:
-    free(portstr);
-    free(buf);
+    free(path);
+    return;
+
+ out_err:
+    fprintf(logfile, "write console item %s (%s) failed\n", item, path);
+    goto out;
+}
+
+void xenstore_write_vncinfo(int port,
+                            const struct sockaddr *addr,
+                            socklen_t addrlen,
+                            const char *password)
+{
+    char *portstr = NULL;
+    const char *addrstr;
+
+    if (pasprintf(&portstr, "%d", port) != -1) {
+        xenstore_write_domain_console_item("vnc-port", portstr);
+        free(portstr);
+    }
+
+    assert(addr->sa_family == AF_INET); 
+    addrstr = inet_ntoa(((const struct sockaddr_in*)addr)->sin_addr);
+    if (!addrstr) {
+        fprintf(logfile, "inet_ntop on vnc-addr failed\n");
+    } else {
+        xenstore_write_domain_console_item("vnc-listen", addrstr);
+    }
+
+    if (password)
+        xenstore_write_domain_console_item("vnc-pass", password);
 }
 
 void xenstore_write_vslots(char *vslots)
-- 
1.5.6.5

[PATCH 2/6] libxl: SECURITY: always honour request for vnc password

[Ian Jackson]

qemu only sets a password on its vnc display if the value for the -vnc
option has the ",password" modifier.  The code for constructing
qemu-dm options was broken and only added this modifier for one of the
cases.

Unfortunately there does not appear to be any code for passing the vnc
password to upstream qemu (ie, in the case where
libxl_build_device_model_args_new is called).  To avoid accidentally
running the domain without a password, check for this situation and
fail an assertion.  This will have to be revisited after 4.1.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_dm.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3bef49a..7518118 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -23,6 +23,7 @@
 #include <signal.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <assert.h>
 #include "libxl_utils.h"
 #include "libxl_internal.h"
 #include "libxl.h"
@@ -55,26 +56,29 @@ static char ** libxl_build_device_model_args_old(libxl__gc *gc,
         flexarray_vappend(dm_args, "-domain-name", info->dom_name, NULL);
 
     if (info->vnc || info->vncdisplay || info->vnclisten || info->vncunused) {
-        flexarray_append(dm_args, "-vnc");
+        char *vncarg;
         if (info->vncdisplay) {
             if (info->vnclisten && strchr(info->vnclisten, ':') == NULL) {
-                flexarray_append(dm_args, 
-                    libxl__sprintf(gc, "%s:%d%s",
+                vncarg = libxl__sprintf(gc, "%s:%d",
                                   info->vnclisten,
-                                  info->vncdisplay,
-                                  info->vncpasswd ? ",password" : ""));
+                                  info->vncdisplay);
             } else {
-                flexarray_append(dm_args, libxl__sprintf(gc, "127.0.0.1:%d", info->vncdisplay));
+                vncarg = libxl__sprintf(gc, "127.0.0.1:%d", info->vncdisplay);
             }
         } else if (info->vnclisten) {
             if (strchr(info->vnclisten, ':') != NULL) {
-                flexarray_append(dm_args, info->vnclisten);
+                vncarg = info->vnclisten;
             } else {
-                flexarray_append(dm_args, libxl__sprintf(gc, "%s:0", info->vnclisten));
+                vncarg = libxl__sprintf(gc, "%s:0", info->vnclisten);
             }
         } else {
-            flexarray_append(dm_args, "127.0.0.1:0");
+            vncarg = "127.0.0.1:0";
         }
+        if (info->vncpasswd)
+            vncarg = libxl__sprintf(gc, "%s,password", vncarg);
+        flexarray_append(dm_args, "-vnc");
+        flexarray_append(dm_args, vncarg);
+        
         if (info->vncunused) {
             flexarray_append(dm_args, "-vncunused");
         }
@@ -190,6 +194,9 @@ static char ** libxl_build_device_model_args_new(libxl__gc *gc,
         int display = 0;
         const char *listen = "127.0.0.1";
 
+        if (info->vncpasswd && info->vncpasswd[0]) {
+            assert(!"missing code for supplying vnc password to qemu");
+        }
         flexarray_append(dm_args, "-vnc");
 
         if (info->vncdisplay) {
-- 
1.5.6.5

[PATCH 3/6] libxl: actually print an error when execve (in libxl__exec) fails

[Ian Jackson]

The header comment says libxl__exec logs errors.  So it should do so.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_exec.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 6c21220..e770fd3 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -55,6 +55,8 @@ void libxl__exec(int stdinfd, int stdoutfd, int stderrfd, const char *arg0,
      * to assume they got DFL. */
 
     execvp(arg0, args);
+
+    fprintf(stderr, "libxl: cannot execute %s: %s\n", arg0, strerror(errno));
     _exit(-1);
 }
 
-- 
1.5.6.5

[PATCH 4/6] libxl: vncviewer: fix use-after-free

[Ian Jackson]

This bug can prevent xl vncviewer from working at all.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2d2b884..374e05e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -848,9 +848,8 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
     }
 
 skip_autopass:
-    libxl__free_all(&gc);
     libxl__exec(autopass_fd, -1, -1, args[0], args);
-    return 0;
+    abort();
 }
 
 /******************************************************************************/
-- 
1.5.6.5

[PATCH 5/6] libxl: vncviewer: unconditionally read listen port address and password

[Ian Jackson]

The /local/domain/DOMID/device/vfb/0/backend path is irrelevant.
libxl does not create it, so the branch would never be taken.

Instead, simply read the target paths of interest.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 374e05e..b386a2a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -791,7 +791,7 @@ int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
 int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
-    const char *vnc_port, *vfb_back;
+    const char *vnc_port;
     const char *vnc_listen = NULL, *vnc_pass = NULL;
     int port = 0, autopass_fd = -1;
     char *vnc_bin, *args[] = {
@@ -807,18 +807,14 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
     if ( vnc_port )
         port = atoi(vnc_port) - 5900;
 
-    vfb_back = libxl__xs_read(&gc, XBT_NULL,
-                            libxl__sprintf(&gc,
-                            "/local/domain/%d/device/vfb/0/backend", domid));
-    if ( vfb_back ) {
-        vnc_listen = libxl__xs_read(&gc, XBT_NULL,
-                            libxl__sprintf(&gc,
+    vnc_listen = libxl__xs_read(&gc, XBT_NULL,
+                                libxl__sprintf(&gc,
                             "/local/domain/%d/console/vnc-listen", domid));
-        if ( autopass )
-            vnc_pass = libxl__xs_read(&gc, XBT_NULL,
-                            libxl__sprintf(&gc,
+
+    if ( autopass )
+        vnc_pass = libxl__xs_read(&gc, XBT_NULL,
+                                  libxl__sprintf(&gc,
                             "/local/domain/%d/console/vnc-pass", domid));
-    }
 
     if ( NULL == vnc_listen )
         vnc_listen = "localhost";
-- 
1.5.6.5

[PATCH 6/6] libxl: vncviewer: make autopass work properly

[Ian Jackson]

The file we write the vnc password to must be rewound back to the
beginning, or the vnc viewer will simply get EOF.

When the syscalls for communicating the password to the vnc client
fail, bomb out with an error messsage rather than blundering on (and
probably producing a spurious password prompt).

Following this patch, xl vncviewer --autopass works, provided the qemu
patch for writing the password to xenstore has also been applied.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl.c |   25 +++++++++++++++++--------
 1 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b386a2a..5c1b3ab 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -827,23 +827,32 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
     if ( vnc_pass ) {
         char tmpname[] = "/tmp/vncautopass.XXXXXX";
         autopass_fd = mkstemp(tmpname);
-        if ( autopass_fd < 0 )
-            goto skip_autopass;
+        if ( autopass_fd < 0 ) {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "mkstemp %s failed", tmpname);
+            exit(-1);
+        }
 
-        if ( unlink(tmpname) )
+        if ( unlink(tmpname) ) {
             /* should never happen */
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unlink %s failed", tmpname);
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "unlink %s failed", tmpname);
+            exit(-1);
+        }
 
         if ( libxl_write_exactly(ctx, autopass_fd, vnc_pass, strlen(vnc_pass),
-                                    tmpname, "vnc password") ) {
-            do { close(autopass_fd); } while(errno == EINTR);
-            goto skip_autopass;
+                                    tmpname, "vnc password") )
+            exit(-1);
+
+        if ( lseek(autopass_fd, SEEK_SET, 0) ) {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "rewind %s (autopass) failed", tmpname);
+            exit(-1);
         }
 
         args[2] = "-autopass";
     }
 
-skip_autopass:
     libxl__exec(autopass_fd, -1, -1, args[0], args);
     abort();
 }
-- 
1.5.6.5

Re: [PATCH 1/1] vnc, xen: write vnc address and password to xenstore

[Stefano Stabellini]

On Tue, 1 Feb 2011, Ian Jackson wrote:
> The xend protocol as actually implemented is:
>   * xend writes:
>      /vm/UUID/vncpasswd = "PASS"   (n0,rDOMID)
>      /local/domain/0/backend/vfb/DOMID/0/vncunused = "0"   (n0,rDOMID)
>      /local/domain/0/backend/vfb/DOMID/0/vnc = "1"   (n0,rDOMID)
>      /local/domain/0/backend/vfb/DOMID/0/vnclisten = "ADDR"   (n0,rDOMID)
>      /local/domain/0/backend/vfb/DOMID/0/vncdisplay = "PORT"   (n0,rDOMID)
>      /local/domain/0/backend/vfb/DOMID/0/vncpasswd = "PASS"   (n0,rDOMID)
>   * qemu reads /vm/UUID/vncpasswd and overwrites it with "\0"
>   * qemu writes
>      /local/domain/DOMID/console/vnc-port = "PORT"   (n0,rDOMID)
>   * xm vncviewer reads entries from backend/vfb,
>     as well as console/vnc-port.
> Much of this is insane.
> 
> xl quite properly does not create anything in backend/vfb for an HVM
> domain with no vfb.  But xl vncviewer needs to know the port number
> and the address and the password.
> 
> So, for now, have qemu write these nodes too:
>      /local/domain/DOMID/console/vnc-listen = "ADDR"   (n0,rDOMID)
>      /local/domain/DOMID/console/vnc-pass = "PASS"   (n0,rDOMID)
> This corresponds to the protocol actually currently implemented in
> libxl.
> 
> We will revisit this after the 4.1 release and invent a non-insane
> protocol.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  qemu-common.h |    6 ++++++
>  qemu-xen.h    |    1 -
>  vl.c          |    1 -
>  vnc.c         |    3 +++
>  xenstore.c    |   57 ++++++++++++++++++++++++++++++++++++++++-----------------
>  5 files changed, 49 insertions(+), 19 deletions(-)
> 
> diff --git a/qemu-common.h b/qemu-common.h
> index 50dfb6b..02d4cc4 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -209,4 +209,10 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count);
>  
>  #endif /* dyngen-exec.h hack */
>  
> +#include "qemu_socket.h"
> +
> +void xenstore_write_vncinfo(int port,
> +                            const struct sockaddr *addr,
> +                            socklen_t addrlen,
> +                            const char *password);
>  #endif
> diff --git a/qemu-xen.h b/qemu-xen.h
> index 0e70dbe..d50c89f 100644
> --- a/qemu-xen.h
> +++ b/qemu-xen.h
> @@ -71,7 +71,6 @@ void xenstore_process_event(void *opaque);
>  void xenstore_record_dm(const char *subpath, const char *state);
>  void xenstore_record_dm_state(const char *state);
>  void xenstore_check_new_media_present(int timeout);
> -void xenstore_write_vncport(int vnc_display);
>  void xenstore_read_vncpasswd(int domid, char *pwbuf, size_t pwbuflen);
>  void xenstore_write_vslots(char *vslots);
>  

why did you add xenstore_write_vncinfo to qemu-common.h instead of
qemu-xen.h?


> diff --git a/vl.c b/vl.c
> index 5f48e1f..f07a659 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -6003,7 +6003,6 @@ int main(int argc, char **argv, char **envp)
>                      vnc_display_port = vnc_display_open(ds, vnc_display, vncunused);
>  		    if (vnc_display_port < 0)
>                          exit(1);
> -		    xenstore_write_vncport(vnc_display_port);
>                  }
>  #if defined(CONFIG_SDL)
>                  if (sdl || !vnc_display)
> diff --git a/vnc.c b/vnc.c
> index ba26f9e..7629dfa 100644
> --- a/vnc.c
> +++ b/vnc.c
> @@ -2768,6 +2768,9 @@ int vnc_display_open(DisplayState *ds, const char *display, int find_unused)
>  	return -1;
>      }
>  
> +    xenstore_write_vncinfo(ntohs(iaddr.sin_port), addr, addrlen,
> +                           vs->password);
> +
>      if (qemu_set_fd_handler2(vs->lsock, vnc_listen_poll, vnc_listen_read, NULL, vs) < 0)
>  	return -1;
>  
> diff --git a/xenstore.c b/xenstore.c
> index d364a5e..173a7c0 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -1149,32 +1149,55 @@ void xenstore_process_event(void *opaque)
>      free(vec);
>  }
>  
> -void xenstore_write_vncport(int display)
> +static void xenstore_write_domain_console_item
> +    (const char *item, const char *val)
>  {
> -    char *buf = NULL, *path;
> -    char *portstr = NULL;
> +    char *dompath;
> +    char *path = NULL;
>  
>      if (xsh == NULL)
>          return;
>  
> -    path = xs_get_domain_path(xsh, domid);
> -    if (path == NULL) {
> -        fprintf(logfile, "xs_get_domain_path() error\n");
> -        goto out;
> -    }
> +    dompath = xs_get_domain_path(xsh, domid);
> +    if (dompath == NULL) goto out_err;
>  
> -    if (pasprintf(&buf, "%s/console/vnc-port", path) == -1)
> -        goto out;
> -
> -    if (pasprintf(&portstr, "%d", display) == -1)
> -        goto out;
> +    if (pasprintf(&path, "%s/console/%s", dompath, item) == -1) goto out_err;
>  
> -    if (xs_write(xsh, XBT_NULL, buf, portstr, strlen(portstr)) == 0)
> -        fprintf(logfile, "xs_write() vncport failed\n");
> +    if (xs_write(xsh, XBT_NULL, path, val, strlen(val)) == 0)
> +        goto out_err;
>  
>   out:
> -    free(portstr);
> -    free(buf);
> +    free(path);
> +    return;
> +
> + out_err:
> +    fprintf(logfile, "write console item %s (%s) failed\n", item, path);
> +    goto out;
> +}
> +
> +void xenstore_write_vncinfo(int port,
> +                            const struct sockaddr *addr,
> +                            socklen_t addrlen,
> +                            const char *password)
> +{
> +    char *portstr = NULL;
> +    const char *addrstr;
> +
> +    if (pasprintf(&portstr, "%d", port) != -1) {
> +        xenstore_write_domain_console_item("vnc-port", portstr);
> +        free(portstr);
> +    }
> +
> +    assert(addr->sa_family == AF_INET); 
> +    addrstr = inet_ntoa(((const struct sockaddr_in*)addr)->sin_addr);
> +    if (!addrstr) {
> +        fprintf(logfile, "inet_ntop on vnc-addr failed\n");
> +    } else {
> +        xenstore_write_domain_console_item("vnc-listen", addrstr);
> +    }
> +
> +    if (password)
> +        xenstore_write_domain_console_item("vnc-pass", password);
>  }
>  
>  void xenstore_write_vslots(char *vslots)
> -- 
> 1.5.6.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

Re: [PATCH 5/6] libxl: vncviewer: unconditionally read listen port address and password

[Stefano Stabellini]

On Tue, 1 Feb 2011, Ian Jackson wrote:
> The /local/domain/DOMID/device/vfb/0/backend path is irrelevant.
> libxl does not create it, so the branch would never be taken.
> 
> Instead, simply read the target paths of interest.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/libxl/libxl.c |   18 +++++++-----------
>  1 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 374e05e..b386a2a 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -791,7 +791,7 @@ int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
>  int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
>  {
>      libxl__gc gc = LIBXL_INIT_GC(ctx);
> -    const char *vnc_port, *vfb_back;
> +    const char *vnc_port;
>      const char *vnc_listen = NULL, *vnc_pass = NULL;
>      int port = 0, autopass_fd = -1;
>      char *vnc_bin, *args[] = {
> @@ -807,18 +807,14 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
>      if ( vnc_port )
>          port = atoi(vnc_port) - 5900;
>  
> -    vfb_back = libxl__xs_read(&gc, XBT_NULL,
> -                            libxl__sprintf(&gc,
> -                            "/local/domain/%d/device/vfb/0/backend", domid));
> -    if ( vfb_back ) {
> -        vnc_listen = libxl__xs_read(&gc, XBT_NULL,
> -                            libxl__sprintf(&gc,
> +    vnc_listen = libxl__xs_read(&gc, XBT_NULL,
> +                                libxl__sprintf(&gc,
>                              "/local/domain/%d/console/vnc-listen", domid));
> -        if ( autopass )
> -            vnc_pass = libxl__xs_read(&gc, XBT_NULL,
> -                            libxl__sprintf(&gc,
> +
> +    if ( autopass )
> +        vnc_pass = libxl__xs_read(&gc, XBT_NULL,
> +                                  libxl__sprintf(&gc,
>                              "/local/domain/%d/console/vnc-pass", domid));
> -    }
>  

these changes don't follow the coding style (but even the original code
does not).

Re: [PATCH 6/6] libxl: vncviewer: make autopass work properly

[Stefano Stabellini]

On Tue, 1 Feb 2011, Ian Jackson wrote:
> The file we write the vnc password to must be rewound back to the
> beginning, or the vnc viewer will simply get EOF.

good catch

> 
> When the syscalls for communicating the password to the vnc client
> fail, bomb out with an error messsage rather than blundering on (and
> probably producing a spurious password prompt).
> 
> Following this patch, xl vncviewer --autopass works, provided the qemu
> patch for writing the password to xenstore has also been applied.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/libxl/libxl.c |   25 +++++++++++++++++--------
>  1 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index b386a2a..5c1b3ab 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -827,23 +827,32 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
>      if ( vnc_pass ) {
>          char tmpname[] = "/tmp/vncautopass.XXXXXX";
>          autopass_fd = mkstemp(tmpname);
> -        if ( autopass_fd < 0 )
> -            goto skip_autopass;
> +        if ( autopass_fd < 0 ) {
> +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> +                             "mkstemp %s failed", tmpname);
> +            exit(-1);
> +        }

I don't think we should call exit here, this is a library not an
executable. However instead of just skipping autopass we should avoid
exec'ing vncviewer completely and return and error.


>  
> -        if ( unlink(tmpname) )
> +        if ( unlink(tmpname) ) {
>              /* should never happen */
> -            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unlink %s failed", tmpname);
> +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> +                             "unlink %s failed", tmpname);
> +            exit(-1);
> +        }
>  

ditto

>          if ( libxl_write_exactly(ctx, autopass_fd, vnc_pass, strlen(vnc_pass),
> -                                    tmpname, "vnc password") ) {
> -            do { close(autopass_fd); } while(errno == EINTR);
> -            goto skip_autopass;
> +                                    tmpname, "vnc password") )
> +            exit(-1);

ditto

> +
> +        if ( lseek(autopass_fd, SEEK_SET, 0) ) {
> +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> +                             "rewind %s (autopass) failed", tmpname);
> +            exit(-1);

ditto

>          }
>  
>          args[2] = "-autopass";
>      }
>  
> -skip_autopass:
>      libxl__exec(autopass_fd, -1, -1, args[0], args);
>      abort();
>  }
> -- 
> 1.5.6.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

Re: [PATCH 6/6] libxl: vncviewer: make autopass work properly

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 6/6] libxl: vncviewer: make autopass work properly"):
> On Tue, 1 Feb 2011, Ian Jackson wrote:
> > +        if ( autopass_fd < 0 ) {
> > +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> > +                             "mkstemp %s failed", tmpname);
> > +            exit(-1);
> > +        }
> 
> I don't think we should call exit here, this is a library not an
> executable. However instead of just skipping autopass we should avoid
> exec'ing vncviewer completely and return and error.

The caller must already tolerate the function simply causing the
process to die, because execve can fail like that.  Ie,
libxl_exec_vncviewer is already called only after fork.  So it seemed
best to have the function always fail the same way.

If we had a function which merely returned the vnc connection info, I
would agree with you that it shouldn't exit.  We should do that in 4.2.

Ian.

Re: [PATCH 1/1] vnc, xen: write vnc address and password to xenstore

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 1/1] vnc, xen: write vnc address and password to xenstore"):
> On Tue, 1 Feb 2011, Ian Jackson wrote:
> > -void xenstore_write_vncport(int vnc_display);
> >  void xenstore_read_vncpasswd(int domid, char *pwbuf, size_t pwbuflen);
> >  void xenstore_write_vslots(char *vslots);
> >  
> why did you add xenstore_write_vncinfo to qemu-common.h instead of
> qemu-xen.h?

Because qemu-xen.h isn't included by vnc.c, and when I tried to add it
it all became a horrible doom of clashing symbols.  This seemed like
the best way out.

Ian.

Re: [PATCH 5/6] libxl: vncviewer: unconditionally read listen port address and password

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 5/6] libxl: vncviewer: unconditionally read listen port address and password"):
> On Tue, 1 Feb 2011, Ian Jackson wrote:
> > +
> > +    if ( autopass )
> > +        vnc_pass = libxl__xs_read(&gc, XBT_NULL,
> > +                                  libxl__sprintf(&gc,
> >                              "/local/domain/%d/console/vnc-pass", domid));
> > -    }
> >  
> 
> these changes don't follow the coding style (but even the original code
> does not).

Indeed.  I thought it best during the freeze to avoid making needless
changes, and also I didn't want to mix a code style cleanup with my
functional change.

Ian.

Re: [PATCH 6/6] libxl: vncviewer: make autopass work properly

[Stefano Stabellini]

On Thu, 3 Feb 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 6/6] libxl: vncviewer: make autopass work properly"):
> > On Tue, 1 Feb 2011, Ian Jackson wrote:
> > > +        if ( autopass_fd < 0 ) {
> > > +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> > > +                             "mkstemp %s failed", tmpname);
> > > +            exit(-1);
> > > +        }
> > 
> > I don't think we should call exit here, this is a library not an
> > executable. However instead of just skipping autopass we should avoid
> > exec'ing vncviewer completely and return and error.
> 
> The caller must already tolerate the function simply causing the
> process to die, because execve can fail like that.  Ie,
> libxl_exec_vncviewer is already called only after fork.  So it seemed
> best to have the function always fail the same way.
> 
> If we had a function which merely returned the vnc connection info, I
> would agree with you that it shouldn't exit.  We should do that in 4.2.

Even though it might be tolerable to have this function exits, I still
don't see any benefits as opposed to failing with an error, considering
that this function returns an integer and the long term plan would be
to return an error anyway.
It is just a matter of checking the return value in
tools/libxl/xl_cmdimpl.c:vncviewer.

Re: [PATCH 6/6] libxl: vncviewer: make autopass work properly

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 6/6] libxl: vncviewer: make autopass work properly"):
> Even though it might be tolerable to have this function exits, I still
> don't see any benefits as opposed to failing with an error, considering
> that this function returns an integer and the long term plan would be
> to return an error anyway.

Very well, how about this.

> It is just a matter of checking the return value in
> tools/libxl/xl_cmdimpl.c:vncviewer.

It already does.  (Nice to see, for a change.)

Ian.


libxl: vncviewer: make autopass work properly

The file we write the vnc password to must be rewound back to the
beginning, or the vnc viewer will simply get EOF.

When the syscalls for communicating the password to the vnc client
fail, bomb out with an error messsage rather than blundering on (and
probably producing a spurious password prompt).

Following this patch, xl vncviewer --autopass works, provided the qemu
patch for writing the password to xenstore has also been applied.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b386a2a..3a351e2 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -827,25 +827,38 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
     if ( vnc_pass ) {
         char tmpname[] = "/tmp/vncautopass.XXXXXX";
         autopass_fd = mkstemp(tmpname);
-        if ( autopass_fd < 0 )
-            goto skip_autopass;
+        if ( autopass_fd < 0 ) {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "mkstemp %s failed", tmpname);
+            goto x_fail;
+        }
 
-        if ( unlink(tmpname) )
+        if ( unlink(tmpname) ) {
             /* should never happen */
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unlink %s failed", tmpname);
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "unlink %s failed", tmpname);
+            goto x_fail;
+        }
 
         if ( libxl_write_exactly(ctx, autopass_fd, vnc_pass, strlen(vnc_pass),
-                                    tmpname, "vnc password") ) {
-            do { close(autopass_fd); } while(errno == EINTR);
-            goto skip_autopass;
+                                    tmpname, "vnc password") )
+            goto x_fail;
+
+        if ( lseek(autopass_fd, SEEK_SET, 0) ) {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "rewind %s (autopass) failed", tmpname);
+            goto x_fail; 
         }
 
         args[2] = "-autopass";
     }
 
-skip_autopass:
     libxl__exec(autopass_fd, -1, -1, args[0], args);
     abort();
+
+ x_fail:
+    libxl__free_all(&gc);
+    return ERROR_FAIL;
 }
 
 /******************************************************************************/

Re: [PATCH 6/6] libxl: vncviewer: make autopass work properly

[Stefano Stabellini]

On Fri, 4 Feb 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 6/6] libxl: vncviewer: make autopass work properly"):
> > Even though it might be tolerable to have this function exits, I still
> > don't see any benefits as opposed to failing with an error, considering
> > that this function returns an integer and the long term plan would be
> > to return an error anyway.
> 
> Very well, how about this.
> 
> > It is just a matter of checking the return value in
> > tools/libxl/xl_cmdimpl.c:vncviewer.
> 
> It already does.  (Nice to see, for a change.)
> 
> Ian.
> 
> 
> libxl: vncviewer: make autopass work properly
> 
> The file we write the vnc password to must be rewound back to the
> beginning, or the vnc viewer will simply get EOF.
> 
> When the syscalls for communicating the password to the vnc client
> fail, bomb out with an error messsage rather than blundering on (and
> probably producing a spurious password prompt).
> 
> Following this patch, xl vncviewer --autopass works, provided the qemu
> patch for writing the password to xenstore has also been applied.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

this patch and the rest of the series:

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>