From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH V6 3/7] libxl: add pvusb API Date: Tue, 29 Sep 2015 18:19:26 +0100 Message-ID: <20150929171926.GA7370@zion.uk.xensource.com> References: <1439202928-24813-1-git-send-email-cyliu@suse.com> <1439202928-24813-4-git-send-email-cyliu@suse.com> <1441721852.24450.120.camel@citrix.com> <55EF1244.107@citrix.com> <55FB04340200006600050E6D@relay2.provo.novell.com> <55FA8DCF.5070405@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <55FA8DCF.5070405@citrix.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: George Dunlap Cc: Juergen Gross , wei.liu2@citrix.com, Ian Campbell , george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, Chun Yan Liu , xen-devel@lists.xen.org, Jim Fehlig , Simon Cao List-Id: xen-devel@lists.xenproject.org On Thu, Sep 17, 2015 at 10:54:23AM +0100, George Dunlap wrote: [...] > > Hi, George, > > > > I'm still confused about the expected look concerning PV/EMU type handling in > > this patch series. > > > > In earlier version, we tried to extract common things in libxl_usb.c and put > > pvusb specific thing in libxl_pvusb.c, prefixed with pvusb_xxx. As you > > suggested, we can leave that when EMU USB patch series added. > > > > Now, about how to handle PV/EMU type in this patch series, I can think of 3 > > ways: > > > > 1. We define the enumeration (contains PV/AUTO only, user interface only allows > > 'pv' or 'not specified', so we handle everything in 'pv' way without further > > check. Leave check and other adjusting things when EMU USB patch series added. > > > > 2. We check domain type and set proper type if not specified (i.e. 'pv' for PV > > guest, 'emu' for HVM guest). In add/remove function, check if type='emu', report > > 'not supported' directly; otherwise, continue do following things. When EMU USB > > patch serires added, need to extract common things and adjust the check place. > > > > 3. Same as 2, but extract common things, only in PV/EMU USB specific part, check > > type, if type='emu', report 'not supported'; otherwise, do pvusb work. When > > adding EMU USB patch series, only need to add EMU USB specific things in the > > type='emu' branch. > > > > Which one is expected? Or none? > > So there are two questions here, first WRT the code, the second WRT the > interface. > > WRT the code, *normally* the first person to submit the code gets to > have it easy, and the second person has to do all the work of > refactoring. So you would be completely within your rights to simply > submit "libxl_usb.c", and make me refactor that into libxl_pvusb.c and > whatever else (probably the qemu stuff would go in libxl_qmp.c). > > Earlier I asked you as a favor to put things in libxl_pvusb.c, and you > were kind enough to do so -- so thank you. Just having things roughly > where they might end up eventually has already been a big help. I'll > have to move some of the code around pretty much no matter what you do. > So I don't think it's worth making any more effort wrt the code itself. > > WRT the interface -- if we do a release with PV defined, but not > EMU/DEVICEMODEL, then when we add that option, we'll have to add Yet > Another LIBL_HAS_BLAH. I would personally like too avoid that. > (Note that I didn't go through all emails) I think adding yet another LIBXL_HAS_BLAH wouldn't be a problem. Chunyan will have to add one anyway. > As it happens, if you were to check this in now at the beginning of the > cycle, it's very likely I could get the EMU side in before the release. > So it's *probably* OK to just write AUTO and PV. > Again, extending the interface shouldn't be a problem -- we do that all the time. So IMHO having AUTO and PV only is OK. > I would personally prefer to play it safe and give all three interface > elements (AUTO, PV, and EMU/DEVICEMODEL), and return ENOTSUPP (or > whatever) for DEVICEMODEL until it's implemented. But that's really a > policy decision for the maintianers at this point. > This works for me too. I don't really see a problem in choosing one way or another TBH. Wei. > -George