fantonifabio@tiscali.it
2013-Feb-11 10:57 UTC
[PATCH] tools/libxl: Added vga parameter for hvm domUs
From: Fabio Fantoni <fabio.fantoni@heliman.it>
Usage:
vga="stdvga"|"cirrus"
- Default option is cirrus.
- Prints error and exit if unknown value is passed.
- stdvga parameter is now deprecated.
- Updated xl.cfg man.
Required patch: Improve videoram setting v5
Is prerequisite for patch: Add qxl support v9
Signed-off-by: Fabio Fantoni <fabio.fantoni@heliman.it>
---
docs/man/xl.cfg.pod.5 | 8 +++++++-
tools/libxl/xl_cmdimpl.c | 14 +++++++++++++-
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 9c5cdcd..9862842 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -979,7 +979,7 @@ The default amount of video ram for stdvga is 8MB which is
sufficient
for e.g. 1600x1200 at 32bpp and videoram option is currently working
only when using the qemu-xen-traditional device-model.
-When using the emulated Cirrus graphics card (B<stdvga=0>)
+When using the emulated Cirrus graphics card (B<vga="cirrus">)
the amount of video ram is fixed at 4MB which is sufficient
for 1024x768 at 32 bpp and videoram option is currently working
only when using the upstream qemu-xen device-model.
@@ -991,6 +991,12 @@ emulated graphics device. The default is false which means
to emulate
a Cirrus Logic GD5446 VGA card. If your guest supports VBE 2.0 or
later (e.g. Windows XP onwards) then you should enable this.
stdvga supports more video ram and bigger resolutions than Cirrus.
+This option is deprecated, use vga="stdvga" instead.
+
+=item B<vga="STRING">
+
+Selects the emulated video card (stdvga|cirrus).
+The default is cirrus.
=item B<vnc=BOOLEAN>
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 080bbd8..f9101ba 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1469,7 +1469,19 @@ skip_vfb:
#undef parse_extra_args
if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
- if (!xlu_cfg_get_long(config, "stdvga", &l, 0))
+ if (!xlu_cfg_get_string (config, "vga", &buf, 0)) {
+ if (!strcmp(buf, "stdvga")) {
+ b_info->u.hvm.vga.kind
+ = LIBXL_VGA_INTERFACE_TYPE_STD;
+ } else if (!strcmp(buf, "cirrus")) {
+ b_info->u.hvm.vga.kind
+ = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+ } else {
+ fprintf(stderr,
+ "Unknown vga \"%s\" specified\n", buf);
+ exit(1);
+ }
+ } else 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;
--
1.7.9.5
Stefano Stabellini
2013-Feb-15 12:26 UTC
Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs
On Mon, 11 Feb 2013, fantonifabio@tiscali.it wrote:> From: Fabio Fantoni <fabio.fantoni@heliman.it> > > Usage: > vga="stdvga"|"cirrus" > > - Default option is cirrus. > - Prints error and exit if unknown value is passed. > - stdvga parameter is now deprecated. > - Updated xl.cfg man. > > Required patch: Improve videoram setting v5 > Is prerequisite for patch: Add qxl support v9 > > Signed-off-by: Fabio Fantoni <fabio.fantoni@heliman.it>Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>> docs/man/xl.cfg.pod.5 | 8 +++++++- > tools/libxl/xl_cmdimpl.c | 14 +++++++++++++- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 > index 9c5cdcd..9862842 100644 > --- a/docs/man/xl.cfg.pod.5 > +++ b/docs/man/xl.cfg.pod.5 > @@ -979,7 +979,7 @@ The default amount of video ram for stdvga is 8MB which is sufficient > for e.g. 1600x1200 at 32bpp and videoram option is currently working > only when using the qemu-xen-traditional device-model. > > -When using the emulated Cirrus graphics card (B<stdvga=0>) > +When using the emulated Cirrus graphics card (B<vga="cirrus">) > the amount of video ram is fixed at 4MB which is sufficient > for 1024x768 at 32 bpp and videoram option is currently working > only when using the upstream qemu-xen device-model. > @@ -991,6 +991,12 @@ emulated graphics device. The default is false which means to emulate > a Cirrus Logic GD5446 VGA card. If your guest supports VBE 2.0 or > later (e.g. Windows XP onwards) then you should enable this. > stdvga supports more video ram and bigger resolutions than Cirrus. > +This option is deprecated, use vga="stdvga" instead. > + > +=item B<vga="STRING"> > + > +Selects the emulated video card (stdvga|cirrus). > +The default is cirrus. > > =item B<vnc=BOOLEAN> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 080bbd8..f9101ba 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -1469,7 +1469,19 @@ skip_vfb: > #undef parse_extra_args > > if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) { > - if (!xlu_cfg_get_long(config, "stdvga", &l, 0)) > + if (!xlu_cfg_get_string (config, "vga", &buf, 0)) { > + if (!strcmp(buf, "stdvga")) { > + b_info->u.hvm.vga.kind > + = LIBXL_VGA_INTERFACE_TYPE_STD; > + } else if (!strcmp(buf, "cirrus")) { > + b_info->u.hvm.vga.kind > + = LIBXL_VGA_INTERFACE_TYPE_CIRRUS; > + } else { > + fprintf(stderr, > + "Unknown vga \"%s\" specified\n", buf); > + exit(1); > + } > + } else 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; > > -- > 1.7.9.5 >
Ian Campbell
2013-Feb-15 13:45 UTC
Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs
On Fri, 2013-02-15 at 12:26 +0000, Stefano Stabellini wrote:> On Mon, 11 Feb 2013, fantonifabio@tiscali.it wrote: > > From: Fabio Fantoni <fabio.fantoni@heliman.it> > > > > Usage: > > vga="stdvga"|"cirrus" > > > > - Default option is cirrus. > > - Prints error and exit if unknown value is passed. > > - stdvga parameter is now deprecated. > > - Updated xl.cfg man. > > > > Required patch: Improve videoram setting v5 > > Is prerequisite for patch: Add qxl support v9 > > > > Signed-off-by: Fabio Fantoni <fabio.fantoni@heliman.it> > > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>Applied, thanks.
Ian Jackson
2013-Feb-15 15:48 UTC
Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs
fantonifabio@tiscali.it writes ("[Xen-devel] [PATCH] tools/libxl: Added vga
parameter for hvm domUs"):> From: Fabio Fantoni <fabio.fantoni@heliman.it>
>
> Usage:
> vga="stdvga"|"cirrus"
>
> - Default option is cirrus.
> - Prints error and exit if unknown value is passed.
> - stdvga parameter is now deprecated.
> - Updated xl.cfg man.
>
> Required patch: Improve videoram setting v5
> Is prerequisite for patch: Add qxl support v9
The code looks good to me, but there are a couple of formatting
problems. Your wrapping of the longer lines is odd (particularly, you
fail to indent the continuations), and there''s a spurious space after
xlu_cfg_get_string. Can you please repost following the style used
elsewhere ?
Thanks,
Ian.
Ian Campbell
2013-Feb-15 16:00 UTC
Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs
On Fri, 2013-02-15 at 15:48 +0000, Ian Jackson wrote:> fantonifabio@tiscali.it writes ("[Xen-devel] [PATCH] tools/libxl: Added vga parameter for hvm domUs"): > > From: Fabio Fantoni <fabio.fantoni@heliman.it> > > > > Usage: > > vga="stdvga"|"cirrus" > > > > - Default option is cirrus. > > - Prints error and exit if unknown value is passed. > > - stdvga parameter is now deprecated. > > - Updated xl.cfg man. > > > > Required patch: Improve videoram setting v5 > > Is prerequisite for patch: Add qxl support v9 > > The code looks good to me, but there are a couple of formatting > problems. Your wrapping of the longer lines is odd (particularly, you > fail to indent the continuations), and there''s a spurious space after > xlu_cfg_get_string. Can you please repost following the style used > elsewhere ?I''m afraid I have already committed this. Sorry for not noticing these style errors. Ian.
Fabio Fantoni
2013-Feb-15 21:54 UTC
Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs
Il 15/02/2013 16:48, Ian Jackson ha scritto:> fantonifabio@tiscali.it writes ("[Xen-devel] [PATCH] tools/libxl: Added vga parameter for hvm domUs"): >> From: Fabio Fantoni <fabio.fantoni@heliman.it> >> >> Usage: >> vga="stdvga"|"cirrus" >> >> - Default option is cirrus. >> - Prints error and exit if unknown value is passed. >> - stdvga parameter is now deprecated. >> - Updated xl.cfg man. >> >> Required patch: Improve videoram setting v5 >> Is prerequisite for patch: Add qxl support v9 > The code looks good to me, but there are a couple of formatting > problems. Your wrapping of the longer lines is odd (particularly, you > fail to indent the continuations), and there''s a spurious space after > xlu_cfg_get_string. Can you please repost following the style used > elsewhere ? > > Thanks, > Ian.Sorry. Can you explain me the error of indentation did I do please?
Ian Campbell
2013-Feb-18 10:06 UTC
Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs
On Fri, 2013-02-15 at 21:54 +0000, Fabio Fantoni wrote:> Il 15/02/2013 16:48, Ian Jackson ha scritto: > > fantonifabio@tiscali.it writes ("[Xen-devel] [PATCH] tools/libxl: Added vga parameter for hvm domUs"): > >> From: Fabio Fantoni <fabio.fantoni@heliman.it> > >> > >> Usage: > >> vga="stdvga"|"cirrus" > >> > >> - Default option is cirrus. > >> - Prints error and exit if unknown value is passed. > >> - stdvga parameter is now deprecated. > >> - Updated xl.cfg man. > >> > >> Required patch: Improve videoram setting v5 > >> Is prerequisite for patch: Add qxl support v9 > > The code looks good to me, but there are a couple of formatting > > problems. Your wrapping of the longer lines is odd (particularly, you > > fail to indent the continuations), and there''s a spurious space after > > xlu_cfg_get_string. Can you please repost following the style used > > elsewhere ? > > > > Thanks, > > Ian. > Sorry. > Can you explain me the error of indentation did I do please?The spurious space after xlu_cfg_get_string, e.g. xlu_cfg_get_string (config, should have been xlu_cfg_get_string(config, However, I count 44 instances with the space and 3 without in this file, so I think you can be forgiven. This extra space has been there from day one... The more important one IMHO is: + if (!strcmp(buf, "stdvga")) { + b_info->u.hvm.vga.kind + = LIBXL_VGA_INTERFACE_TYPE_STD; I noticed this when I reviewed but when I looked at the file I concluded you were just copying a prevailing style so I let it slide. Looking again I think I was looking at the file with this patch applied, which makes that logic rather circular ;-) It seems like you were trying to avoid going over the 80 column limit, although in this case it doesn''t seem like the line would actually be that long. If wrapping is required then it would be more normal to indent as: + if (!strcmp(buf, "stdvga")) { + b_info->u.hvm.vga.kind + = LIBXL_VGA_INTERFACE_TYPE_STD; (i.e indent the wrapped line by one level). However given that all these things fit on one line using at most 72 columns: 8<------ xl: fix line wrapping oddness These lines are strangely wrapped and in any case fit within 80 columns when unwrapped. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 003b129..a80cda6 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -1476,14 +1476,11 @@ skip_vfb: if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) { if (!xlu_cfg_get_string (config, "vga", &buf, 0)) { if (!strcmp(buf, "stdvga")) { - b_info->u.hvm.vga.kind - = LIBXL_VGA_INTERFACE_TYPE_STD; + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD; } else if (!strcmp(buf, "cirrus")) { - b_info->u.hvm.vga.kind - = LIBXL_VGA_INTERFACE_TYPE_CIRRUS; + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS; } else { - fprintf(stderr, - "Unknown vga \"%s\" specified\n", buf); + fprintf(stderr, "Unknown vga \"%s\" specified\n", buf); exit(1); } } else if (!xlu_cfg_get_long(config, "stdvga", &l, 0))
Fabio Fantoni
2013-Feb-18 10:51 UTC
Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs
Il 18/02/2013 11:06, Ian Campbell ha scritto:> On Fri, 2013-02-15 at 21:54 +0000, Fabio Fantoni wrote: >> Il 15/02/2013 16:48, Ian Jackson ha scritto: >>> fantonifabio@tiscali.it writes ("[Xen-devel] [PATCH] tools/libxl: Added vga parameter for hvm domUs"): >>>> From: Fabio Fantoni <fabio.fantoni@heliman.it> >>>> >>>> Usage: >>>> vga="stdvga"|"cirrus" >>>> >>>> - Default option is cirrus. >>>> - Prints error and exit if unknown value is passed. >>>> - stdvga parameter is now deprecated. >>>> - Updated xl.cfg man. >>>> >>>> Required patch: Improve videoram setting v5 >>>> Is prerequisite for patch: Add qxl support v9 >>> The code looks good to me, but there are a couple of formatting >>> problems. Your wrapping of the longer lines is odd (particularly, you >>> fail to indent the continuations), and there''s a spurious space after >>> xlu_cfg_get_string. Can you please repost following the style used >>> elsewhere ? >>> >>> Thanks, >>> Ian. >> Sorry. >> Can you explain me the error of indentation did I do please? > The spurious space after xlu_cfg_get_string, e.g. > xlu_cfg_get_string (config, > should have been > xlu_cfg_get_string(config, > > However, I count 44 instances with the space and 3 without in this file, > so I think you can be forgiven. This extra space has been there from day > one... > > The more important one IMHO is: > > + if (!strcmp(buf, "stdvga")) { > + b_info->u.hvm.vga.kind > + = LIBXL_VGA_INTERFACE_TYPE_STD; > > I noticed this when I reviewed but when I looked at the file I concluded > you were just copying a prevailing style so I let it slide. Looking > again I think I was looking at the file with this patch applied, which > makes that logic rather circular ;-) > > It seems like you were trying to avoid going over the 80 column limit, > although in this case it doesn''t seem like the line would actually be > that long. If wrapping is required then it would be more normal to > indent as: > > + if (!strcmp(buf, "stdvga")) { > + b_info->u.hvm.vga.kind > + = LIBXL_VGA_INTERFACE_TYPE_STD; > > (i.e indent the wrapped line by one level). However given that all these > things fit on one line using at most 72 columns: > > 8<------ > xl: fix line wrapping oddness > > These lines are strangely wrapped and in any case fit within 80 columns > when unwrapped. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 003b129..a80cda6 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -1476,14 +1476,11 @@ skip_vfb: > if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) { > if (!xlu_cfg_get_string (config, "vga", &buf, 0)) { > if (!strcmp(buf, "stdvga")) { > - b_info->u.hvm.vga.kind > - = LIBXL_VGA_INTERFACE_TYPE_STD; > + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD; > } else if (!strcmp(buf, "cirrus")) { > - b_info->u.hvm.vga.kind > - = LIBXL_VGA_INTERFACE_TYPE_CIRRUS; > + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS; > } else { > - fprintf(stderr, > - "Unknown vga \"%s\" specified\n", buf); > + fprintf(stderr, "Unknown vga \"%s\" specified\n", buf); > exit(1); > } > } else if (!xlu_cfg_get_long(config, "stdvga", &l, 0)) > > > >Sorry for my coding style errors, I saw this mail only after posting v10 of qxl patch, I''ll do v11 after posting your coding style fix.