From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH V4] xl: create VFB for PV guest when VNC is specified Date: Mon, 6 Jan 2014 12:46:20 -0500 Message-ID: <20140106174620.GA28636@phenom.dumpdata.com> References: <1387320825-21953-1-git-send-email-wei.liu2@citrix.com> <1387372333.28680.6.camel@kazak.uk.xensource.com> <20131218134629.GG25969@zion.uk.xensource.com> <1389026370.31766.83.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1389026370.31766.83.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: George Dunlap , Ian Jackson , Wei Liu , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > > > > Signed-off-by: Wei Liu > > > > > > Looks good. > > > Acked-by: Ian Campbell > > > > > > 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 > >