changeset: 25453:7bd08f83a2ce
user: Zhou Peng <ailvpeng25@gmail.com>
date: Tue Jun 05 17:39:37 2012 +0800
files: tools/libxl/libxl.h tools/libxl/libxl_create.c
tools/libxl/libxl_dm.c tools/libxl/libxl_types.idl
tools/libxl/xl_cmdimpl.c tools/libxl/xl_sxp.c
description:
tools:libxl: refactor stdvga opinon support.
Be ready to add and describe new vga interface
Signed-off-by: Zhou Peng <ailvpeng25@gmail.com>
diff -r 6bea63e6c780 -r 7bd08f83a2ce tools/libxl/libxl.h
--- a/tools/libxl/libxl.h Sat Jun 02 08:39:45 2012 +0100
+++ b/tools/libxl/libxl.h Tue Jun 05 17:39:37 2012 +0800
@@ -342,6 +342,7 @@ typedef struct libxl__ctx libxl_ctx;
#define LIBXL_TIMER_MODE_DEFAULT -1
#define LIBXL_MEMKB_DEFAULT ~0ULL
+#define LIBXL_VGA_INTERFACE_TYPE_DEFAULT -1
#include "_libxl_types.h"
diff -r 6bea63e6c780 -r 7bd08f83a2ce tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c Sat Jun 02 08:39:45 2012 +0100
+++ b/tools/libxl/libxl_create.c Tue Jun 05 17:39:37 2012 +0800
@@ -193,7 +193,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.type == LIBXL_VGA_INTERFACE_TYPE_DEFAULT)
+ b_info->u.hvm.vga.type = 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 6bea63e6c780 -r 7bd08f83a2ce tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c Sat Jun 02 08:39:45 2012 +0100
+++ b/tools/libxl/libxl_dm.c Tue Jun 05 17:39:37 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.type) {
+ 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.type) {
+ 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 6bea63e6c780 -r 7bd08f83a2ce tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl Sat Jun 02 08:39:45 2012 +0100
+++ b/tools/libxl/libxl_types.idl Tue Jun 05 17:39:37 2012 +0800
@@ -125,9 +125,19 @@ libxl_shutdown_reason = Enumeration("shu
(4, "watchdog"),
])
+libxl_vga_interface_type = Enumeration("vga_interface_type", [
+ (0, "CIRRUS"),
+ (1, "STD"),
+ ], init_val = "LIBXL_VGA_INTERFACE_TYPE_DEFAULT")
+
#
# Complex libxl types
#
+
+libxl_vga_interface_info = Struct("vga_interface_info", [
+ ("type", libxl_vga_interface_type),
+ ])
+
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
ZhouPeng writes ("[Xen-devel] [PATCH 1/3] libxl:refactor stdvga option
support v2"):> description:
> tools:libxl: refactor stdvga opinon support.
>
> Be ready to add and describe new vga interface
Thanks.
If Ian C is happy with the idl changes then I am happy with this
interface change for 4.2.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
However I think patches 2 and 3 of your series come too late for the
4.2 freeze. Thanks for showing them to us, though. We should
reconsider them after 4.2.
Ian.
On Wed, Jun 6, 2012 at 6:50 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:> ZhouPeng writes ("[Xen-devel] [PATCH 1/3] libxl:refactor stdvga option support v2"): >> description: >> tools:libxl: refactor stdvga opinon support. >> >> Be ready to add and describe new vga interface > > Thanks. > > If Ian C is happy with the idl changes then I am happy with this > interface change for 4.2. > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > However I think patches 2 and 3 of your series come too late for the > 4.2 freeze. Thanks for showing them to us, though. We should > reconsider them after 4.2.It''s ok to wait until the 4.3 dev cycle. Thanks.> Ian.-- Zhou Peng
Ian Campbell
2012-Jun-06 11:47 UTC
Re: [PATCH 1/3] libxl:refactor stdvga option support v2
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: 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 LIBXL_VGA_INTERFACE_TYPE_DEFAULT not be part of the IDL definition of the type? 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); > >
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 couldSorry, 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
Ian Campbell
2012-Jun-20 18:38 UTC
Re: [PATCH 1/3] libxl:refactor stdvga option support v2
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_DEFAULTTIMER_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); > >> > >> > > > > > > >
New version patch.
-----
tools:libxl: refactor stdvga opinon support.
Be ready to add and describe new vga interface
Signed-off-by: Zhou Peng <ailvpeng25@gmail.com>
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 Wed Jun 27 20:06:39 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 Wed Jun 27 20:06:39 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 Wed Jun 27 20:06:39 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 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;
+
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 Wed Jun 27 20:06:39 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 21, 2012 at 2:38 AM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:> 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);
>> >>
>> >>
>> >
>> >
>>
>>
>>
>
>
>
>
>
>
>
>
>
--
Zhou Peng
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Ian Campbell
2012-Jun-27 15:28 UTC
Re: [PATCH 1/3] libxl:refactor stdvga option support v2
> 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. Ian.
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
Ian Campbell
2012-Jun-28 05:33 UTC
Re: [PATCH 1/3] libxl:refactor stdvga option support v2
On Thu, 2012-06-28 at 02:42 +0100, ZhouPeng wrote:> 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.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.
version 4
Thanks.
--
tools:libxl: refactor stdvga opinon support.
Be ready to add and describe new vga interface
Signed-off-by: Zhou Peng <ailvpeng25@gmail.com>
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@citrix.com>
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@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.
>
> 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.
>
--
Zhou Peng
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Ian Campbell
2012-Jun-28 08:40 UTC
Re: [PATCH 1/3] libxl:refactor stdvga option support v2
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@gmail.com>Acked-by: Ian Campbell <ian.campbell@citrix.com>> > 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@citrix.com> 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@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. > > > > 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. > > > > >
Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/3] libxl:refactor stdvga
option support v2"):> 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@gmail.com>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks,
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>