[PATCH] xl: create VFB for PV guest when VNC is specified

[PATCH] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

This replicate a Xend behavior, when you specify:

  vnc=1
  vnclisten=XXXX
  vncpasswd=XXXX

in a PV guest's config file, it creates a VFB for you.

Fixes bug #25.
http://bugs.xenproject.org/xen/bug/25

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/xl_cmdimpl.c |   42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index bd26bcc..06c516c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1357,11 +1357,12 @@ skip_nic:
         fprintf(stderr, "WARNING: vif2: netchannel2 is deprecated and not supported by xl\n");
     }
 
+    d_config->num_vfbs = 0;
+    d_config->num_vkbs = 0;
+    d_config->vfbs = NULL;
+    d_config->vkbs = NULL;
+
     if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) {
-        d_config->num_vfbs = 0;
-        d_config->num_vkbs = 0;
-        d_config->vfbs = NULL;
-        d_config->vkbs = NULL;
         while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != NULL) {
             libxl_device_vfb *vfb;
             libxl_device_vkb *vkb;
@@ -1608,6 +1609,39 @@ skip_vfb:
 
 #undef parse_extra_args
 
+    if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
+        libxl_defbool vnc_enabled;
+
+        xlu_cfg_get_defbool(config, "vnc", &vnc_enabled, 0);
+
+        if (libxl_defbool_val(vnc_enabled)) {
+            libxl_device_vfb *vfb;
+            libxl_device_vkb *vkb;
+
+            d_config->vfbs = (libxl_device_vfb *)
+                realloc(d_config->vfbs, sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1));
+            vfb = d_config->vfbs + d_config->num_vfbs;
+            libxl_device_vfb_init(vfb);
+            vfb->devid = d_config->num_vfbs;
+
+            d_config->vkbs = (libxl_device_vkb *)
+                realloc(d_config->vkbs, sizeof(libxl_device_vkb) * (d_config->num_vkbs + 1));
+            vkb = d_config->vkbs + d_config->num_vkbs;
+            libxl_device_vkb_init(vkb);
+            vkb->devid = d_config->num_vkbs;
+
+            libxl_defbool_set(&vfb->vnc.enable, true);
+            xlu_cfg_replace_string(config, "vnclisten", &vfb->vnc.listen, 0);
+            xlu_cfg_replace_string(config, "vncpasswd", &vfb->vnc.passwd, 0);
+            if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
+                vfb->vnc.display = l;
+            xlu_cfg_get_defbool(config, "vncunused", &vfb->vnc.findunused, 0);
+
+            d_config->num_vfbs++;
+            d_config->num_vkbs++;
+        }
+    }
+
     if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         if (!xlu_cfg_get_string (config, "vga", &buf, 0)) {
             if (!strcmp(buf, "stdvga")) {
-- 
1.7.10.4

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Konrad Rzeszutek Wilk]

On Fri, Dec 13, 2013 at 09:03:24PM +0000, Wei Liu wrote:
> This replicate a Xend behavior, when you specify:
> 
>   vnc=1
>   vnclisten=XXXX
>   vncpasswd=XXXX
> 
> in a PV guest's config file, it creates a VFB for you.
> 
> Fixes bug #25.
> http://bugs.xenproject.org/xen/bug/25
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Konrad Wilk <konrad.wilk@oracle.com>

Reported-by: .. my name.

Hadn't tested it yet - I can do it on Tuesday if you are OK with it.
> ---
>  tools/libxl/xl_cmdimpl.c |   42 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index bd26bcc..06c516c 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1357,11 +1357,12 @@ skip_nic:
>          fprintf(stderr, "WARNING: vif2: netchannel2 is deprecated and not supported by xl\n");
>      }
>  
> +    d_config->num_vfbs = 0;
> +    d_config->num_vkbs = 0;
> +    d_config->vfbs = NULL;
> +    d_config->vkbs = NULL;
> +
>      if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) {
> -        d_config->num_vfbs = 0;
> -        d_config->num_vkbs = 0;
> -        d_config->vfbs = NULL;
> -        d_config->vkbs = NULL;
>          while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != NULL) {
>              libxl_device_vfb *vfb;
>              libxl_device_vkb *vkb;
> @@ -1608,6 +1609,39 @@ skip_vfb:
>  
>  #undef parse_extra_args
>  
> +    if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
> +        libxl_defbool vnc_enabled;
> +
> +        xlu_cfg_get_defbool(config, "vnc", &vnc_enabled, 0);
> +
> +        if (libxl_defbool_val(vnc_enabled)) {
> +            libxl_device_vfb *vfb;
> +            libxl_device_vkb *vkb;
> +
> +            d_config->vfbs = (libxl_device_vfb *)
> +                realloc(d_config->vfbs, sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1));
> +            vfb = d_config->vfbs + d_config->num_vfbs;
> +            libxl_device_vfb_init(vfb);
> +            vfb->devid = d_config->num_vfbs;
> +
> +            d_config->vkbs = (libxl_device_vkb *)
> +                realloc(d_config->vkbs, sizeof(libxl_device_vkb) * (d_config->num_vkbs + 1));
> +            vkb = d_config->vkbs + d_config->num_vkbs;
> +            libxl_device_vkb_init(vkb);
> +            vkb->devid = d_config->num_vkbs;
> +
> +            libxl_defbool_set(&vfb->vnc.enable, true);
> +            xlu_cfg_replace_string(config, "vnclisten", &vfb->vnc.listen, 0);
> +            xlu_cfg_replace_string(config, "vncpasswd", &vfb->vnc.passwd, 0);
> +            if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
> +                vfb->vnc.display = l;
> +            xlu_cfg_get_defbool(config, "vncunused", &vfb->vnc.findunused, 0);
> +
> +            d_config->num_vfbs++;
> +            d_config->num_vkbs++;
> +        }
> +    }
> +
>      if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>          if (!xlu_cfg_get_string (config, "vga", &buf, 0)) {
>              if (!strcmp(buf, "stdvga")) {
> -- 
> 1.7.10.4
> 

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

On Sat, Dec 14, 2013 at 5:27 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Fri, Dec 13, 2013 at 09:03:24PM +0000, Wei Liu wrote:
>> This replicate a Xend behavior, when you specify:
>>
>>   vnc=1
>>   vnclisten=XXXX
>>   vncpasswd=XXXX
>>
>> in a PV guest's config file, it creates a VFB for you.
>>
>> Fixes bug #25.
>> http://bugs.xenproject.org/xen/bug/25
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Konrad Wilk <konrad.wilk@oracle.com>
>
> Reported-by: .. my name.
>
> Hadn't tested it yet - I can do it on Tuesday if you are OK with it.

Yes, I'm fine with it. Take your time.

Wei.

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Ian Jackson]

Wei Liu writes ("[PATCH] xl: create VFB for PV guest when VNC is specified"):
> This replicate a Xend behavior, when you specify:

Thanks.  I think the principle is sound.  I have some quibbles.

> +    if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
> +        libxl_defbool vnc_enabled;
> +
> +        xlu_cfg_get_defbool(config, "vnc", &vnc_enabled, 0);

Missing error check.

> +        if (libxl_defbool_val(vnc_enabled)) {
> +            libxl_device_vfb *vfb;
> +            libxl_device_vkb *vkb;
> +
> +            d_config->vfbs = (libxl_device_vfb *)
> +                realloc(d_config->vfbs, sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1));

This should be xrealloc, and wrapped to 80 columns.  But see below.

> +            vfb = d_config->vfbs + d_config->num_vfbs;
> +            libxl_device_vfb_init(vfb);
> +            vfb->devid = d_config->num_vfbs;
> +
> +            d_config->vkbs = (libxl_device_vkb *)
> +                realloc(d_config->vkbs, sizeof(libxl_device_vkb) * (d_config->num_vkbs + 1));
> +            vkb = d_config->vkbs + d_config->num_vkbs;
> +            libxl_device_vkb_init(vkb);
> +            vkb->devid = d_config->num_vkbs;
> +
> +            libxl_defbool_set(&vfb->vnc.enable, true);

There's a lot of this kind of boilerplate.  I'm tempted to suggest
making a macro to do this.  Searching for "devid =" found 4 call sites
if that line is included in the macro; searching for "realloc" found
me 6 call sites if it isn't.  And that's before your two additional
ones.  Ian C, what do you think ?

> +            xlu_cfg_replace_string(config, "vnclisten", &vfb->vnc.listen, 0);
> +            xlu_cfg_replace_string(config, "vncpasswd", &vfb->vnc.passwd, 0);
> +            if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
> +                vfb->vnc.display = l;
> +            xlu_cfg_get_defbool(config, "vncunused", &vfb->vnc.findunused, 0);

This duplicates the HVM VNC option parsing.  It should be factored out
into a subroutine.

> +
> +            d_config->num_vfbs++;
> +            d_config->num_vkbs++;
> +        }
> +    }

Thanks,
Ian.

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Ian Campbell]

On Mon, 2013-12-16 at 12:17 +0000, Ian Jackson wrote:
> Wei Liu writes ("[PATCH] xl: create VFB for PV guest when VNC is specified"):
> > This replicate a Xend behavior, when you specify:
> 
> Thanks.  I think the principle is sound.  I have some quibbles.
> 
> > +    if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
> > +        libxl_defbool vnc_enabled;
> > +
> > +        xlu_cfg_get_defbool(config, "vnc", &vnc_enabled, 0);
> 
> Missing error check.
> 
> > +        if (libxl_defbool_val(vnc_enabled)) {

The result of xlu_cfg_get_defbool can be neither true nor false, it can
be "the default", which will cause this function to fail (abort even?).

I think you want xlu_cfg_get_long here and to use the normal boolean
logic.



> > +            libxl_device_vfb *vfb;
> > +            libxl_device_vkb *vkb;
> > +
> > +            d_config->vfbs = (libxl_device_vfb *)
> > +                realloc(d_config->vfbs, sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1));
> 
> This should be xrealloc, and wrapped to 80 columns.  But see below.

This also suggests that we might be trying to support both the
"toplevel" vnc options and the "vfb = []" style at the same time.

IMHO the vfb option should take precedence -- i.e. we should ignore the
toplevel options if it is present.

