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

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