1:01 a.m.
Extract ReserveName/GenerateName from netdevtap and netdevmacvlan as
common functions. Along with them, export Lock/Unlock functions
to protect these processes.
So tap, macvlan and veth can reuse them to simplify their
implementations.
Shi Lei (5):
netdev: Introduce several helper functions for generating unique netdev name
netdevtap: Use common helper function to create unique tap name
netdevmacvlan: Use helper function to create unique macvlan/macvtap name
netdev: Enable virNetDevGenerateName to support veth
netdevveth: Simplify virNetDevVethCreate by using virNetDevGenerateName
src/libvirt_private.syms | 4 +
src/lxc/lxc_process.c | 3 +
src/util/virnetdev.c | 142 +++++++++++++++++++++++++++++++++++
src/util/virnetdev.h | 36 +++++++++
src/util/virnetdevmacvlan.c | 107 ++++++--------------------
src/util/virnetdevmacvlan.h | 6 --
src/util/virnetdevtap.c | 58 +-------------
src/util/virnetdevveth.c | 146 ++++++++++--------------------------
8 files changed, 251 insertions(+), 251 deletions(-)
--
2.25.1
1:01 a.m.
New subject: [PATCH 1/5] netdev: Introduce several helper functions for generating unique netdev name
Extract ReserveName/GenerateName from netdevtap and netdevmacvlan as
common helper functions. Along with them, export Lock/Unlock functions
to protect these processes.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/libvirt_private.syms | 4 ++
src/util/virnetdev.c | 140 +++++++++++++++++++++++++++++++++++++++
src/util/virnetdev.h | 33 +++++++++
3 files changed, 177 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2f640ef1..be3148c9 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2551,6 +2551,7 @@ virNetDevDelMulti;
virNetDevExists;
virNetDevFeatureTypeFromString;
virNetDevFeatureTypeToString;
+virNetDevGenerateName;
virNetDevGetFeatures;
virNetDevGetIndex;
virNetDevGetLinkInfo;
@@ -2572,8 +2573,10 @@ virNetDevGetVLanID;
virNetDevIfStateTypeFromString;
virNetDevIfStateTypeToString;
virNetDevIsVirtualFunction;
+virNetDevLockGenName;
virNetDevPFGetVF;
virNetDevReadNetConfig;
+virNetDevReserveName;
virNetDevRunEthernetScript;
virNetDevRxFilterFree;
virNetDevRxFilterModeTypeFromString;
@@ -2594,6 +2597,7 @@ virNetDevSetRcvMulti;
virNetDevSetRootQDisc;
virNetDevSetupControl;
virNetDevSysfsFile;
+virNetDevUnlockGenName;
virNetDevValidateConfig;
virNetDevVFInterfaceStats;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5104bbe7..5ff8e35f 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -17,6 +17,7 @@
*/
#include <config.h>
+#include <math.h>
#include "virnetdev.h"
#include "viralloc.h"
@@ -95,6 +96,22 @@ VIR_LOG_INIT("util.netdev");
(FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
#endif
+VIR_ENUM_IMPL(virNetDevGenNameType,
+ VIR_NET_DEV_GEN_NAME_LAST,
+ "none",
+ "tap",
+ "macvtap",
+ "macvlan",
+);
+
+static virNetDevGenName
+virNetDevGenNames[VIR_NET_DEV_GEN_NAME_LAST] = {
+ {0, NULL, VIR_MUTEX_INITIALIZER},
+ {-1, VIR_NET_GENERATED_TAP_PREFIX, VIR_MUTEX_INITIALIZER},
+ {-1, VIR_NET_GENERATED_MACVTAP_PREFIX, VIR_MUTEX_INITIALIZER},
+ {-1, VIR_NET_GENERATED_MACVLAN_PREFIX, VIR_MUTEX_INITIALIZER},
+};
+
typedef enum {
VIR_MCAST_TYPE_INDEX_TOKEN,
VIR_MCAST_TYPE_NAME_TOKEN,
@@ -3516,3 +3533,126 @@ virNetDevSetRootQDisc(const char *ifname,
return 0;
}
+
+
+/**
+ * virNetDevReserveName:
+ * @name: name of an existing network device
+ * @type: type of the network device
+ * @locked: whether this process is locked by the internal mutex
+ *
+ * Reserve a network device name, so that any new network device
+ * created with an autogenerated name will use a number higher
+ * than the number in the given device name.
+ *
+ * Returns nothing.
+ */
+void
+virNetDevReserveName(const char *name,
+ virNetDevGenNameType type,
+ bool locked)
+{
+ unsigned int id;
+ const char *idstr = NULL;
+
+ if (type && STRPREFIX(name, virNetDevGenNames[type].prefix)) {
+
+ VIR_INFO("marking device in use: '%s'", name);
+
+ idstr = name + strlen(virNetDevGenNames[type].prefix);
+
+ if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
+ if (locked)
+ virMutexLock(&virNetDevGenNames[type].mutex);
+
+ if (virNetDevGenNames[type].lastID < (int)id)
+ virNetDevGenNames[type].lastID = id;
+
+ if (locked)
+ virMutexUnlock(&virNetDevGenNames[type].mutex);
+ }
+ }
+}
+
+
+/**
+ * virNetDevGenerateName:
+ * @ifname: pointer to pointer to string containing template
+ *
+ * generate a new (currently unused) name for a new network device based
+ * on the templace string in @ifname - replace %d with the reserved id,
+ * 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.
+ */
+int
+virNetDevGenerateName(char **ifname, virNetDevGenNameType type)
+{
+ int id;
+ const char *prefix = virNetDevGenNames[type].prefix;
+ double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(prefix));
+ int maxID = INT_MAX;
+ int attempts = 0;
+
+ if (maxIDd <= (double)INT_MAX)
+ maxID = (int)maxIDd;
+
+ do {
+ g_autofree char *try = NULL;
+
+ id = ++virNetDevGenNames[type].lastID;
+
+ /* reset before overflow */
+ if (virNetDevGenNames[type].lastID >= maxID)
+ virNetDevGenNames[type].lastID = -1;
+
+ if (*ifname)
+ try = g_strdup_printf(*ifname, id);
+ else
+ try = g_strdup_printf("%s%d", prefix, 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"),
+ prefix);
+ return -1;
+}
+
+
+/**
+ * virNetDevLockGenName:
+ * @type: type of the network device
+ *
+ * Lock the internal mutex for creation for this network device type.
+ *
+ * Returns nothing.
+ */
+void
+virNetDevLockGenName(virNetDevGenNameType type)
+{
+ virMutexLock(&virNetDevGenNames[type].mutex);
+}
+
+
+/**
+ * virNetDevUnlockGenName:
+ * @type: type of the network device
+ *
+ * Unlock the internal mutex for creation for this network device type.
+ *
+ * Returns nothing.
+ */
+void
+virNetDevUnlockGenName(virNetDevGenNameType type)
+{
+ virMutexUnlock(&virNetDevGenNames[type].mutex);
+}
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 53e606c6..19f37b61 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -40,6 +40,12 @@ typedef void virIfreq;
*/
#define VIR_NET_GENERATED_TAP_PREFIX "vnet"
+/* 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"
+
typedef enum {
VIR_NETDEV_RX_FILTER_MODE_NONE = 0,
VIR_NETDEV_RX_FILTER_MODE_NORMAL,
@@ -145,6 +151,24 @@ struct _virNetDevCoalesce {
uint32_t rate_sample_interval;
};
+typedef enum {
+ VIR_NET_DEV_GEN_NAME_NONE = 0,
+ VIR_NET_DEV_GEN_NAME_TAP,
+ VIR_NET_DEV_GEN_NAME_MACVTAP,
+ VIR_NET_DEV_GEN_NAME_MACVLAN,
+ VIR_NET_DEV_GEN_NAME_LAST
+} virNetDevGenNameType;
+
+VIR_ENUM_DECL(virNetDevGenNameType);
+
+typedef struct _virNetDevGenName virNetDevGenName;
+typedef virNetDevGenName *virNetDevGenNamePtr;
+struct _virNetDevGenName {
+ int lastID; /* not "unsigned" because callers use %d */
+ const char *prefix;
+ virMutex mutex;
+};
+
int virNetDevSetupControl(const char *ifname,
virIfreq *ifr)
@@ -321,3 +345,12 @@ int virNetDevVFInterfaceStats(virPCIDeviceAddressPtr vfAddr,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevRxFilter, virNetDevRxFilterFree);
+
+void virNetDevReserveName(const char *name,
+ virNetDevGenNameType type,
+ bool locked);
+
+int virNetDevGenerateName(char **ifname, virNetDevGenNameType type);
+
+void virNetDevLockGenName(virNetDevGenNameType type);
+void virNetDevUnlockGenName(virNetDevGenNameType type);
--
2.25.1
5:37 p.m.
New subject: [PATCH 1/5] netdev: Introduce several helper functions for generating unique netdev name
First - thanks for taking this on, and doing it the right way! (i.e.
refactoring my two changes down into a common set of functions to be
reused, rather than duplicating the code). I can't really say why I
didn't go the extra bit when I made the changes to virnetdevtap.c and
virnetdevmacvlan.c - I really should have cleaned it up as you've done
in patches 1 & 2 here.
On 12/4/20 2:01 AM, Shi Lei wrote:
Extract ReserveName/GenerateName from netdevtap and netdevmacvlan as
common helper functions. Along with them, export Lock/Unlock functions
to protect these processes.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/libvirt_private.syms | 4 ++
src/util/virnetdev.c | 140 +++++++++++++++++++++++++++++++++++++++
src/util/virnetdev.h | 33 +++++++++
3 files changed, 177 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2f640ef1..be3148c9 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2551,6 +2551,7 @@ virNetDevDelMulti;
virNetDevExists;
virNetDevFeatureTypeFromString;
virNetDevFeatureTypeToString;
+virNetDevGenerateName;
virNetDevGetFeatures;
virNetDevGetIndex;
virNetDevGetLinkInfo;
@@ -2572,8 +2573,10 @@ virNetDevGetVLanID;
virNetDevIfStateTypeFromString;
virNetDevIfStateTypeToString;
virNetDevIsVirtualFunction;
+virNetDevLockGenName;
virNetDevPFGetVF;
virNetDevReadNetConfig;
+virNetDevReserveName;
virNetDevRunEthernetScript;
virNetDevRxFilterFree;
virNetDevRxFilterModeTypeFromString;
@@ -2594,6 +2597,7 @@ virNetDevSetRcvMulti;
virNetDevSetRootQDisc;
virNetDevSetupControl;
virNetDevSysfsFile;
+virNetDevUnlockGenName;
virNetDevValidateConfig;
virNetDevVFInterfaceStats;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5104bbe7..5ff8e35f 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -17,6 +17,7 @@
*/
#include <config.h>
+#include <math.h>
#include "virnetdev.h"
#include "viralloc.h"
@@ -95,6 +96,22 @@ VIR_LOG_INIT("util.netdev");
(FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
#endif
+VIR_ENUM_IMPL(virNetDevGenNameType,
+ VIR_NET_DEV_GEN_NAME_LAST,
+ "none",
+ "tap",
+ "macvtap",
+ "macvlan",
+);
You've added VIR_ENUM_IMPL and VIR_ENUM_DECL, but haven't actually used
the corresponding virNetDevGenNameTypeToString() and
virNetDevGenNameTypeFromString() functions anywhere. Unless it will be
used, I'd say it would be just as well to leave it out. (and if you do
define it, probably use "vnet" for the tap device entry, since it's not
really important that the device is a tap device, just that it's a
network device whose name starts with "vnet").
+static virNetDevGenName
+virNetDevGenNames[VIR_NET_DEV_GEN_NAME_LAST] = {
+ {0, NULL, VIR_MUTEX_INITIALIZER},
So you have this extra entry in the table at [0] because the first enum
value is VIR_NET_DEV_GEN_NAME_NONE. Since that value would never be
possible, I think you should just make the first value in the enum be
VIR_NET_DEV_GEN_NAME_VNET (instead of ..._TAP), and then you don't need
the unused entry in the table.
+ {-1, VIR_NET_GENERATED_TAP_PREFIX, VIR_MUTEX_INITIALIZER},
+ {-1, VIR_NET_GENERATED_MACVTAP_PREFIX, VIR_MUTEX_INITIALIZER},
+ {-1, VIR_NET_GENERATED_MACVLAN_PREFIX, VIR_MUTEX_INITIALIZER},
+};
+
typedef enum {
VIR_MCAST_TYPE_INDEX_TOKEN,
VIR_MCAST_TYPE_NAME_TOKEN,
@@ -3516,3 +3533,126 @@ virNetDevSetRootQDisc(const char *ifname,
return 0;
}
+
+
+/**
+ * virNetDevReserveName:
+ * @name: name of an existing network device
+ * @type: type of the network device
+ * @locked: whether this process is locked by the internal mutex
+ *
+ * Reserve a network device name, so that any new network device
+ * created with an autogenerated name will use a number higher
+ * than the number in the given device name.
+ *
+ * Returns nothing.
+ */
+void
+virNetDevReserveName(const char *name,
+ virNetDevGenNameType type,
+ bool locked)
+{
+ unsigned int id;
+ const char *idstr = NULL;
+
+ if (type && STRPREFIX(name, virNetDevGenNames[type].prefix)) {
If you're going to worry about range-checking type, make sure that it's
< ..._LAST (0 will be a valid value if you follow my recommendation
above to remove ..._NONE)
+
+ VIR_INFO("marking device in use: '%s'", name);
+
+ idstr = name + strlen(virNetDevGenNames[type].prefix);
+
+ if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
+ if (locked)
+ virMutexLock(&virNetDevGenNames[type].mutex);
+
+ if (virNetDevGenNames[type].lastID < (int)id)
+ virNetDevGenNames[type].lastID = id;
+
+ if (locked)
+ virMutexUnlock(&virNetDevGenNames[type].mutex);
+ }
+ }
+}
+
+
+/**
+ * virNetDevGenerateName:
+ * @ifname: pointer to pointer to string containing template
+ *
+ * generate a new (currently unused) name for a new network device based
+ * on the templace string in @ifname - replace %d with the reserved id,
+ * 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.
+ */
+int
+virNetDevGenerateName(char **ifname, virNetDevGenNameType type)
+{
+ int id;
+ const char *prefix = virNetDevGenNames[type].prefix;
+ double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(prefix));
+ int maxID = INT_MAX;
+ int attempts = 0;
+
+ if (maxIDd <= (double)INT_MAX)
+ maxID = (int)maxIDd;
+
+ do {
+ g_autofree char *try = NULL;
+
+ id = ++virNetDevGenNames[type].lastID;
+
+ /* reset before overflow */
+ if (virNetDevGenNames[type].lastID >= maxID)
+ virNetDevGenNames[type].lastID = -1;
+
+ if (*ifname)
+ try = g_strdup_printf(*ifname, id);
An interesting twist! I like the direction of this - instead of relying
on the kernel to generate the names in the case of someone defining
their device with, e.g. <target dev='myprefix%d'/>, you send it through
our generator. Sending a user-provided string to printf as the format
string makes me uneasy though. Also, what if ifname is an
unparameterized name (e.g. "mytap"), and there is already a device named
"mytap"? In that case, it will spin through the loop 10000 times trying
the same device name, although we know from the start that it's not
going to work.
How about if, instead of blindly doing this, we check to see if the
device name has a single "%" that is followed by "d"? If it's
anything
other than that (or NULL) then we just return with the string unchanged.
+ else
+ try = g_strdup_printf("%s%d", prefix, 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"),
+ prefix);
+ return -1;
+}
+
+
+/**
+ * virNetDevLockGenName:
+ * @type: type of the network device
+ *
+ * Lock the internal mutex for creation for this network device type.
+ *
+ * Returns nothing.
+ */
+void
+virNetDevLockGenName(virNetDevGenNameType type)
+{
+ virMutexLock(&virNetDevGenNames[type].mutex);
+}
+
+
+/**
+ * virNetDevUnlockGenName:
+ * @type: type of the network device
+ *
+ * Unlock the internal mutex for creation for this network device type.
+ *
+ * Returns nothing.
+ */
+void
+virNetDevUnlockGenName(virNetDevGenNameType type)
+{
+ virMutexUnlock(&virNetDevGenNames[type].mutex);
+}
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 53e606c6..19f37b61 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -40,6 +40,12 @@ typedef void virIfreq;
*/
#define VIR_NET_GENERATED_TAP_PREFIX "vnet"
+/* 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"
+
typedef enum {
VIR_NETDEV_RX_FILTER_MODE_NONE = 0,
VIR_NETDEV_RX_FILTER_MODE_NORMAL,
@@ -145,6 +151,24 @@ struct _virNetDevCoalesce {
uint32_t rate_sample_interval;
};
+typedef enum {
+ VIR_NET_DEV_GEN_NAME_NONE = 0,
See comment above about removing ..._NONE
+ VIR_NET_DEV_GEN_NAME_TAP,
+ VIR_NET_DEV_GEN_NAME_MACVTAP,
+ VIR_NET_DEV_GEN_NAME_MACVLAN,
+ VIR_NET_DEV_GEN_NAME_LAST
+} virNetDevGenNameType;
+
+VIR_ENUM_DECL(virNetDevGenNameType);
Also about removing VIR_ENUM_DECL() (unless we end up with a reason for
virNetDevGenNameType(To|From)String())
+
+typedef struct _virNetDevGenName virNetDevGenName;
+typedef virNetDevGenName *virNetDevGenNamePtr;
+struct _virNetDevGenName {
+ int lastID; /* not "unsigned" because callers use %d */
+ const char *prefix;
+ virMutex mutex;
+};
+
int virNetDevSetupControl(const char *ifname,
virIfreq *ifr)
@@ -321,3 +345,12 @@ int virNetDevVFInterfaceStats(virPCIDeviceAddressPtr vfAddr,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevRxFilter, virNetDevRxFilterFree);
+
+void virNetDevReserveName(const char *name,
+ virNetDevGenNameType type,
+ bool locked);
+
+int virNetDevGenerateName(char **ifname, virNetDevGenNameType type);
+
+void virNetDevLockGenName(virNetDevGenNameType type);
+void virNetDevUnlockGenName(virNetDevGenNameType type);
Otherwise this looks good!
12:46 a.m.
New subject: [PATCH 1/5] netdev: Introduce several helper functions for generating unique netdev name
On 2020-12-08 at 07:37, Laine Stump wrote:
First - thanks for taking this on, and doing it the right way! (i.e.
refactoring my two changes down into a common set of functions to be
reused, rather than duplicating the code). I can't really say why I
didn't go the extra bit when I made the changes to virnetdevtap.c and
virnetdevmacvlan.c - I really should have cleaned it up as you've done
in patches 1 & 2 here.
On 12/4/20 2:01 AM, Shi Lei wrote:
> Extract ReserveName/GenerateName from netdevtap and netdevmacvlan as
> common helper functions. Along with them, export Lock/Unlock functions
> to protect these processes.
>
> Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
> ---
> src/libvirt_private.syms | 4 ++
> src/util/virnetdev.c | 140 +++++++++++++++++++++++++++++++++++++++
> src/util/virnetdev.h | 33 +++++++++
> 3 files changed, 177 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2f640ef1..be3148c9 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2551,6 +2551,7 @@ virNetDevDelMulti;
> virNetDevExists;
> virNetDevFeatureTypeFromString;
> virNetDevFeatureTypeToString;
> +virNetDevGenerateName;
> virNetDevGetFeatures;
> virNetDevGetIndex;
> virNetDevGetLinkInfo;
> @@ -2572,8 +2573,10 @@ virNetDevGetVLanID;
> virNetDevIfStateTypeFromString;
> virNetDevIfStateTypeToString;
> virNetDevIsVirtualFunction;
> +virNetDevLockGenName;
> virNetDevPFGetVF;
> virNetDevReadNetConfig;
> +virNetDevReserveName;
> virNetDevRunEthernetScript;
> virNetDevRxFilterFree;
> virNetDevRxFilterModeTypeFromString;
> @@ -2594,6 +2597,7 @@ virNetDevSetRcvMulti;
> virNetDevSetRootQDisc;
> virNetDevSetupControl;
> virNetDevSysfsFile;
> +virNetDevUnlockGenName;
> virNetDevValidateConfig;
> virNetDevVFInterfaceStats;
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 5104bbe7..5ff8e35f 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -17,6 +17,7 @@
> */
>
> #include <config.h>
> +#include <math.h>
>
> #include "virnetdev.h"
> #include "viralloc.h"
> @@ -95,6 +96,22 @@ VIR_LOG_INIT("util.netdev");
> (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
> #endif
>
> +VIR_ENUM_IMPL(virNetDevGenNameType,
> + VIR_NET_DEV_GEN_NAME_LAST,
> + "none",
> + "tap",
> + "macvtap",
> + "macvlan",
> +);
You've added VIR_ENUM_IMPL and VIR_ENUM_DECL, but haven't actually used
the corresponding virNetDevGenNameTypeToString() and
virNetDevGenNameTypeFromString() functions anywhere. Unless it will be
used, I'd say it would be just as well to leave it out. (and if you do
define it, probably use "vnet" for the tap device entry, since it's not
really important that the device is a tap device, just that it's a
network device whose name starts with "vnet").
Okay.
> +static virNetDevGenName
> +virNetDevGenNames[VIR_NET_DEV_GEN_NAME_LAST] = {
> + {0, NULL, VIR_MUTEX_INITIALIZER},
So you have this extra entry in the table at [0] because the first enum
value is VIR_NET_DEV_GEN_NAME_NONE. Since that value would never be
possible, I think you should just make the first value in the enum be
VIR_NET_DEV_GEN_NAME_VNET (instead of ..._TAP), and then you don't need
the unused entry in the table.
Okay.
> + {-1, VIR_NET_GENERATED_TAP_PREFIX, VIR_MUTEX_INITIALIZER},
> + {-1, VIR_NET_GENERATED_MACVTAP_PREFIX, VIR_MUTEX_INITIALIZER},
> + {-1, VIR_NET_GENERATED_MACVLAN_PREFIX, VIR_MUTEX_INITIALIZER},
> +};
> +
> typedef enum {
> VIR_MCAST_TYPE_INDEX_TOKEN,
> VIR_MCAST_TYPE_NAME_TOKEN,
> @@ -3516,3 +3533,126 @@ virNetDevSetRootQDisc(const char *ifname,
>
> return 0;
> }
> +
> +
> +/**
> + * virNetDevReserveName:
> + * @name: name of an existing network device
> + * @type: type of the network device
> + * @locked: whether this process is locked by the internal mutex
> + *
> + * Reserve a network device name, so that any new network device
> + * created with an autogenerated name will use a number higher
> + * than the number in the given device name.
> + *
> + * Returns nothing.
> + */
> +void
> +virNetDevReserveName(const char *name,
> + virNetDevGenNameType type,
> + bool locked)
> +{
> + unsigned int id;
> + const char *idstr = NULL;
> +
> + if (type && STRPREFIX(name, virNetDevGenNames[type].prefix)) {
If you're going to worry about range-checking type, make sure that it's
< ..._LAST (0 will be a valid value if you follow my recommendation
above to remove ..._NONE)
Okay.
> +
> + VIR_INFO("marking device in use: '%s'", name);
> +
> + idstr = name + strlen(virNetDevGenNames[type].prefix);
> +
> + if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
> + if (locked)
> + virMutexLock(&virNetDevGenNames[type].mutex);
> +
> + if (virNetDevGenNames[type].lastID < (int)id)
> + virNetDevGenNames[type].lastID = id;
> +
> + if (locked)
> + virMutexUnlock(&virNetDevGenNames[type].mutex);
> + }
> + }
> +}
> +
> +
> +/**
> + * virNetDevGenerateName:
> + * @ifname: pointer to pointer to string containing template
> + *
> + * generate a new (currently unused) name for a new network device based
> + * on the templace string in @ifname - replace %d with the reserved id,
> + * 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.
> + */
> +int
> +virNetDevGenerateName(char **ifname, virNetDevGenNameType type)
> +{
> + int id;
> + const char *prefix = virNetDevGenNames[type].prefix;
> + double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(prefix));
> + int maxID = INT_MAX;
> + int attempts = 0;
> +
> + if (maxIDd <= (double)INT_MAX)
> + maxID = (int)maxIDd;
> +
> + do {
> + g_autofree char *try = NULL;
> +
> + id = ++virNetDevGenNames[type].lastID;
> +
> + /* reset before overflow */
> + if (virNetDevGenNames[type].lastID >= maxID)
> + virNetDevGenNames[type].lastID = -1;
> +
> + if (*ifname)
> + try = g_strdup_printf(*ifname, id);
An interesting twist! I like the direction of this - instead of relying
on the kernel to generate the names in the case of someone defining
their device with, e.g. <target dev='myprefix%d'/>, you send it through
our generator. Sending a user-provided string to printf as the format
string makes me uneasy though. Also, what if ifname is an
unparameterized name (e.g. "mytap"), and there is already a device named
"mytap"? In that case, it will spin through the loop 10000 times trying
the same device name, although we know from the start that it's not
going to work.
How about if, instead of blindly doing this, we check to see if the
device name has a single "%" that is followed by "d"? If it's
anything
other than that (or NULL) then we just return with the string unchanged.
Okay. In the meanwhile, I should modify the explanation of this functions.
From now on, the *ifname can be a template, NULL or a user-defined
name.
> + else
> + try = g_strdup_printf("%s%d", prefix, 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"),
> + prefix);
> + return -1;
> +}
> +
> +
> +/**
> + * virNetDevLockGenName:
> + * @type: type of the network device
> + *
> + * Lock the internal mutex for creation for this network device type.
> + *
> + * Returns nothing.
> + */
> +void
> +virNetDevLockGenName(virNetDevGenNameType type)
> +{
> + virMutexLock(&virNetDevGenNames[type].mutex);
> +}
> +
> +
> +/**
> + * virNetDevUnlockGenName:
> + * @type: type of the network device
> + *
> + * Unlock the internal mutex for creation for this network device type.
> + *
> + * Returns nothing.
> + */
> +void
> +virNetDevUnlockGenName(virNetDevGenNameType type)
> +{
> + virMutexUnlock(&virNetDevGenNames[type].mutex);
> +}
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 53e606c6..19f37b61 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -40,6 +40,12 @@ typedef void virIfreq;
> */
> #define VIR_NET_GENERATED_TAP_PREFIX "vnet"
>
> +/* 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"
> +
> typedef enum {
> VIR_NETDEV_RX_FILTER_MODE_NONE = 0,
> VIR_NETDEV_RX_FILTER_MODE_NORMAL,
> @@ -145,6 +151,24 @@ struct _virNetDevCoalesce {
> uint32_t rate_sample_interval;
> };
>
> +typedef enum {
> + VIR_NET_DEV_GEN_NAME_NONE = 0,
See comment above about removing ..._NONE
Okay.
> + VIR_NET_DEV_GEN_NAME_TAP,
> + VIR_NET_DEV_GEN_NAME_MACVTAP,
> + VIR_NET_DEV_GEN_NAME_MACVLAN,
> + VIR_NET_DEV_GEN_NAME_LAST
> +} virNetDevGenNameType;
> +
> +VIR_ENUM_DECL(virNetDevGenNameType);
Also about removing VIR_ENUM_DECL() (unless we end up with a reason for
virNetDevGenNameType(To|From)String())
Okay.
> +
> +typedef struct _virNetDevGenName virNetDevGenName;
> +typedef virNetDevGenName *virNetDevGenNamePtr;
> +struct _virNetDevGenName {
> + int lastID; /* not "unsigned" because callers use %d */
> + const char *prefix;
> + virMutex mutex;
> +};
> +
>
> int virNetDevSetupControl(const char *ifname,
> virIfreq *ifr)
> @@ -321,3 +345,12 @@ int virNetDevVFInterfaceStats(virPCIDeviceAddressPtr vfAddr,
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevRxFilter, virNetDevRxFilterFree);
> +
> +void virNetDevReserveName(const char *name,
> + virNetDevGenNameType type,
> + bool locked);
> +
> +int virNetDevGenerateName(char **ifname, virNetDevGenNameType type);
> +
> +void virNetDevLockGenName(virNetDevGenNameType type);
> +void virNetDevUnlockGenName(virNetDevGenNameType type);
Otherwise this looks good!
1:01 a.m.
New subject: [PATCH 2/5] netdevtap: Use common helper function to create unique tap name
Simplify GenerateName/ReserveName for netdevtap by using common
functions.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/util/virnetdevtap.c | 58 +++--------------------------------------
1 file changed, 4 insertions(+), 54 deletions(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 198607b5..38b2f171 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -55,9 +55,6 @@
VIR_LOG_INIT("util.netdevtap");
-virMutex virNetDevTapCreateMutex = VIR_MUTEX_INITIALIZER;
-static int virNetDevTapLastID = -1; /* not "unsigned" because callers use %d
*/
-
/**
* virNetDevTapReserveName:
@@ -72,25 +69,7 @@ static int virNetDevTapLastID = -1; /* not "unsigned" because
callers use %d */
void
virNetDevTapReserveName(const char *name)
{
- unsigned int id;
- const char *idstr = NULL;
-
-
- if (STRPREFIX(name, VIR_NET_GENERATED_TAP_PREFIX)) {
-
- VIR_INFO("marking device in use: '%s'", name);
-
- idstr = name + strlen(VIR_NET_GENERATED_TAP_PREFIX);
-
- if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
- virMutexLock(&virNetDevTapCreateMutex);
-
- if (virNetDevTapLastID < (int)id)
- virNetDevTapLastID = id;
-
- virMutexUnlock(&virNetDevTapCreateMutex);
- }
- }
+ virNetDevReserveName(name, VIR_NET_DEV_GEN_NAME_TAP, true);
}
@@ -199,36 +178,7 @@ virNetDevTapGetRealDeviceName(char *ifname G_GNUC_UNUSED)
static int
virNetDevTapGenerateName(char **ifname)
{
- int id;
- double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(VIR_NET_GENERATED_TAP_PREFIX));
- int maxID = INT_MAX;
- int attempts = 0;
-
- if (maxIDd <= (double)INT_MAX)
- maxID = (int)maxIDd;
-
- do {
- g_autofree char *try = NULL;
-
- id = ++virNetDevTapLastID;
-
- /* reset before overflow */
- if (virNetDevTapLastID >= maxID)
- virNetDevTapLastID = -1;
-
- try = g_strdup_printf(*ifname, 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"),
- VIR_NET_GENERATED_TAP_PREFIX);
- return -1;
+ return virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_TAP);
}
@@ -263,7 +213,7 @@ int virNetDevTapCreate(char **ifname,
int ret = -1;
int fd = -1;
- virMutexLock(&virNetDevTapCreateMutex);
+ virNetDevLockGenName(VIR_NET_DEV_GEN_NAME_TAP);
/* if ifname is "vnet%d", then auto-generate a name for the new
* device (the kernel could do this for us, but has a bad habit of
@@ -333,7 +283,7 @@ int virNetDevTapCreate(char **ifname,
ret = 0;
cleanup:
- virMutexUnlock(&virNetDevTapCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_TAP);
if (ret < 0) {
VIR_FORCE_CLOSE(fd);
while (i--)
--
2.25.1
5:42 p.m.
New subject: [PATCH 2/5] netdevtap: Use common helper function to create unique tap name
On 12/4/20 2:01 AM, Shi Lei wrote:
Simplify GenerateName/ReserveName for netdevtap by using common
functions.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/util/virnetdevtap.c | 58 +++--------------------------------------
1 file changed, 4 insertions(+), 54 deletions(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 198607b5..38b2f171 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -55,9 +55,6 @@
VIR_LOG_INIT("util.netdevtap");
-virMutex virNetDevTapCreateMutex = VIR_MUTEX_INITIALIZER;
-static int virNetDevTapLastID = -1; /* not "unsigned" because callers use %d
*/
-
/**
* virNetDevTapReserveName:
@@ -72,25 +69,7 @@ static int virNetDevTapLastID = -1; /* not "unsigned"
because callers use %d */
void
virNetDevTapReserveName(const char *name)
{
- unsigned int id;
- const char *idstr = NULL;
-
-
- if (STRPREFIX(name, VIR_NET_GENERATED_TAP_PREFIX)) {
-
- VIR_INFO("marking device in use: '%s'", name);
-
- idstr = name + strlen(VIR_NET_GENERATED_TAP_PREFIX);
-
- if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
- virMutexLock(&virNetDevTapCreateMutex);
-
- if (virNetDevTapLastID < (int)id)
- virNetDevTapLastID = id;
-
- virMutexUnlock(&virNetDevTapCreateMutex);
- }
- }
+ virNetDevReserveName(name, VIR_NET_DEV_GEN_NAME_TAP, true);
I think we can just call virNetDevReserveName() directly, rather than
keeping virNetDevTapReserveName() as a go-between...
}
@@ -199,36 +178,7 @@ virNetDevTapGetRealDeviceName(char *ifname G_GNUC_UNUSED)
static int
virNetDevTapGenerateName(char **ifname)
{
- int id;
- double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(VIR_NET_GENERATED_TAP_PREFIX));
- int maxID = INT_MAX;
- int attempts = 0;
-
- if (maxIDd <= (double)INT_MAX)
- maxID = (int)maxIDd;
-
- do {
- g_autofree char *try = NULL;
-
- id = ++virNetDevTapLastID;
-
- /* reset before overflow */
- if (virNetDevTapLastID >= maxID)
- virNetDevTapLastID = -1;
-
- try = g_strdup_printf(*ifname, 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"),
- VIR_NET_GENERATED_TAP_PREFIX);
- return -1;
+ return virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_TAP);
Same here.
}
@@ -263,7 +213,7 @@ int virNetDevTapCreate(char **ifname,
int ret = -1;
int fd = -1;
- virMutexLock(&virNetDevTapCreateMutex);
+ virNetDevLockGenName(VIR_NET_DEV_GEN_NAME_TAP);
/* if ifname is "vnet%d", then auto-generate a name for the new
* device (the kernel could do this for us, but has a bad habit of
The code around here is going to need to be updated to take advantage of
the "g_strdup_printf(*ifname, id)" in your new Generate function. (as a
matter of fact, the functions that call virNetDevTapCreate() should
probably also be updated to stop them from adding in "vnet%d" when
ifname is empty - we can just let it remain empty until the call to
virNetDevGenerateName().
@@ -333,7 +283,7 @@ int virNetDevTapCreate(char **ifname,
ret = 0;
cleanup:
- virMutexUnlock(&virNetDevTapCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_TAP);
if (ret < 0) {
VIR_FORCE_CLOSE(fd);
while (i--)
12:57 a.m.
New subject: [PATCH 2/5] netdevtap: Use common helper function to create unique tap name
On 2020-12-08 at 07:42, Laine Stump wrote:
On 12/4/20 2:01 AM, Shi Lei wrote:
> Simplify GenerateName/ReserveName for netdevtap by using common
> functions.
>
> Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
> ---
> src/util/virnetdevtap.c | 58 +++--------------------------------------
> 1 file changed, 4 insertions(+), 54 deletions(-)
>
> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> index 198607b5..38b2f171 100644
> --- a/src/util/virnetdevtap.c
> +++ b/src/util/virnetdevtap.c
> @@ -55,9 +55,6 @@
>
> VIR_LOG_INIT("util.netdevtap");
>
> -virMutex virNetDevTapCreateMutex = VIR_MUTEX_INITIALIZER;
> -static int virNetDevTapLastID = -1; /* not "unsigned" because callers use
%d */
> -
>
> /**
> * virNetDevTapReserveName:
> @@ -72,25 +69,7 @@ static int virNetDevTapLastID = -1; /* not "unsigned"
because callers use %d */
> void
> virNetDevTapReserveName(const char *name)
> {
> - unsigned int id;
> - const char *idstr = NULL;
> -
> -
> - if (STRPREFIX(name, VIR_NET_GENERATED_TAP_PREFIX)) {
> -
> - VIR_INFO("marking device in use: '%s'", name);
> -
> - idstr = name + strlen(VIR_NET_GENERATED_TAP_PREFIX);
> -
> - if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
> - virMutexLock(&virNetDevTapCreateMutex);
> -
> - if (virNetDevTapLastID < (int)id)
> - virNetDevTapLastID = id;
> -
> - virMutexUnlock(&virNetDevTapCreateMutex);
> - }
> - }
> + virNetDevReserveName(name, VIR_NET_DEV_GEN_NAME_TAP, true);
I think we can just call virNetDevReserveName() directly, rather than
keeping virNetDevTapReserveName() as a go-between...
Okay.
> }
>
>
> @@ -199,36 +178,7 @@ virNetDevTapGetRealDeviceName(char *ifname G_GNUC_UNUSED)
> static int
> virNetDevTapGenerateName(char **ifname)
> {
> - int id;
> - double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(VIR_NET_GENERATED_TAP_PREFIX));
> - int maxID = INT_MAX;
> - int attempts = 0;
> -
> - if (maxIDd <= (double)INT_MAX)
> - maxID = (int)maxIDd;
> -
> - do {
> - g_autofree char *try = NULL;
> -
> - id = ++virNetDevTapLastID;
> -
> - /* reset before overflow */
> - if (virNetDevTapLastID >= maxID)
> - virNetDevTapLastID = -1;
> -
> - try = g_strdup_printf(*ifname, 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"),
> - VIR_NET_GENERATED_TAP_PREFIX);
> - return -1;
> + return virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_TAP);
Same here.
Okay.
> }
>
>
> @@ -263,7 +213,7 @@ int virNetDevTapCreate(char **ifname,
> int ret = -1;
> int fd = -1;
>
> - virMutexLock(&virNetDevTapCreateMutex);
> + virNetDevLockGenName(VIR_NET_DEV_GEN_NAME_TAP);
>
> /* if ifname is "vnet%d", then auto-generate a name for the new
> * device (the kernel could do this for us, but has a bad habit of
The code around here is going to need to be updated to take advantage of
the "g_strdup_printf(*ifname, id)" in your new Generate function. (as a
matter of fact, the functions that call virNetDevTapCreate() should
probably also be updated to stop them from adding in "vnet%d" when
ifname is empty - we can just let it remain empty until the call to
virNetDevGenerateName().
Okay. I'll find out those functions and prevent them from doing such things.
> @@ -333,7 +283,7 @@ int virNetDevTapCreate(char **ifname,
> ret = 0;
>
> cleanup:
> - virMutexUnlock(&virNetDevTapCreateMutex);
> + virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_TAP);
> if (ret < 0) {
> VIR_FORCE_CLOSE(fd);
> while (i--)
5:44 p.m.
New subject: [PATCH 2/5] netdevtap: Use common helper function to create unique tap name
On 12/4/20 2:01 AM, Shi Lei wrote:
Simplify GenerateName/ReserveName for netdevtap by using common
functions.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/util/virnetdevtap.c | 58 +++--------------------------------------
1 file changed, 4 insertions(+), 54 deletions(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 198607b5..38b2f171 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -55,9 +55,6 @@
Also, I noticed when looking at patch 3 that you removed #include
<math.h> due to pow() no longer being needed - I think you need to do
that in this file too.
VIR_LOG_INIT("util.netdevtap");
12:59 a.m.
New subject: [PATCH 2/5] netdevtap: Use common helper function to create unique tap name
On 2020-12-08 at 07:44, Laine Stump wrote:
On 12/4/20 2:01 AM, Shi Lei wrote:
> Simplify GenerateName/ReserveName for netdevtap by using common
> functions.
>
> Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
> ---
> src/util/virnetdevtap.c | 58 +++--------------------------------------
> 1 file changed, 4 insertions(+), 54 deletions(-)
>
> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> index 198607b5..38b2f171 100644
> --- a/src/util/virnetdevtap.c
> +++ b/src/util/virnetdevtap.c
> @@ -55,9 +55,6 @@
Also, I noticed when looking at patch 3 that you removed #include
<math.h> due to pow() no longer being needed - I think you need to do
that in this file too.
Oh. Thanks for reminding me.
>
> VIR_LOG_INIT("util.netdevtap");
>
1:01 a.m.
New subject: [PATCH 3/5] netdevmacvlan: Use helper function to create unique macvlan/macvtap name
Simplify ReserveName/GenerateName for macvlan and macvtap by using
common functions.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/util/virnetdevmacvlan.c | 107 ++++++++----------------------------
src/util/virnetdevmacvlan.h | 6 --
2 files changed, 22 insertions(+), 91 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 72f0d670..7f58d7ca 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"
@@ -64,39 +63,20 @@ VIR_LOG_INIT("util.netdevmacvlan");
# define VIR_NET_GENERATED_PREFIX \
((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \
VIR_NET_GENERATED_MACVTAP_PREFIX : VIR_NET_GENERATED_MACVLAN_PREFIX)
+# define VIR_NET_CREATE_TYPE \
+ ((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \
+ VIR_NET_DEV_GEN_NAME_MACVTAP : VIR_NET_DEV_GEN_NAME_MACVLAN)
-virMutex virNetDevMacVLanCreateMutex = VIR_MUTEX_INITIALIZER;
-static int virNetDevMacVTapLastID = -1;
-static int virNetDevMacVLanLastID = -1;
-
-
-static void
-virNetDevMacVLanReserveNameInternal(const char *name)
+static virNetDevGenNameType
+virNetDevMacVLanGetTypeByName(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;
- }
+ if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX))
+ return VIR_NET_DEV_GEN_NAME_MACVTAP;
+ else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX))
+ return VIR_NET_DEV_GEN_NAME_MACVLAN;
+ else
+ return VIR_NET_DEV_GEN_NAME_NONE;
}
@@ -113,9 +93,7 @@ virNetDevMacVLanReserveNameInternal(const char *name)
void
virNetDevMacVLanReserveName(const char *name)
{
- virMutexLock(&virNetDevMacVLanCreateMutex);
- virNetDevMacVLanReserveNameInternal(name);
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevReserveName(name, virNetDevMacVLanGetTypeByName(name), true);
}
@@ -136,50 +114,7 @@ virNetDevMacVLanReserveName(const char *name)
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;
+ return virNetDevGenerateName(ifname, VIR_NET_CREATE_TYPE);
}
@@ -832,7 +767,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
return -1;
}
- virMutexLock(&virNetDevMacVLanCreateMutex);
+ virNetDevLockGenName(VIR_NET_CREATE_TYPE);
if (ifnameRequested) {
int rc;
@@ -843,7 +778,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
VIR_INFO("Requested macvtap device name: %s", ifnameRequested);
if ((rc = virNetDevExists(ifnameRequested)) < 0) {
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
return -1;
}
@@ -854,14 +789,16 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
virReportSystemError(EEXIST,
_("Unable to create device
'%s'"),
ifnameRequested);
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
return -1;
}
} else {
/* ifnameRequested is available. try to open it */
- virNetDevMacVLanReserveNameInternal(ifnameRequested);
+ virNetDevReserveName(ifnameRequested,
+ VIR_NET_CREATE_TYPE,
+ false);
if (virNetDevMacVLanCreate(ifnameRequested, type, macaddress,
linkdev, macvtapMode) == 0) {
@@ -874,7 +811,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
* autogenerated named, so there is nothing else to
* try - fail and return.
*/
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
return -1;
}
}
@@ -888,13 +825,13 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
if (virNetDevMacVLanGenerateName(&ifname, flags) < 0 ||
virNetDevMacVLanCreate(ifname, type, macaddress,
linkdev, macvtapMode) < 0) {
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
return -1;
}
}
/* all done creating the device */
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
if (virNetDevVPortProfileAssociate(ifname,
virtPortProfile,
diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
index 48800a8f..cbdfef59 100644
--- a/src/util/virnetdevmacvlan.h
+++ b/src/util/virnetdevmacvlan.h
@@ -48,12 +48,6 @@ 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)
--
2.25.1
7:23 p.m.
New subject: [PATCH 3/5] netdevmacvlan: Use helper function to create unique macvlan/macvtap name
On 12/4/20 2:01 AM, Shi Lei wrote:
Simplify ReserveName/GenerateName for macvlan and macvtap by using
common functions.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/util/virnetdevmacvlan.c | 107 ++++++++----------------------------
src/util/virnetdevmacvlan.h | 6 --
2 files changed, 22 insertions(+), 91 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 72f0d670..7f58d7ca 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"
@@ -64,39 +63,20 @@ VIR_LOG_INIT("util.netdevmacvlan");
# define VIR_NET_GENERATED_PREFIX \
((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \
VIR_NET_GENERATED_MACVTAP_PREFIX : VIR_NET_GENERATED_MACVLAN_PREFIX)
^^ can't this (and the *_PATTERN #defines above it) be removed? (I
haven't applied the patches and checked yet, but hopefully anything that
needs those can be encapsulated in the new functions). If it's still
lingering around, don't worry about it too much - it can be cleaned up
after the fact, but if it can be trivially removed, then now would be a
good time to do it.
(looking into the non-patched file, it looks like
virNetDevMacVLanCreateWithVPortProfile() is called with a flag set
(VIR_NETDEV_MACVLAN_CREATE_WITH_TAP), and we then set char *type =
VIR_NET_GENERATED_PREFIX, and that is just sent down unchanged to
virNetDevMacVLanCreate(). We could instead modified
virNetDevMacVLanCreate to accept flags and set the string inside that
function according to the flags. Especially since the string in
virNetDevMacVLanCreate() isn't used as a "PREFIX" for the device name -
it is sent to netlink as the completely-unrelated-to-device-name device
*type*, which just coincidentally is the same as our chosen name prefix
- I think we could easily eliminate the *_PREFIX macros in this file.
+# define VIR_NET_CREATE_TYPE \
+ ((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \
+ VIR_NET_DEV_GEN_NAME_MACVTAP : VIR_NET_DEV_GEN_NAME_MACVLAN)
-virMutex virNetDevMacVLanCreateMutex = VIR_MUTEX_INITIALIZER;
-static int virNetDevMacVTapLastID = -1;
-static int virNetDevMacVLanLastID = -1;
-
-
-static void
-virNetDevMacVLanReserveNameInternal(const char *name)
+static virNetDevGenNameType
+virNetDevMacVLanGetTypeByName(const char *name)
I *think* once we do what I suggested above, this new function will no
longer be needed.
{
- 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;
- }
+ if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX))
+ return VIR_NET_DEV_GEN_NAME_MACVTAP;
+ else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX))
+ return VIR_NET_DEV_GEN_NAME_MACVLAN;
+ else
+ return VIR_NET_DEV_GEN_NAME_NONE;
}
@@ -113,9 +93,7 @@ virNetDevMacVLanReserveNameInternal(const char *name)
void
virNetDevMacVLanReserveName(const char *name)
{
- virMutexLock(&virNetDevMacVLanCreateMutex);
- virNetDevMacVLanReserveNameInternal(name);
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevReserveName(name, virNetDevMacVLanGetTypeByName(name), true);
As with virnetdevtap.c - I think we should call the new common function
directly, rather than keeping this indirection in.
}
@@ -136,50 +114,7 @@ virNetDevMacVLanReserveName(const char *name)
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;
+ return virNetDevGenerateName(ifname, VIR_NET_CREATE_TYPE);
Same here.
}
@@ -832,7 +767,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
return -1;
}
- virMutexLock(&virNetDevMacVLanCreateMutex);
+ virNetDevLockGenName(VIR_NET_CREATE_TYPE);
if (ifnameRequested) {
int rc;
@@ -843,7 +778,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
VIR_INFO("Requested macvtap device name: %s", ifnameRequested);
if ((rc = virNetDevExists(ifnameRequested)) < 0) {
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
return -1;
}
@@ -854,14 +789,16 @@ virNetDevMacVLanCreateWithVPortProfile(const char
*ifnameRequested,
virReportSystemError(EEXIST,
_("Unable to create device
'%s'"),
ifnameRequested);
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
return -1;
}
} else {
/* ifnameRequested is available. try to open it */
- virNetDevMacVLanReserveNameInternal(ifnameRequested);
+ virNetDevReserveName(ifnameRequested,
+ VIR_NET_CREATE_TYPE,
+ false);
if (virNetDevMacVLanCreate(ifnameRequested, type, macaddress,
linkdev, macvtapMode) == 0) {
@@ -874,7 +811,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
* autogenerated named, so there is nothing else to
* try - fail and return.
*/
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
return -1;
}
}
@@ -888,13 +825,13 @@ virNetDevMacVLanCreateWithVPortProfile(const char
*ifnameRequested,
if (virNetDevMacVLanGenerateName(&ifname, flags) < 0 ||
virNetDevMacVLanCreate(ifname, type, macaddress,
linkdev, macvtapMode) < 0) {
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
return -1;
}
}
/* all done creating the device */
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
if (virNetDevVPortProfileAssociate(ifname,
virtPortProfile,
diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
index 48800a8f..cbdfef59 100644
--- a/src/util/virnetdevmacvlan.h
+++ b/src/util/virnetdevmacvlan.h
@@ -48,12 +48,6 @@ 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)
1:05 a.m.
New subject: [PATCH 3/5] netdevmacvlan: Use helper function to create unique macvlan/macvtap name
On 2020-12-08 at 09:23, Laine Stump wrote:
On 12/4/20 2:01 AM, Shi Lei wrote:
> Simplify ReserveName/GenerateName for macvlan and macvtap by using
> common functions.
>
> Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
> ---
> src/util/virnetdevmacvlan.c | 107 ++++++++----------------------------
> src/util/virnetdevmacvlan.h | 6 --
> 2 files changed, 22 insertions(+), 91 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 72f0d670..7f58d7ca 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"
> @@ -64,39 +63,20 @@ VIR_LOG_INIT("util.netdevmacvlan");
> # define VIR_NET_GENERATED_PREFIX \
> ((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \
> VIR_NET_GENERATED_MACVTAP_PREFIX : VIR_NET_GENERATED_MACVLAN_PREFIX)
^^ can't this (and the *_PATTERN #defines above it) be removed? (I
haven't applied the patches and checked yet, but hopefully anything that
needs those can be encapsulated in the new functions). If it's still
lingering around, don't worry about it too much - it can be cleaned up
after the fact, but if it can be trivially removed, then now would be a
good time to do it.
(looking into the non-patched file, it looks like
virNetDevMacVLanCreateWithVPortProfile() is called with a flag set
(VIR_NETDEV_MACVLAN_CREATE_WITH_TAP), and we then set char *type =
VIR_NET_GENERATED_PREFIX, and that is just sent down unchanged to
virNetDevMacVLanCreate(). We could instead modified
virNetDevMacVLanCreate to accept flags and set the string inside that
function according to the flags. Especially since the string in
virNetDevMacVLanCreate() isn't used as a "PREFIX" for the device name -
it is sent to netlink as the completely-unrelated-to-device-name device
*type*, which just coincidentally is the same as our chosen name prefix
- I think we could easily eliminate the *_PREFIX macros in this file.
Okay. I'll cleanup those stuff.
> +# define VIR_NET_CREATE_TYPE \
> + ((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \
> + VIR_NET_DEV_GEN_NAME_MACVTAP : VIR_NET_DEV_GEN_NAME_MACVLAN)
>
>
> -virMutex virNetDevMacVLanCreateMutex = VIR_MUTEX_INITIALIZER;
> -static int virNetDevMacVTapLastID = -1;
> -static int virNetDevMacVLanLastID = -1;
> -
> -
> -static void
> -virNetDevMacVLanReserveNameInternal(const char *name)
> +static virNetDevGenNameType
> +virNetDevMacVLanGetTypeByName(const char *name)
I *think* once we do what I suggested above, this new function will no
longer be needed.
Okay.
> {
> - 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;
> - }
> + if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX))
> + return VIR_NET_DEV_GEN_NAME_MACVTAP;
> + else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX))
> + return VIR_NET_DEV_GEN_NAME_MACVLAN;
> + else
> + return VIR_NET_DEV_GEN_NAME_NONE;
> }
>
>
> @@ -113,9 +93,7 @@ virNetDevMacVLanReserveNameInternal(const char *name)
> void
> virNetDevMacVLanReserveName(const char *name)
> {
> - virMutexLock(&virNetDevMacVLanCreateMutex);
> - virNetDevMacVLanReserveNameInternal(name);
> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
> + virNetDevReserveName(name, virNetDevMacVLanGetTypeByName(name), true);
As with virnetdevtap.c - I think we should call the new common function
directly, rather than keeping this indirection in.
Yes.
> }
>
>
> @@ -136,50 +114,7 @@ virNetDevMacVLanReserveName(const char *name)
> 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;
> + return virNetDevGenerateName(ifname, VIR_NET_CREATE_TYPE);
Same here.
Okay.
> }
>
>
> @@ -832,7 +767,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char
*ifnameRequested,
> return -1;
> }
>
> - virMutexLock(&virNetDevMacVLanCreateMutex);
> + virNetDevLockGenName(VIR_NET_CREATE_TYPE);
>
> if (ifnameRequested) {
> int rc;
> @@ -843,7 +778,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char
*ifnameRequested,
> VIR_INFO("Requested macvtap device name: %s", ifnameRequested);
>
> if ((rc = virNetDevExists(ifnameRequested)) < 0) {
> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
> + virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
> return -1;
> }
>
> @@ -854,14 +789,16 @@ virNetDevMacVLanCreateWithVPortProfile(const char
*ifnameRequested,
> virReportSystemError(EEXIST,
> _("Unable to create device
'%s'"),
> ifnameRequested);
> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
> + virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
> return -1;
> }
> } else {
>
> /* ifnameRequested is available. try to open it */
>
> - virNetDevMacVLanReserveNameInternal(ifnameRequested);
> + virNetDevReserveName(ifnameRequested,
> + VIR_NET_CREATE_TYPE,
> + false);
>
> if (virNetDevMacVLanCreate(ifnameRequested, type, macaddress,
> linkdev, macvtapMode) == 0) {
> @@ -874,7 +811,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char
*ifnameRequested,
> * autogenerated named, so there is nothing else to
> * try - fail and return.
> */
> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
> + virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
> return -1;
> }
> }
> @@ -888,13 +825,13 @@ virNetDevMacVLanCreateWithVPortProfile(const char
*ifnameRequested,
> if (virNetDevMacVLanGenerateName(&ifname, flags) < 0 ||
> virNetDevMacVLanCreate(ifname, type, macaddress,
> linkdev, macvtapMode) < 0) {
> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
> + virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
> return -1;
> }
> }
>
> /* all done creating the device */
> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
> + virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
>
> if (virNetDevVPortProfileAssociate(ifname,
> virtPortProfile,
> diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
> index 48800a8f..cbdfef59 100644
> --- a/src/util/virnetdevmacvlan.h
> +++ b/src/util/virnetdevmacvlan.h
> @@ -48,12 +48,6 @@ 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)
1:01 a.m.
New subject: [PATCH 4/5] netdev: Enable virNetDevGenerateName to support veth
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/util/virnetdev.c | 2 ++
src/util/virnetdev.h | 3 +++
2 files changed, 5 insertions(+)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5ff8e35f..ff1b1fa0 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -102,6 +102,7 @@ VIR_ENUM_IMPL(virNetDevGenNameType,
"tap",
"macvtap",
"macvlan",
+ "veth",
);
static virNetDevGenName
@@ -110,6 +111,7 @@ virNetDevGenNames[VIR_NET_DEV_GEN_NAME_LAST] = {
{-1, VIR_NET_GENERATED_TAP_PREFIX, VIR_MUTEX_INITIALIZER},
{-1, VIR_NET_GENERATED_MACVTAP_PREFIX, VIR_MUTEX_INITIALIZER},
{-1, VIR_NET_GENERATED_MACVLAN_PREFIX, VIR_MUTEX_INITIALIZER},
+ {-1, VIR_NET_GENERATED_VETH_PREFIX, VIR_MUTEX_INITIALIZER},
};
typedef enum {
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 19f37b61..097d0f8e 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -40,6 +40,8 @@ typedef void virIfreq;
*/
#define VIR_NET_GENERATED_TAP_PREFIX "vnet"
+#define VIR_NET_GENERATED_VETH_PREFIX "veth"
+
/* libvirt will start macvtap/macvlan interface names with one of
* these prefixes when it auto-generates the name
*/
@@ -156,6 +158,7 @@ typedef enum {
VIR_NET_DEV_GEN_NAME_TAP,
VIR_NET_DEV_GEN_NAME_MACVTAP,
VIR_NET_DEV_GEN_NAME_MACVLAN,
+ VIR_NET_DEV_GEN_NAME_VETH,
VIR_NET_DEV_GEN_NAME_LAST
} virNetDevGenNameType;
--
2.25.1
7:29 p.m.
New subject: [PATCH 4/5] netdev: Enable virNetDevGenerateName to support veth
On 12/4/20 2:01 AM, Shi Lei wrote:
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/util/virnetdev.c | 2 ++
src/util/virnetdev.h | 3 +++
2 files changed, 5 insertions(+)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5ff8e35f..ff1b1fa0 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -102,6 +102,7 @@ VIR_ENUM_IMPL(virNetDevGenNameType,
"tap",
"macvtap",
"macvlan",
+ "veth",
As discussed in patch 1, this ENUM_IMPL is unnecessary.
);
static virNetDevGenName
@@ -110,6 +111,7 @@ virNetDevGenNames[VIR_NET_DEV_GEN_NAME_LAST] = {
{-1, VIR_NET_GENERATED_TAP_PREFIX, VIR_MUTEX_INITIALIZER},
{-1, VIR_NET_GENERATED_MACVTAP_PREFIX, VIR_MUTEX_INITIALIZER},
{-1, VIR_NET_GENERATED_MACVLAN_PREFIX, VIR_MUTEX_INITIALIZER},
+ {-1, VIR_NET_GENERATED_VETH_PREFIX, VIR_MUTEX_INITIALIZER},
};
typedef enum {
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 19f37b61..097d0f8e 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -40,6 +40,8 @@ typedef void virIfreq;
*/
#define VIR_NET_GENERATED_TAP_PREFIX "vnet"
+#define VIR_NET_GENERATED_VETH_PREFIX "veth"
Up until now, libvirt has named the veth devices as "vnetN", not
"vethN". I don't know that it would cause any problem to change to using
"vethN" (since already-running domains would have their "vnetN" device
name available in the domain status - only newly started domains would
use "vethN"). However I don't see any concrete reason for making that
change.
That being the case, I think you can just drop this patch, and use the
TAP prefix in the next patch (actually that's a good indication that it
shouldn't be called VIR_NET_DEV_GEN_NAME_TAP, but should instead be
called VIR_NET_DEV_GEN_NAME_VNET).
+
/* libvirt will start macvtap/macvlan interface names with one of
* these prefixes when it auto-generates the name
*/
@@ -156,6 +158,7 @@ typedef enum {
VIR_NET_DEV_GEN_NAME_TAP,
VIR_NET_DEV_GEN_NAME_MACVTAP,
VIR_NET_DEV_GEN_NAME_MACVLAN,
+ VIR_NET_DEV_GEN_NAME_VETH,
VIR_NET_DEV_GEN_NAME_LAST
} virNetDevGenNameType;
1:06 a.m.
New subject: [PATCH 4/5] netdev: Enable virNetDevGenerateName to support veth
On 2020-12-08 at 09:29, Laine Stump wrote:
On 12/4/20 2:01 AM, Shi Lei wrote:
> Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
> ---
> src/util/virnetdev.c | 2 ++
> src/util/virnetdev.h | 3 +++
> 2 files changed, 5 insertions(+)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 5ff8e35f..ff1b1fa0 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -102,6 +102,7 @@ VIR_ENUM_IMPL(virNetDevGenNameType,
> "tap",
> "macvtap",
> "macvlan",
> + "veth",
As discussed in patch 1, this ENUM_IMPL is unnecessary.
Okay.
> );
>
> static virNetDevGenName
> @@ -110,6 +111,7 @@ virNetDevGenNames[VIR_NET_DEV_GEN_NAME_LAST] = {
> {-1, VIR_NET_GENERATED_TAP_PREFIX, VIR_MUTEX_INITIALIZER},
> {-1, VIR_NET_GENERATED_MACVTAP_PREFIX, VIR_MUTEX_INITIALIZER},
> {-1, VIR_NET_GENERATED_MACVLAN_PREFIX, VIR_MUTEX_INITIALIZER},
> + {-1, VIR_NET_GENERATED_VETH_PREFIX, VIR_MUTEX_INITIALIZER},
> };
>
> typedef enum {
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 19f37b61..097d0f8e 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -40,6 +40,8 @@ typedef void virIfreq;
> */
> #define VIR_NET_GENERATED_TAP_PREFIX "vnet"
>
> +#define VIR_NET_GENERATED_VETH_PREFIX "veth"
Up until now, libvirt has named the veth devices as "vnetN", not
"vethN". I don't know that it would cause any problem to change to using
"vethN" (since already-running domains would have their "vnetN"
device
name available in the domain status - only newly started domains would
use "vethN"). However I don't see any concrete reason for making that
change.
That being the case, I think you can just drop this patch, and use the
TAP prefix in the next patch (actually that's a good indication that it
shouldn't be called VIR_NET_DEV_GEN_NAME_TAP, but should instead be
called VIR_NET_DEV_GEN_NAME_VNET).
Okay.
> +
> /* libvirt will start macvtap/macvlan interface names with one of
> * these prefixes when it auto-generates the name
> */
> @@ -156,6 +158,7 @@ typedef enum {
> VIR_NET_DEV_GEN_NAME_TAP,
> VIR_NET_DEV_GEN_NAME_MACVTAP,
> VIR_NET_DEV_GEN_NAME_MACVLAN,
> + VIR_NET_DEV_GEN_NAME_VETH,
> VIR_NET_DEV_GEN_NAME_LAST
> } virNetDevGenNameType;
>
1:01 a.m.
New subject: [PATCH 5/5] netdevveth: Simplify virNetDevVethCreate by using virNetDevGenerateName
Simplify virNetDevVethCreate by using common GenerateName/ReserveName
functions.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/lxc/lxc_process.c | 3 +
src/util/virnetdevveth.c | 146 +++++++++++----------------------------
2 files changed, 43 insertions(+), 106 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 7e07f49f..5f277a22 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -287,6 +287,9 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
VIR_DEBUG("calling vethCreate()");
parentVeth = net->ifname;
+ if (parentVeth)
+ virNetDevReserveName(parentVeth, VIR_NET_DEV_GEN_NAME_VETH, true);
+
if (virNetDevVethCreate(&parentVeth, &containerVeth) < 0)
return NULL;
VIR_DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
index b3eee1af..2e3d0c82 100644
--- a/src/util/virnetdevveth.c
+++ b/src/util/virnetdevveth.c
@@ -32,48 +32,6 @@
VIR_LOG_INIT("util.netdevveth");
-/* Functions */
-
-virMutex virNetDevVethCreateMutex = VIR_MUTEX_INITIALIZER;
-
-static int virNetDevVethExists(int devNum)
-{
- int ret;
- g_autofree char *path = NULL;
-
- path = g_strdup_printf(SYSFS_NET_DIR "vnet%d/", devNum);
- ret = virFileExists(path) ? 1 : 0;
- VIR_DEBUG("Checked dev vnet%d usage: %d", devNum, ret);
- return ret;
-}
-
-/**
- * virNetDevVethGetFreeNum:
- * @startDev: device number to start at (x in vethx)
- *
- * Looks in /sys/class/net/ to find the first available veth device
- * name.
- *
- * Returns non-negative device number on success or -1 in case of error
- */
-static int virNetDevVethGetFreeNum(int startDev)
-{
- int devNum;
-
-#define MAX_DEV_NUM 65536
-
- for (devNum = startDev; devNum < MAX_DEV_NUM; devNum++) {
- int ret = virNetDevVethExists(devNum);
- if (ret < 0)
- return -1;
- if (ret == 0)
- return devNum;
- }
-
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("No free veth devices available"));
- return -1;
-}
/**
* virNetDevVethCreate:
@@ -102,77 +60,53 @@ static int virNetDevVethGetFreeNum(int startDev)
*/
int virNetDevVethCreate(char** veth1, char** veth2)
{
- int ret = -1;
- int vethNum = 0;
- size_t i;
-
- /*
- * We might race with other containers, but this is reasonably
- * unlikely, so don't do too many retries for device creation
- */
- virMutexLock(&virNetDevVethCreateMutex);
-#define MAX_VETH_RETRIES 10
-
- for (i = 0; i < MAX_VETH_RETRIES; i++) {
- g_autofree char *veth1auto = NULL;
- g_autofree char *veth2auto = NULL;
- g_autoptr(virCommand) cmd = NULL;
-
- int status;
- if (!*veth1) {
- int veth1num;
- if ((veth1num = virNetDevVethGetFreeNum(vethNum)) < 0)
- goto cleanup;
-
- veth1auto = g_strdup_printf("vnet%d", veth1num);
- vethNum = veth1num + 1;
- }
- if (!*veth2) {
- int veth2num;
- if ((veth2num = virNetDevVethGetFreeNum(vethNum)) < 0)
- goto cleanup;
+ int status;
+ g_autofree char *veth1auto = NULL;
+ g_autofree char *veth2auto = NULL;
+ g_autoptr(virCommand) cmd = NULL;
- veth2auto = g_strdup_printf("vnet%d", veth2num);
- vethNum = veth2num + 1;
- }
+ virNetDevLockGenName(VIR_NET_DEV_GEN_NAME_VETH);
- cmd = virCommandNew("ip");
- virCommandAddArgList(cmd, "link", "add",
- *veth1 ? *veth1 : veth1auto,
- "type", "veth", "peer",
"name",
- *veth2 ? *veth2 : veth2auto,
- NULL);
-
- if (virCommandRun(cmd, &status) < 0)
- goto cleanup;
-
- if (status == 0) {
- if (veth1auto) {
- *veth1 = veth1auto;
- veth1auto = NULL;
- }
- if (veth2auto) {
- *veth2 = veth2auto;
- veth2auto = NULL;
- }
- VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2);
- ret = 0;
- goto cleanup;
+ if (!*veth1) {
+ if (virNetDevGenerateName(&veth1auto, VIR_NET_DEV_GEN_NAME_VETH) < 0) {
+ virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH);
+ return -1;
+ }
+ }
+ if (!*veth2) {
+ if (virNetDevGenerateName(&veth2auto, VIR_NET_DEV_GEN_NAME_VETH) < 0) {
+ virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH);
+ return -1;
}
+ }
+
+ virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH);
- VIR_DEBUG("Failed to create veth host: %s guest: %s: %d",
- *veth1 ? *veth1 : veth1auto,
- *veth2 ? *veth2 : veth2auto,
- status);
+ cmd = virCommandNew("ip");
+ virCommandAddArgList(cmd, "link", "add",
+ *veth1 ? *veth1 : veth1auto,
+ "type", "veth", "peer",
"name",
+ *veth2 ? *veth2 : veth2auto,
+ NULL);
+
+ if (virCommandRun(cmd, &status) < 0 || status) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Failed to allocate free veth
pair"));
+ return -1;
}
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to allocate free veth pair after %d attempts"),
- MAX_VETH_RETRIES);
+ VIR_DEBUG("create veth host: %s guest: %s: %d",
+ *veth1 ? *veth1 : veth1auto,
+ *veth2 ? *veth2 : veth2auto,
+ status);
+
+ if (veth1auto)
+ *veth1 = g_steal_pointer(&veth1auto);
+ if (veth2auto)
+ *veth2 = g_steal_pointer(&veth2auto);
- cleanup:
- virMutexUnlock(&virNetDevVethCreateMutex);
- return ret;
+ VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2);
+ return 0;
}
/**
--
2.25.1
7:42 p.m.
New subject: [PATCH 5/5] netdevveth: Simplify virNetDevVethCreate by using virNetDevGenerateName
On 12/4/20 2:01 AM, Shi Lei wrote:
Simplify virNetDevVethCreate by using common
GenerateName/ReserveName
functions.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/lxc/lxc_process.c | 3 +
src/util/virnetdevveth.c | 146 +++++++++++----------------------------
2 files changed, 43 insertions(+), 106 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 7e07f49f..5f277a22 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -287,6 +287,9 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
VIR_DEBUG("calling vethCreate()");
parentVeth = net->ifname;
+ if (parentVeth)
+ virNetDevReserveName(parentVeth, VIR_NET_DEV_GEN_NAME_VETH, true);
+
if (virNetDevVethCreate(&parentVeth, &containerVeth) < 0)
return NULL;
VIR_DEBUG("parentVeth: %s, containerVeth: %s", parentVeth,
containerVeth);
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
index b3eee1af..2e3d0c82 100644
--- a/src/util/virnetdevveth.c
+++ b/src/util/virnetdevveth.c
@@ -32,48 +32,6 @@
VIR_LOG_INIT("util.netdevveth");
-/* Functions */
-
-virMutex virNetDevVethCreateMutex = VIR_MUTEX_INITIALIZER;
-
-static int virNetDevVethExists(int devNum)
-{
- int ret;
- g_autofree char *path = NULL;
-
- path = g_strdup_printf(SYSFS_NET_DIR "vnet%d/", devNum);
- ret = virFileExists(path) ? 1 : 0;
- VIR_DEBUG("Checked dev vnet%d usage: %d", devNum, ret);
- return ret;
-}
-
-/**
- * virNetDevVethGetFreeNum:
- * @startDev: device number to start at (x in vethx)
- *
- * Looks in /sys/class/net/ to find the first available veth device
- * name.
- *
- * Returns non-negative device number on success or -1 in case of error
- */
-static int virNetDevVethGetFreeNum(int startDev)
-{
- int devNum;
-
-#define MAX_DEV_NUM 65536
-
- for (devNum = startDev; devNum < MAX_DEV_NUM; devNum++) {
- int ret = virNetDevVethExists(devNum);
- if (ret < 0)
- return -1;
- if (ret == 0)
- return devNum;
- }
-
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("No free veth devices available"));
- return -1;
-}
/**
* virNetDevVethCreate:
@@ -102,77 +60,53 @@ static int virNetDevVethGetFreeNum(int startDev)
*/
int virNetDevVethCreate(char** veth1, char** veth2)
{
- int ret = -1;
- int vethNum = 0;
- size_t i;
-
- /*
- * We might race with other containers, but this is reasonably
- * unlikely, so don't do too many retries for device creation
- */
- virMutexLock(&virNetDevVethCreateMutex);
-#define MAX_VETH_RETRIES 10
-
- for (i = 0; i < MAX_VETH_RETRIES; i++) {
- g_autofree char *veth1auto = NULL;
- g_autofree char *veth2auto = NULL;
- g_autoptr(virCommand) cmd = NULL;
-
- int status;
- if (!*veth1) {
- int veth1num;
- if ((veth1num = virNetDevVethGetFreeNum(vethNum)) < 0)
- goto cleanup;
-
- veth1auto = g_strdup_printf("vnet%d", veth1num);
- vethNum = veth1num + 1;
- }
- if (!*veth2) {
- int veth2num;
- if ((veth2num = virNetDevVethGetFreeNum(vethNum)) < 0)
- goto cleanup;
+ int status;
+ g_autofree char *veth1auto = NULL;
+ g_autofree char *veth2auto = NULL;
+ g_autoptr(virCommand) cmd = NULL;
- veth2auto = g_strdup_printf("vnet%d", veth2num);
- vethNum = veth2num + 1;
- }
+ virNetDevLockGenName(VIR_NET_DEV_GEN_NAME_VETH);
- cmd = virCommandNew("ip");
- virCommandAddArgList(cmd, "link", "add",
- *veth1 ? *veth1 : veth1auto,
- "type", "veth", "peer",
"name",
- *veth2 ? *veth2 : veth2auto,
- NULL);
-
- if (virCommandRun(cmd, &status) < 0)
- goto cleanup;
-
- if (status == 0) {
- if (veth1auto) {
- *veth1 = veth1auto;
- veth1auto = NULL;
- }
- if (veth2auto) {
- *veth2 = veth2auto;
- veth2auto = NULL;
- }
- VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2);
- ret = 0;
- goto cleanup;
+ if (!*veth1) {
+ if (virNetDevGenerateName(&veth1auto, VIR_NET_DEV_GEN_NAME_VETH) < 0) {
+ virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH);
+ return -1;
+ }
+ }
+ if (!*veth2) {
+ if (virNetDevGenerateName(&veth2auto, VIR_NET_DEV_GEN_NAME_VETH) < 0) {
+ virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH);
+ return -1;
}
+ }
+
+ virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH);
I can't think of a scenario where it would actually solve any race
condition, but in the tap and macvlan uses, the lock is acquired before
generating the new name, and not cleared until after the new device is
created. I guess really since the libvirtd process will never use the
same name twice, and since all the locking we could ever put within
libvirtd will not prevent some *other* process from using the name we've
just chosen, that it's unnecessary to hold the lock for so long in the
other cases - i.e., while typing this I've convinced myself not that we
should move this unlock down further, but that we can shorten the
section where we hold the lock in the tap and macvlan cases.
- VIR_DEBUG("Failed to create veth host: %s guest: %s: %d",
- *veth1 ? *veth1 : veth1auto,
- *veth2 ? *veth2 : veth2auto,
- status);
+ cmd = virCommandNew("ip");
+ virCommandAddArgList(cmd, "link", "add",
+ *veth1 ? *veth1 : veth1auto,
+ "type", "veth", "peer",
"name",
+ *veth2 ? *veth2 : veth2auto,
+ NULL);
+
+ if (virCommandRun(cmd, &status) < 0 || status) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Failed to allocate free veth
pair"));
+ return -1;
}
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to allocate free veth pair after %d attempts"),
- MAX_VETH_RETRIES);
+ VIR_DEBUG("create veth host: %s guest: %s: %d",
+ *veth1 ? *veth1 : veth1auto,
+ *veth2 ? *veth2 : veth2auto,
+ status);
+
+ if (veth1auto)
+ *veth1 = g_steal_pointer(&veth1auto);
+ if (veth2auto)
+ *veth2 = g_steal_pointer(&veth2auto);
- cleanup:
- virMutexUnlock(&virNetDevVethCreateMutex);
- return ret;
+ VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2);
+ return 0;
}
/**
(Definitely after going through these patches, I think that this series
should be applied *before* the "use netlink to create veth devices"
series, and that that series should get rid of the "status" arg to
VethCreate())
1:22 a.m.
New subject: [PATCH 5/5] netdevveth: Simplify virNetDevVethCreate by using virNetDevGenerateName
On 2020-12-08 at 09:42, Laine Stump wrote:
On 12/4/20 2:01 AM, Shi Lei wrote:
> Simplify virNetDevVethCreate by using common GenerateName/ReserveName
> functions.
> Signed-off-by: Shi Lei <shi_lei(a)massclouds.com > ---
> src/lxc/lxc_process.c | 3 +
> src/util/virnetdevveth.c | 146 +++++++++++----------------------------
> 2 files changed, 43 insertions(+), 106 deletions(-)
> diff --git a/src/lxc/lxc_process.c
b/src/lxc/lxc_process.c
> index 7e07f49f..5f277a22 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -287,6 +287,9 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
>
> VIR_DEBUG("calling vethCreate()");
> parentVeth = net->ifname;
> + if (parentVeth)
> + virNetDevReserveName(parentVeth, VIR_NET_DEV_GEN_NAME_VETH, true);
> +
> if (virNetDevVethCreate(&parentVeth, &containerVeth) < 0)
> return NULL;
> VIR_DEBUG("parentVeth: %s, containerVeth: %s", parentVeth,
containerVeth);
> diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
> index b3eee1af..2e3d0c82 100644
> --- a/src/util/virnetdevveth.c
> +++ b/src/util/virnetdevveth.c
> @@ -32,48 +32,6 @@
>
> VIR_LOG_INIT("util.netdevveth");
>
> -/* Functions */
> -
> -virMutex virNetDevVethCreateMutex = VIR_MUTEX_INITIALIZER;
> -
> -static int virNetDevVethExists(int devNum)
> -{
> - int ret;
> - g_autofree char *path = NULL;
> -
> - path = g_strdup_printf(SYSFS_NET_DIR "vnet%d/", devNum);
> - ret = virFileExists(path) ? 1 : 0;
> - VIR_DEBUG("Checked dev vnet%d usage: %d", devNum, ret);
> - return ret;
> -}
> -
> -/**
> - * virNetDevVethGetFreeNum:
> - * @startDev: device number to start at (x in vethx)
> - *
> - * Looks in /sys/class/net/ to find the first available veth device
> - * name.
> - *
> - * Returns non-negative device number on success or -1 in case of error
> - */
> -static int virNetDevVethGetFreeNum(int startDev)
> -{
> - int devNum;
> -
> -#define MAX_DEV_NUM 65536
> -
> - for (devNum = startDev; devNum < MAX_DEV_NUM; devNum++) {
> - int ret = virNetDevVethExists(devNum);
> - if (ret < 0)
> - return -1;
> - if (ret == 0)
> - return devNum;
> - }
> -
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("No free veth devices available"));
> - return -1;
> -}
>
> /**
> * virNetDevVethCreate:
> @@ -102,77 +60,53 @@ static int virNetDevVethGetFreeNum(int startDev)
> */
> int virNetDevVethCreate(char** veth1, char** veth2)
> {
> - int ret = -1;
> - int vethNum = 0;
> - size_t i;
> -
> - /*
> - * We might race with other containers, but this is reasonably
> - * unlikely, so don't do too many retries for device creation
> - */
> - virMutexLock(&virNetDevVethCreateMutex);
> -#define MAX_VETH_RETRIES 10
> -
> - for (i = 0; i < MAX_VETH_RETRIES; i++) {
> - g_autofree char *veth1auto = NULL;
> - g_autofree char *veth2auto = NULL;
> - g_autoptr(virCommand) cmd = NULL;
> -
> - int status;
> - if (!*veth1) {
> - int veth1num;
> - if ((veth1num = virNetDevVethGetFreeNum(vethNum)) < 0)
> - goto cleanup;
> -
> - veth1auto = g_strdup_printf("vnet%d", veth1num);
> - vethNum = veth1num + 1;
> - }
> - if (!*veth2) {
> - int veth2num;
> - if ((veth2num = virNetDevVethGetFreeNum(vethNum)) < 0)
> - goto cleanup;
> + int status;
> + g_autofree char *veth1auto = NULL;
> + g_autofree char *veth2auto = NULL;
> + g_autoptr(virCommand) cmd = NULL;
>
> - veth2auto = g_strdup_printf("vnet%d", veth2num);
> - vethNum = veth2num + 1;
> - }
> + virNetDevLockGenName(VIR_NET_DEV_GEN_NAME_VETH);
>
> - cmd = virCommandNew("ip");
> - virCommandAddArgList(cmd, "link", "add",
> - *veth1 ? *veth1 : veth1auto,
> - "type", "veth", "peer",
"name",
> - *veth2 ? *veth2 : veth2auto,
> - NULL);
> -
> - if (virCommandRun(cmd, &status) < 0)
> - goto cleanup;
> -
> - if (status == 0) {
> - if (veth1auto) {
> - *veth1 = veth1auto;
> - veth1auto = NULL;
> - }
> - if (veth2auto) {
> - *veth2 = veth2auto;
> - veth2auto = NULL;
> - }
> - VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2);
> - ret = 0;
> - goto cleanup;
> + if (!*veth1) {
> + if (virNetDevGenerateName(&veth1auto, VIR_NET_DEV_GEN_NAME_VETH) < 0)
{
> + virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH);
> + return -1;
> + }
> + }
> + if (!*veth2) {
> + if (virNetDevGenerateName(&veth2auto, VIR_NET_DEV_GEN_NAME_VETH) < 0)
{
> + virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH);
> + return -1;
> }
> + }
> +
> + virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH);
I can't think of a scenario where it would actually solve any race
condition, but in the tap and macvlan uses, the lock is acquired before
generating the new name, and not cleared until after the new device is
created. I guess really since the libvirtd process will never use the
same name twice, and since all the locking we could ever put within
libvirtd will not prevent some *other* process from using the name we've
just chosen, that it's unnecessary to hold the lock for so long in the
other cases - i.e., while typing this I've convinced myself not that we
should move this unlock down further, but that we can shorten the
section where we hold the lock in the tap and macvlan cases.
Okay.
>
> - VIR_DEBUG("Failed to create veth host: %s guest: %s: %d",
> - *veth1 ? *veth1 : veth1auto,
> - *veth2 ? *veth2 : veth2auto,
> - status);
> + cmd = virCommandNew("ip");
> + virCommandAddArgList(cmd, "link", "add",
> + *veth1 ? *veth1 : veth1auto,
> + "type", "veth", "peer",
"name",
> + *veth2 ? *veth2 : veth2auto,
> + NULL);
> +
> + if (virCommandRun(cmd, &status) < 0 || status) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("Failed to allocate free veth
pair"));
> + return -1;
> }
>
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to allocate free veth pair after %d
attempts"),
> - MAX_VETH_RETRIES);
> + VIR_DEBUG("create veth host: %s guest: %s: %d",
> + *veth1 ? *veth1 : veth1auto,
> + *veth2 ? *veth2 : veth2auto,
> + status);
> +
> + if (veth1auto)
> + *veth1 = g_steal_pointer(&veth1auto);
> + if (veth2auto)
> + *veth2 = g_steal_pointer(&veth2auto);
>
> - cleanup:
> - virMutexUnlock(&virNetDevVethCreateMutex);
> - return ret;
> + VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2);
> + return 0;
> }
>
> /**
(Definitely after going through these patches, I think that this series
should be applied *before* the "use netlink to create veth devices"
series, and that that series should get rid of the "status" arg to
VethCreate())
Okay. I'm going to commit this series before that.
Regards,
Shi Lei
1481
days inactive
1485
days old
17 comments
3 participants
participants (3)
-
Laine Stump -
Laine Stump -
Shi Lei