[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V4] xl: create VFB for PV guest when VNC is specified
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |