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

[Jim Fehlig <jfehlig@xxxxxxxx>]

> On May 28, 2015, at 4:07 AM, Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
> 
>> 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.

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
> 

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