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

[Eric Blake]

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