Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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

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