On May 28, 2015, at 4:07 AM, Michal Privoznik
<mprivozn(a)redhat.com> wrote:
> On 09.05.2015 00:31, Jim Fehlig wrote:
> Signed-off-by: Jim Fehlig <jfehlig(a)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