[libvirt] [PATCH 1/2] Modify generic ethernet interface so it will work when sVirt is enabled with qemu

[Eric Blake]

On 09/20/2011 05:17 PM, Tyler Coumbes wrote:
> Add a generic utility library for working with the TUN driver (/dev/net/tun).

Thanks for posting these patches.  Next time, can you convince your 
mailer to send the series as a single thread (that is, make both 1/2 and 
2/2 have In-Reply-To set to point to 0/2)?  'git send-email' makes this 
relatively easy.  Also, sending patches with a diffstat makes it easier 
to see at a glance how much the patch involves.

>
> ---
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 738ee91..ddd1b77 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -88,7 +88,8 @@ UTIL_SOURCES =							\
>   		util/xml.c util/xml.h				\
>   		util/virterror.c util/virterror_internal.h	\
>   		util/virkeycode.c util/virkeycode.h		\
> -		util/virkeymaps.h
> +		util/virkeymaps.h \
> +		util/tunctl.c util/tunctl.h

Being a new file, it would probably be better to go with a name starting 
with 'vir', as in virtunctl (that is, this file represents libvirt's 
wrappers around TUN, and not a generic TUN manipulation library).

>
>   EXTRA_DIST += $(srcdir)/util/virkeymaps.h $(srcdir)/util/keymaps.csv \
>   		$(srcdir)/util/virkeycode-mapgen.py
> @@ -1182,6 +1183,8 @@ if WITH_NETWORK
>   USED_SYM_FILES += libvirt_network.syms
>   endif
>
> +USED_SYM_FILES += libvirt_tunctl.syms

If [vir]tunctl is compiled unconditionally, then we don't need a new 
.syms file - just stick the new symbols in libvirt_private.syms.  On the 
other hand, since TUN tends to be a Linux-specific concept, I think you 
need to compile this code conditionally, at which point maybe the 
existing libvirt_linux.syms is a better fit.

> +
>   EXTRA_DIST += \
>     libvirt_public.syms		\
>     libvirt_private.syms		\

And if all else fails and you really do need to create a new .syms file, 
then it needs to be listed in EXTRA_DIST.

> diff --git a/src/libvirt_tunctl.syms b/src/libvirt_tunctl.syms
> new file mode 100644
> index 0000000..d1e00bb
> --- /dev/null
> +++ b/src/libvirt_tunctl.syms
> @@ -0,0 +1,5 @@
> +#tunctl.h
> +createTap;
> +delTap;
> +tapSetInterfaceUp;
> +tapSetInterfaceMac;

This naming isn't namespace clean.  It might be better to go with:

virTunCreateTap
virTunDeleteTap
virTunSetInterfaceUp
virTunSetInterfaceMac

Oh, and if all of the functions are named tap instead of tun, then maybe 
naming the file virtap instead of virtun would make more sense.  Which 
is it, tap or tun?

> diff --git a/src/util/tunctl.c b/src/util/tunctl.c
> new file mode 100644
> index 0000000..e758e6d
> --- /dev/null
> +++ b/src/util/tunctl.c
> @@ -0,0 +1,295 @@
> +/*
> + * Copyright (C) 2007, 2009, 2011 Red Hat, Inc.

It's okay to list earlier years if your file was copying substantial 
layout from another file with those years, but if the file really is 
from scratch, it may be okay to list just 2011.
What file were you copying from,

> + *
> + * Authors:
> + */

Feel free to list yourself, if you'd like.  This line looks awkward with 
no author listing.

> +
> +# include<config.h>
> +# include "tunctl.h"
> +# include "virfile.h"
> +
> +# include<stdlib.h>
> +# include<stdio.h>
> +# include<string.h>
> +# include<unistd.h>
> +# include<fcntl.h>
> +# include<errno.h>
> +# include<arpa/inet.h>
> +# include<sys/types.h>
> +# include<sys/socket.h>
> +# include<sys/ioctl.h>
> +# include<paths.h>
> +# include<sys/wait.h>
> +
> +# include<linux/param.h>      /* HZ                 */

Yep, definitely Linux-specific, so the Makefile.am will need to be 
written so that this file is only conditionally compiled - basically, 
your new file should be in the same conditionals as the existing 
src/util/bridge.c.

> +# ifdef IFF_VNET_HDR
> +static int tapProbeVnetHdr(int tapfd)
> +{
> +#  if defined(IFF_VNET_HDR)&&  defined(TUNGETFEATURES)&&  defined(TUNGETIFF)

This looks very similar to brProbeVnetHdr in src/util/bridge.c.  If you 
are going to refactor code into a new file for larger reuse later, then 
it may be better to do the full code motion in a single commit, rather 
than to add the new function now and delete the original code later. 
That's not a hard-and-fast rule, but whichever way you go, it would be 
nice to mention in the commit message that you are adding the new code 
here in order to refactor common code out of bridge.c later.  It took me 
a while to realize that you were reusing code rather than writing it 
from scratch.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org