[PATCH 1/4] libxl: populate build_info vfb in separate function

[PATCH 1/4] libxl: populate build_info vfb in separate function

[Jim Fehlig]

For HVM domains, vfb info must be populated in the libxl_domain_build_info
stuct.  Currently this is done in the libxlMakeVfbList function, but IMO
it would be cleaner to populate the build_info vfb in a separate
libxlMakeBuildInfoVfb function.  libxlMakeVfbList would then handle only
vfb devices, simiar to the other libxlMake<device>List functions.

A future patch will extend libxlMakeBuildInfoVfb to support SPICE.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_conf.c | 79 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 31 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index fccada5..8552c77 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1323,37 +1323,6 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
     d_config->vkbs = x_vkbs;
     d_config->num_vfbs = d_config->num_vkbs = nvfbs;
 
-    /*
-     * VNC or SDL info must also be set in libxl_domain_build_info
-     * for HVM domains.  Use the first vfb device.
-     */
-    if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
-        libxl_domain_build_info *b_info = &d_config->b_info;
-        libxl_device_vfb vfb = d_config->vfbs[0];
-
-        if (libxl_defbool_val(vfb.vnc.enable)) {
-            libxl_defbool_set(&b_info->u.hvm.vnc.enable, true);
-            if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0)
-                goto error;
-            if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0)
-                goto error;
-            b_info->u.hvm.vnc.display = vfb.vnc.display;
-            libxl_defbool_set(&b_info->u.hvm.vnc.findunused,
-                              libxl_defbool_val(vfb.vnc.findunused));
-        } else if (libxl_defbool_val(vfb.sdl.enable)) {
-            libxl_defbool_set(&b_info->u.hvm.sdl.enable, true);
-            libxl_defbool_set(&b_info->u.hvm.sdl.opengl,
-                              libxl_defbool_val(vfb.sdl.opengl));
-            if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0)
-                goto error;
-            if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) < 0)
-                goto error;
-        }
-
-        if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0)
-            goto error;
-    }
-
     return 0;
 
  error:
@@ -1367,6 +1336,51 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
 }
 
 /*
+ * Populate vfb info in libxl_domain_build_info struct for HVM domains.
+ * Use first libxl_device_vfb device in libxl_domain_config->vfbs.
+ * Prior to calling this function, libxlMakeVfbList must be called to
+ * populate libxl_domain_config->vfbs.
+ */
+static int
+libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+    libxl_domain_build_info *b_info = &d_config->b_info;
+    libxl_device_vfb x_vfb;
+
+    if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
+        return 0;
+
+    if (d_config->num_vfbs == 0)
+        return 0;
+
+    x_vfb = d_config->vfbs[0];
+
+    if (libxl_defbool_val(x_vfb.vnc.enable)) {
+        libxl_defbool_set(&b_info->u.hvm.vnc.enable, true);
+        if (VIR_STRDUP(b_info->u.hvm.vnc.listen, x_vfb.vnc.listen) < 0)
+            return -1;
+        if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, x_vfb.vnc.passwd) < 0)
+            return -1;
+        b_info->u.hvm.vnc.display = x_vfb.vnc.display;
+        libxl_defbool_set(&b_info->u.hvm.vnc.findunused,
+                          libxl_defbool_val(x_vfb.vnc.findunused));
+    } else if (libxl_defbool_val(x_vfb.sdl.enable)) {
+        libxl_defbool_set(&b_info->u.hvm.sdl.enable, true);
+        libxl_defbool_set(&b_info->u.hvm.sdl.opengl,
+                          libxl_defbool_val(x_vfb.sdl.opengl));
+        if (VIR_STRDUP(b_info->u.hvm.sdl.display, x_vfb.sdl.display) < 0)
+            return -1;
+        if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, x_vfb.sdl.xauthority) < 0)
+            return -1;
+    }
+
+    if (VIR_STRDUP(b_info->u.hvm.keymap, x_vfb.keymap) < 0)
+        return -1;
+
+    return 0;
+}
+
+/*
  * Get domain0 autoballoon configuration.  Honor user-specified
  * setting in libxl.conf first.  If not specified, autoballooning
  * is disabled when domain0's memory is set with 'dom0_mem'.
@@ -1764,6 +1778,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
     if (libxlMakeVfbList(graphicsports, def, d_config) < 0)
         return -1;
 
+    if (libxlMakeBuildInfoVfb(def, d_config) < 0)
+        return -1;
+
     if (libxlMakePCIList(def, d_config) < 0)
         return -1;
 
-- 
1.8.4.5

[PATCH 2/4] libxl: change reservedVNCPorts to reservedGraphicsPorts

[Jim Fehlig]

A later change will use the PortAllocator for SPICE too.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_conf.h   | 2 +-
 src/libxl/libxl_domain.c | 4 ++--
 src/libxl/libxl_driver.c | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 0a1c0db..9c29b1e 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -140,7 +140,7 @@ struct _libxlDriverPrivate {
     virObjectEventStatePtr domainEventState;
 
     /* Immutable pointer, self-locking APIs */
