Re: Oldest supported Xen version in upstream QEMU (Was: Re: [Minios-devel] [PATCH v2 0/15+5+5] Begin to disentangle libxenctrl and provide some stable libraries)

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-09-24 at 23:19 +0100, Stefano Stabellini wrote:
> On Thu, 24 Sep 2015, Ian Campbell wrote:
> > On Thu, 2015-09-24 at 20:33 +0100, Stefano Stabellini wrote:
> > > On Thu, 24 Sep 2015, Ian Campbell wrote:
> > > > On Wed, 2015-09-23 at 18:36 +0100, Stefano Stabellini wrote:
> > > > > On Wed, 23 Sep 2015, Ian Campbell wrote:
> > > > > > On Tue, 2015-09-22 at 22:31 +0100, Stefano Stabellini wrote: 
> > > > > > > The oldest Xen version I build-test for every pull request is
> > > Xen 4.0.0,
> > > > 
> > > > I setup a build trees for 4.0 thru 4.6 yesterday to test this, what
> > > a
> > > > pain 4.1 and 4.0 are to build with a modern gcc! (Mostly newer
> > > compiler
> > > > warnings and mostly, but not all, fixes which I could just backport
> > > > from newer Xen, the exceptions were a couple of things which were
> > > > removed before they needed to be fixed)
> > > > 
> > > > > > > I think it is very reasonable to remove anything older than
> > > that.
> > > > > > > I am OK with removing Xen 4.0.0 too, but I would like a
> > > warning to be
> > > > > > > sent ahead of time to qemu-devel to see if anybody complains.
> > > > > > 
> > > > > > There is not much point in removing <=3.4 support and keeping
> > > 4.0, since
> > > > > > 4.0.0 was the last one which used a plain int as a handle,
> > > anything older
> > > > > > than 4.0.0 is trivial if 4.0.0 is supported.
> > > > > > 
> > > > > > One approach I am considering in order to keep 4.0.0 support
> > > and earlier
> > > > > > was to turn the "int fd" for <=4.0 into a pointer by having the
> > > open
> > > > > > wrapper do malloc(sizeof int) and the using wrappers do
> > > xc_foo(*handle).
> > > > > > 
> > > > > > This way all the different variants take pointers and we have
> > > less hoops to
> > > > > > jump through to typedef everything in the correct way for each
> > > variant.
> > > > > > 
> > > > > > If you would rather avoid doing that then I think dropping
> > > 4.0.0 support
> > > > > > would be the way to go and I'll send a mail to qemu-devel.
> > > > >  
> > > > > I would rather drop 4.0 support.
> > > > 
> > > > Supporting 4.0 didn't turn out quite as ugly as I had feared.
> > > > 
> > > > So before I send an email to qemu-devel to propose dropping 4.0
> > > what do
> > > > you think of the following which handles the evtchn case, there is
> > > a
> > > > similar patch for gnttab and a (yet to be written) patch for the
> > > > foreign memory mapping case.
> > > > 
> > > > The relevant bit for this discussion is the 4.0.0 definition of
> > > > xenevtchn_open in xen_common.h, the rest is just adjusting it to
> > > use
> > > > the API of the new library (for reasons explained in the commit
> > > > message).
> > > 
> > > I think that it is OK in principle.
> > > 
> > > 
> > > > diff --git a/include/hw/xen/xen_common.h
> > > b/include/hw/xen/xen_common.h
> > > > index 5923290..5700c1b 100644
> > > > --- a/include/hw/xen/xen_common.h
> > > > +++ b/include/hw/xen/xen_common.h
> > > > @@ -39,17 +39,37 @@ static inline void *xc_map_foreign_bulk(int
> > > xc_handle, uint32_t dom, int prot,
> > > >  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 410
> > > >  
> > > >  typedef int XenXC;
> > > > -typedef int XenEvtchn;
> > > > +typedef int xenevtchn_handle;
> > > >  typedef int XenGnttab;
> > >  
> > > ...
> > > 
> > > > @@ -108,17 +128,20 @@ static inline void xs_close(struct xs_handle
> > > *xsh)
> > > >  #else
> > > >  
> > > >  typedef xc_interface *XenXC;
> > > > -typedef xc_evtchn *XenEvtchn;
> > > > +typedef xc_evtchn xenevtchn_handle;
> > > >  typedef xc_gnttab *XenGnttab;
> > > >  
> > > 
> > > There is no reasons why we couldn't have a small compat shim on Xen >
> > > 4.6 too, so I would change the definition of XenEvtchn for newer
> > > versions of Xen and avoid some of the renaming in this patch to
> > > reduce
> > > the changes.
> > > 
> > > For example, why not define xc_evtchn_fd as xenevtchn_fd for Xen >
> > > 4.6?
> > > So that we don't need to go and rename all the call sites?
> > 
> > The idea was that the code would use the new stable API names from the
> > stable libraries going forward, rather than using a shim to turn the
> > stable APIs back into the old ones.
> 
> I don't think that is very important from QEMU's point of view, using a
> shim is just fine, especially if it reduces the patch size to 5 lines of
> code :-)

Is patch size really the major consideration here? IMHO it is simply less
confusing to have no shim (since one doesn't need to translate the names
when reading differing code bases) and with time the shim layers can drop
away leading to less complexity.

Also, I've already written all the patches, the renamings were very
mechanical and at this point it would actually take me longer to undo them.

I'll do that if you insist, but I think the justification for sticking with
a shim is very flimsy.

Ian.