* [PATCH] xl: create VFB for PV guest when VNC is specified
@ 2013-12-13 21:03 Wei Liu
2013-12-14 17:27 ` Konrad Rzeszutek Wilk
2013-12-16 12:17 ` Ian Jackson
0 siblings, 2 replies; 16+ messages in thread
From: Wei Liu @ 2013-12-13 21:03 UTC (permalink / raw
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Ian Campbell
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-13 21:03 [PATCH] xl: create VFB for PV guest when VNC is specified Wei Liu
@ 2013-12-14 17:27 ` Konrad Rzeszutek Wilk
2013-12-14 19:53 ` Wei Liu
2013-12-16 12:17 ` Ian Jackson
1 sibling, 1 reply; 16+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-12-14 17:27 UTC (permalink / raw
To: Wei Liu; +Cc: Ian Jackson, Ian Campbell, xen-devel
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-14 17:27 ` Konrad Rzeszutek Wilk
@ 2013-12-14 19:53 ` Wei Liu
0 siblings, 0 replies; 16+ messages in thread
From: Wei Liu @ 2013-12-14 19:53 UTC (permalink / raw
To: Konrad Rzeszutek Wilk
Cc: Ian Jackson, Wei Liu, Ian Campbell, xen-devel@lists.xen.org
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-13 21:03 [PATCH] xl: create VFB for PV guest when VNC is specified Wei Liu
2013-12-14 17:27 ` Konrad Rzeszutek Wilk
@ 2013-12-16 12:17 ` Ian Jackson
2013-12-16 12:36 ` Ian Campbell
2013-12-16 12:38 ` Wei Liu
1 sibling, 2 replies; 16+ messages in thread
From: Ian Jackson @ 2013-12-16 12:17 UTC (permalink / raw
To: Wei Liu; +Cc: Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 12:17 ` Ian Jackson
@ 2013-12-16 12:36 ` Ian Campbell
2013-12-16 12:42 ` Ian Jackson
2013-12-16 13:00 ` Wei Liu
2013-12-16 12:38 ` Wei Liu
1 sibling, 2 replies; 16+ messages in thread
From: Ian Campbell @ 2013-12-16 12:36 UTC (permalink / raw
To: Ian Jackson; +Cc: Wei Liu, xen-devel
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.
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 12:17 ` Ian Jackson
2013-12-16 12:36 ` Ian Campbell
@ 2013-12-16 12:38 ` Wei Liu
2013-12-16 12:43 ` Ian Jackson
1 sibling, 1 reply; 16+ messages in thread
From: Wei Liu @ 2013-12-16 12:38 UTC (permalink / raw
To: Ian Jackson; +Cc: Wei Liu, Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 12:36 ` Ian Campbell
@ 2013-12-16 12:42 ` Ian Jackson
2013-12-16 12:53 ` Ian Campbell
2013-12-16 13:00 ` Wei Liu
1 sibling, 1 reply; 16+ messages in thread
From: Ian Jackson @ 2013-12-16 12:42 UTC (permalink / raw
To: Ian Campbell; +Cc: Wei Liu, xen-devel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 12:38 ` Wei Liu
@ 2013-12-16 12:43 ` Ian Jackson
2013-12-16 12:46 ` Wei Liu
0 siblings, 1 reply; 16+ messages in thread
From: Ian Jackson @ 2013-12-16 12:43 UTC (permalink / raw
To: Wei Liu; +Cc: Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 12:43 ` Ian Jackson
@ 2013-12-16 12:46 ` Wei Liu
2013-12-16 12:50 ` Ian Jackson
0 siblings, 1 reply; 16+ messages in thread
From: Wei Liu @ 2013-12-16 12:46 UTC (permalink / raw
To: Ian Jackson; +Cc: Wei Liu, Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 12:46 ` Wei Liu
@ 2013-12-16 12:50 ` Ian Jackson
0 siblings, 0 replies; 16+ messages in thread
From: Ian Jackson @ 2013-12-16 12:50 UTC (permalink / raw
To: Wei Liu; +Cc: Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 12:42 ` Ian Jackson
@ 2013-12-16 12:53 ` Ian Campbell
0 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2013-12-16 12:53 UTC (permalink / raw
To: Ian Jackson; +Cc: Wei Liu, xen-devel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 12:36 ` Ian Campbell
2013-12-16 12:42 ` Ian Jackson
@ 2013-12-16 13:00 ` Wei Liu
1 sibling, 0 replies; 16+ messages in thread
From: Wei Liu @ 2013-12-16 13:00 UTC (permalink / raw
To: Ian Campbell; +Cc: Wei Liu, Ian Jackson, xen-devel
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.
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] xl: create VFB for PV guest when VNC is specified
@ 2013-12-16 16:42 Wei Liu
2013-12-16 16:43 ` Wei Liu
2013-12-16 16:57 ` Ian Campbell
0 siblings, 2 replies; 16+ messages in thread
From: Wei Liu @ 2013-12-16 16:42 UTC (permalink / raw
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Ian Campbell
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 16:42 Wei Liu
@ 2013-12-16 16:43 ` Wei Liu
2013-12-16 16:57 ` Ian Campbell
1 sibling, 0 replies; 16+ messages in thread
From: Wei Liu @ 2013-12-16 16:43 UTC (permalink / raw
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Ian Campbell
This is version 2 of that patch, I forgot to add that in subject line,
sorry.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 16:42 Wei Liu
2013-12-16 16:43 ` Wei Liu
@ 2013-12-16 16:57 ` Ian Campbell
2013-12-16 17:07 ` Ian Jackson
1 sibling, 1 reply; 16+ messages in thread
From: Ian Campbell @ 2013-12-16 16:57 UTC (permalink / raw
To: Wei Liu; +Cc: Ian Jackson, xen-devel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xl: create VFB for PV guest when VNC is specified
2013-12-16 16:57 ` Ian Campbell
@ 2013-12-16 17:07 ` Ian Jackson
0 siblings, 0 replies; 16+ messages in thread
From: Ian Jackson @ 2013-12-16 17:07 UTC (permalink / raw
To: Ian Campbell; +Cc: Wei Liu, xen-devel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-12-16 17:07 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-13 21:03 [PATCH] xl: create VFB for PV guest when VNC is specified Wei Liu
2013-12-14 17:27 ` Konrad Rzeszutek Wilk
2013-12-14 19:53 ` Wei Liu
2013-12-16 12:17 ` Ian Jackson
2013-12-16 12:36 ` Ian Campbell
2013-12-16 12:42 ` Ian Jackson
2013-12-16 12:53 ` Ian Campbell
2013-12-16 13:00 ` Wei Liu
2013-12-16 12:38 ` Wei Liu
2013-12-16 12:43 ` Ian Jackson
2013-12-16 12:46 ` Wei Liu
2013-12-16 12:50 ` Ian Jackson
-- strict thread matches above, loose matches on Subject: below --
2013-12-16 16:42 Wei Liu
2013-12-16 16:43 ` Wei Liu
2013-12-16 16:57 ` Ian Campbell
2013-12-16 17:07 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.