[PATCH 0/5] netdev: Extract GenerateName/ReserveName as common functions
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
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@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
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@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!
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@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!
Simplify GenerateName/ReserveName for netdevtap by using common functions. Signed-off-by: Shi Lei <shi_lei@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
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@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--)
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@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--)
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@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");
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@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");
Simplify ReserveName/GenerateName for macvlan and macvtap by using common functions. Signed-off-by: Shi Lei <shi_lei@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
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@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)
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@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)
Signed-off-by: Shi Lei <shi_lei@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
On 12/4/20 2:01 AM, Shi Lei wrote:
Signed-off-by: Shi Lei <shi_lei@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;
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@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;
Simplify virNetDevVethCreate by using common GenerateName/ReserveName functions. Signed-off-by: Shi Lei <shi_lei@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
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@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())
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@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
participants (3)
-
Laine Stump -
Laine Stump -
Shi Lei