The alternative would be some sort of union of the toplevel options and
the first vfb given, but that sounds a bit mad...

I suppose there is also the question of what xend did here.

> > +            vfb = d_config->vfbs + d_config->num_vfbs;
> > +            libxl_device_vfb_init(vfb);
> > +            vfb->devid = d_config->num_vfbs;
> > +
> > +            d_config->vkbs = (libxl_device_vkb *)
> > +                realloc(d_config->vkbs, sizeof(libxl_device_vkb) * (d_config->num_vkbs + 1));
> > +            vkb = d_config->vkbs + d_config->num_vkbs;
> > +            libxl_device_vkb_init(vkb);
> > +            vkb->devid = d_config->num_vkbs;
> > +
> > +            libxl_defbool_set(&vfb->vnc.enable, true);
> 
> There's a lot of this kind of boilerplate.  I'm tempted to suggest
> making a macro to do this.  Searching for "devid =" found 4 call sites
> if that line is included in the macro; searching for "realloc" found
> me 6 call sites if it isn't.  And that's before your two additional
> ones.  Ian C, what do you think ?

Some helpers for dealing with allocating and resizing/appending to libxl
Array types might be a useful addition to the library itself, i.e. idl
generated? Otherwise an xl macro might indeed be handy.

I'm not sure what the 4 vs. 6 "if that line is/isn't included in" is
referring too though.

> > +            xlu_cfg_replace_string(config, "vnclisten", &vfb->vnc.listen, 0);
> > +            xlu_cfg_replace_string(config, "vncpasswd", &vfb->vnc.passwd, 0);
> > +            if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
> > +                vfb->vnc.display = l;
> > +            xlu_cfg_get_defbool(config, "vncunused", &vfb->vnc.findunused, 0);
> 
> This duplicates the HVM VNC option parsing.  It should be factored out
> into a subroutine.

Ack.

That makes me wonder if the top level options shouldn't be populating
&b_info->u.pv.vnc.* (and if that shouldn't have been a non-keyed field)
while vfb = fills in d_config->vfbs. That is a bigger change on the
libxl side though and maybe doesn't make quite as much sense as with
the .

> 
> > +
> > +            d_config->num_vfbs++;
> > +            d_config->num_vkbs++;
> > +        }
> > +    }
> 
> Thanks,
> Ian.
> 

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] xl: create VFB for PV guest when VNC is specified"):
> On Mon, 2013-12-16 at 12:17 +0000, Ian Jackson wrote:
> > This should be xrealloc, and wrapped to 80 columns.  But see below.
> 
> This also suggests that we might be trying to support both the
> "toplevel" vnc options and the "vfb = []" style at the same time.
> 
> IMHO the vfb option should take precedence -- i.e. we should ignore the
> toplevel options if it is present.

Oh, yes.  Good point.

> The alternative would be some sort of union of the toplevel options and
> the first vfb given, but that sounds a bit mad...
> 
> I suppose there is also the question of what xend did here.

Perhaps Konrad can let us know...

> > There's a lot of this kind of boilerplate.  I'm tempted to suggest
> > making a macro to do this.  Searching for "devid =" found 4 call sites
> > if that line is included in the macro; searching for "realloc" found
> > me 6 call sites if it isn't.  And that's before your two additional
> > ones.  Ian C, what do you think ?
> 
> Some helpers for dealing with allocating and resizing/appending to libxl
> Array types might be a useful addition to the library itself, i.e. idl
> generated? Otherwise an xl macro might indeed be handy.
> 
> I'm not sure what the 4 vs. 6 "if that line is/isn't included in" is
> referring too though.

There are four instances of this kind of boilerplate which contain
            SPONGE->devid = d_config->num_SPONGEs;
and six that lack it.

> > This duplicates the HVM VNC option parsing.  It should be factored out
> > into a subroutine.
> 
> Ack.
> 
> That makes me wonder if the top level options shouldn't be populating
> &b_info->u.pv.vnc.* (and if that shouldn't have been a non-keyed field)
> while vfb = fills in d_config->vfbs. That is a bigger change on the
> libxl side though and maybe doesn't make quite as much sense as with
> the .

As with the "." ?

Ian.

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Ian Campbell]

On Mon, 2013-12-16 at 12:42 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH] xl: create VFB for PV guest when VNC is specified"):
> > On Mon, 2013-12-16 at 12:17 +0000, Ian Jackson wrote:
> > > This should be xrealloc, and wrapped to 80 columns.  But see below.
> > 
> > This also suggests that we might be trying to support both the
> > "toplevel" vnc options and the "vfb = []" style at the same time.
> > 
> > IMHO the vfb option should take precedence -- i.e. we should ignore the
> > toplevel options if it is present.
> 
> Oh, yes.  Good point.
> 
> > The alternative would be some sort of union of the toplevel options and
> > the first vfb given, but that sounds a bit mad...
> > 
> > I suppose there is also the question of what xend did here.
> 
> Perhaps Konrad can let us know...
> 
> > > There's a lot of this kind of boilerplate.  I'm tempted to suggest
> > > making a macro to do this.  Searching for "devid =" found 4 call sites
> > > if that line is included in the macro; searching for "realloc" found
> > > me 6 call sites if it isn't.  And that's before your two additional
> > > ones.  Ian C, what do you think ?
> > 
> > Some helpers for dealing with allocating and resizing/appending to libxl
> > Array types might be a useful addition to the library itself, i.e. idl
> > generated? Otherwise an xl macro might indeed be handy.
> > 
> > I'm not sure what the 4 vs. 6 "if that line is/isn't included in" is
> > referring too though.
> 
> There are four instances of this kind of boilerplate which contain
>             SPONGE->devid = d_config->num_SPONGEs;
> and six that lack it.

Oh right, yes.

> > > This duplicates the HVM VNC option parsing.  It should be factored out
> > > into a subroutine.
> > 
> > Ack.
> > 
> > That makes me wonder if the top level options shouldn't be populating
> > &b_info->u.pv.vnc.* (and if that shouldn't have been a non-keyed field)
> > while vfb = fills in d_config->vfbs. That is a bigger change on the
> > libxl side though and maybe doesn't make quite as much sense as with
> > the .
> 
> As with the "." ?

Erm... I think I was about to say something which was nonsense and only
partially aborted...

What I *should* have said is that HVM has an emulated VGA card which is
what &b_info->u.hvm.vnc is configuring and which is separate to any PVFB
given to the guest. So it doesn't make sense to have b_info->u.pv.vnc
because there is no VGA.

(Incidentally, I expect even xend only meant for the toplevel vnc
options to configure the VGA and the leakage into PV guest's pvfb[0] is
just an unfortunate art we need to deal with)

Ian.

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

On Mon, Dec 16, 2013 at 12:36:31PM +0000, Ian Campbell wrote:
> On Mon, 2013-12-16 at 12:17 +0000, Ian Jackson wrote:
[...]
> 
> > > +            libxl_device_vfb *vfb;
> > > +            libxl_device_vkb *vkb;
> > > +
> > > +            d_config->vfbs = (libxl_device_vfb *)
> > > +                realloc(d_config->vfbs, sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1));
> > 
> > This should be xrealloc, and wrapped to 80 columns.  But see below.
> 
> This also suggests that we might be trying to support both the
> "toplevel" vnc options and the "vfb = []" style at the same time.
> 

Yes this is done on purpose as I don't know the complete set of possible
user config files. Probably Konrad can let us know what he expects.

> IMHO the vfb option should take precedence -- i.e. we should ignore the
> toplevel options if it is present.
> 
> The alternative would be some sort of union of the toplevel options and
> the first vfb given, but that sounds a bit mad...
> 
> I suppose there is also the question of what xend did here.
> 

I did look at Xend code, if there's top VNC option present it creates a
VFB. That's why I said "replicates" a Xend behavior in commit message.
I didn't go further than that though so I don't know one take precedence
over the other.

> > > +            vfb = d_config->vfbs + d_config->num_vfbs;
> > > +            libxl_device_vfb_init(vfb);
> > > +            vfb->devid = d_config->num_vfbs;
> > > +
[...]
> > > +            if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
> > > +                vfb->vnc.display = l;
> > > +            xlu_cfg_get_defbool(config, "vncunused", &vfb->vnc.findunused, 0);
> > 
> > This duplicates the HVM VNC option parsing.  It should be factored out
> > into a subroutine.
> 
> Ack.
> 
> That makes me wonder if the top level options shouldn't be populating
> &b_info->u.pv.vnc.* (and if that shouldn't have been a non-keyed field)
> while vfb = fills in d_config->vfbs. That is a bigger change on the
> libxl side though and maybe doesn't make quite as much sense as with
> the .
> 

with the what?

Wei.

> > 
> > > +
> > > +            d_config->num_vfbs++;
> > > +            d_config->num_vkbs++;
> > > +        }
> > > +    }
> > 
> > Thanks,
> > Ian.
> > 
> 

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

On Mon, Dec 16, 2013 at 12:17:02PM +0000, Ian Jackson wrote:
> Wei Liu writes ("[PATCH] xl: create VFB for PV guest when VNC is specified"):
> > This replicate a Xend behavior, when you specify:
> 
> Thanks.  I think the principle is sound.  I have some quibbles.
> 
> > +    if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
> > +        libxl_defbool vnc_enabled;
> > +
> > +        xlu_cfg_get_defbool(config, "vnc", &vnc_enabled, 0);
> 
> Missing error check.
> 

No call site ever error-checks this. Probably a separate patch to add
error check to all of them?

> > +        if (libxl_defbool_val(vnc_enabled)) {
> > +            libxl_device_vfb *vfb;
> > +            libxl_device_vkb *vkb;
> > +
> > +            d_config->vfbs = (libxl_device_vfb *)
> > +                realloc(d_config->vfbs, sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1));
> 
> This should be xrealloc, and wrapped to 80 columns.  But see below.
> 

Ack.

> > +            vfb = d_config->vfbs + d_config->num_vfbs;
> > +            libxl_device_vfb_init(vfb);
> > +            vfb->devid = d_config->num_vfbs;
> > +
> > +            d_config->vkbs = (libxl_device_vkb *)
> > +                realloc(d_config->vkbs, sizeof(libxl_device_vkb) * (d_config->num_vkbs + 1));
> > +            vkb = d_config->vkbs + d_config->num_vkbs;
> > +            libxl_device_vkb_init(vkb);
> > +            vkb->devid = d_config->num_vkbs;
> > +
> > +            libxl_defbool_set(&vfb->vnc.enable, true);
> 
> There's a lot of this kind of boilerplate.  I'm tempted to suggest
> making a macro to do this.  Searching for "devid =" found 4 call sites
> if that line is included in the macro; searching for "realloc" found
> me 6 call sites if it isn't.  And that's before your two additional
> ones.  Ian C, what do you think ?
> 

I'm fine with either way. It's maintainers' call now. :-)

> > +            xlu_cfg_replace_string(config, "vnclisten", &vfb->vnc.listen, 0);
> > +            xlu_cfg_replace_string(config, "vncpasswd", &vfb->vnc.passwd, 0);
> > +            if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
> > +                vfb->vnc.display = l;
> > +            xlu_cfg_get_defbool(config, "vncunused", &vfb->vnc.findunused, 0);
> 
> This duplicates the HVM VNC option parsing.  It should be factored out
> into a subroutine.
> 

Ack.

Wei.

> > +
> > +            d_config->num_vfbs++;
> > +            d_config->num_vkbs++;
> > +        }
> > +    }
> 
> Thanks,
> Ian.

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] xl: create VFB for PV guest when VNC is specified"):
> On Mon, Dec 16, 2013 at 12:17:02PM +0000, Ian Jackson wrote:
> > Missing error check.
> 
> No call site ever error-checks this. Probably a separate patch to add
> error check to all of them?

Maybe we should do something different.  E.g., make all these
functions return void and stash the fact that there was an error away
somewhere, and provide and call a new "was the5re an error" function.

That way if you write multiple errors in the file you get them
reported all at once.

Ian.

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

On Mon, Dec 16, 2013 at 12:43:57PM +0000, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH] xl: create VFB for PV guest when VNC is specified"):
> > On Mon, Dec 16, 2013 at 12:17:02PM +0000, Ian Jackson wrote:
> > > Missing error check.
> > 
> > No call site ever error-checks this. Probably a separate patch to add
> > error check to all of them?
> 
> Maybe we should do something different.  E.g., make all these
> functions return void and stash the fact that there was an error away
> somewhere, and provide and call a new "was the5re an error" function.
> 
> That way if you write multiple errors in the file you get them
> reported all at once.
> 

IMHO this doesn't sound like 4.4 material, too big a patch for a bug
fix...

Wei.

> Ian.

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] xl: create VFB for PV guest when VNC is specified"):
> IMHO this doesn't sound like 4.4 material, too big a patch for a bug
> fix...

Yes.  So, err, carry on and don't care about the errors :-/

Ian.

[PATCH] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

This replicates a Xend behavior, when you specify:

  vnc=1
  vnclisten=XXXX
  vncpasswd=XXXX

in a PV guest's config file, it creates a VFB for you.

Fixes bug #25.
http://bugs.xenproject.org/xen/bug/25

Reported-by: Konrad Wilk <konrad.wilk@oracle.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in V2:
* use macros to reduce code duplication
* vfb=[] take precedence over top level VNC options
---
---
 tools/libxl/xl_cmdimpl.c |   96 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 72 insertions(+), 24 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index bd26bcc..f3f4d71 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1357,11 +1357,37 @@ skip_nic:
         fprintf(stderr, "WARNING: vif2: netchannel2 is deprecated and not supported by xl\n");
     }
 