-    virPortAllocatorPtr reservedVNCPorts;
+    virPortAllocatorPtr reservedGraphicsPorts;
 
     /* Immutable pointer, self-locking APIs */
     virPortAllocatorPtr migrationPorts;
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 5f5f8e5..68dd28e 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -715,7 +715,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
         vm->def->graphics[0]->data.vnc.autoport) {
         vnc_port = vm->def->graphics[0]->data.vnc.port;
         if (vnc_port >= LIBXL_VNC_PORT_MIN) {
-            if (virPortAllocatorRelease(driver->reservedVNCPorts,
+            if (virPortAllocatorRelease(driver->reservedGraphicsPorts,
                                         vnc_port) < 0)
                 VIR_DEBUG("Could not mark port %d as unused", vnc_port);
         }
@@ -979,7 +979,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
         VIR_FREE(managed_save_path);
     }
 
-    if (libxlBuildDomainConfig(driver->reservedVNCPorts, vm->def,
+    if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,
                                cfg->ctx, &d_config) < 0)
         goto cleanup;
 
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 60c139e..730ca77 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -410,7 +410,7 @@ libxlStateCleanup(void)
     virObjectUnref(libxl_driver->config);
     virObjectUnref(libxl_driver->xmlopt);
     virObjectUnref(libxl_driver->domains);
-    virObjectUnref(libxl_driver->reservedVNCPorts);
+    virObjectUnref(libxl_driver->reservedGraphicsPorts);
     virObjectUnref(libxl_driver->migrationPorts);
     virLockManagerPluginUnref(libxl_driver->lockManager);
 
@@ -523,7 +523,7 @@ libxlStateInitialize(bool privileged,
     }
 
     /* Allocate bitmap for vnc port reservation */
-    if (!(libxl_driver->reservedVNCPorts =
+    if (!(libxl_driver->reservedGraphicsPorts =
           virPortAllocatorNew(_("VNC"),
                               LIBXL_VNC_PORT_MIN,
                               LIBXL_VNC_PORT_MAX,
-- 
1.8.4.5

[PATCH 3/4] libxl: support SPICE graphics for HVM domains

[Jim Fehlig]

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 66 insertions(+), 6 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8552c77..5bb0425 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1337,22 +1337,82 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
 
 /*
  * Populate vfb info in libxl_domain_build_info struct for HVM domains.
- * Use first libxl_device_vfb device in libxl_domain_config->vfbs.
- * Prior to calling this function, libxlMakeVfbList must be called to
- * populate libxl_domain_config->vfbs.
+ * Prefer SPICE, otherwise use first libxl_device_vfb device in
+ * libxl_domain_config->vfbs. Prior to calling this function,
+ * libxlMakeVfbList must be called to populate libxl_domain_config->vfbs.
  */
 static int
-libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config)
+libxlMakeBuildInfoVfb(virPortAllocatorPtr graphicsports,
+                      virDomainDefPtr def,
+                      libxl_domain_config *d_config)
 {
     libxl_domain_build_info *b_info = &d_config->b_info;
     libxl_device_vfb x_vfb;
+    size_t i;
 
     if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
         return 0;
 
-    if (d_config->num_vfbs == 0)
+    if (def->ngraphics == 0)
         return 0;
 
+    for (i = 0; i < def->ngraphics; i++) {
+        virDomainGraphicsDefPtr l_vfb = def->graphics[0];
+        unsigned short port;
+        const char *listenAddr;
+
+        if (l_vfb->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE)
+            continue;
+
+        libxl_defbool_set(&b_info->u.hvm.spice.enable, true);
+
+        if (l_vfb->data.spice.autoport) {
+            if (virPortAllocatorAcquire(graphicsports, &port) < 0)
+                return -1;
+            l_vfb->data.spice.port = port;
+        }
+        b_info->u.hvm.spice.port = l_vfb->data.spice.port;
+
+        listenAddr = virDomainGraphicsListenGetAddress(l_vfb, 0);
+        if (VIR_STRDUP(b_info->u.hvm.spice.host, listenAddr) < 0)
+            return -1;
+
+        if (VIR_STRDUP(b_info->u.hvm.keymap, l_vfb->data.spice.keymap) < 0)
+            return -1;
+
+        if (l_vfb->data.spice.auth.passwd) {
+            if (VIR_STRDUP(b_info->u.hvm.spice.passwd,
+                           l_vfb->data.spice.auth.passwd) < 0)
+                return -1;
+            libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, false);
+        } else {
+            libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, true);
+        }
+
+        switch (l_vfb->data.spice.mousemode) {
+            /* client mouse mode is default in xl.cfg */
+        case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT:
+        case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT:
+            libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, true);
+            break;
+        case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER:
+            libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, false);
+            break;
+        }
+
+#ifdef LIBXL_HAVE_SPICE_VDAGENT
+        if (l_vfb->data.spice.copypaste == VIR_TRISTATE_BOOL_YES) {
+            libxl_defbool_set(&b_info->u.hvm.spice.vdagent, true);
+            libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, true);
+        } else {
+            libxl_defbool_set(&b_info->u.hvm.spice.vdagent, false);
+            libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, false);
+        }
+#endif
+
+        return 0;
+    }
+
     x_vfb = d_config->vfbs[0];
 
     if (libxl_defbool_val(x_vfb.vnc.enable)) {
@@ -1778,7 +1838,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
     if (libxlMakeVfbList(graphicsports, def, d_config) < 0)
         return -1;
 
-    if (libxlMakeBuildInfoVfb(def, d_config) < 0)
+    if (libxlMakeBuildInfoVfb(graphicsports, def, d_config) < 0)
         return -1;
 
     if (libxlMakePCIList(def, d_config) < 0)
-- 
1.8.4.5

[PATCH 4/4] libxl: support QXL video device

[Jim Fehlig]

libxl recently gained support for QXL video device.  Support
it in the libxl driver too.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---

Note:
Commit 161212ef in xen.git staging branch provides QXL support in libxl

 src/libxl/libxl_conf.c   | 11 +++++++++++
 src/libxl/libxl_domain.c |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 5bb0425..f2cffc7 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1736,6 +1736,17 @@ libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
             }
             break;
 
+#ifdef LIBXL_HAVE_QXL
+        case VIR_DOMAIN_VIDEO_TYPE_QXL:
+            b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_QXL;
+            if (def->videos[0]->vram < 128 * 1024) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("videoram must be at least 128MB for QXL"));
+                return -1;
+            }
+            break;
+#endif
+
         default:
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("video type %s is not supported by libxl"),
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 68dd28e..c7f0ed9 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -324,6 +324,10 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
                     dev->data.video->vram = 4 * 1024;
             }
             break;
+        case VIR_DOMAIN_VIDEO_TYPE_QXL:
+            if (dev->data.video->vram == 0)
+                dev->data.video->vram = 128 * 1024;
+            break;
         }
     }
 
-- 
1.8.4.5

Re: [PATCH 1/4] libxl: populate build_info vfb in separate function

[Konrad Rzeszutek Wilk]

On Fri, May 08, 2015 at 04:31:02PM -0600, Jim Fehlig wrote:
> For HVM domains, vfb info must be populated in the libxl_domain_build_info
> stuct.  Currently this is done in the libxlMakeVfbList function, but IMO

struct
> it would be cleaner to populate the build_info vfb in a separate
> libxlMakeBuildInfoVfb function.  libxlMakeVfbList would then handle only
> vfb devices, simiar to the other libxlMake<device>List functions.
> 
> A future patch will extend libxlMakeBuildInfoVfb to support SPICE.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_conf.c | 79 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 48 insertions(+), 31 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index fccada5..8552c77 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1323,37 +1323,6 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
>      d_config->vkbs = x_vkbs;
>      d_config->num_vfbs = d_config->num_vkbs = nvfbs;
>  
> -    /*
> -     * VNC or SDL info must also be set in libxl_domain_build_info
> -     * for HVM domains.  Use the first vfb device.
> -     */
> -    if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> -        libxl_domain_build_info *b_info = &d_config->b_info;
> -        libxl_device_vfb vfb = d_config->vfbs[0];
> -
> -        if (libxl_defbool_val(vfb.vnc.enable)) {
> -            libxl_defbool_set(&b_info->u.hvm.vnc.enable, true);
> -            if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0)
> -                goto error;
> -            if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0)
> -                goto error;
> -            b_info->u.hvm.vnc.display = vfb.vnc.display;
> -            libxl_defbool_set(&b_info->u.hvm.vnc.findunused,
> -                              libxl_defbool_val(vfb.vnc.findunused));
> -        } else if (libxl_defbool_val(vfb.sdl.enable)) {
> -            libxl_defbool_set(&b_info->u.hvm.sdl.enable, true);
> -            libxl_defbool_set(&b_info->u.hvm.sdl.opengl,
> -                              libxl_defbool_val(vfb.sdl.opengl));
> -            if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0)
> -                goto error;
> -            if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) < 0)
> -                goto error;
> -        }
> -
> -        if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0)
> -            goto error;
> -    }
> -
>      return 0;
>  
>   error:
> @@ -1367,6 +1336,51 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
>  }
>  
>  /*
> + * Populate vfb info in libxl_domain_build_info struct for HVM domains.
> + * Use first libxl_device_vfb device in libxl_domain_config->vfbs.
> + * Prior to calling this function, libxlMakeVfbList must be called to
> + * populate libxl_domain_config->vfbs.
> + */
> +static int
> +libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config)
> +{
> +    libxl_domain_build_info *b_info = &d_config->b_info;
> +    libxl_device_vfb x_vfb;
> +
> +    if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
> +        return 0;
> +
> +    if (d_config->num_vfbs == 0)
> +        return 0;
> +
> +    x_vfb = d_config->vfbs[0];
> +
> +    if (libxl_defbool_val(x_vfb.vnc.enable)) {
> +        libxl_defbool_set(&b_info->u.hvm.vnc.enable, true);
> +        if (VIR_STRDUP(b_info->u.hvm.vnc.listen, x_vfb.vnc.listen) < 0)
> +            return -1;
> +        if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, x_vfb.vnc.passwd) < 0)
> +            return -1;
> +        b_info->u.hvm.vnc.display = x_vfb.vnc.display;
> +        libxl_defbool_set(&b_info->u.hvm.vnc.findunused,
> +                          libxl_defbool_val(x_vfb.vnc.findunused));
> +    } else if (libxl_defbool_val(x_vfb.sdl.enable)) {
> +        libxl_defbool_set(&b_info->u.hvm.sdl.enable, true);
> +        libxl_defbool_set(&b_info->u.hvm.sdl.opengl,
> +                          libxl_defbool_val(x_vfb.sdl.opengl));
> +        if (VIR_STRDUP(b_info->u.hvm.sdl.display, x_vfb.sdl.display) < 0)
> +            return -1;
> +        if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, x_vfb.sdl.xauthority) < 0)
> +            return -1;
> +    }
> +
> +    if (VIR_STRDUP(b_info->u.hvm.keymap, x_vfb.keymap) < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +/*
>   * Get domain0 autoballoon configuration.  Honor user-specified
>   * setting in libxl.conf first.  If not specified, autoballooning
>   * is disabled when domain0's memory is set with 'dom0_mem'.
> @@ -1764,6 +1778,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>      if (libxlMakeVfbList(graphicsports, def, d_config) < 0)
>          return -1;
>  
> +    if (libxlMakeBuildInfoVfb(def, d_config) < 0)
> +        return -1;
> +
>      if (libxlMakePCIList(def, d_config) < 0)
>          return -1;
>  
> -- 
> 1.8.4.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1/4] libxl: populate build_info vfb in separate function

[Jim Fehlig]

Konrad Rzeszutek Wilk wrote:
> On Fri, May 08, 2015 at 04:31:02PM -0600, Jim Fehlig wrote:
>   
>> For HVM domains, vfb info must be populated in the libxl_domain_build_info
>> stuct.  Currently this is done in the libxlMakeVfbList function, but IMO
>>     
>
> struct
>   

Thanks.  I've fixed the typo in my local branch but will wait for
additional comments before posting a V2 (if necessary).

Regards,
Jim

>> it would be cleaner to populate the build_info vfb in a separate
>> libxlMakeBuildInfoVfb function.  libxlMakeVfbList would then handle only
>> vfb devices, simiar to the other libxlMake<device>List functions.
>>
>> A future patch will extend libxlMakeBuildInfoVfb to support SPICE.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>  src/libxl/libxl_conf.c | 79 ++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 48 insertions(+), 31 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index fccada5..8552c77 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -1323,37 +1323,6 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
>>      d_config->vkbs = x_vkbs;
>>      d_config->num_vfbs = d_config->num_vkbs = nvfbs;
>>  
>> -    /*
>> -     * VNC or SDL info must also be set in libxl_domain_build_info
>> -     * for HVM domains.  Use the first vfb device.
>> -     */
>> -    if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
>> -        libxl_domain_build_info *b_info = &d_config->b_info;
>> -        libxl_device_vfb vfb = d_config->vfbs[0];
>> -
>> -        if (libxl_defbool_val(vfb.vnc.enable)) {
>> -            libxl_defbool_set(&b_info->u.hvm.vnc.enable, true);
>> -            if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0)
>> -                goto error;
>> -            if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0)
>> -                goto error;
>> -            b_info->u.hvm.vnc.display = vfb.vnc.display;
>> -            libxl_defbool_set(&b_info->u.hvm.vnc.findunused,
>> -                              libxl_defbool_val(vfb.vnc.findunused));
>> -        } else if (libxl_defbool_val(vfb.sdl.enable)) {
>> -            libxl_defbool_set(&b_info->u.hvm.sdl.enable, true);
>> -            libxl_defbool_set(&b_info->u.hvm.sdl.opengl,
>> -                              libxl_defbool_val(vfb.sdl.opengl));
>> -            if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0)
>> -                goto error;
>> -            if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) < 0)
>> -                goto error;
>> -        }
>> -
>> -        if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0)
>> -            goto error;
>> -    }
>> -
>>      return 0;
>>  
>>   error:
>> @@ -1367,6 +1336,51 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
>>  }
>>  
>>  /*
>> + * Populate vfb info in libxl_domain_build_info struct for HVM domains.
>> + * Use first libxl_device_vfb device in libxl_domain_config->vfbs.
>> + * Prior to calling this function, libxlMakeVfbList must be called to
>> + * populate libxl_domain_config->vfbs.
>> + */
>> +static int
>> +libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config)
>> +{
>> +    libxl_domain_build_info *b_info = &d_config->b_info;
>> +    libxl_device_vfb x_vfb;
>> +
>> +    if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
>> +        return 0;
>> +
>> +    if (d_config->num_vfbs == 0)
>> +        return 0;
>> +
>> +    x_vfb = d_config->vfbs[0];
>> +
>> +    if (libxl_defbool_val(x_vfb.vnc.enable)) {
>> +        libxl_defbool_set(&b_info->u.hvm.vnc.enable, true);
>> +        if (VIR_STRDUP(b_info->u.hvm.vnc.listen, x_vfb.vnc.listen) < 0)
>> +            return -1;
>> +        if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, x_vfb.vnc.passwd) < 0)
>> +            return -1;
>> +        b_info->u.hvm.vnc.display = x_vfb.vnc.display;
>> +        libxl_defbool_set(&b_info->u.hvm.vnc.findunused,
>> +                          libxl_defbool_val(x_vfb.vnc.findunused));
>> +    } else if (libxl_defbool_val(x_vfb.sdl.enable)) {
>> +        libxl_defbool_set(&b_info->u.hvm.sdl.enable, true);
>> +        libxl_defbool_set(&b_info->u.hvm.sdl.opengl,
>> +                          libxl_defbool_val(x_vfb.sdl.opengl));
>> +        if (VIR_STRDUP(b_info->u.hvm.sdl.display, x_vfb.sdl.display) < 0)
>> +            return -1;
>> +        if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, x_vfb.sdl.xauthority) < 0)
>> +            return -1;
>> +    }
>> +
>> +    if (VIR_STRDUP(b_info->u.hvm.keymap, x_vfb.keymap) < 0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>>   * Get domain0 autoballoon configuration.  Honor user-specified
>>   * setting in libxl.conf first.  If not specified, autoballooning
>>   * is disabled when domain0's memory is set with 'dom0_mem'.
>> @@ -1764,6 +1778,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>>      if (libxlMakeVfbList(graphicsports, def, d_config) < 0)
>>          return -1;
>>  
>> +    if (libxlMakeBuildInfoVfb(def, d_config) < 0)
>> +        return -1;
>> +
>>      if (libxlMakePCIList(def, d_config) < 0)
>>          return -1;
>>  
>> -- 
>> 1.8.4.5
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>     
>
>   

Re: [libvirt] [PATCH 3/4] libxl: support SPICE graphics for HVM domains

[Michal Privoznik]

On 09.05.2015 00:31, Jim Fehlig wrote:
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 8552c77..5bb0425 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1337,22 +1337,82 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
>  
>  /*
>   * Populate vfb info in libxl_domain_build_info struct for HVM domains.
> - * Use first libxl_device_vfb device in libxl_domain_config->vfbs.
> - * Prior to calling this function, libxlMakeVfbList must be called to
> - * populate libxl_domain_config->vfbs.
> + * Prefer SPICE, otherwise use first libxl_device_vfb device in
> + * libxl_domain_config->vfbs. Prior to calling this function,
> + * libxlMakeVfbList must be called to populate libxl_domain_config->vfbs.
>   */
>  static int
> -libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config)
> +libxlMakeBuildInfoVfb(virPortAllocatorPtr graphicsports,
> +                      virDomainDefPtr def,
> +                      libxl_domain_config *d_config)
>  {
>      libxl_domain_build_info *b_info = &d_config->b_info;
>      libxl_device_vfb x_vfb;
> +    size_t i;
>  
>      if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
>          return 0;
>  
> -    if (d_config->num_vfbs == 0)
> +    if (def->ngraphics == 0)
>          return 0;
>  
> +    for (i = 0; i < def->ngraphics; i++) {
> +        virDomainGraphicsDefPtr l_vfb = def->graphics[0];

This seems really awkward to me. So you're using for() loop just to
check if the first graphics card (assuming there can't be more than one
anyway) is SPICE. If not, you could use 'continue' to continue with VNC.
I think this obfucates the code. Just move this into a separate function
and call it from here.

> +        unsigned short port;
> +        const char *listenAddr;
> +
> +        if (l_vfb->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE)
> +            continue;
> +
> +        libxl_defbool_set(&b_info->u.hvm.spice.enable, true);
> +
> +        if (l_vfb->data.spice.autoport) {
> +            if (virPortAllocatorAcquire(graphicsports, &port) < 0)
> +                return -1;
> +            l_vfb->data.spice.port = port;
> +        }
> +        b_info->u.hvm.spice.port = l_vfb->data.spice.port;
> +
> +        listenAddr = virDomainGraphicsListenGetAddress(l_vfb, 0);
> +        if (VIR_STRDUP(b_info->u.hvm.spice.host, listenAddr) < 0)
> +            return -1;
> +
> +        if (VIR_STRDUP(b_info->u.hvm.keymap, l_vfb->data.spice.keymap) < 0)
> +            return -1;
> +
> +        if (l_vfb->data.spice.auth.passwd) {
> +            if (VIR_STRDUP(b_info->u.hvm.spice.passwd,
> +                           l_vfb->data.spice.auth.passwd) < 0)
> +                return -1;
> +            libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, false);
> +        } else {
> +            libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, true);
> +        }
> +
> +        switch (l_vfb->data.spice.mousemode) {
> +            /* client mouse mode is default in xl.cfg */
> +        case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT:
> +        case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT:
> +            libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, true);
> +            break;
> +        case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER:
> +            libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, false);
> +            break;
> +        }
> +
> +#ifdef LIBXL_HAVE_SPICE_VDAGENT
> +        if (l_vfb->data.spice.copypaste == VIR_TRISTATE_BOOL_YES) {
> +            libxl_defbool_set(&b_info->u.hvm.spice.vdagent, true);
> +            libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, true);
> +        } else {
> +            libxl_defbool_set(&b_info->u.hvm.spice.vdagent, false);
> +            libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, false);
> +        }
> +#endif
> +
> +        return 0;
> +    }
> +
>      x_vfb = d_config->vfbs[0];
>  
>      if (libxl_defbool_val(x_vfb.vnc.enable)) {
> @@ -1778,7 +1838,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>      if (libxlMakeVfbList(graphicsports, def, d_config) < 0)
>          return -1;
>  
> -    if (libxlMakeBuildInfoVfb(def, d_config) < 0)
> +    if (libxlMakeBuildInfoVfb(graphicsports, def, d_config) < 0)
>          return -1;
>  
>      if (libxlMakePCIList(def, d_config) < 0)
> 

Otherwise looking good.

Michal

Re: [libvirt] [PATCH 0/4] libxl: support SPICE graphics

[Michal Privoznik]

On 09.05.2015 00:31, Jim Fehlig wrote:
> This series provides support for SPICE graphics in the libxl driver.
> The first patch is mostly code movement.  The second patch is a trivial
> name change of a structure member.  Patch3 populates the
> libxl_domain_build_info struct with SPICE info.  SPICE isn't as nice
> without QXL, so patch4 provides support for QXL video device.
> 
> Jim Fehlig (4):
>   libxl: populate build_info vfb in separate function
>   libxl: change reservedVNCPorts to reservedGraphicsPorts
>   libxl: support SPICE graphics for HVM domains
>   libxl: support QXL video device
> 
>  src/libxl/libxl_conf.c   | 150 +++++++++++++++++++++++++++++++++++++----------
>  src/libxl/libxl_conf.h   |   2 +-
>  src/libxl/libxl_domain.c |   8 ++-
>  src/libxl/libxl_driver.c |   4 +-
>  4 files changed, 128 insertions(+), 36 deletions(-)
> 

ACK to all, but please see my comment to 3/4 before pushing.

Even though we are in freeze, this has been lying around for a while
therefore I think it's safe to push these.

Michal

Re: [libvirt] [PATCH 3/4] libxl: support SPICE graphics for HVM domains

[Jim Fehlig]

> On May 28, 2015, at 4:07 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> On 09.05.2015 00:31, Jim Fehlig wrote:
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>> src/libxl/libxl_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 66 insertions(+), 6 deletions(-)
>> 
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index 8552c77..5bb0425 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -1337,22 +1337,82 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
>> 
>> /*
>>  * Populate vfb info in libxl_domain_build_info struct for HVM domains.
>> - * Use first libxl_device_vfb device in libxl_domain_config->vfbs.
>> - * Prior to calling this function, libxlMakeVfbList must be called to
>> - * populate libxl_domain_config->vfbs.
>> + * Prefer SPICE, otherwise use first libxl_device_vfb device in
>> + * libxl_domain_config->vfbs. Prior to calling this function,
>> + * libxlMakeVfbList must be called to populate libxl_domain_config->vfbs.
>>  */
>> static int
>> -libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config)
>> +libxlMakeBuildInfoVfb(virPortAllocatorPtr graphicsports,
>> +                      virDomainDefPtr def,
>> +                      libxl_domain_config *d_config)
>> {
>>     libxl_domain_build_info *b_info = &d_config->b_info;
>>     libxl_device_vfb x_vfb;
>> +    size_t i;
>> 
>>     if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
>>         return 0;
>> 
>> -    if (d_config->num_vfbs == 0)
>> +    if (def->ngraphics == 0)
>>         return 0;
>> 
>> +    for (i = 0; i < def->ngraphics; i++) {
>> +        virDomainGraphicsDefPtr l_vfb = def->graphics[0];
> 
> This seems really awkward to me. So you're using for() loop just to
> check if the first graphics card (assuming there can't be more than one
> anyway) is SPICE. If not, you could use 'continue' to continue with VNC.
> I think this obfucates the code. Just move this into a separate function
> and call it from here.

That's actually a bug, it should be def->graphics[i].  The idea is to prefer SPICE, but fall back to the first graphics device if no SPICE device is found. I mentioned this in the function comment. Perhaps that part of the comment should be moved to the for loop?

Regards,
Jim

> 
>> +        unsigned short port;
>> +        const char *listenAddr;
>> +
>> +        if (l_vfb->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE)
>> +            continue;
>> +
>> +        libxl_defbool_set(&b_info->u.hvm.spice.enable, true);
>> +
>> +        if (l_vfb->data.spice.autoport) {
>> +            if (virPortAllocatorAcquire(graphicsports, &port) < 0)
>> +                return -1;
>> +            l_vfb->data.spice.port = port;
>> +        }
>> +        b_info->u.hvm.spice.port = l_vfb->data.spice.port;
>> +
>> +        listenAddr = virDomainGraphicsListenGetAddress(l_vfb, 0);
>> +        if (VIR_STRDUP(b_info->u.hvm.spice.host, listenAddr) < 0)
>> +            return -1;
>> +
>> +        if (VIR_STRDUP(b_info->u.hvm.keymap, l_vfb->data.spice.keymap) < 0)
>> +            return -1;
>> +
>> +        if (l_vfb->data.spice.auth.passwd) {
>> +            if (VIR_STRDUP(b_info->u.hvm.spice.passwd,
>> +                           l_vfb->data.spice.auth.passwd) < 0)
>> +                return -1;
>> +            libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, false);
>> +        } else {
>> +            libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, true);
>> +        }
>> +
>> +        switch (l_vfb->data.spice.mousemode) {
>> +            /* client mouse mode is default in xl.cfg */
>> +        case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT:
>> +        case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT:
>> +            libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, true);
>> +            break;
>> +        case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER:
>> +            libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, false);
>> +            break;
>> +        }
>> +
>> +#ifdef LIBXL_HAVE_SPICE_VDAGENT
>> +        if (l_vfb->data.spice.copypaste == VIR_TRISTATE_BOOL_YES) {
>> +            libxl_defbool_set(&b_info->u.hvm.spice.vdagent, true);
>> +            libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, true);
>> +        } else {
>> +            libxl_defbool_set(&b_info->u.hvm.spice.vdagent, false);
>> +            libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, false);
>> +        }
>> +#endif
>> +
>> +        return 0;
>> +    }
>> +
>>     x_vfb = d_config->vfbs[0];
>> 
>>     if (libxl_defbool_val(x_vfb.vnc.enable)) {
>> @@ -1778,7 +1838,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>>     if (libxlMakeVfbList(graphicsports, def, d_config) < 0)
>>         return -1;
>> 
>> -    if (libxlMakeBuildInfoVfb(def, d_config) < 0)
>> +    if (libxlMakeBuildInfoVfb(graphicsports, def, d_config) < 0)
>>         return -1;
>> 
>>     if (libxlMakePCIList(def, d_config) < 0)
> 
> Otherwise looking good.
> 
> Michal
> 

Re: [libvirt] [PATCH 3/4] libxl: support SPICE graphics for HVM domains

[Michal Privoznik]

On 28.05.2015 17:45, Jim Fehlig wrote:
> 
> 
>> On May 28, 2015, at 4:07 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>>> On 09.05.2015 00:31, Jim Fehlig wrote:
>>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>>> ---
>>> src/libxl/libxl_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++-----
>>> 1 file changed, 66 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index 8552c77..5bb0425 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -1337,22 +1337,82 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
>>>
>>> /*
>>>  * Populate vfb info in libxl_domain_build_info struct for HVM domains.
>>> - * Use first libxl_device_vfb device in libxl_domain_config->vfbs.
>>> - * Prior to calling this function, libxlMakeVfbList must be called to
>>> - * populate libxl_domain_config->vfbs.
>>> + * Prefer SPICE, otherwise use first libxl_device_vfb device in
>>> + * libxl_domain_config->vfbs. Prior to calling this function,
>>> + * libxlMakeVfbList must be called to populate libxl_domain_config->vfbs.
>>>  */
>>> static int
>>> -libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config)
>>> +libxlMakeBuildInfoVfb(virPortAllocatorPtr graphicsports,
>>> +                      virDomainDefPtr def,
>>> +                      libxl_domain_config *d_config)
>>> {
>>>     libxl_domain_build_info *b_info = &d_config->b_info;
>>>     libxl_device_vfb x_vfb;
>>> +    size_t i;
>>>
>>>     if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
>>>         return 0;
>>>
>>> -    if (d_config->num_vfbs == 0)
>>> +    if (def->ngraphics == 0)
>>>         return 0;
>>>
>>> +    for (i = 0; i < def->ngraphics; i++) {
>>> +        virDomainGraphicsDefPtr l_vfb = def->graphics[0];
>>
>> This seems really awkward to me. So you're using for() loop just to
>> check if the first graphics card (assuming there can't be more than one
>> anyway) is SPICE. If not, you could use 'continue' to continue with VNC.
>> I think this obfucates the code. Just move this into a separate function
>> and call it from here.
> 
> That's actually a bug, it should be def->graphics[i].  The idea is to prefer SPICE, but fall back to the first graphics device if no SPICE device is found. I mentioned this in the function comment. Perhaps that part of the comment should be moved to the for loop?
> 

Yes, that sounds reasonable to me.

Michal

Re: [libvirt] [PATCH 0/4] libxl: support SPICE graphics

[Jim Fehlig]

On 05/28/2015 05:10 AM, Michal Privoznik wrote:
> On 09.05.2015 00:31, Jim Fehlig wrote:
>> This series provides support for SPICE graphics in the libxl driver.
>> The first patch is mostly code movement.  The second patch is a trivial
>> name change of a structure member.  Patch3 populates the
>> libxl_domain_build_info struct with SPICE info.  SPICE isn't as nice
>> without QXL, so patch4 provides support for QXL video device.
>>
>> Jim Fehlig (4):
>>    libxl: populate build_info vfb in separate function
>>    libxl: change reservedVNCPorts to reservedGraphicsPorts
>>    libxl: support SPICE graphics for HVM domains
>>    libxl: support QXL video device
>>
>>   src/libxl/libxl_conf.c   | 150 +++++++++++++++++++++++++++++++++++++----------
>>   src/libxl/libxl_conf.h   |   2 +-
>>   src/libxl/libxl_domain.c |   8 ++-
>>   src/libxl/libxl_driver.c |   4 +-
>>   4 files changed, 128 insertions(+), 36 deletions(-)
>>
> ACK to all, but please see my comment to 3/4 before pushing.
>
> Even though we are in freeze, this has been lying around for a while
> therefore I think it's safe to push these.

Thanks for the review and finding the bug in 3/4!  I changed it as we discussed 
and pushed the series.

Regards,
Jim