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

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



Long subject line; I trimmed it:

bridge: modify for use when sVirt is enabled with qemu

Use 'git shortlog -30' to get a feel for typical subjects.

On 10/24/2011 05:43 PM, Tyler Coumbes wrote:
This refactors the TAP creation code out of brAddTap into a new
function brCreateTap to allow it to be used on its own. I have also
changed ifSetInterfaceMac to brSetInterfaceMac and exported it since
it is will be needed by code outside of util/bridge.c in the next
patch.

  AUTHORS                 |    1 +
  src/libvirt_bridge.syms |    2 +
  src/util/bridge.c       |  116 +++++++++++++++++++++++++++++++----------------
  src/util/bridge.h       |    9 ++++
  4 files changed, 89 insertions(+), 39 deletions(-)
+++ b/src/libvirt_bridge.syms
@@ -12,10 +12,12 @@ brAddTap;
  brDeleteTap;
  brDeleteBridge;
  brDelInetAddress;
+brCreateTap;

Not sorted, but easy to fix.

-static int ifSetInterfaceMac(brControl *ctl, const char *ifname,
+int brSetInterfaceMac(brControl *ctl, const char *ifname,
                               const unsigned char *macaddr)

I touched up the indentation here to match.

  {
      struct ifreq ifr;
@@ -478,32 +478,12 @@ brAddTap(brControl *ctl,
           bool up,
           int *tapfd)
  {
-    int fd;
-    struct ifreq ifr;
-
      if (!ctl || !ctl->fd || !bridge || !ifname)
          return EINVAL;

-    if ((fd = open("/dev/net/tun", O_RDWR))<  0)
-      return errno;
-
-    memset(&ifr, 0, sizeof(ifr));
-
-    ifr.ifr_flags = IFF_TAP|IFF_NO_PI;
+    errno = brCreateTap(ctl, ifname, vnet_hdr, tapfd);

Generally, we prefer returning -1 on failure, rather than a positive errno value; but this is pre-existing.


-# ifdef IFF_VNET_HDR
-    if (vnet_hdr&&  brProbeVnetHdr(fd))
-        ifr.ifr_flags |= IFF_VNET_HDR;
-# else
-    (void) vnet_hdr;
-# endif
-
-    if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) {
-        errno = EINVAL;
-        goto error;
-    }
-
-    if (ioctl(fd, TUNSETIFF,&ifr)<  0)
+    if(*tapfd<  0 || errno)

space after if.

  }



+/**
+ * brCreateTap:
+ * @ctl: bridge control pointer
+ * @ifname: the interface name
+ * @vnet_hr: whether to try enabling IFF_VNET_HDR
+ * @tapfd: file descriptor return value for the new tap device
+ *
+ * Creates a tap interface.
+ * If the @tapfd parameter is supplied, the open tap device file
+ * descriptor will be returned, otherwise the TAP device will be made
+ * persistent and closed. The caller must use brDeleteTap to remove
+ * a persistent TAP devices when it is no longer needed.
+ *
+ * Returns 0 in case of success or an errno code in case of failure.

So your choice of convention isn't typical, but fits the existing code.

+ */
+
+int brCreateTap (brControl *ctl ATTRIBUTE_UNUSED,

No space after function names.

+                 char **ifname,
+                 int vnet_hdr,
+                 int *tapfd)
+{
+
+    int fd;

Extra blank line.

+    struct ifreq ifr;
+
+    if (!ifname)
+        return EINVAL;
+
+    if ((fd = open("/dev/net/tun", O_RDWR))<  0)
+      return errno;

Inconsistent indentation.

+
+    memset(&ifr, 0, sizeof(ifr));
+
+    ifr.ifr_flags = IFF_TAP|IFF_NO_PI;
+
+# ifdef IFF_VNET_HDR
+    if (vnet_hdr&&  brProbeVnetHdr(fd))
+        ifr.ifr_flags |= IFF_VNET_HDR;
+# else
+    (void) vnet_hdr;

I don't like the cast to void; marking the parameter ATTRIBUTE_UNUSED is sufficient.

But those are minor.  So:

ACK. I squashed this in and pushed.

diff --git i/src/libvirt_bridge.syms w/src/libvirt_bridge.syms
index 669830b..626f6ee 100644
--- i/src/libvirt_bridge.syms
+++ w/src/libvirt_bridge.syms
@@ -9,10 +9,10 @@ brAddBridge;
 brAddInetAddress;
 brAddInterface;
 brAddTap;
-brDeleteTap;
-brDeleteBridge;
-brDelInetAddress;
 brCreateTap;
+brDelInetAddress;
+brDeleteBridge;
+brDeleteTap;
 brHasBridge;
 brInit;
 brSetEnableSTP;
diff --git i/src/util/bridge.c w/src/util/bridge.c
index 4875f52..952f0f3 100644
--- i/src/util/bridge.c
+++ w/src/util/bridge.c
@@ -288,8 +288,9 @@ brDeleteInterface(brControl *ctl ATTRIBUTE_UNUSED,
  *
  * Returns 0 in case of success or an errno code in case of failure.
  */
-int brSetInterfaceMac(brControl *ctl, const char *ifname,
-                             const unsigned char *macaddr)
+int
+brSetInterfaceMac(brControl *ctl, const char *ifname,
+                  const unsigned char *macaddr)
 {
     struct ifreq ifr;

@@ -483,7 +484,7 @@ brAddTap(brControl *ctl,

     errno = brCreateTap(ctl, ifname, vnet_hdr, tapfd);

-    if(*tapfd < 0 || errno)
+    if (*tapfd < 0 || errno)
         goto error;

     /* We need to set the interface MAC before adding it
@@ -789,12 +790,12 @@ cleanup:
  * Returns 0 in case of success or an errno code in case of failure.
  */

-int brCreateTap (brControl *ctl ATTRIBUTE_UNUSED,
-                 char **ifname,
-                 int vnet_hdr,
-                 int *tapfd)
+int
+brCreateTap(brControl *ctl ATTRIBUTE_UNUSED,
+            char **ifname,
+            int vnet_hdr ATTRIBUTE_UNUSED,
+            int *tapfd)
 {
-
     int fd;
     struct ifreq ifr;

@@ -802,7 +803,7 @@ int brCreateTap (brControl *ctl ATTRIBUTE_UNUSED,
         return EINVAL;

     if ((fd = open("/dev/net/tun", O_RDWR)) < 0)
-      return errno;
+        return errno;

     memset(&ifr, 0, sizeof(ifr));

@@ -811,8 +812,6 @@ int brCreateTap (brControl *ctl ATTRIBUTE_UNUSED,
 # ifdef IFF_VNET_HDR
     if (vnet_hdr && brProbeVnetHdr(fd))
         ifr.ifr_flags |= IFF_VNET_HDR;
-# else
-    (void) vnet_hdr;
 # endif

     if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) {

--
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]