Re: [PATCHv2 3/5] netdevmacvlan: Use helper function to create unique macvlan/macvtap name

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

 



On 12/13/20 8:20 PM, Shi Lei wrote:
On 2020-12-14 at 08:58, Laine Stump wrote:
On 12/9/20 10:00 PM, Shi Lei wrote:
Simplify ReserveName/GenerateName for macvlan and macvtap by using
common functions.

Signed-off-by: Shi Lei <shi_lei@xxxxxxxxxxxxxx>
---
    src/libvirt_private.syms    |   1 -
    src/lxc/lxc_process.c       |   2 +-
    src/qemu/qemu_process.c     |   2 +-
    src/util/virnetdevmacvlan.c | 177 +++++-------------------------------
    src/util/virnetdevmacvlan.h |  14 +--
    5 files changed, 26 insertions(+), 170 deletions(-)
You probably don't have the libxl driver enabled in your builds, so you
missed this change (which I've squashed in):

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 6af274cb1b..3488d7ec08 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -364,7 +364,7 @@ libxlReconnectNotifyNets(virDomainDefPtr def)
            * impolite.
            */
           if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT)
-            virNetDevMacVLanReserveName(net->ifname);
+            virNetDevReserveName(net->ifname);

Sorry for that. I fix it and send V3.


No reason to send a v3 for something that simple! I could just fix it up here.


However, if you are going to send a V3, I just sent a comment about an addition that is needed in Patch 1 that is a bit more than just an omitted function name change - you'll want to put that (or something similar) in if you repost.


Also, it occurred to me just now while looking at that - it would be nice if the name change from VIR_NET_GENERATED_TAP_PREFIX to VIR_NET_GENERATED_VNET_PREFIX was a patch by itself (prerequisite to all the other patches). That one isn't really necessary, though.


So if you're okay with me making the change I just posted in my reply for Patch 1, I can just push it all right now - no need for a V3 (I think! I'm still looking at patch 4 and 5 one last time - I should be done looking at them in 30 minutes or so).



Shi Lei

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 64ef01e1..4d6ae84b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2655,7 +2655,6 @@ virNetDevMacVLanDelete;
    virNetDevMacVLanDeleteWithVPortProfile;
    virNetDevMacVLanIsMacvtap;
    virNetDevMacVLanModeTypeFromString;
-virNetDevMacVLanReserveName;
    virNetDevMacVLanRestartWithVPortProfile;
    virNetDevMacVLanTapOpen;
    virNetDevMacVLanTapSetup;
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 0f818e2e..eb29431e 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1641,7 +1641,7 @@ virLXCProcessReconnectNotifyNets(virDomainDefPtr def)
             * impolite.
             */
            if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT)
-            virNetDevMacVLanReserveName(net->ifname);
+            virNetDevReserveName(net->ifname);
           if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
                if (!conn && !(conn = virGetConnectNetwork()))
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1d54f201..6244ade4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3390,7 +3390,7 @@ qemuProcessNotifyNets(virDomainDefPtr def)
             */
            switch (virDomainNetGetActualType(net)) {
            case VIR_DOMAIN_NET_TYPE_DIRECT:
-            virNetDevMacVLanReserveName(net->ifname);
+            virNetDevReserveName(net->ifname);
                break;
            case VIR_DOMAIN_NET_TYPE_BRIDGE:
            case VIR_DOMAIN_NET_TYPE_NETWORK:
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 72f0d670..36b13133 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -45,7 +45,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode,
   # include <net/if.h>
    # include <linux/if_tun.h>
-# include <math.h>
   # include "viralloc.h"
    # include "virlog.h"
@@ -59,129 +58,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode,
   VIR_LOG_INIT("util.netdevmacvlan"); -# define VIR_NET_GENERATED_MACVTAP_PATTERN VIR_NET_GENERATED_MACVTAP_PREFIX "%d"
-# define VIR_NET_GENERATED_MACVLAN_PATTERN VIR_NET_GENERATED_MACVLAN_PREFIX "%d"
-# define VIR_NET_GENERATED_PREFIX \
-    ((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \
-     VIR_NET_GENERATED_MACVTAP_PREFIX : VIR_NET_GENERATED_MACVLAN_PREFIX)
-
-
-virMutex virNetDevMacVLanCreateMutex = VIR_MUTEX_INITIALIZER;
-static int virNetDevMacVTapLastID = -1;
-static int virNetDevMacVLanLastID = -1;
-
-
-static void
-virNetDevMacVLanReserveNameInternal(const char *name)
-{
-    unsigned int id;
-    const char *idstr = NULL;
-    int *lastID = NULL;
-    int len;
-
-    if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX)) {
-        lastID = &virNetDevMacVTapLastID;
-        len = strlen(VIR_NET_GENERATED_MACVTAP_PREFIX);
-    } else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX)) {
-        lastID = &virNetDevMacVTapLastID;
-        len = strlen(VIR_NET_GENERATED_MACVLAN_PREFIX);
-    } else {
-        return;
-    }
-
-    VIR_INFO("marking device in use: '%s'", name);
-
-    idstr = name + len;
-
-    if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
-        if (*lastID < (int)id)
-            *lastID = id;
-    }
-}
-
-
-/**
- * virNetDevMacVLanReserveName:
- * @name: name of an existing macvtap/macvlan device
- *
- * Set the value of virNetDevMacV(Lan|Tap)LastID to assure that any
- * new device created with an autogenerated name will use a number
- * higher than the number in the given device name.
- *
- * Returns nothing.
- */
-void
-virNetDevMacVLanReserveName(const char *name)
-{
-    virMutexLock(&virNetDevMacVLanCreateMutex);
-    virNetDevMacVLanReserveNameInternal(name);
-    virMutexUnlock(&virNetDevMacVLanCreateMutex);
-}
-
-
-/**
- * virNetDevMacVLanGenerateName:
- * @ifname: pointer to pointer to string containing template
- * @lastID: counter to add to the template to form the name
- *
- * generate a new (currently unused) name for a new macvtap/macvlan
- * device based on the template string in @ifname - replace %d with
- * ++(*counter), and keep trying new values until one is found
- * that doesn't already exist, or we've tried 10000 different
- * names. Once a usable name is found, replace the template with the
- * actual name.
- *
- * Returns 0 on success, -1 on failure.
- */
-static int
-virNetDevMacVLanGenerateName(char **ifname, unsigned int flags)
-{
-    const char *prefix;
-    const char *iftemplate;
-    int *lastID;
-    int id;
-    double maxIDd;
-    int maxID = INT_MAX;
-    int attempts = 0;
-
-    if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) {
-        prefix = VIR_NET_GENERATED_MACVTAP_PREFIX;
-        iftemplate = VIR_NET_GENERATED_MACVTAP_PREFIX "%d";
-        lastID = &virNetDevMacVTapLastID;
-    } else {
-        prefix = VIR_NET_GENERATED_MACVLAN_PREFIX;
-        iftemplate = VIR_NET_GENERATED_MACVLAN_PREFIX "%d";
-        lastID = &virNetDevMacVLanLastID;
-    }
-
-    maxIDd = pow(10, IFNAMSIZ - 1 - strlen(prefix));
-    if (maxIDd <= (double)INT_MAX)
-        maxID = (int)maxIDd;
-
-    do {
-        g_autofree char *try = NULL;
-
-        id = ++(*lastID);
-
-        /* reset before overflow */
-        if (*lastID == maxID)
-            *lastID = -1;
-
-        try = g_strdup_printf(iftemplate, id);
-
-        if (!virNetDevExists(try)) {
-            g_free(*ifname);
-            *ifname = g_steal_pointer(&try);
-            return 0;
-        }
-    } while (++attempts < 10000);
-
-    virReportError(VIR_ERR_INTERNAL_ERROR,
-                   _("no unused %s names available"),
-                   *ifname);
-    return -1;
-}
-
   /**
     * virNetDevMacVLanIsMacvtap:
@@ -209,13 +85,10 @@ virNetDevMacVLanIsMacvtap(const char *ifname)
     * virNetDevMacVLanCreate:
     *
     * @ifname: The name the interface is supposed to have; optional parameter
- * @type: The type of device, i.e., "macvtap", "macvlan"
     * @macaddress: The MAC address of the device
     * @srcdev: The name of the 'link' device
     * @macvlan_mode: The macvlan mode to use
- * @retry: Pointer to integer that will be '1' upon return if an interface
- *         with the same name already exists and it is worth to try
- *         again with a different name
+ * @flags: OR of virNetDevMacVLanCreateFlags.
     *
     * Create a macvtap device with the given properties.
     *
@@ -223,19 +96,21 @@ virNetDevMacVLanIsMacvtap(const char *ifname)
     */
    int
    virNetDevMacVLanCreate(const char *ifname,
-                       const char *type,
                           const virMacAddr *macaddress,
                           const char *srcdev,
-                       uint32_t macvlan_mode)
+                       uint32_t macvlan_mode,
+                       unsigned int flags)
    {
        int error = 0;
        int ifindex = 0;
+    const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
+                       VIR_NET_GENERATED_MACVTAP_PREFIX :
+                       VIR_NET_GENERATED_MACVLAN_PREFIX;
        virNetlinkNewLinkData data = {
            .macvlan_mode = &macvlan_mode,
            .mac = macaddress,
        };
-
        if (virNetDevGetIndex(srcdev, &ifindex) < 0)
            return -1;
@@ -795,7 +670,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
                                           size_t tapfdSize,
                                           unsigned int flags)
    {
-    const char *type = VIR_NET_GENERATED_PREFIX;
        g_autofree char *ifname = NULL;
        uint32_t macvtapMode;
        int vf = -1;
@@ -832,8 +706,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
               return -1;
        }
-    virMutexLock(&virNetDevMacVLanCreateMutex);
-
        if (ifnameRequested) {
            int rc;
            bool isAutoName > @@ -842,10 +714,8 @@ virNetDevMacVLanCreateWithVPortProfile(const
char *ifnameRequested,
           VIR_INFO("Requested macvtap device name: %s", ifnameRequested); -        if ((rc = virNetDevExists(ifnameRequested)) < 0) {
-            virMutexUnlock(&virNetDevMacVLanCreateMutex);
+        if ((rc = virNetDevExists(ifnameRequested)) < 0)
                return -1;
-        }
           if (rc) {
                /* ifnameRequested is already being used */
@@ -854,17 +724,16 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
                    virReportSystemError(EEXIST,
                                         _("Unable to create device '%s'"),
                                         ifnameRequested);
-                virMutexUnlock(&virNetDevMacVLanCreateMutex);
                    return -1;
                }
            } else {
               /* ifnameRequested is available. try to open it */ -            virNetDevMacVLanReserveNameInternal(ifnameRequested);
+            virNetDevReserveName(ifnameRequested);
-            if (virNetDevMacVLanCreate(ifnameRequested, type, macaddress,
-                                       linkdev, macvtapMode) == 0) {
+            if (virNetDevMacVLanCreate(ifnameRequested, macaddress,
+                                       linkdev, macvtapMode, flags) == 0) {
                   /* virNetDevMacVLanCreate() was successful - use this name */
                    ifname = g_strdup(ifnameRequested);
@@ -874,7 +743,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
                     * autogenerated named, so there is nothing else to
                     * try - fail and return.
                     */
-                virMutexUnlock(&virNetDevMacVLanCreateMutex);
                    return -1;
                }
            }
@@ -885,16 +753,19 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
             * autogenerated name, so now we look for an unused
             * autogenerated name.
             */
-        if (virNetDevMacVLanGenerateName(&ifname, flags) < 0 ||
-            virNetDevMacVLanCreate(ifname, type, macaddress,
-                                   linkdev, macvtapMode) < 0) {
-            virMutexUnlock(&virNetDevMacVLanCreateMutex);
+        virNetDevGenNameType type;
+        if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP)
+            type = VIR_NET_DEV_GEN_NAME_MACVTAP;
+        else
+            type = VIR_NET_DEV_GEN_NAME_MACVLAN;
+
+        if (virNetDevGenerateName(&ifname, type) < 0 ||
+            virNetDevMacVLanCreate(ifname, macaddress,
+                                   linkdev, macvtapMode, flags) < 0)
                return -1;
-        }
        }
       /* all done creating the device */
-    virMutexUnlock(&virNetDevMacVLanCreateMutex);
       if (virNetDevVPortProfileAssociate(ifname,
                                           virtPortProfile,
@@ -1050,10 +921,10 @@ bool virNetDevMacVLanIsMacvtap(const char *ifname G_GNUC_UNUSED)
    }
   int virNetDevMacVLanCreate(const char *ifname G_GNUC_UNUSED,
-                           const char *type G_GNUC_UNUSED,
                               const virMacAddr *macaddress G_GNUC_UNUSED,
                               const char *srcdev G_GNUC_UNUSED,
-                           uint32_t macvlan_mode G_GNUC_UNUSED)
+                           uint32_t macvlan_mode G_GNUC_UNUSED,
+                           unsigned int fflags G_GNUC_UNUSED)
    {
        virReportSystemError(ENOSYS, "%s",
                             _("Cannot create macvlan devices on this platform"));
@@ -1141,10 +1012,4 @@ int virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname G_GNUC_UNUSE
                             _("Cannot create macvlan devices on this platform"));
        return -1;
    }
-
-void virNetDevMacVLanReserveName(const char *name G_GNUC_UNUSED)
-{
-    virReportSystemError(ENOSYS, "%s",
-                         _("Cannot create macvlan devices on this platform"));
-}
    #endif /* ! WITH_LIBNL */
diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
index 48800a8f..0a013fc2 100644
--- a/src/util/virnetdevmacvlan.h
+++ b/src/util/virnetdevmacvlan.h
@@ -48,23 +48,15 @@ typedef enum {
       VIR_NETDEV_MACVLAN_VNET_HDR          = 1 << 2,
    } virNetDevMacVLanCreateFlags;
-/* libvirt will start macvtap/macvlan interface names with one of
- * these prefixes when it auto-generates the name
- */
-#define VIR_NET_GENERATED_MACVTAP_PREFIX "macvtap"
-#define VIR_NET_GENERATED_MACVLAN_PREFIX "macvlan"
-
-void virNetDevMacVLanReserveName(const char *name);
-
    bool virNetDevMacVLanIsMacvtap(const char *ifname)
       ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE;
   int virNetDevMacVLanCreate(const char *ifname,
-                           const char *type,
                               const virMacAddr *macaddress,
                               const char *srcdev,
-                           uint32_t macvlan_mode)
-    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
+                           uint32_t macvlan_mode,
+                           unsigned int flags)
+    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
        G_GNUC_WARN_UNUSED_RESULT;
   int virNetDevMacVLanDelete(const char *ifname)





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux