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

[ZhouPeng <zpengxen@gmail.com>]

On Wed, Jun 6, 2012 at 7:47 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2012-06-05 at 12:19 +0100, ZhouPeng wrote:
>>  # Complex libxl types
>>  #
>> +
>> +libxl_vga_interface_info = Struct("vga_interface_info", [
>> +    ("type",    libxl_vga_interface_type),
>> +    ])
>
> Unfortunately "type" is a reserved word in ocaml (and possibly other
> languages, which causes the bindings to fail to build:
Thanks for your review.

I didn't build against ocaml.
I 'make install' in tools/libxl directly.
>        make[4]: Entering directory `/local/scratch/ianc/devel/committer.git/tools/ocaml/libs/xl'
>         MLDEP
>        File "xenlight.ml", line 116, characters 2-6:
>        Error: Syntax error
>         MLI      xenlight.cmi
>        File "xenlight.mli", line 116, characters 2-6:
>        Error: Syntax error: 'end' expected
>        File "xenlight.mli", line 113, characters 28-31:
>        Error: This 'sig' might be unmatched
>
> Ideally we'd make the bindings generator do appropriate substitutions on
> keywords but the usual workaround is to s/type/kind for the field name.
>
> BTW, I wasn't going to bounce the patch over this but could

Sorry, I'm not farmiliar with the vocabulary  'bounce',
do you mean s/type/kind is necessary or not?
I think you mean necessary, right?
> LIBXL_VGA_INTERFACE_TYPE_DEFAULT not be part of the IDL definition of
> the type?
do you mean this way below?
libxl_vga_interface_type = Enumeration("vga_interface_type", [
    (-1, "DEFAULT"),
    (0, "CIRRUS"),
    (1, "STD"),
    ], init_val = "LIBXL_VGA_INTERFACE_TYPE_DEFAULT")

In my understanding, LIBXL_VGA_INTERFACE_TYPE_DEFAULT is not really
a meaningful vga type (even not present default vga to use, but just tag the
variable has never be initialized, so in meaningless state).
It's only used to check if libxl_vga_interface_type var is
initialized (whether stdvga setted by vm.cfg), to
avoid random value initialized.

It's equal with LIBXL_MEMKB_DEFAULT in this context.

It's the same with LIBXL_TIMER_MODE_DEFAULT
> I'm not sure why we don't do the same for
> LIBXL_TIMER_MODE_DEFAULT already.
>
> Ian.
>
>
>> +
>>  libxl_vnc_info = Struct("vnc_info", [
>>      ("enable",        libxl_defbool),
>>      # "address:port" that should be listened on
>> @@ -281,7 +291,7 @@ libxl_domain_build_info = Struct("domain
>>                                         ("nested_hvm",       libxl_defbool),
>>                                         ("incr_generationid",libxl_defbool),
>>                                         ("nographic",        libxl_defbool),
>> -                                       ("stdvga",           libxl_defbool),
>> +                                       ("vga",
>> libxl_vga_interface_info),
>>                                         ("vnc",              libxl_vnc_info),
>>                                         # keyboard layout, default is
>> en-us keyboard
>>                                         ("keymap",           string),
>> diff -r 6bea63e6c780 -r 7bd08f83a2ce tools/libxl/xl_cmdimpl.c
>> --- a/tools/libxl/xl_cmdimpl.c        Sat Jun 02 08:39:45 2012 +0100
>> +++ b/tools/libxl/xl_cmdimpl.c        Tue Jun 05 17:39:37 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.type = LIBXL_VGA_INTERFACE_TYPE_STD;
>> +
>>          xlu_cfg_get_defbool(config, "vnc", &b_info->u.hvm.vnc.enable, 0);
>>          xlu_cfg_replace_string (config, "vnclisten",
>>                                  &b_info->u.hvm.vnc.listen, 0);
>> diff -r 6bea63e6c780 -r 7bd08f83a2ce tools/libxl/xl_sxp.c
>> --- a/tools/libxl/xl_sxp.c    Sat Jun 02 08:39:45 2012 +0100
>> +++ b/tools/libxl/xl_sxp.c    Tue Jun 05 17:39:37 2012 +0800
>> @@ -110,8 +110,9 @@ void printf_info_sexp(int domid, libxl_d
>>                 libxl_defbool_to_string(b_info->u.hvm.nested_hvm));
>>          printf("\t\t\t(no_incr_generationid %s)\n",
>>                 libxl_defbool_to_string(b_info->u.hvm.incr_generationid));
>> -        printf("\t\t\t(stdvga %s)\n",
>> -               libxl_defbool_to_string(b_info->u.hvm.stdvga));
>> +        printf("\t\t\t(stdvga %s)\n", b_info->u.hvm.vga.type ==
>> +                                      LIBXL_VGA_INTERFACE_TYPE_STD ?
>> +                                      "True" : "False");
>>          printf("\t\t\t(vnc %s)\n",
>>                 libxl_defbool_to_string(b_info->u.hvm.vnc.enable));
>>          printf("\t\t\t(vnclisten %s)\n", b_info->u.hvm.vnc.listen);
>>
>>
>
>



-- 
Zhou Peng