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

Re: [libvirt] [PATCH v3]: set and restore MAC address of a NIC when using PASSTHROUGH mode



On 06/21/2011 05:08 AM, Gerhard Stenzel wrote:
This is another rework of the patch from Dirk addressing all comments
received so far.

The following patch addresses the problem that when a PASSTHROUGH
mode DIRECT NIC connection is made the MAC address of the NIC is
not automatically set and reset to the configured VM MAC and
back again.

The attached patch fixes this problem by setting and resetting the MAC
while remembering the previous setting while the VM is running.
This also works if libvirtd is restarted while the VM is running.

the patch passes make syntax-check

Signed-off-by: Dirk Herrendoerfer<d.herrendoerfer at herrendoerfer.name>
Signed-off-by: Gerhard Stenzel<gerhard stenzel de ibm com>

---


Index: libvirt/src/qemu/qemu_command.c
===================================================================
--- libvirt.orig/src/qemu/qemu_command.c
+++ libvirt/src/qemu/qemu_command.c
@@ -128,7 +128,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def
      rc = openMacvtapTap(net->ifname, net->mac, net->data.direct.linkdev,
                          net->data.direct.mode, vnet_hdr, def->uuid,
                          &net->data.direct.virtPortProfile,&res_ifname,
-                        vmop);
+                        vmop, driver->stateDir);
      if (rc>= 0) {
          qemuAuditNetDevice(def, net, res_ifname, true);
          VIR_FREE(net->ifname);
@@ -149,7 +149,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def
              if (err) {
                  VIR_FORCE_CLOSE(rc);
                  delMacvtap(net->ifname, net->mac, net->data.direct.linkdev,
-&net->data.direct.virtPortProfile);
+                           net->data.direct.mode,
+&net->data.direct.virtPortProfile,
+                           driver->stateDir);
                  VIR_FREE(net->ifname);
              }
          }
Index: libvirt/src/qemu/qemu_process.c
===================================================================
--- libvirt.orig/src/qemu/qemu_process.c
+++ libvirt/src/qemu/qemu_process.c
@@ -2876,7 +2876,8 @@ void qemuProcessStop(struct qemud_driver
          virDomainNetDefPtr net = def->nets[i];
          if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
              delMacvtap(net->ifname, net->mac, net->data.direct.linkdev,
-&net->data.direct.virtPortProfile);
+                       net->data.direct.mode,
+&net->data.direct.virtPortProfile, driver->stateDir);
              VIR_FREE(net->ifname);
          }
      }
Index: libvirt/src/util/macvtap.c
===================================================================
--- libvirt.orig/src/util/macvtap.c
+++ libvirt/src/util/macvtap.c
@@ -545,6 +545,106 @@ configMacvtapTap(int tapfd, int vnet_hdr
      return 0;
  }

