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

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 09.05.2015 00:31, Jim Fehlig wrote:
> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel