> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list