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

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 28.05.2015 17:45, Jim Fehlig wrote:
> 
> 
>> 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?
> 

Yes, that sounds reasonable to me.

Michal

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