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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-06-28 at 08:56 +0100, ZhouPeng wrote:
> version 4
> 
> Thanks.
> --
> 
> tools:libxl: refactor stdvga opinon support.
> 
> Be ready to add and describe new vga interface
> 
> Signed-off-by: Zhou Peng <ailvpeng25@xxxxxxxxx>

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

> 
> diff -r 592d15bd4d5e tools/libxl/libxl_create.c
> --- a/tools/libxl/libxl_create.c      Fri May 18 16:19:21 2012 +0100
> +++ b/tools/libxl/libxl_create.c      Thu Jun 28 15:50:10 2012 +0800
> @@ -189,7 +189,8 @@ int libxl__domain_build_info_setdefault(
>              if (!b_info->u.hvm.boot) return ERROR_NOMEM;
>          }
> 
> -        libxl_defbool_setdefault(&b_info->u.hvm.stdvga, false);
> +        if (!b_info->u.hvm.vga.kind)
> +            b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
>          libxl_defbool_setdefault(&b_info->u.hvm.vnc.enable, true);
>          if (libxl_defbool_val(b_info->u.hvm.vnc.enable)) {
>              libxl_defbool_setdefault(&b_info->u.hvm.vnc.findunused, true);
> diff -r 592d15bd4d5e tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c  Fri May 18 16:19:21 2012 +0100
> +++ b/tools/libxl/libxl_dm.c  Thu Jun 28 15:50:10 2012 +0800
> @@ -175,8 +175,13 @@ static char ** libxl__build_device_model
>                                     libxl__sizekb_to_mb(b_info->video_memkb)),
>                      NULL);
>          }
> -        if (libxl_defbool_val(b_info->u.hvm.stdvga)) {
> +
> +        switch (b_info->u.hvm.vga.kind) {
> +        case LIBXL_VGA_INTERFACE_TYPE_STD:
>              flexarray_append(dm_args, "-std-vga");
> +            break;
> +        case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
> +            break;
>          }
> 
>          if (b_info->u.hvm.boot) {
> @@ -418,8 +423,13 @@ static char ** libxl__build_device_model
>              flexarray_append(dm_args, spiceoptions);
>          }
> 
> -        if (libxl_defbool_val(b_info->u.hvm.stdvga)) {
> -                flexarray_vappend(dm_args, "-vga", "std", NULL);
> +        switch (b_info->u.hvm.vga.kind) {
> +        case LIBXL_VGA_INTERFACE_TYPE_STD:
> +            flexarray_vappend(dm_args, "-vga", "std", NULL);
> +            break;
> +        case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
> +            flexarray_vappend(dm_args, "-vga", "cirrus", NULL);
> +            break;
>          }
> 
>          if (b_info->u.hvm.boot) {
> 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     Thu Jun 28 15:50:10 2012 +0800
> @@ -125,9 +125,19 @@ libxl_shutdown_reason = Enumeration("shu
>      (4, "watchdog"),
>      ])
> 
> +libxl_vga_interface_type = Enumeration("vga_interface_type", [
> +    (1, "CIRRUS"),
> +    (2, "STD"),
> +    ], init_val = 0)
> +
>  #
>  # Complex libxl types
>  #
> +
> +libxl_vga_interface_info = Struct("vga_interface_info", [
> +    ("kind",    libxl_vga_interface_type),
> +    ])
> +
>  libxl_vnc_info = Struct("vnc_info", [
>      ("enable",        libxl_defbool),
>      # "address:port" that should be listened on
> @@ -286,7 +296,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 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        Thu Jun 28 15:50:10 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))
> +            b_info->u.hvm.vga.kind = l ? LIBXL_VGA_INTERFACE_TYPE_STD :
> +                                         LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> +
>          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    Thu Jun 28 15:50:10 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.kind ==
> +                                      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);
> 
> On Thu, Jun 28, 2012 at 1:33 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> > On Thu, 2012-06-28 at 02:42 +0100, ZhouPeng wrote:
> >> On Wed, Jun 27, 2012 at 11:28 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 
> >> 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.
> >
> > This is in libxl, as I said above xl should not rely on the default
> > remaining CIRRUS.
> >
> > If the user says "stdvga=0" then xl needs to explicitly ask for CIRRUS,
> > despite the fact that it currently happens to be the default.
> >
> > If we make the libxl default BLARGLE in the future then stdvga=0 still
> > needs to mean CIRRUS.
> >
> >> +        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.
> >
> 
> 
> 



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