Re: [Xen-devel] [PATCH V4] xl: create VFB for PV guest when VNC is specified

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > 
> > Looks good.
> > Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > 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



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel