Re: [Qemu-devel] [PATCH] net: Disable netmap backend when not supported

[Stefan Hajnoczi <stefanha@gmail.com>]

On Wed, Feb 19, 2014 at 10:30:03AM -0800, Luigi Rizzo wrote:
> On Wed, Feb 19, 2014 at 7:30 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Fri, Feb 14, 2014 at 05:40:24PM +0100, Vincenzo Maffione wrote:
> > > diff --git a/configure b/configure
> > > index 88133a1..61eb932 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -2118,6 +2118,9 @@ if test "$netmap" != "no" ; then
> > >  #include <net/if.h>
> > >  #include <net/netmap.h>
> > >  #include <net/netmap_user.h>
> > > +#if (NETMAP_API < 11) || (NETMAP_API > 15)
> > > +#error
> > > +#endif
> >
> > Why error when NETMAP_API > 15?
> >
> 
> this is meant to simulate a minor/major version number.
> We will mark minor new features with values up to 15,
> and if something happens that requires a change in the
> backend we will move above 15, at which point we
> will submit backend fixes and also the necessary
> update to ./configure

I see.  A comment in the code would be nice to explain that.

> > > -    ring->cur = NETMAP_RING_NEXT(ring, i);
> > > -    ring->avail--;
> > > +    ring->cur = ring->head = nm_ring_next(ring, i);
> > >      ioctl(s->me.fd, NIOCTXSYNC, NULL);
> > >
> > >      return size;
> >
> > Are these changes related to the NETMAP_WITH_LIBS macro?  Please do that
> > in a separate patch so we keep the version checking change separate from
> > the NETMAP_WITH_LIBS change.
> >
> 
> netmap version 11 and above has NETMAP_WITH_LIBS,
> while previous versions do not, so this ./configure
> change has to go together with the change in the backend.
> 
> The netmap 11 code has already been committed to the FreeBSD
> source repositories (for HEAD, 10 and 9) and to
> code.google.com/p/netmap/ (for those who want it on linux).
> 
> So there is really no point, in my opinion, to make one
> intermediate commit just to ./configure to disable
> netmap detection on FreeBSD (it is already off on linux),
> immediately followed by this one that uses the new feature.
> 
> Just to clarify: with one exception (fields in struct netmap_ring)
> the netmap changes that we have are not at the kernel/user boundary
> but in a library which avoids replicating long and boring code
> (interface name parsing, parameter setting) in applications.
> 
> Avoiding the single incompatible struct change would have
> been of course possible, but at the cost
> extra complexity in the kernel and in userspace
> (to support two slightly different interfaces).
> Since netmap is (so far) relatively little used I thought it
> was more important to fix the API and keep it simple.

Thanks for explaining.  Please put justification for the
NETMAP_WITH_LIBS changes in the commit description.

Stefan