+#define add_vfb_vkb(vfb,vkb)                                            \
+        do {                                                            \
+            libxl_device_vfb *_vfb;                                     \
+            libxl_device_vkb *_vkb;                                     \
+                                                                        \
+            d_config->vfbs = (libxl_device_vfb *)                       \
+                xrealloc(d_config->vfbs,                                \
+                         sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1)); \
+            _vfb = d_config->vfbs + d_config->num_vfbs;                 \
+            libxl_device_vfb_init(_vfb);                                \
+            _vfb->devid = d_config->num_vfbs;                           \
+                                                                        \
+            d_config->vkbs = (libxl_device_vkb *)                       \
+                xrealloc(d_config->vkbs,                                \
+                         sizeof(libxl_device_vkb) * (d_config->num_vkbs + 1)); \
+            _vkb = d_config->vkbs + d_config->num_vkbs;                 \
+            libxl_device_vkb_init(_vkb);                                \
+            _vkb->devid = d_config->num_vkbs;                           \
+                                                                        \
+            d_config->num_vfbs++;                                       \
+            d_config->num_vkbs++;                                       \
+            (vfb) = _vfb;                                               \
+            (vkb) = _vkb;                                               \
+        } while (0)
+
+    d_config->num_vfbs = 0;
+    d_config->num_vkbs = 0;
+    d_config->vfbs = NULL;
+    d_config->vkbs = NULL;
+
     if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) {
-        d_config->num_vfbs = 0;
-        d_config->num_vkbs = 0;
-        d_config->vfbs = NULL;
-        d_config->vkbs = NULL;
         while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != NULL) {
             libxl_device_vfb *vfb;
             libxl_device_vkb *vkb;
@@ -1369,15 +1395,7 @@ skip_nic:
             char *buf2 = strdup(buf);
             char *p, *p2;
 
-            d_config->vfbs = (libxl_device_vfb *) realloc(d_config->vfbs, sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1));
-            vfb = d_config->vfbs + d_config->num_vfbs;
-            libxl_device_vfb_init(vfb);
-            vfb->devid = d_config->num_vfbs;
-
-            d_config->vkbs = (libxl_device_vkb *) realloc(d_config->vkbs, sizeof(libxl_device_vkb) * (d_config->num_vkbs + 1));
-            vkb = d_config->vkbs + d_config->num_vkbs;
-            libxl_device_vkb_init(vkb);
-            vkb->devid = d_config->num_vkbs;
+            add_vfb_vkb(vfb, vkb);
 
             p = strtok(buf2, ",");
             if (!p)
@@ -1418,8 +1436,6 @@ skip_nic:
 
 skip_vfb:
             free(buf2);
-            d_config->num_vfbs++;
-            d_config->num_vkbs++;
         }
     }
 
@@ -1608,6 +1624,47 @@ skip_vfb:
 
 #undef parse_extra_args
 
+#define parse_top_level_vnc_options(enable,listen,passwd,display,unused) \
+    do {                                                                \
+        long _l;                                                        \
+        xlu_cfg_get_defbool(config, "vnc", (enable), 0);                \
+        xlu_cfg_replace_string (config, "vnclisten", (listen), 0);      \
+        xlu_cfg_replace_string (config, "vncpasswd", (passwd), 0);      \
+        if (!xlu_cfg_get_long (config, "vncdisplay", &_l, 0))           \
+            (display) = _l;                                             \
+        xlu_cfg_get_defbool(config, "vncunused", (unused), 0);          \
+    } while (0)
+
+    /* If we've already got vfb=[] for PV guestthen ignore top level
+     * VNC config. */
+    if (c_info->type == LIBXL_DOMAIN_TYPE_PV && !d_config->num_vfbs) {
+        long vnc_enabled = 0;
+
+        if (!xlu_cfg_get_long (config, "vnc", &l, 0))
+            vnc_enabled = l;
+
+        if (vnc_enabled) {
+            libxl_device_vfb *vfb;
+            libxl_device_vkb *vkb;
+
+            add_vfb_vkb(vfb, vkb);
+
+            parse_top_level_vnc_options(&vfb->vnc.enable,
+                                        &vfb->vnc.listen,
+                                        &vfb->vnc.passwd,
+                                        vfb->vnc.display,
+                                        &vfb->vnc.findunused);
+
+        }
+    } else {
+        parse_top_level_vnc_options(&b_info->u.hvm.vnc.enable,
+                                    &b_info->u.hvm.vnc.listen,
+                                    &b_info->u.hvm.vnc.passwd,
+                                    b_info->u.hvm.vnc.display,
+                                    &b_info->u.hvm.vnc.findunused);
+    }
+#undef parse_top_level_vnc_options
+
     if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         if (!xlu_cfg_get_string (config, "vga", &buf, 0)) {
             if (!strcmp(buf, "stdvga")) {
@@ -1622,15 +1679,6 @@ skip_vfb:
             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);
-        xlu_cfg_replace_string (config, "vncpasswd",
-                                &b_info->u.hvm.vnc.passwd, 0);
-        if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
-            b_info->u.hvm.vnc.display = l;
-        xlu_cfg_get_defbool(config, "vncunused",
-                            &b_info->u.hvm.vnc.findunused, 0);
         xlu_cfg_replace_string (config, "keymap", &b_info->u.hvm.keymap, 0);
         xlu_cfg_get_defbool(config, "sdl", &b_info->u.hvm.sdl.enable, 0);
         xlu_cfg_get_defbool(config, "opengl", &b_info->u.hvm.sdl.opengl, 0);
-- 
1.7.10.4

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

This is version 2 of that patch, I forgot to add that in subject line,
sorry.

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Ian Campbell]

