[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]