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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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