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

[Ian Campbell <Ian.Campbell@citrix.com>]

Sorry for the late response, I seem to have lost this reply on my desktop somewhere...

On Thu, 2012-06-07 at 03:34 +0100, ZhouPeng wrote:
> 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?

Yes, it is necessary.

By "bounce" I meant ask you to resubmit. IOW I wasn't going to ask you
to resubmit over the LIBXL_VGA_INTERFACE_TYPE_DEFAULT part, but I
figured since you needed to fix the ocaml stuff I would mention it.

> 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.

Actually, looking at the definition of libxl_vga_interface_type you
should avoid using 0 as a real thing and then this becomes
        libxl_vga_interface_type = Enumeration("vga_interface_type", [
        	(1, "CIRRUS"),
        	(2, "STD"),
        ])

Since the "default default" is 0 you can test for default just by
using !b_info.... here.

You'll notice that most other enums in the IDL have this property except
where the specific values come from elsewhere (like timer mode).

Several enums also include "UNKNOWN" as an explicit entry, which is much
the same as including "DEFAULT" IMHO.

> It's equal with LIBXL_MEMKB_DEFAULT in this context.

This is semantically a bit different to the enum case.

> It's the same with LIBXL_TIMER_MODE_DEFAULT

TIMER_MODE is not a good example to follow because the specific values
come from the hypervisor and libxl simple matches them.

> > 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);
> >>
> >>
> >
> >
> 
> 
>