+/**
+ * replaceMacAdress:
+ * @macaddress: new MAC address for interface
+ * @linkdev: name of interface
+ * @stateDir: directory to store old MAC address
+ *
+ * Returns 0 on success, -1 in case of fatal error, error code otherwise.
+ *
+ */
+static int
+replaceMacAdress(const unsigned char *macaddress,
+                 const char *linkdev,
+                 char *stateDir)
+{
+    unsigned char oldmac[6];
+    int rc;
+
+    rc = ifaceGetMacaddr(linkdev, oldmac);
+
+    if (rc) {
+        virReportSystemError(rc,
+                             _("Getting MAC address from '%s' "
+                               "to '%02x:%02x:%02x:%02x:%02x:%02x' failed."),
+                             linkdev,
+                             oldmac[0], oldmac[1], oldmac[2],
+                             oldmac[3], oldmac[4], oldmac[5]);
+    } else {
+        char *path = NULL;
+
+        char macstr[VIR_MAC_STRING_BUFLEN];
+        if (virAsprintf(&path, "%s/%s",
Minor nit, one empty line after the macstr and remove the empty line before it.
+                        stateDir,
+                        linkdev)<  0) {
+            virReportOOMError();
+            return errno;
+        }
+        virFormatMacAddr(oldmac, macstr);
+        if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY)<  0) {
+            virReportSystemError(errno, _("Unable to preserve mac for %s"),
+                                 linkdev);
+            return errno;
+        }
+    }
+
+    rc = ifaceSetMacaddr(linkdev, macaddress);
+    if (rc) {
+        virReportSystemError(errno,
+                             _("Setting MAC address on  '%s' to "
+                               "'%02x:%02x:%02x:%02x:%02x:%02x' failed."),
+                             linkdev,
+                             macaddress[0], macaddress[1], macaddress[2],
+                             macaddress[3], macaddress[4], macaddress[5]);
+    }
+    return rc;
+}
+
+/**
+ * restoreMacAddress:
+ * @linkdev: name of interface
+ * @stateDir: directory containing old MAC address
+ *
+ * Returns 0 on success, -1 in case of fatal error, error code otherwise.
+ *
+ */
+static int
+restoreMacAddress(const char *linkdev,
+                  char *stateDir)
+{
+    int ret;
+    char *oldmacname = NULL;
+    char *macstr = NULL;
+    char *path = NULL;
+    unsigned char oldmac[6];
+
+    if (virAsprintf(&path, "%s/%s",
+                    stateDir,
+                    linkdev)<  0) {
+        virReportOOMError();
+        return -1;
+    }
+
+    if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN,&macstr)<  0) {
+        virReportSystemError(errno, _("Unable to preserve mac for %s"),
+                             linkdev);
+        return errno;
+    }
+
+    if (virParseMacAddr(macstr,&oldmac[0]) != 0) {
+        virReportSystemError(VIR_ERR_INTERNAL_ERROR,
+                             _("Cannot parse MAC address from '%s'"),
+                             oldmacname);
Due to the VIR_ERROr_INTERNAL_ERROR  call macvtapError here.
+        return -1;
+    }
+
+    /*reset mac and remove file-ignore results*/
+    ret = ifaceSetMacaddr(linkdev, oldmac);
In this case I'd check the return code and return -1 if ret != 0.
+    ret = unlink(path);
Here you can probably leave 'ret =' out.
+    VIR_FREE(macstr);
+    return 0;
+}

  /**
   * openMacvtapTap:
@@ -575,7 +675,8 @@ openMacvtapTap(const char *tgifname,
                 const unsigned char *vmuuid,
                 virVirtualPortProfileParamsPtr virtPortProfile,
                 char **res_ifname,
-               enum virVMOperationType vmOp)
+               enum virVMOperationType vmOp,
+               char *stateDir)
  {
      const char *type = "macvtap";
      int c, rc;
@@ -589,6 +690,21 @@ openMacvtapTap(const char *tgifname,

      VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, virVMOperationTypeToString(vmOp));

+    /** Note: When using PASSTHROUGH mode with MACVTAP devices the link
+     * device's MAC address must be set to the VMs MAC address. In
+     * order to not confuse the first switch or bridge in line this MAC
+     * address must be reset when the VM is shut down.
+     * This is especially important when using SRIOV capable cards that
+     * emulate their switch in firmware.
+     */
+    if (mode == VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU) {
+        if (replaceMacAdress(macaddress, linkdev, stateDir) != 0) {
+            virReportSystemError(errno,
+                                 _("cannot replace MAC address on $s"),
+                                 linkdev);
It looks like replaceMacAddress() already reports error messages, so another one here is not necessary. It would have to be %s rather than $s. Do a 'return -1' here.
+        }
+    }
+
      if (tgifname) {
          if(ifaceGetIndex(false, tgifname,&ifindex) == 0) {
              if (STRPREFIX(tgifname,
@@ -684,8 +800,18 @@ void
  delMacvtap(const char *ifname,
             const unsigned char *macaddr,
             const char *linkdev,
-           virVirtualPortProfileParamsPtr virtPortProfile)
+           int mode,
+           virVirtualPortProfileParamsPtr virtPortProfile,
+           char *stateDir)
  {
+    if (mode == VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU) {
+        if (restoreMacAddress(linkdev, stateDir) != 0) {
+            virReportSystemError(errno,
+                                 _("cannot restore MAC address on $s"),
+                                 linkdev);
+        }
Don't report the error, restoreMacAddress already did.
+    }
+
      if (ifname) {
          vpDisassociatePortProfileId(ifname, macaddr,
                                      linkdev,
Index: libvirt/src/util/macvtap.h
===================================================================
--- libvirt.orig/src/util/macvtap.h
+++ libvirt/src/util/macvtap.h
@@ -83,12 +83,15 @@ int openMacvtapTap(const char *ifname,
                     const unsigned char *vmuuid,
                     virVirtualPortProfileParamsPtr virtPortProfile,
                     char **res_ifname,
-                   enum virVMOperationType vmop);
+                   enum virVMOperationType vmop,
+                   char *stateDir);

  void delMacvtap(const char *ifname,
                  const unsigned char *macaddress,
                  const char *linkdev,
-                virVirtualPortProfileParamsPtr virtPortProfile);
+                int mode,
+                virVirtualPortProfileParamsPtr virtPortProfile,
+                char *stateDir);

  int vpAssociatePortProfileId(const char *macvtap_ifname,
                               const unsigned char *macvtap_macaddr,
Index: libvirt/src/qemu/qemu_hotplug.c
===================================================================
--- libvirt.orig/src/qemu/qemu_hotplug.c
+++ libvirt/src/qemu/qemu_hotplug.c
@@ -1610,7 +1610,9 @@ int qemuDomainDetachNetDevice(struct qem
  #if WITH_MACVTAP
      if (detach->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
          delMacvtap(detach->ifname, detach->mac, detach->data.direct.linkdev,
-&detach->data.direct.virtPortProfile);
+                   detach->data.direct.mode,
+&detach->data.direct.virtPortProfile,
+                   driver->stateDir);
          VIR_FREE(detach->ifname);
      }
  #endif
Index: libvirt/src/util/interface.c
===================================================================
--- libvirt.orig/src/util/interface.c
+++ libvirt/src/util/interface.c
@@ -390,3 +390,102 @@ ifaceGetVlanID(const char *vlanifname AT
      return ENOSYS;
  }
  #endif /* __linux__ */
+
+/**
+ * ifaceGetMacaddr:
+ * @ifname: interface name to set MTU for
+ * @macaddr: MAC address (VIR_MAC_BUFLEN in size)
+ *
+ * This function gets the @macaddr for a given interface @ifname.
+ *
+ * Returns 0 in case of success or an errno code in case of failure.
+ */
+#ifdef __linux__
+int
+ifaceGetMacaddr(const char *ifname,
+                unsigned char *macaddr)
+{
+    struct ifreq ifr;
+    int fd;
+
+    if (!ifname)
+        return EINVAL;
+
+    fd = socket(AF_INET, SOCK_STREAM, 0);
+    if(fd<  0){
+        return errno;
+    }
+
Nit, remove '{' and '}'
+    memset(&ifr, 0, sizeof(struct ifreq));
+    if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL)
+        return EINVAL;
+
+    if(ioctl(fd, SIOCGIFHWADDR, (char *)&ifr) != 0){
+        return errno;
+    }
+
Minor nit, remove '{' and '}'.
+    memcpy(macaddr, ifr.ifr_ifru.ifru_hwaddr.sa_data, VIR_MAC_BUFLEN);
+
+    return 0;
+}
+
+#else
+
+int
+ifaceGetMacaddr(const char *ifname,
+                unsigned char *macaddr)
+{
+    return ENOSYS;
+}
+
+#endif /* __linux__ */
+
+/**
+ * ifaceSetMacaddr:
+ * @ifname: interface name to set MTU for
+ * @macaddr: MAC address (VIR_MAC_BUFLEN in size)
+ *
+ * This function sets the @macaddr for a given interface @ifname. This
+ * gets rid of the kernel's automatically assigned random MAC.
+ *
+ * Returns 0 in case of success or an errno code in case of failure.
+ */
+#ifdef __linux__
+int
+ifaceSetMacaddr(const char *ifname,
+                const unsigned char *macaddr)
+{
+    struct ifreq ifr;
+    int fd;
+
+    if (!ifname)
+        return EINVAL;
+
+    fd = socket(AF_INET, SOCK_STREAM, 0);
+    if(fd<  0){
+        return errno;
+    }
+
Also here.
+    memset(&ifr, 0, sizeof(struct ifreq));
+    if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL)
+        return EINVAL;
+
+    /* To fill ifr.ifr_hdaddr.sa_family field */
+    if (ioctl(fd, SIOCGIFHWADDR,&ifr) != 0)
+        return errno;
+
+    memcpy(ifr.ifr_hwaddr.sa_data, macaddr, VIR_MAC_BUFLEN);
+
+    return ioctl(fd, SIOCSIFHWADDR,&ifr) == 0 ? 0 : errno;
+}
+
+#else
+
+int
+ifaceSetMacaddr(const char *ifname,
+                const unsigned char *macaddr)
+{
+    return ENOSYS;
+}
+
+#endif /* __linux__ */
Index: libvirt/src/util/interface.h
===================================================================
--- libvirt.orig/src/util/interface.h
+++ libvirt/src/util/interface.h
@@ -32,4 +32,8 @@ int ifaceGetIndex(bool reportError, cons

  int ifaceGetVlanID(const char *vlanifname, int *vlanid);

+int ifaceSetMacaddr(const char *ifname, const unsigned char *macaddr);
+
+int ifaceGetMacaddr(const char *ifname, unsigned char *macaddr);
+
  #endif /* __VIR_INTERFACE_H__ */
Index: libvirt/src/libvirt_private.syms
===================================================================
--- libvirt.orig/src/libvirt_private.syms
+++ libvirt/src/libvirt_private.syms
@@ -507,7 +507,9 @@ ifaceCheck;
  ifaceCtrl;
  ifaceGetFlags;
  ifaceGetIndex;
+ifaceGetMacaddr;
  ifaceGetVlanID;
+ifaceSetMacaddr;
  ifaceIsUp;

===================================================================

Looks generally better IMO. Please send a v4.

   Stefan
Best regards,

Gerhard Stenzel
-------------------------------------------------------------------------------------
IBM Deutschland Research&  Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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