Re: [Xen-devel] [PATCH]libxl:refactor the code of stdvga option support

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-05-28 at 09:52 +0100, ZhouPeng wrote:
> refactor the code of stdvga option support.
> 
> Be ready to add and describe new vga interface

Are you proposing this for 4.2, which is currently in feature freeze?

I'd be inclined to accept this particular for 4.2 due to the API change,
but I'm curious what the "new vga interface" is going to be.

> Signed-off-by: Zhou Peng <ailvpeng25@xxxxxxxxx>
> 
> diff -r 592d15bd4d5e tools/libxl/libxl_types.idl
> --- a/tools/libxl/libxl_types.idl     Fri May 18 16:19:21 2012 +0100
> +++ b/tools/libxl/libxl_types.idl     Mon May 28 16:10:02 2012 +0800
> @@ -125,9 +125,20 @@ libxl_shutdown_reason = Enumeration("shu
>      (4, "watchdog"),
>      ])
> 
> +libxl_vga_interface_type = Enumeration("vga_interface_type", [
> +    (0, "DEFAULT"),

"DEFAULT" here just means "whatever qemu gives you if you don't say
otherwise"? What actually is that? I think we'd be better off having it
as an explicit option too and using setdefault to convert DEFAULT->that.

> +    (1, "STD"),
> +    ])
> +
>  #
>  # Complex libxl types
>  #
> +
> +libxl_vga_interface_info = Struct("vga_interface_info", [
> +    ("type",    libxl_vga_interface_type),
> +    ("vramkb",  MemKB),

I can't see "vramkb" being used anywhere. Did you mean to include it in
the followup "new vga interface" patch?

> +    ])
> +
>  libxl_vnc_info = Struct("vnc_info", [
>      ("enable",        libxl_defbool),
>      # "address:port" that should be listened on
> @@ -286,7 +297,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),

Looks like this bit is whitespace damaged.

http://wiki.xen.org/wiki/Submitting_Xen_Patches has some hints on using
the mercurial patchbomb tool to avoid this and also a link to
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/email-clients.txt;hb=HEAD
which gives advice on using various mailclients to manually send patches
without mangling them.

> 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        Mon May 28 16:10:02 2012 +0800
> @@ -1256,7 +1256,12 @@ 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);
> +        libxl_defbool vga;
> +        b_info->u.hvm.vga.type = LIBXL_VGA_INTERFACE_TYPE_DEFAULT;
> +        if (!xlu_cfg_get_defbool(config, "stdvga", &vga, 0))
> +            if (libxl_defbool_val(vga))

I don't think you need to launder this through a defbool, since it is no
longer one at the libxl interface. Use xlu_cfg_get_long into &l and
then 
        if (l) 
                b_info->u.hvm.vga.type = LIBXL_VGA_INTERFACE_TYPE_STD;

> +                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 592d15bd4d5e tools/libxl/xl_sxp.c
> --- a/tools/libxl/xl_sxp.c    Fri May 18 16:19:21 2012 +0100
> +++ b/tools/libxl/xl_sxp.c    Mon May 28 16:10:02 2012 +0800
> @@ -110,8 +110,10 @@ 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));
> +        libxl_defbool stdvga;
> +        libxl_defbool_set(&stdvga,
> +                   b_info->u.hvm.vga.type == LIBXL_VGA_INTERFACE_TYPE_STD);
> +        printf("\t\t\t(stdvga %s)\n", libxl_defbool_to_string(stdvga));

Likewise I think this is just
        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);
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel