Re: [PATCH 1/4] libxl: populate build_info vfb in separate function

[Jim Fehlig <jfehlig@suse.com>]

Konrad Rzeszutek Wilk wrote:
> On Fri, May 08, 2015 at 04:31:02PM -0600, Jim Fehlig wrote:
>   
>> For HVM domains, vfb info must be populated in the libxl_domain_build_info
>> stuct.  Currently this is done in the libxlMakeVfbList function, but IMO
>>     
>
> struct
>   

Thanks.  I've fixed the typo in my local branch but will wait for
additional comments before posting a V2 (if necessary).

Regards,
Jim

>> it would be cleaner to populate the build_info vfb in a separate
>> libxlMakeBuildInfoVfb function.  libxlMakeVfbList would then handle only
>> vfb devices, simiar to the other libxlMake<device>List functions.
>>
>> A future patch will extend libxlMakeBuildInfoVfb to support SPICE.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>  src/libxl/libxl_conf.c | 79 ++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 48 insertions(+), 31 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index fccada5..8552c77 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -1323,37 +1323,6 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
>>      d_config->vkbs = x_vkbs;
>>      d_config->num_vfbs = d_config->num_vkbs = nvfbs;
>>  
>> -    /*
>> -     * VNC or SDL info must also be set in libxl_domain_build_info
>> -     * for HVM domains.  Use the first vfb device.
>> -     */
>> -    if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
>> -        libxl_domain_build_info *b_info = &d_config->b_info;
>> -        libxl_device_vfb vfb = d_config->vfbs[0];
>> -
>> -        if (libxl_defbool_val(vfb.vnc.enable)) {
>> -            libxl_defbool_set(&b_info->u.hvm.vnc.enable, true);
>> -            if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0)
>> -                goto error;
>> -            if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0)
>> -                goto error;
>> -            b_info->u.hvm.vnc.display = vfb.vnc.display;
>> -            libxl_defbool_set(&b_info->u.hvm.vnc.findunused,
>> -                              libxl_defbool_val(vfb.vnc.findunused));
>> -        } else if (libxl_defbool_val(vfb.sdl.enable)) {
>> -            libxl_defbool_set(&b_info->u.hvm.sdl.enable, true);
>> -            libxl_defbool_set(&b_info->u.hvm.sdl.opengl,
>> -                              libxl_defbool_val(vfb.sdl.opengl));
>> -            if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0)
>> -                goto error;
>> -            if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) < 0)
>> -                goto error;
>> -        }
>> -
>> -        if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0)
>> -            goto error;
>> -    }
>> -
>>      return 0;
>>  
>>   error:
>> @@ -1367,6 +1336,51 @@ 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.
>> + */
>> +static int
>> +libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config)
>> +{
>> +    libxl_domain_build_info *b_info = &d_config->b_info;
>> +    libxl_device_vfb x_vfb;
>> +
>> +    if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
>> +        return 0;
>> +
>> +    if (d_config->num_vfbs == 0)
>> +        return 0;
>> +
>> +    x_vfb = d_config->vfbs[0];
>> +
>> +    if (libxl_defbool_val(x_vfb.vnc.enable)) {
>> +        libxl_defbool_set(&b_info->u.hvm.vnc.enable, true);
>> +        if (VIR_STRDUP(b_info->u.hvm.vnc.listen, x_vfb.vnc.listen) < 0)
>> +            return -1;
>> +        if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, x_vfb.vnc.passwd) < 0)
>> +            return -1;
>> +        b_info->u.hvm.vnc.display = x_vfb.vnc.display;
>> +        libxl_defbool_set(&b_info->u.hvm.vnc.findunused,
>> +                          libxl_defbool_val(x_vfb.vnc.findunused));
>> +    } else if (libxl_defbool_val(x_vfb.sdl.enable)) {
>> +        libxl_defbool_set(&b_info->u.hvm.sdl.enable, true);
>> +        libxl_defbool_set(&b_info->u.hvm.sdl.opengl,
>> +                          libxl_defbool_val(x_vfb.sdl.opengl));
>> +        if (VIR_STRDUP(b_info->u.hvm.sdl.display, x_vfb.sdl.display) < 0)
>> +            return -1;
>> +        if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, x_vfb.sdl.xauthority) < 0)
>> +            return -1;
>> +    }
>> +
>> +    if (VIR_STRDUP(b_info->u.hvm.keymap, x_vfb.keymap) < 0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>>   * Get domain0 autoballoon configuration.  Honor user-specified
>>   * setting in libxl.conf first.  If not specified, autoballooning
>>   * is disabled when domain0's memory is set with 'dom0_mem'.
>> @@ -1764,6 +1778,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>>      if (libxlMakeVfbList(graphicsports, def, d_config) < 0)
>>          return -1;
>>  
>> +    if (libxlMakeBuildInfoVfb(def, d_config) < 0)
>> +        return -1;
>> +
>>      if (libxlMakePCIList(def, d_config) < 0)
>>          return -1;
>>  
>> -- 
>> 1.8.4.5
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>     
>
>