* [PATCH V4] xl: create VFB for PV guest when VNC is specified
@ 2013-12-17 22:53 Wei Liu
2013-12-18 13:12 ` Ian Campbell
0 siblings, 1 reply; 12+ messages in thread
From: Wei Liu @ 2013-12-17 22:53 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Ian Campbell
This replicates a Xend behavior. When you specify 'vnc=1' and there's no
'vfb=[]' in a PV guest's config file, xl parses all top level VNC options and
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 V4:
* improve macro to extend array
* fix off-by-one error in V3
Changes in V3:
* ARRAY_EXTEND macro
* make parse_top_level_vnc_options a function
Changes in V2:
* use macros to reduce code duplication
* vfb=[] take precedence over top level VNC options
---
tools/libxl/xl_cmdimpl.c | 78 ++++++++++++++++++++++++++++++++--------------
1 file changed, 55 insertions(+), 23 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index bd26bcc..f323377 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -315,6 +315,16 @@ static void *xrealloc(void *ptr, size_t sz) {
return r;
}
+#define ARRAY_EXTEND_INIT(array,count,initfn) \
+ ({ \
+ typeof((count)) array_extend_old_count = (count); \
+ (count)++; \
+ (array) = xrealloc((array), sizeof(*array) * (count)); \
+ (initfn)(&(array)[array_extend_old_count]); \
+ (array)[array_extend_old_count].devid = array_extend_old_count; \
+ &(array)[array_extend_old_count]; \
+ })
+
#define LOG(_f, _a...) dolog(__FILE__, __LINE__, __func__, _f "\n", ##_a)
static void dolog(const char *file, int line, const char *func, char *fmt, ...)
@@ -686,6 +696,19 @@ static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
return rc;
}
+static void parse_top_level_vnc_options(XLU_Config *config,
+ libxl_vnc_info *vnc)
+{
+ long l;
+
+ xlu_cfg_get_defbool(config, "vnc", &vnc->enable, 0);
+ xlu_cfg_replace_string (config, "vnclisten", &vnc->listen, 0);
+ xlu_cfg_replace_string (config, "vncpasswd", &vnc->passwd, 0);
+ if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
+ vnc->display = l;
+ xlu_cfg_get_defbool(config, "vncunused", &vnc->findunused, 0);
+}
+
static void parse_config_data(const char *config_source,
const char *config_data,
int config_len,
@@ -1357,11 +1380,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;
@@ -1369,15 +1393,11 @@ 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;
+ vfb = ARRAY_EXTEND_INIT(d_config->vfbs, d_config->num_vfbs,
+ libxl_device_vfb_init);
- 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;
+ vkb = ARRAY_EXTEND_INIT(d_config->vkbs, d_config->num_vkbs,
+ libxl_device_vkb_init);
p = strtok(buf2, ",");
if (!p)
@@ -1418,8 +1438,6 @@ skip_nic:
skip_vfb:
free(buf2);
- d_config->num_vfbs++;
- d_config->num_vkbs++;
}
}
@@ -1608,6 +1626,29 @@ skip_vfb:
#undef parse_extra_args
+ /* If we've already got vfb=[] for PV guest then 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;
+
+ vfb = ARRAY_EXTEND_INIT(d_config->vfbs, d_config->num_vfbs,
+ libxl_device_vfb_init);
+
+ vkb = ARRAY_EXTEND_INIT(d_config->vkbs, d_config->num_vkbs,
+ libxl_device_vkb_init);
+
+ parse_top_level_vnc_options(config, &vfb->vnc);
+ }
+ } else
+ parse_top_level_vnc_options(config, &b_info->u.hvm.vnc);
+
if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
if (!xlu_cfg_get_string (config, "vga", &buf, 0)) {
if (!strcmp(buf, "stdvga")) {
@@ -1622,15 +1663,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] 12+ messages in thread* Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified
2013-12-17 22:53 [PATCH V4] xl: create VFB for PV guest when VNC is specified Wei Liu
@ 2013-12-18 13:12 ` Ian Campbell
2013-12-18 13:46 ` Wei Liu
0 siblings, 1 reply; 12+ messages in thread
From: Ian Campbell @ 2013-12-18 13:12 UTC (permalink / raw)
To: Wei Liu; +Cc: Ian Jackson, xen-devel
On Tue, 2013-12-17 at 22:53 +0000, Wei Liu wrote:
> This replicates a Xend behavior. When you specify 'vnc=1' and there's no
> 'vfb=[]' in a PV guest's config file, xl parses all top level VNC options and
> 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>
Looks good.
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Has this had a discussion about the 4.4 release? It's somewhere between
a bug fix and a new feature, I suppose more of a bug fix.
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified
2013-12-18 13:12 ` Ian Campbell
@ 2013-12-18 13:46 ` Wei Liu
2014-01-06 16:39 ` Ian Campbell
0 siblings, 1 reply; 12+ messages in thread
From: Wei Liu @ 2013-12-18 13:46 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Ian Jackson, Wei Liu, xen-devel
On Wed, Dec 18, 2013 at 01:12:13PM +0000, Ian Campbell wrote:
> On Tue, 2013-12-17 at 22:53 +0000, Wei Liu wrote:
> > This replicates a Xend behavior. When you specify 'vnc=1' and there's no
> > 'vfb=[]' in a PV guest's config file, xl parses all top level VNC options and
> > 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>
>
> Looks good.
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Has this had a discussion about the 4.4 release? It's somewhere between
> a bug fix and a new feature, I suppose more of a bug fix.
>
Yes it is a bug fix.
CC'ing George now.
Wei.
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > ---
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified
2013-12-18 13:46 ` Wei Liu
@ 2014-01-06 16:39 ` Ian Campbell
2014-01-06 17:27 ` Wei Liu
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Ian Campbell @ 2014-01-06 16:39 UTC (permalink / raw)
To: Wei Liu; +Cc: George Dunlap, Ian Jackson, xen-devel
On Wed, 2013-12-18 at 13:46 +0000, Wei Liu wrote:
> On Wed, Dec 18, 2013 at 01:12:13PM +0000, Ian Campbell wrote:
> > On Tue, 2013-12-17 at 22:53 +0000, Wei Liu wrote:
> > > This replicates a Xend behavior. When you specify 'vnc=1' and there's no
> > > 'vfb=[]' in a PV guest's config file, xl parses all top level VNC options and
> > > 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>
> >
> > Looks good.
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > Has this had a discussion about the 4.4 release? It's somewhere between
> > a bug fix and a new feature, I suppose more of a bug fix.
> >
>
> Yes it is a bug fix.
>
> CC'ing George now.
And now George has gone away and left me holding the can, smart move on
his part ;-)
With RM hat on my main concern here is that this smells an awful lot
like a new feature and not strictly a bug fix (the presence of a bug is
a bit of a red-herring, it's notionally a wishlist bug even if it isn't
currently tagged as such).
On the flip side we have been giving somewhat special dispensation to
"bugs" of the form "xl does not do $something_xend_did" and treating
them as something stronger than wishlist (although I'm not sure how much
stronger). I notice that George has moved all of those under backlog in
the latest 4.4 dev update though (see [1]), so I'm not sure if he would
still apply this rule (were I to know what it actually was).
Konrad, to what extent is this a blocker for you (or the OVM tooling)
vs. it just being something you spotted by random chance?
So considering the guidelines George left[2]:
The patch contributes to an "awesome release" by allowing some set of
existing xm configuration files to work as is, which is something we are
keen on in order to continue to migrate users over. There is a
workaround though (rewrite the cfg file to use the other syntax, which
works), which although falling short of our desire for this to "just
work" is not immensely complex to apply.
The potential risk is that this breaks the existing vfb syntax which
works, or it breaks the hvm stuff. This is of particular concern because
I don't think any of that is covered by osstest (except perhaps hvm
console, but that might only be on some other error when osstest takes a
screenshot), so the probability of finding it before release is reliant
on manual testing/test days/user testing etc.
Are you happy that all the existing options keep working?
The patch is big enough that it isn't "obviously correct". . THe patch
is textually large because it contain two refactorings
(ARRAY_EXTEND_INIT and parse_top_level_vnc_options). ARRAY_EXTEND_INIT
had pretty comprehensive review from me and Ian J as it was constructed.
parse_top_... is really just code motion (although I'm a bit concerned
that the hvm callsite has moved too). With my RM hat off and my
maintainer hat on I've reviewed it and it looks good. (RM hat back on)
So, I think I'm a bit border line but slightly on the side of accepting
it for 4.4 unless anyone has a counter opinion.
Ian.
[1] http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Meta-items_.28composed_of_other_items.29
[2] http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Exception_guidelines_for_after_the_code_freeze
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified
2014-01-06 16:39 ` Ian Campbell
@ 2014-01-06 17:27 ` Wei Liu
2014-01-06 17:29 ` Ian Jackson
2014-01-06 17:46 ` Konrad Rzeszutek Wilk
2 siblings, 0 replies; 12+ messages in thread
From: Wei Liu @ 2014-01-06 17:27 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Ian Jackson, Wei Liu, xen-devel
On Mon, Jan 06, 2014 at 04:39:30PM +0000, Ian Campbell wrote:
> On Wed, 2013-12-18 at 13:46 +0000, Wei Liu wrote:
> > On Wed, Dec 18, 2013 at 01:12:13PM +0000, Ian Campbell wrote:
> > > On Tue, 2013-12-17 at 22:53 +0000, Wei Liu wrote:
> > > > This replicates a Xend behavior. When you specify 'vnc=1' and there's no
> > > > 'vfb=[]' in a PV guest's config file, xl parses all top level VNC options and
> > > > 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>
> > >
> > > Looks good.
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > >
> > > Has this had a discussion about the 4.4 release? It's somewhere between
> > > a bug fix and a new feature, I suppose more of a bug fix.
> > >
> >
> > Yes it is a bug fix.
> >
> > CC'ing George now.
>
> And now George has gone away and left me holding the can, smart move on
> his part ;-)
>
> With RM hat on my main concern here is that this smells an awful lot
> like a new feature and not strictly a bug fix (the presence of a bug is
> a bit of a red-herring, it's notionally a wishlist bug even if it isn't
> currently tagged as such).
>
> On the flip side we have been giving somewhat special dispensation to
> "bugs" of the form "xl does not do $something_xend_did" and treating
> them as something stronger than wishlist (although I'm not sure how much
> stronger). I notice that George has moved all of those under backlog in
> the latest 4.4 dev update though (see [1]), so I'm not sure if he would
> still apply this rule (were I to know what it actually was).
>
FWIW the wiki page you looked at was updated on Dec 5 while in
<CAFLBxZZYMU3wJey4tZN9pVrfeTeujLQLBRbiGe-WFevpO2UhvQ@mail.gmail.com>
which was sent later on Dec 13. This bug was the first one in "open"
category. That's why I picked this one in the first place. :-)
That said, I'm not urging to merge this in. This is just my side work
when I was relatively free. I'm fine with any decision you make.
Wei.
> [1] http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Meta-items_.28composed_of_other_items.29
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified
2014-01-06 16:39 ` Ian Campbell
2014-01-06 17:27 ` Wei Liu
@ 2014-01-06 17:29 ` Ian Jackson
2014-01-06 17:56 ` Wei Liu
2014-01-06 17:46 ` Konrad Rzeszutek Wilk
2 siblings, 1 reply; 12+ messages in thread
From: Ian Jackson @ 2014-01-06 17:29 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Wei Liu, xen-devel
Ian Campbell writes ("Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified"):
> And now George has gone away and left me holding the can, smart move on
> his part ;-)
I have also reviewed this patch again.
Firstly,
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Now, onto the RM questions.
> The potential risk is that this breaks the existing vfb syntax which
> works, or it breaks the hvm stuff. This is of particular concern because
> I don't think any of that is covered by osstest (except perhaps hvm
> console, but that might only be on some other error when osstest takes a
> screenshot), so the probability of finding it before release is reliant
> on manual testing/test days/user testing etc.
> Are you happy that all the existing options keep working?
Wei, could you tell us what configuration(s) you tested ?
> The patch is big enough that it isn't "obviously correct". . THe patch
> is textually large because it contain two refactorings
> (ARRAY_EXTEND_INIT and parse_top_level_vnc_options). ARRAY_EXTEND_INIT
> had pretty comprehensive review from me and Ian J as it was constructed.
More to the point, if ARRAY_EXTEND_INIT and the use site pattern were
broken it would totally break vfb entirely. I think we'd be likely to
detect this well before release if it hadn't been spotted already.
> parse_top_... is really just code motion (although I'm a bit concerned
> that the hvm callsite has moved too). With my RM hat off and my
> maintainer hat on I've reviewed it and it looks good. (RM hat back on)
The autotester does use vnc, so it should detect any systematic bugs
introduced here. So so long as the change is sufficiently systematic
I think we are fine.
> So, I think I'm a bit border line but slightly on the side of accepting
> it for 4.4 unless anyone has a counter opinion.
Right, I don't disagree, particularly if Wei's reply to my question
about testing is reassuring.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified
2014-01-06 17:29 ` Ian Jackson
@ 2014-01-06 17:56 ` Wei Liu
2014-01-07 16:31 ` Ian Campbell
0 siblings, 1 reply; 12+ messages in thread
From: Wei Liu @ 2014-01-06 17:56 UTC (permalink / raw)
To: Ian Jackson; +Cc: George Dunlap, Wei Liu, Ian Campbell, xen-devel
On Mon, Jan 06, 2014 at 05:29:01PM +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified"):
> > And now George has gone away and left me holding the can, smart move on
> > his part ;-)
>
> I have also reviewed this patch again.
>
> Firstly,
>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> Now, onto the RM questions.
>
> > The potential risk is that this breaks the existing vfb syntax which
> > works, or it breaks the hvm stuff. This is of particular concern because
> > I don't think any of that is covered by osstest (except perhaps hvm
> > console, but that might only be on some other error when osstest takes a
> > screenshot), so the probability of finding it before release is reliant
> > on manual testing/test days/user testing etc.
> > Are you happy that all the existing options keep working?
>
> Wei, could you tell us what configuration(s) you tested ?
>
#1 PV guest (to see if it causes regression):
vfb = ['vnc=1,vncunused=1,vnclisten=0.0.0.0']
#2 PV guest (to see if it fixes the bug):
vnc=1
vncunused=1
vnclisten='0.0.0.0'
#3 PV guest (to test if if will create more than 1 VFB):
vfb = ['vnc=1,vncunused=1,vnclisten=0.0.0.0']
vnc=1
vncunused=1
vnclisten='0.0.0.0'
#4 HVM guest (to see if it causes regression):
vnc=1
vncunused=1
vnclisten='0.0.0.0'
#5 HVM guest (to see if xl errneously parse vfb= for HVM):
vfb = ['vnc=1,vncunused=1,vnclisten=0.0.0.0']
Wei.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified
2014-01-06 17:56 ` Wei Liu
@ 2014-01-07 16:31 ` Ian Campbell
2014-01-08 14:35 ` Ian Campbell
0 siblings, 1 reply; 12+ messages in thread
From: Ian Campbell @ 2014-01-07 16:31 UTC (permalink / raw)
To: Wei Liu; +Cc: George Dunlap, Ian Jackson, xen-devel
On Mon, 2014-01-06 at 17:56 +0000, Wei Liu wrote:
> On Mon, Jan 06, 2014 at 05:29:01PM +0000, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified"):
> > > And now George has gone away and left me holding the can, smart move on
> > > his part ;-)
> >
> > I have also reviewed this patch again.
> >
> > Firstly,
> >
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> >
> > Now, onto the RM questions.
> >
> > > The potential risk is that this breaks the existing vfb syntax which
> > > works, or it breaks the hvm stuff. This is of particular concern because
> > > I don't think any of that is covered by osstest (except perhaps hvm
> > > console, but that might only be on some other error when osstest takes a
> > > screenshot), so the probability of finding it before release is reliant
> > > on manual testing/test days/user testing etc.
> > > Are you happy that all the existing options keep working?
> >
> > Wei, could you tell us what configuration(s) you tested ?
> >
>
[.snip.]
That looks pretty comprehensive.
Given that George has this in his list of open items for 4.4 with
"blocker?" (his query marker) I'm inclined towards giving a release
exception here.
Release-acked-by: Ian Campbell
Ian.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified
2014-01-06 16:39 ` Ian Campbell
2014-01-06 17:27 ` Wei Liu
2014-01-06 17:29 ` Ian Jackson
@ 2014-01-06 17:46 ` Konrad Rzeszutek Wilk
2014-01-08 14:35 ` Ian Campbell
2 siblings, 1 reply; 12+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-01-06 17:46 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Ian Jackson, Wei Liu, xen-devel
On Mon, Jan 06, 2014 at 04:39:30PM +0000, Ian Campbell wrote:
> On Wed, 2013-12-18 at 13:46 +0000, Wei Liu wrote:
> > On Wed, Dec 18, 2013 at 01:12:13PM +0000, Ian Campbell wrote:
> > > On Tue, 2013-12-17 at 22:53 +0000, Wei Liu wrote:
> > > > This replicates a Xend behavior. When you specify 'vnc=1' and there's no
> > > > 'vfb=[]' in a PV guest's config file, xl parses all top level VNC options and
> > > > 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>
> > >
> > > Looks good.
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > >
> > > Has this had a discussion about the 4.4 release? It's somewhere between
> > > a bug fix and a new feature, I suppose more of a bug fix.
> > >
> >
> > Yes it is a bug fix.
> >
> > CC'ing George now.
>
> And now George has gone away and left me holding the can, smart move on
> his part ;-)
>
> With RM hat on my main concern here is that this smells an awful lot
> like a new feature and not strictly a bug fix (the presence of a bug is
> a bit of a red-herring, it's notionally a wishlist bug even if it isn't
> currently tagged as such).
>
> On the flip side we have been giving somewhat special dispensation to
> "bugs" of the form "xl does not do $something_xend_did" and treating
> them as something stronger than wishlist (although I'm not sure how much
> stronger). I notice that George has moved all of those under backlog in
> the latest 4.4 dev update though (see [1]), so I'm not sure if he would
> still apply this rule (were I to know what it actually was).
>
> Konrad, to what extent is this a blocker for you (or the OVM tooling)
> vs. it just being something you spotted by random chance?
No blocker. Just me diligently filling issues with 'xend vs xl'
as I spot them along.
>
> So considering the guidelines George left[2]:
>
> The patch contributes to an "awesome release" by allowing some set of
> existing xm configuration files to work as is, which is something we are
> keen on in order to continue to migrate users over. There is a
> workaround though (rewrite the cfg file to use the other syntax, which
> works), which although falling short of our desire for this to "just
> work" is not immensely complex to apply.
>
> The potential risk is that this breaks the existing vfb syntax which
> works, or it breaks the hvm stuff. This is of particular concern because
> I don't think any of that is covered by osstest (except perhaps hvm
> console, but that might only be on some other error when osstest takes a
> screenshot), so the probability of finding it before release is reliant
> on manual testing/test days/user testing etc.
> Are you happy that all the existing options keep working?
>
> The patch is big enough that it isn't "obviously correct". . THe patch
> is textually large because it contain two refactorings
> (ARRAY_EXTEND_INIT and parse_top_level_vnc_options). ARRAY_EXTEND_INIT
> had pretty comprehensive review from me and Ian J as it was constructed.
> parse_top_... is really just code motion (although I'm a bit concerned
> that the hvm callsite has moved too). With my RM hat off and my
> maintainer hat on I've reviewed it and it looks good. (RM hat back on)
>
> So, I think I'm a bit border line but slightly on the side of accepting
> it for 4.4 unless anyone has a counter opinion.
>
> Ian.
>
> [1] http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Meta-items_.28composed_of_other_items.29
> [2] http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Exception_guidelines_for_after_the_code_freeze
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified
2014-01-06 17:46 ` Konrad Rzeszutek Wilk
@ 2014-01-08 14:35 ` Ian Campbell
0 siblings, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2014-01-08 14:35 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: George Dunlap, Ian Jackson, Wei Liu, xen-devel
On Mon, 2014-01-06 at 12:46 -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 06, 2014 at 04:39:30PM +0000, Ian Campbell wrote:
> > Konrad, to what extent is this a blocker for you (or the OVM tooling)
> > vs. it just being something you spotted by random chance?
>
> No blocker. Just me diligently filling issues with 'xend vs xl'
> as I spot them along.
Thanks for doing so. In the end I gave this a release exception anyway
(see other subthread).
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-01-08 14:45 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-17 22:53 [PATCH V4] xl: create VFB for PV guest when VNC is specified Wei Liu
2013-12-18 13:12 ` Ian Campbell
2013-12-18 13:46 ` Wei Liu
2014-01-06 16:39 ` Ian Campbell
2014-01-06 17:27 ` Wei Liu
2014-01-06 17:29 ` Ian Jackson
2014-01-06 17:56 ` Wei Liu
2014-01-07 16:31 ` Ian Campbell
2014-01-08 14:35 ` Ian Campbell
2014-01-08 14:45 ` Processed: " xen
2014-01-06 17:46 ` Konrad Rzeszutek Wilk
2014-01-08 14:35 ` Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).