On Mon, 2013-12-16 at 16:42 +0000, Wei Liu wrote:
> This replicates a Xend behavior, when you specify:
> 
>   vnc=1
>   vnclisten=XXXX
>   vncpasswd=XXXX
> 
> in a PV guest's config file, it creates a VFB for you.
> 
> Fixes bug #25.
> http://bugs.xenproject.org/xen/bug/25
> 
> Reported-by: Konrad Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> ---
> Changes in V2:
> * use macros to reduce code duplication
> * vfb=[] take precedence over top level VNC options
> ---
> ---
>  tools/libxl/xl_cmdimpl.c |   96 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 72 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index bd26bcc..f3f4d71 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1357,11 +1357,37 @@ skip_nic:
>          fprintf(stderr, "WARNING: vif2: netchannel2 is deprecated and not supported by xl\n");
>      }
>  
> +#define add_vfb_vkb(vfb,vkb)                                            \

I think what Ian was suggesting was a single macro which could expand an
arbitrary array given as a parameter, rather than this, which is pretty
fuggly. e.g.
	vfb = ARRAY_EXTEND(d_config->vfbs, d_config->num_vfbs)
or something (making liberal use of typeof() etc). Perhaps even
                vfb = ARRAY_EXTEND(d_config, vfbs)
and some cpp token pasting.

Once you have that you can use it for both vkb and vfb.

I think you would be better off skipping the ->devid set, but if you
want to keep it you'd want to call it something more suitable like
NEW_DEVICE() perhaps.

You don't need to use the new macro everywhere right now during code
freeze but we can flip everything in 4.5.

> @@ -1608,6 +1624,47 @@ skip_vfb:
>  
>  #undef parse_extra_args
>  
> +#define parse_top_level_vnc_options(enable,listen,passwd,display,unused) \

I think this could be a function, which is would be preferable to a
macro if possible.

Ian.

Re: [PATCH] xl: create VFB for PV guest when VNC is specified

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] xl: create VFB for PV guest when VNC is specified"):
> I think what Ian was suggesting was a single macro which could expand an
> arbitrary array given as a parameter, rather than this, which is pretty
> fuggly. e.g.
> 	vfb = ARRAY_EXTEND(d_config->vfbs, d_config->num_vfbs)
> or something (making liberal use of typeof() etc). Perhaps even
>                 vfb = ARRAY_EXTEND(d_config, vfbs)
> and some cpp token pasting.

Exactly.

> Once you have that you can use it for both vkb and vfb.
> 
> I think you would be better off skipping the ->devid set, but if you
> want to keep it you'd want to call it something more suitable like
> NEW_DEVICE() perhaps.
> 
> You don't need to use the new macro everywhere right now during code
> freeze but we can flip everything in 4.5.

Right.

> > @@ -1608,6 +1624,47 @@ skip_vfb:
> >  
> >  #undef parse_extra_args
> >  
> > +#define parse_top_level_vnc_options(enable,listen,passwd,display,unused) \
> 
> I think this could be a function, which is would be preferable to a
> macro if possible.

Absolutely.

Ian.