[PATCH] tools/libxl: Added vga parameter for hvm domUs

[PATCH] tools/libxl: Added vga parameter for hvm domUs

[fantonifabio]

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

Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs

[Stefano Stabellini]

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
> 

Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs

[Ian Campbell]

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.

Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs

[Ian Jackson]

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.

Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs

[Ian Campbell]

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.

Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs

[Fabio Fantoni]

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?

Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs

[Ian Campbell]

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

Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs

[Fabio Fantoni]

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.