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