Re: [Xen-devel] 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)

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 25 Sep 2015, Ian Campbell wrote:
> 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.

I just don't think is important to use the new names in QEMU. Of course
it is important to be consistent within the QEMU code base, but I don't
think is necessary or required to be consistent with external code
bases.  The shim has worked very well in QEMU for years and I am quite
happy to keep using it.

That said, there is nothing that screams "bished!" more than discussing
the naming scheme of xc calls in QEMU, so I won't draw the line on this.

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