[PATCH 0/2] tools: enable sdl display and xauth

[PATCH 0/2] tools: enable sdl display and xauth

[Olaf Hering]

For some reason DISPLAY and XAUTHORITY are not set by libxl. 
Thats not an issue for xl(1) because it can reuse the DISPLAY from the
calling process. But libvirtd will benefit from being able to pass the
configured display and xauth settings.

Olaf

Olaf Hering (2):
  libxl: pass environment to device model
  libxl: set DISPLAY and XAUTHORITY if sdl is enabled

 docs/man/xl.cfg.pod.5  |  7 ++--
 tools/libxl/libxl_dm.c | 86 ++++++++++++++++++++++++++++++++------------------
 2 files changed, 58 insertions(+), 35 deletions(-)

[PATCH 1/2] libxl: pass environment to device model

[Olaf Hering]

Prepare device-model setup functions to pass also environment variables
to the spawned process. This is required for upcoming changes which will
set DISPLAY and XAUTHORITY for SDL.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dm.c | 76 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 28 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 30c1578..3db12ba 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -122,9 +122,10 @@ static const char *dm_keymap(const libxl_domain_config *guest_config)
         return NULL;
 }
 
-static char ** libxl__build_device_model_args_old(libxl__gc *gc,
+static int libxl__build_device_model_args_old(libxl__gc *gc,
                                         const char *dm, int domid,
                                         const libxl_domain_config *guest_config,
+                                        char ***args, char ***envs,
                                         const libxl__domain_build_state *state)
 {
     const libxl_domain_create_info *c_info = &guest_config->c_info;
@@ -135,8 +136,9 @@ static char ** libxl__build_device_model_args_old(libxl__gc *gc,
     const int num_nics = guest_config->num_nics;
     const char *keymap = dm_keymap(guest_config);
     int i;
-    flexarray_t *dm_args;
+    flexarray_t *dm_args, *dm_envs;
     dm_args = flexarray_make(gc, 16, 1);
+    dm_envs = flexarray_make(gc, 16, 1);
 
     flexarray_vappend(dm_args, dm,
                       "-d", libxl__sprintf(gc, "%d", domid), NULL);
@@ -161,7 +163,7 @@ static char ** libxl__build_device_model_args_old(libxl__gc *gc,
             if (strchr(vnc->listen, ':') != NULL) {
                 if (vnc->display) {
                     LOG(ERROR, "vncdisplay set, vnclisten contains display");
-                    return NULL;
+                    return ERROR_INVAL;
                 }
                 vncarg = vnc->listen;
             } else {
@@ -207,14 +209,14 @@ static char ** libxl__build_device_model_args_old(libxl__gc *gc,
         if (b_info->kernel) {
             LOG(ERROR, "HVM direct kernel boot is not supported by "
                 "qemu-xen-traditional");
-            return NULL;
+            return ERROR_INVAL;
         }
 
         if (b_info->u.hvm.serial || b_info->u.hvm.serial_list) {
             if ( b_info->u.hvm.serial && b_info->u.hvm.serial_list )
             {
                 LOG(ERROR, "Both serial and serial_list set");
-                return NULL;
+                return ERROR_INVAL;
             }
             if (b_info->u.hvm.serial) {
                 flexarray_vappend(dm_args,
@@ -262,7 +264,7 @@ static char ** libxl__build_device_model_args_old(libxl__gc *gc,
             if ( b_info->u.hvm.usbdevice && b_info->u.hvm.usbdevice_list )
             {
                 LOG(ERROR, "Both usbdevice and usbdevice_list set");
-                return NULL;
+                return ERROR_INVAL;
             }
             flexarray_append(dm_args, "-usb");
             if (b_info->u.hvm.usbdevice) {
@@ -353,7 +355,11 @@ static char ** libxl__build_device_model_args_old(libxl__gc *gc,
         abort();
     }
     flexarray_append(dm_args, NULL);
-    return (char **) flexarray_contents(dm_args);
+    *args = (char **) flexarray_contents(dm_args);
+    flexarray_append(dm_envs, NULL);
+    if (envs)
+        *envs = (char **) flexarray_contents(dm_envs);
+    return 0;
 }
 
 static const char *qemu_disk_format_string(libxl_disk_format format)
@@ -416,9 +422,10 @@ static char *dm_spice_options(libxl__gc *gc,
     return opt;
 }
 
-static char ** libxl__build_device_model_args_new(libxl__gc *gc,
+static int libxl__build_device_model_args_new(libxl__gc *gc,
                                         const char *dm, int guest_domid,
                                         const libxl_domain_config *guest_config,
+                                        char ***args, char ***envs,
                                         const libxl__domain_build_state *state,
                                         int *dm_state_fd)
 {
@@ -433,12 +440,13 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     const libxl_sdl_info *sdl = dm_sdl(guest_config);
     const char *keymap = dm_keymap(guest_config);
     char *machinearg;
-    flexarray_t *dm_args;
+    flexarray_t *dm_args, *dm_envs;
     int i, connection, devid;
     uint64_t ram_size;
     const char *path, *chardev;
 
     dm_args = flexarray_make(gc, 16, 1);
+    dm_envs = flexarray_make(gc, 16, 1);
 
     flexarray_vappend(dm_args, dm,
                       "-xen-domid",
@@ -479,7 +487,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                 /* We've forgotten to add the clause */
                 LOG(ERROR, "%s: unknown channel connection %d",
                     __func__, connection);
-                return NULL;
+                return ERROR_INVAL;
         }
         flexarray_append(dm_args, "-chardev");
         flexarray_append(dm_args, (void*)chardev);
@@ -516,7 +524,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             if (strchr(vnc->listen, ':') != NULL) {
                 if (vnc->display) {
                     LOG(ERROR, "vncdisplay set, vnclisten contains display");
-                    return NULL;
+                    return ERROR_INVAL;
                 }
                 vncarg = vnc->listen;
             } else {
@@ -575,7 +583,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             if ( b_info->u.hvm.serial && b_info->u.hvm.serial_list )
             {
                 LOG(ERROR, "Both serial and serial_list set");
-                return NULL;
+                return ERROR_INVAL;
             }
             if (b_info->u.hvm.serial) {
                 flexarray_vappend(dm_args,
@@ -600,7 +608,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             const libxl_spice_info *spice = &b_info->u.hvm.spice;
             char *spiceoptions = dm_spice_options(gc, spice);
             if (!spiceoptions)
-                return NULL;
+                return ERROR_INVAL;
 
             flexarray_append(dm_args, "-spice");
             flexarray_append(dm_args, spiceoptions);
@@ -637,7 +645,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             if ( b_info->u.hvm.usbdevice && b_info->u.hvm.usbdevice_list )
             {
                 LOG(ERROR, "Both usbdevice and usbdevice_list set");
-                return NULL;
+                return ERROR_INVAL;
             }
             flexarray_append(dm_args, "-usb");
             if (b_info->u.hvm.usbdevice) {
@@ -675,7 +683,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             default:
                 LOG(ERROR, "usbversion parameter is invalid, "
                     "must be between 1 and 3");
-                return NULL;
+                return ERROR_INVAL;
             }
             if (b_info->u.hvm.spice.usbredirection >= 0 &&
                 b_info->u.hvm.spice.usbredirection < 5) {
@@ -687,7 +695,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             } else {
                 LOG(ERROR, "usbredirection parameter is invalid, "
                     "it must be between 1 and 4");
-                return NULL;
+                return ERROR_INVAL;
             }
         }
         if (b_info->u.hvm.soundhw) {
@@ -872,12 +880,17 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
         }
     }
     flexarray_append(dm_args, NULL);
-    return (char **) flexarray_contents(dm_args);
+    *args = (char **) flexarray_contents(dm_args);
+    flexarray_append(dm_envs, NULL);
+    if (envs)
+        *envs = (char **) flexarray_contents(dm_envs);
+    return 0;
 }
 
-static char ** libxl__build_device_model_args(libxl__gc *gc,
+static int libxl__build_device_model_args(libxl__gc *gc,
                                         const char *dm, int guest_domid,
                                         const libxl_domain_config *guest_config,
+                                        char ***args, char ***envs,
                                         const libxl__domain_build_state *state,
                                         int *dm_state_fd)
 /* dm_state_fd may be NULL iff caller knows we are using old stubdom
@@ -889,17 +902,19 @@ static char ** libxl__build_device_model_args(libxl__gc *gc,
     case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
         return libxl__build_device_model_args_old(gc, dm,
                                                   guest_domid, guest_config,
+                                                  args, envs,
                                                   state);
     case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
         assert(dm_state_fd != NULL);
         assert(*dm_state_fd < 0);
         return libxl__build_device_model_args_new(gc, dm,
                                                   guest_domid, guest_config,
+                                                  args, envs,
                                                   state, dm_state_fd);
     default:
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unknown device model version %d",
                          guest_config->b_info.device_model_version);
-        return NULL;
+        return ERROR_INVAL;
     }
 }
 
@@ -1109,9 +1124,10 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     if (ret)
         goto out;
 
-    args = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
-                                          guest_config, d_state, NULL);
-    if (!args) {
+    ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
+                                         guest_config, &args, NULL,
+                                         d_state, NULL);
+    if (ret) {
         ret = ERROR_FAIL;
         goto out;
     }
@@ -1393,7 +1409,7 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     char *path;
     int logfile_w, null;
     int rc;
-    char **args, **arg;
+    char **args, **arg, **envs;
     xs_transaction_t t;
     char *vm_path;
     char **pass_stuff;
@@ -1415,12 +1431,11 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
         rc = ERROR_FAIL;
         goto out;
     }
-    args = libxl__build_device_model_args(gc, dm, domid, guest_config, state,
+    rc = libxl__build_device_model_args(gc, dm, domid, guest_config,
+                                          &args, &envs, state,
                                           &dm_state_fd);
-    if (!args) {
-        rc = ERROR_FAIL;
+    if (rc)
         goto out;
-    }
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         path = xs_get_domain_path(ctx->xsh, domid);
@@ -1487,6 +1502,11 @@ retry_transaction:
     LIBXL__LOG(CTX, XTL_DEBUG, "Spawning device-model %s with arguments:", dm);
     for (arg = args; *arg; arg++)
         LIBXL__LOG(CTX, XTL_DEBUG, "  %s", *arg);
+    if (*envs) {
+        LOG(DEBUG, "Spawning device-model %s with additional environment:", dm);
+        for (arg = envs; *arg; arg += 2)
+            LOG(DEBUG, "  %s=%s", arg[0], arg[1]);
+    }
 
     spawn->what = GCSPRINTF("domain %d device model", domid);
     spawn->xspath = GCSPRINTF("/local/domain/0/device-model/%d/state", domid);
@@ -1504,7 +1524,7 @@ retry_transaction:
         goto out_close;
     if (!rc) { /* inner child */
         setsid();
-        libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL);
+        libxl__exec(gc, null, logfile_w, logfile_w, dm, args, envs);
     }
 
     rc = 0;

[PATCH 2/2] libxl: set DISPLAY and XAUTHORITY if sdl is enabled

[Olaf Hering]

With this change the following domU.cfg will show the VM window on the
local dom0 display, instead of the remote side (it via ssh -X login):
  vnc=0
  sdl=1
  display=":0"
  xauthority="/run/gdm/auth-for-olaf-nMXhOi/database"

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 docs/man/xl.cfg.pod.5  |  7 ++-----
 tools/libxl/libxl_dm.c | 10 ++++++++--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index d395e56..0bc1031 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -619,15 +619,12 @@ Simple DirectMedia Layer). The default is to not enable this mode.
 =item C<display=DISPLAY>
 
 Specifies the X Window display that should be used when the sdl option
-is used. Note: passing this value to the device-model is not currently
-implemented, so providing this option will have no effect.
+is used.
 
 =item C<xauthority=XAUTHORITY>
 
 Specifies the path to the X authority file that should be used to
-connect to the X server when the sdl option is used. Note: passing
-this value to the device-model is not currently implemented, so
-providing this option will have no effect.
+connect to the X server when the sdl option is used.
 
 =item C<opengl=BOOLEAN>
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3db12ba..18b27f8 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -196,7 +196,10 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
         if (!libxl_defbool_val(sdl->opengl)) {
             flexarray_append(dm_args, "-disable-opengl");
         }
-        /* XXX sdl->{display,xauthority} into $DISPLAY/$XAUTHORITY */
+        if (sdl->display)
+            flexarray_append_pair(dm_envs, "DISPLAY", sdl->display);
+        if (sdl->xauthority)
+            flexarray_append_pair(dm_envs, "XAUTHORITY", sdl->xauthority);
     }
     if (keymap) {
         flexarray_vappend(dm_args, "-k", keymap, NULL);
@@ -560,7 +563,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
 
     if (sdl) {
         flexarray_append(dm_args, "-sdl");
-        /* XXX sdl->{display,xauthority} into $DISPLAY/$XAUTHORITY */
+        if (sdl->display)
+            flexarray_append_pair(dm_envs, "DISPLAY", sdl->display);
+        if (sdl->xauthority)
+            flexarray_append_pair(dm_envs, "XAUTHORITY", sdl->xauthority);
     }
 
     if (keymap) {

Re: [PATCH 2/2] libxl: set DISPLAY and XAUTHORITY if sdl is enabled

[Olaf Hering]

On Mon, Apr 20, Olaf Hering wrote:

> With this change the following domU.cfg will show the VM window on the
> local dom0 display, instead of the remote side (it via ssh -X login):
>   vnc=0
>   sdl=1
>   display=":0"
>   xauthority="/run/gdm/auth-for-olaf-nMXhOi/database"

libvirt works as well with this xml config:
<graphics type='sdl' display=':0' xauth='/run/gdm/auth-for-olaf-nMXhOi/database'/>

Olaf

Re: [PATCH 1/2] libxl: pass environment to device model

[Ian Campbell]

On Mon, 2015-04-20 at 13:40 +0000, Olaf Hering wrote:
> Prepare device-model setup functions to pass also environment variables
> to the spawned process. This is required for upcoming changes which will
> set DISPLAY and XAUTHORITY for SDL.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 2/2] libxl: set DISPLAY and XAUTHORITY if sdl is enabled

[Ian Campbell]

On Mon, 2015-04-20 at 13:40 +0000, Olaf Hering wrote:
> With this change the following domU.cfg will show the VM window on the
> local dom0 display, instead of the remote side (it via ssh -X login):
>   vnc=0
>   sdl=1
>   display=":0"
>   xauthority="/run/gdm/auth-for-olaf-nMXhOi/database"

ISTR thinking in the course of
http://xenbits.xen.org/xsa/advisory-119.html that this was something of
a misfeature, but I can see now that allowing a daemon to interact with
its CLI in a way which makes this work does seem useful.

> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 0/2] tools: enable sdl display and xauth

[Ian Campbell]

On Mon, 2015-04-20 at 13:40 +0000, Olaf Hering wrote:
> For some reason DISPLAY and XAUTHORITY are not set by libxl. 
> Thats not an issue for xl(1) because it can reuse the DISPLAY from the
> calling process. But libvirtd will benefit from being able to pass the
> configured display and xauth settings.

Applied. Sorry for the delay.