Re: [PATCH 1/3] libxl:refactor stdvga option support v2

[ZhouPeng <zpengxen@gmail.com>]

On Wed, Jun 27, 2012 at 11:28 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> diff -r 592d15bd4d5e tools/libxl/xl_cmdimpl.c
>> --- a/tools/libxl/xl_cmdimpl.c  Fri May 18 16:19:21 2012 +0100
>> +++ b/tools/libxl/xl_cmdimpl.c  Wed Jun 27 20:06:39 2012 +0800
>> @@ -1256,7 +1256,10 @@ skip_vfb:
>>  #undef parse_extra_args
>>
>>      if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>> -        xlu_cfg_get_defbool(config, "stdvga", &b_info->u.hvm.stdvga, 0);
>> +        if (!xlu_cfg_get_long(config, "stdvga", &l, 0))
>> +            if (l)
>> +                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
>
> I think this needs to be:
>          if (!xlu_cfg_get_long(config, "stdvga", &l, 0))
>                b_info->u.hvm.vga.kind = l ? \
>                                         LIBXL_VGA_INTERFACE_TYPE_STD : \
>                                         LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
>
> so that both "stdvga=0" and "stdvga=1" result in what the user asked for
> without relying on the libxl default remaining CIRRUS.

IMHO, this make simple to be complex.

I think the check  and set-default later in libxl_create.c as a whole is enough.
+        if (!b_info->u.hvm.vga.kind)
+            b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;

'as a whole' here, I means if more vga type added in the future, we
don't need to set the default
one by one, redundantly.
> Ian.
>

-- 
Zhou Peng