On 12/9/20 10:00 PM, Shi Lei wrote:

Extract ReserveName/GenerateName from netdevtap and netdevmacvlan as common helper functions.

Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/bhyve/bhyve_command.c | 4 +- src/conf/domain_conf.c | 4 +- src/interface/interface_backend_udev.c | 2 +- src/libvirt_private.syms | 2 + src/qemu/qemu_interface.c | 8 +- src/util/virnetdev.c | 116 +++++++++++++++++++++++++ src/util/virnetdev.h | 27 +++++- src/util/virnetdevtap.c | 10 +-- 8 files changed, 158 insertions(+), 15 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index acf3a5a4..4cf98c0e 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -80,10 +80,10 @@ bhyveBuildNetArgStr(const virDomainDef *def, }

if (!net->ifname || - STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) || + STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) || strchr(net->ifname, '%')) { VIR_FREE(net->ifname); - net->ifname = g_strdup(VIR_NET_GENERATED_TAP_PREFIX "%d"); + net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d"); }

if (!dryRun) { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 23415b32..403ecab8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12037,7 +12037,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,

if (def->managed_tap != VIR_TRISTATE_BOOL_NO && ifname && (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && - (STRPREFIX(ifname, VIR_NET_GENERATED_TAP_PREFIX) || + (STRPREFIX(ifname, VIR_NET_GENERATED_VNET_PREFIX) || STRPREFIX(ifname, VIR_NET_GENERATED_MACVTAP_PREFIX) || STRPREFIX(ifname, VIR_NET_GENERATED_MACVLAN_PREFIX) || (prefix && STRPREFIX(ifname, prefix)))) { @@ -26460,7 +26460,7 @@ virDomainNetDefFormat(virBufferPtr buf, if (def->ifname && (def->managed_tap == VIR_TRISTATE_BOOL_NO || !((flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) && - (STRPREFIX(def->ifname, VIR_NET_GENERATED_TAP_PREFIX) || + (STRPREFIX(def->ifname, VIR_NET_GENERATED_VNET_PREFIX) || STRPREFIX(def->ifname, VIR_NET_GENERATED_MACVTAP_PREFIX) || STRPREFIX(def->ifname, VIR_NET_GENERATED_MACVLAN_PREFIX) || (prefix && STRPREFIX(def->ifname, prefix)))))) { diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 173c4fc3..6a94a450 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -544,7 +544,7 @@ udevBridgeScanDirFilter(const struct dirent *entry) * vnet%d. Improvements to this check are welcome. */ if (strlen(entry->d_name) >= 5) { - if (STRPREFIX(entry->d_name, VIR_NET_GENERATED_TAP_PREFIX) && + if (STRPREFIX(entry->d_name, VIR_NET_GENERATED_VNET_PREFIX) && g_ascii_isdigit(entry->d_name[4])) return 0; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 992488f7..c0f50856 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2551,6 +2551,7 @@ virNetDevDelMulti; virNetDevExists; virNetDevFeatureTypeFromString; virNetDevFeatureTypeToString; +virNetDevGenerateName; virNetDevGetFeatures; virNetDevGetIndex; virNetDevGetLinkInfo; @@ -2574,6 +2575,7 @@ virNetDevIfStateTypeToString; virNetDevIsVirtualFunction; virNetDevPFGetVF; virNetDevReadNetConfig; +virNetDevReserveName; virNetDevRunEthernetScript; virNetDevRxFilterFree; virNetDevRxFilterModeTypeFromString; diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index 32b397d2..197c0aa2 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -456,10 +456,10 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, } } else { if (!net->ifname || - STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) || + STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) || strchr(net->ifname, '%')) { VIR_FREE(net->ifname); - net->ifname = g_strdup(VIR_NET_GENERATED_TAP_PREFIX "%d"); + net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d"); /* avoid exposing vnet%d in getXMLDesc or error outputs */ template_ifname = true; } @@ -560,10 +560,10 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def, }

if (!net->ifname || - STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) || + STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) || strchr(net->ifname, '%')) { VIR_FREE(net->ifname); - net->ifname = g_strdup(VIR_NET_GENERATED_TAP_PREFIX "%d"); + net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d"); /* avoid exposing vnet%d in getXMLDesc or error outputs */ template_ifname = true; } diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 5104bbe7..bd1ca1d8 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,14 @@ VIR_LOG_INIT("util.netdev"); (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index)) #endif

+ +static virNetDevGenName +virNetDevGenNames[VIR_NET_DEV_GEN_NAME_LAST] = { + {-1, VIR_NET_GENERATED_VNET_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 +3525,110 @@ virNetDevSetRootQDisc(const char *ifname,

return 0; } + + +/** + * virNetDevReserveName: + * @name: name of an existing network device + * + * 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) +{ + unsigned int id; + const char *idstr = NULL; + virNetDevGenNameType type; + + if (STRPREFIX(name, VIR_NET_GENERATED_VNET_PREFIX)) + type = VIR_NET_DEV_GEN_NAME_VNET; + else if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX)) + type = VIR_NET_DEV_GEN_NAME_MACVTAP; + else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX)) + type = VIR_NET_DEV_GEN_NAME_MACVLAN; + else + return; + + VIR_INFO("marking device in use: '%s'", name); + + idstr = name + strlen(virNetDevGenNames[type].prefix); + + if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) { + virMutexLock(&virNetDevGenNames[type].mutex); + + if (virNetDevGenNames[type].lastID < (int)id) + virNetDevGenNames[type].lastID = id; + + virMutexUnlock(&virNetDevGenNames[type].mutex); + } +} + + +/** + * virNetDevGenerateName: + * @ifname: pointer to pointer to string which can be a template, + * NULL or user-provided name. + * @type: type of the network device + * + * generate a new (currently unused) name for a new network device based + * on @ifname. If string pointed by @ifname is a template, replace %d + * with the reserved id; if that string is NULL, just generate a new + * name. 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. + * + * Note: if string pointed by @ifname is NOT a template or NULL, leave + * it unchanged and return it directly. + * + * 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; + + /* The @ifname is not a template, leave it unchanged. */ + if (*ifname && strstr(*ifname, "%d") == NULL)

This would still attempt to generate a name for something that had multiple format specifiers in it, e.g. "vnet%d%n", which could lead to "Bad Things(tm)". I *think* it would be sufficient to avoid this if we just checked for multiple occurences of %, something like this:    if (*ifname &&        (strchr(*ifname, '%') != strrchr(*ifname, '%') ||         strstr(*ifname, "%d") == NULL)) {        return 0;    } (The idea here is that if strchr and strrchr are the same, that means there's only a single '%'. So if we get past this check, we know that either the string is empty, or that it contains a single %d and no other format specifiers). If you're okay with me squashing that change, I can just do that and push it, or if you'd rather do it some other way and re-post, that's fine too - just let me know.

+ return 0; + + if (maxIDd <= (double)INT_MAX) + maxID = (int)maxIDd; + + do { + g_autofree char *try = NULL; + + virMutexLock(&virNetDevGenNames[type].mutex); + + id = ++virNetDevGenNames[type].lastID; + + /* reset before overflow */ + if (virNetDevGenNames[type].lastID >= maxID) + virNetDevGenNames[type].lastID = -1; + + virMutexUnlock(&virNetDevGenNames[type].mutex); + + 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; +} diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 53e606c6..f0160127 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -38,7 +38,13 @@ typedef void virIfreq; /* Used for prefix of ifname of any tap device name generated * dynamically by libvirt, cannot be used for a persistent network name. */ -#define VIR_NET_GENERATED_TAP_PREFIX "vnet" +#define VIR_NET_GENERATED_VNET_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, @@ -145,6 +151,21 @@ struct _virNetDevCoalesce { uint32_t rate_sample_interval; };

+typedef enum { + VIR_NET_DEV_GEN_NAME_VNET, + VIR_NET_DEV_GEN_NAME_MACVTAP, + VIR_NET_DEV_GEN_NAME_MACVLAN, + VIR_NET_DEV_GEN_NAME_LAST +} 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 +342,7 @@ int virNetDevVFInterfaceStats(virPCIDeviceAddressPtr vfAddr, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevRxFilter, virNetDevRxFilterFree); + +void virNetDevReserveName(const char *name); + +int virNetDevGenerateName(char **ifname, virNetDevGenNameType type); diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 198607b5..9354cc10 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -76,11 +76,11 @@ virNetDevTapReserveName(const char *name) const char *idstr = NULL;

- if (STRPREFIX(name, VIR_NET_GENERATED_TAP_PREFIX)) { + if (STRPREFIX(name, VIR_NET_GENERATED_VNET_PREFIX)) {

VIR_INFO("marking device in use: '%s'", name);

- idstr = name + strlen(VIR_NET_GENERATED_TAP_PREFIX); + idstr = name + strlen(VIR_NET_GENERATED_VNET_PREFIX);

if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) { virMutexLock(&virNetDevTapCreateMutex); @@ -200,7 +200,7 @@ static int virNetDevTapGenerateName(char **ifname) { int id; - double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(VIR_NET_GENERATED_TAP_PREFIX)); + double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(VIR_NET_GENERATED_VNET_PREFIX)); int maxID = INT_MAX; int attempts = 0;

@@ -227,7 +227,7 @@ virNetDevTapGenerateName(char **ifname)

virReportError(VIR_ERR_INTERNAL_ERROR, _("no unused %s names available"), - VIR_NET_GENERATED_TAP_PREFIX); + VIR_NET_GENERATED_VNET_PREFIX); return -1; }

@@ -270,7 +270,7 @@ int virNetDevTapCreate(char **ifname, * immediately re-using names that have just been released, which * can lead to race conditions). */ - if (STREQ(*ifname, VIR_NET_GENERATED_TAP_PREFIX "%d") && + if (STREQ(*ifname, VIR_NET_GENERATED_VNET_PREFIX "%d") && virNetDevTapGenerateName(ifname) < 0) { goto cleanup; }

On 12/9/20 10:00 PM, Shi Lei wrote:

Simplify ReserveName/GenerateName for macvlan and macvtap by using common functions.

Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/libvirt_private.syms | 1 - src/lxc/lxc_process.c | 2 +- src/qemu/qemu_process.c | 2 +- src/util/virnetdevmacvlan.c | 177 +++++------------------------------- src/util/virnetdevmacvlan.h | 14 +-- 5 files changed, 26 insertions(+), 170 deletions(-)

You probably don't have the libxl driver enabled in your builds, so you missed this change (which I've squashed in): diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 6af274cb1b..3488d7ec08 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -364,7 +364,7 @@ libxlReconnectNotifyNets(virDomainDefPtr def) * impolite. */ if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) - virNetDevMacVLanReserveName(net->ifname); + virNetDevReserveName(net->ifname);

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 64ef01e1..4d6ae84b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2655,7 +2655,6 @@ virNetDevMacVLanDelete; virNetDevMacVLanDeleteWithVPortProfile; virNetDevMacVLanIsMacvtap; virNetDevMacVLanModeTypeFromString; -virNetDevMacVLanReserveName; virNetDevMacVLanRestartWithVPortProfile; virNetDevMacVLanTapOpen; virNetDevMacVLanTapSetup; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0f818e2e..eb29431e 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1641,7 +1641,7 @@ virLXCProcessReconnectNotifyNets(virDomainDefPtr def) * impolite. */ if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) - virNetDevMacVLanReserveName(net->ifname); + virNetDevReserveName(net->ifname);

if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (!conn && !(conn = virGetConnectNetwork())) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1d54f201..6244ade4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3390,7 +3390,7 @@ qemuProcessNotifyNets(virDomainDefPtr def) */ switch (virDomainNetGetActualType(net)) { case VIR_DOMAIN_NET_TYPE_DIRECT: - virNetDevMacVLanReserveName(net->ifname); + virNetDevReserveName(net->ifname); break; case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 72f0d670..36b13133 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -45,7 +45,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode,

# include <net/if.h> # include <linux/if_tun.h> -# include <math.h>

# include "viralloc.h" # include "virlog.h" @@ -59,129 +58,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode,

VIR_LOG_INIT("util.netdevmacvlan");

-# define VIR_NET_GENERATED_MACVTAP_PATTERN VIR_NET_GENERATED_MACVTAP_PREFIX "%d" -# define VIR_NET_GENERATED_MACVLAN_PATTERN VIR_NET_GENERATED_MACVLAN_PREFIX "%d" -# define VIR_NET_GENERATED_PREFIX \ - ((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \ - VIR_NET_GENERATED_MACVTAP_PREFIX : VIR_NET_GENERATED_MACVLAN_PREFIX) - - -virMutex virNetDevMacVLanCreateMutex = VIR_MUTEX_INITIALIZER; -static int virNetDevMacVTapLastID = -1; -static int virNetDevMacVLanLastID = -1; - - -static void -virNetDevMacVLanReserveNameInternal(const char *name) -{ - unsigned int id; - const char *idstr = NULL; - int *lastID = NULL; - int len; - - if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX)) { - lastID = &virNetDevMacVTapLastID; - len = strlen(VIR_NET_GENERATED_MACVTAP_PREFIX); - } else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX)) { - lastID = &virNetDevMacVTapLastID; - len = strlen(VIR_NET_GENERATED_MACVLAN_PREFIX); - } else { - return; - } - - VIR_INFO("marking device in use: '%s'", name); - - idstr = name + len; - - if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) { - if (*lastID < (int)id) - *lastID = id; - } -} - - -/** - * virNetDevMacVLanReserveName: - * @name: name of an existing macvtap/macvlan device - * - * Set the value of virNetDevMacV(Lan|Tap)LastID to assure that any - * new device created with an autogenerated name will use a number - * higher than the number in the given device name. - * - * Returns nothing. - */ -void -virNetDevMacVLanReserveName(const char *name) -{ - virMutexLock(&virNetDevMacVLanCreateMutex); - virNetDevMacVLanReserveNameInternal(name); - virMutexUnlock(&virNetDevMacVLanCreateMutex); -} - - -/** - * virNetDevMacVLanGenerateName: - * @ifname: pointer to pointer to string containing template - * @lastID: counter to add to the template to form the name - * - * generate a new (currently unused) name for a new macvtap/macvlan - * device based on the template string in @ifname - replace %d with - * ++(*counter), and keep trying new values until one is found - * that doesn't already exist, or we've tried 10000 different - * names. Once a usable name is found, replace the template with the - * actual name. - * - * Returns 0 on success, -1 on failure. - */ -static int -virNetDevMacVLanGenerateName(char **ifname, unsigned int flags) -{ - const char *prefix; - const char *iftemplate; - int *lastID; - int id; - double maxIDd; - int maxID = INT_MAX; - int attempts = 0; - - if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { - prefix = VIR_NET_GENERATED_MACVTAP_PREFIX; - iftemplate = VIR_NET_GENERATED_MACVTAP_PREFIX "%d"; - lastID = &virNetDevMacVTapLastID; - } else { - prefix = VIR_NET_GENERATED_MACVLAN_PREFIX; - iftemplate = VIR_NET_GENERATED_MACVLAN_PREFIX "%d"; - lastID = &virNetDevMacVLanLastID; - } - - maxIDd = pow(10, IFNAMSIZ - 1 - strlen(prefix)); - if (maxIDd <= (double)INT_MAX) - maxID = (int)maxIDd; - - do { - g_autofree char *try = NULL; - - id = ++(*lastID); - - /* reset before overflow */ - if (*lastID == maxID) - *lastID = -1; - - try = g_strdup_printf(iftemplate, id); - - if (!virNetDevExists(try)) { - g_free(*ifname); - *ifname = g_steal_pointer(&try); - return 0; - } - } while (++attempts < 10000); - - virReportError(VIR_ERR_INTERNAL_ERROR, - _("no unused %s names available"), - *ifname); - return -1; -} -

/** * virNetDevMacVLanIsMacvtap: @@ -209,13 +85,10 @@ virNetDevMacVLanIsMacvtap(const char *ifname) * virNetDevMacVLanCreate: * * @ifname: The name the interface is supposed to have; optional parameter - * @type: The type of device, i.e., "macvtap", "macvlan" * @macaddress: The MAC address of the device * @srcdev: The name of the 'link' device * @macvlan_mode: The macvlan mode to use - * @retry: Pointer to integer that will be '1' upon return if an interface - * with the same name already exists and it is worth to try - * again with a different name + * @flags: OR of virNetDevMacVLanCreateFlags. * * Create a macvtap device with the given properties. * @@ -223,19 +96,21 @@ virNetDevMacVLanIsMacvtap(const char *ifname) */ int virNetDevMacVLanCreate(const char *ifname, - const char *type, const virMacAddr *macaddress, const char *srcdev, - uint32_t macvlan_mode) + uint32_t macvlan_mode, + unsigned int flags) { int error = 0; int ifindex = 0; + const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + VIR_NET_GENERATED_MACVTAP_PREFIX : + VIR_NET_GENERATED_MACVLAN_PREFIX; virNetlinkNewLinkData data = { .macvlan_mode = &macvlan_mode, .mac = macaddress, };

- if (virNetDevGetIndex(srcdev, &ifindex) < 0) return -1;

@@ -795,7 +670,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, size_t tapfdSize, unsigned int flags) { - const char *type = VIR_NET_GENERATED_PREFIX; g_autofree char *ifname = NULL; uint32_t macvtapMode; int vf = -1; @@ -832,8 +706,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, return -1; }

- virMutexLock(&virNetDevMacVLanCreateMutex); - if (ifnameRequested) { int rc; bool isAutoName > @@ -842,10 +714,8 @@ virNetDevMacVLanCreateWithVPortProfile(const

char *ifnameRequested,

VIR_INFO("Requested macvtap device name: %s", ifnameRequested);

- if ((rc = virNetDevExists(ifnameRequested)) < 0) { - virMutexUnlock(&virNetDevMacVLanCreateMutex); + if ((rc = virNetDevExists(ifnameRequested)) < 0) return -1; - }

if (rc) { /* ifnameRequested is already being used */ @@ -854,17 +724,16 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, virReportSystemError(EEXIST, _("Unable to create device '%s'"), ifnameRequested); - virMutexUnlock(&virNetDevMacVLanCreateMutex); return -1; } } else {

/* ifnameRequested is available. try to open it */

- virNetDevMacVLanReserveNameInternal(ifnameRequested); + virNetDevReserveName(ifnameRequested);

- if (virNetDevMacVLanCreate(ifnameRequested, type, macaddress, - linkdev, macvtapMode) == 0) { + if (virNetDevMacVLanCreate(ifnameRequested, macaddress, + linkdev, macvtapMode, flags) == 0) {

/* virNetDevMacVLanCreate() was successful - use this name */ ifname = g_strdup(ifnameRequested); @@ -874,7 +743,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, * autogenerated named, so there is nothing else to * try - fail and return. */ - virMutexUnlock(&virNetDevMacVLanCreateMutex); return -1; } } @@ -885,16 +753,19 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, * autogenerated name, so now we look for an unused * autogenerated name. */ - if (virNetDevMacVLanGenerateName(&ifname, flags) < 0 || - virNetDevMacVLanCreate(ifname, type, macaddress, - linkdev, macvtapMode) < 0) { - virMutexUnlock(&virNetDevMacVLanCreateMutex); + virNetDevGenNameType type; + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) + type = VIR_NET_DEV_GEN_NAME_MACVTAP; + else + type = VIR_NET_DEV_GEN_NAME_MACVLAN; + + if (virNetDevGenerateName(&ifname, type) < 0 || + virNetDevMacVLanCreate(ifname, macaddress, + linkdev, macvtapMode, flags) < 0) return -1; - } }

/* all done creating the device */ - virMutexUnlock(&virNetDevMacVLanCreateMutex);

if (virNetDevVPortProfileAssociate(ifname, virtPortProfile, @@ -1050,10 +921,10 @@ bool virNetDevMacVLanIsMacvtap(const char *ifname G_GNUC_UNUSED) }

int virNetDevMacVLanCreate(const char *ifname G_GNUC_UNUSED, - const char *type G_GNUC_UNUSED, const virMacAddr *macaddress G_GNUC_UNUSED, const char *srcdev G_GNUC_UNUSED, - uint32_t macvlan_mode G_GNUC_UNUSED) + uint32_t macvlan_mode G_GNUC_UNUSED, + unsigned int fflags G_GNUC_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Cannot create macvlan devices on this platform")); @@ -1141,10 +1012,4 @@ int virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname G_GNUC_UNUSE _("Cannot create macvlan devices on this platform")); return -1; } - -void virNetDevMacVLanReserveName(const char *name G_GNUC_UNUSED) -{ - virReportSystemError(ENOSYS, "%s", - _("Cannot create macvlan devices on this platform")); -} #endif /* ! WITH_LIBNL */ diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 48800a8f..0a013fc2 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -48,23 +48,15 @@ typedef enum { VIR_NETDEV_MACVLAN_VNET_HDR = 1 << 2, } virNetDevMacVLanCreateFlags;

-/* libvirt will start macvtap/macvlan interface names with one of - * these prefixes when it auto-generates the name - */ -#define VIR_NET_GENERATED_MACVTAP_PREFIX "macvtap" -#define VIR_NET_GENERATED_MACVLAN_PREFIX "macvlan" - -void virNetDevMacVLanReserveName(const char *name); - bool virNetDevMacVLanIsMacvtap(const char *ifname) ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE;

int virNetDevMacVLanCreate(const char *ifname, - const char *type, const virMacAddr *macaddress, const char *srcdev, - uint32_t macvlan_mode) - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) + uint32_t macvlan_mode, + unsigned int flags) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT;

int virNetDevMacVLanDelete(const char *ifname)

On 12/13/20 8:20 PM, Shi Lei wrote:

On 2020-12-14 at 08:58, Laine Stump wrote:

...

On 12/9/20 10:00 PM, Shi Lei wrote:

...

Simplify ReserveName/GenerateName for macvlan and macvtap by using common functions.

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

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

Sorry for that. I fix it and send V3.

No reason to send a v3 for something that simple! I could just fix it up here. However, if you are going to send a V3, I just sent a comment about an addition that is needed in Patch 1 that is a bit more than just an omitted function name change - you'll want to put that (or something similar) in if you repost. Also, it occurred to me just now while looking at that - it would be nice if the name change from VIR_NET_GENERATED_TAP_PREFIX to VIR_NET_GENERATED_VNET_PREFIX was a patch by itself (prerequisite to all the other patches). That one isn't really necessary, though. So if you're okay with me making the change I just posted in my reply for Patch 1, I can just push it all right now - no need for a V3 (I think! I'm still looking at patch 4 and 5 one last time - I should be done looking at them in 30 minutes or so).

Shi Lei

...

...

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 64ef01e1..4d6ae84b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2655,7 +2655,6 @@ virNetDevMacVLanDelete;    virNetDevMacVLanDeleteWithVPortProfile;    virNetDevMacVLanIsMacvtap;    virNetDevMacVLanModeTypeFromString; -virNetDevMacVLanReserveName;    virNetDevMacVLanRestartWithVPortProfile;    virNetDevMacVLanTapOpen;    virNetDevMacVLanTapSetup; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0f818e2e..eb29431e 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1641,7 +1641,7 @@ virLXCProcessReconnectNotifyNets(virDomainDefPtr def)             * impolite.             */            if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) -            virNetDevMacVLanReserveName(net->ifname); +            virNetDevReserveName(net->ifname);

           if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {                if (!conn && !(conn = virGetConnectNetwork())) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1d54f201..6244ade4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3390,7 +3390,7 @@ qemuProcessNotifyNets(virDomainDefPtr def)             */            switch (virDomainNetGetActualType(net)) {            case VIR_DOMAIN_NET_TYPE_DIRECT: -            virNetDevMacVLanReserveName(net->ifname); +            virNetDevReserveName(net->ifname);                break;            case VIR_DOMAIN_NET_TYPE_BRIDGE:            case VIR_DOMAIN_NET_TYPE_NETWORK: diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 72f0d670..36b13133 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -45,7 +45,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode,

   # include <net/if.h>    # include <linux/if_tun.h> -# include <math.h>

   # include "viralloc.h"    # include "virlog.h" @@ -59,129 +58,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode,

   VIR_LOG_INIT("util.netdevmacvlan");

-# define VIR_NET_GENERATED_MACVTAP_PATTERN VIR_NET_GENERATED_MACVTAP_PREFIX "%d" -# define VIR_NET_GENERATED_MACVLAN_PATTERN VIR_NET_GENERATED_MACVLAN_PREFIX "%d" -# define VIR_NET_GENERATED_PREFIX \ -    ((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \ -     VIR_NET_GENERATED_MACVTAP_PREFIX : VIR_NET_GENERATED_MACVLAN_PREFIX) - - -virMutex virNetDevMacVLanCreateMutex = VIR_MUTEX_INITIALIZER; -static int virNetDevMacVTapLastID = -1; -static int virNetDevMacVLanLastID = -1; - - -static void -virNetDevMacVLanReserveNameInternal(const char *name) -{ -    unsigned int id; -    const char *idstr = NULL; -    int *lastID = NULL; -    int len; - -    if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX)) { -        lastID = &virNetDevMacVTapLastID; -        len = strlen(VIR_NET_GENERATED_MACVTAP_PREFIX); -    } else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX)) { -        lastID = &virNetDevMacVTapLastID; -        len = strlen(VIR_NET_GENERATED_MACVLAN_PREFIX); -    } else { -        return; -    } - -    VIR_INFO("marking device in use: '%s'", name); - -    idstr = name + len; - -    if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) { -        if (*lastID < (int)id) -            *lastID = id; -    } -} - - -/** - * virNetDevMacVLanReserveName: - * @name: name of an existing macvtap/macvlan device - * - * Set the value of virNetDevMacV(Lan|Tap)LastID to assure that any - * new device created with an autogenerated name will use a number - * higher than the number in the given device name. - * - * Returns nothing. - */ -void -virNetDevMacVLanReserveName(const char *name) -{ -    virMutexLock(&virNetDevMacVLanCreateMutex); -    virNetDevMacVLanReserveNameInternal(name); -    virMutexUnlock(&virNetDevMacVLanCreateMutex); -} - - -/** - * virNetDevMacVLanGenerateName: - * @ifname: pointer to pointer to string containing template - * @lastID: counter to add to the template to form the name - * - * generate a new (currently unused) name for a new macvtap/macvlan - * device based on the template string in @ifname - replace %d with - * ++(*counter), and keep trying new values until one is found - * that doesn't already exist, or we've tried 10000 different - * names. Once a usable name is found, replace the template with the - * actual name. - * - * Returns 0 on success, -1 on failure. - */ -static int -virNetDevMacVLanGenerateName(char **ifname, unsigned int flags) -{ -    const char *prefix; -    const char *iftemplate; -    int *lastID; -    int id; -    double maxIDd; -    int maxID = INT_MAX; -    int attempts = 0; - -    if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { -        prefix = VIR_NET_GENERATED_MACVTAP_PREFIX; -        iftemplate = VIR_NET_GENERATED_MACVTAP_PREFIX "%d"; -        lastID = &virNetDevMacVTapLastID; -    } else { -        prefix = VIR_NET_GENERATED_MACVLAN_PREFIX; -        iftemplate = VIR_NET_GENERATED_MACVLAN_PREFIX "%d"; -        lastID = &virNetDevMacVLanLastID; -    } - -    maxIDd = pow(10, IFNAMSIZ - 1 - strlen(prefix)); -    if (maxIDd <= (double)INT_MAX) -        maxID = (int)maxIDd; - -    do { -        g_autofree char *try = NULL; - -        id = ++(*lastID); - -        /* reset before overflow */ -        if (*lastID == maxID) -            *lastID = -1; - -        try = g_strdup_printf(iftemplate, id); - -        if (!virNetDevExists(try)) { -            g_free(*ifname); -            *ifname = g_steal_pointer(&try); -            return 0; -        } -    } while (++attempts < 10000); - -    virReportError(VIR_ERR_INTERNAL_ERROR, -                   _("no unused %s names available"), -                   *ifname); -    return -1; -} -

   /**     * virNetDevMacVLanIsMacvtap: @@ -209,13 +85,10 @@ virNetDevMacVLanIsMacvtap(const char *ifname)     * virNetDevMacVLanCreate:     *     * @ifname: The name the interface is supposed to have; optional parameter - * @type: The type of device, i.e., "macvtap", "macvlan"     * @macaddress: The MAC address of the device     * @srcdev: The name of the 'link' device     * @macvlan_mode: The macvlan mode to use - * @retry: Pointer to integer that will be '1' upon return if an interface - *         with the same name already exists and it is worth to try - *         again with a different name + * @flags: OR of virNetDevMacVLanCreateFlags.     *     * Create a macvtap device with the given properties.     * @@ -223,19 +96,21 @@ virNetDevMacVLanIsMacvtap(const char *ifname)     */    int    virNetDevMacVLanCreate(const char *ifname, -                       const char *type,                           const virMacAddr *macaddress,                           const char *srcdev, -                       uint32_t macvlan_mode) +                       uint32_t macvlan_mode, +                       unsigned int flags)    {        int error = 0;        int ifindex = 0; +    const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? +                       VIR_NET_GENERATED_MACVTAP_PREFIX : +                       VIR_NET_GENERATED_MACVLAN_PREFIX;        virNetlinkNewLinkData data = {            .macvlan_mode = &macvlan_mode,            .mac = macaddress,        };

-        if (virNetDevGetIndex(srcdev, &ifindex) < 0)            return -1;

@@ -795,7 +670,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,                                           size_t tapfdSize,                                           unsigned int flags)    { -    const char *type = VIR_NET_GENERATED_PREFIX;        g_autofree char *ifname = NULL;        uint32_t macvtapMode;        int vf = -1; @@ -832,8 +706,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,               return -1;        }

-    virMutexLock(&virNetDevMacVLanCreateMutex); -        if (ifnameRequested) {            int rc;            bool isAutoName > @@ -842,10 +714,8 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,

           VIR_INFO("Requested macvtap device name: %s", ifnameRequested);

-        if ((rc = virNetDevExists(ifnameRequested)) < 0) { -            virMutexUnlock(&virNetDevMacVLanCreateMutex); +        if ((rc = virNetDevExists(ifnameRequested)) < 0)                return -1; -        }

           if (rc) {                /* ifnameRequested is already being used */ @@ -854,17 +724,16 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,                    virReportSystemError(EEXIST,                                         _("Unable to create device '%s'"),                                         ifnameRequested); -                virMutexUnlock(&virNetDevMacVLanCreateMutex);                    return -1;                }            } else {

               /* ifnameRequested is available. try to open it */

-            virNetDevMacVLanReserveNameInternal(ifnameRequested); +            virNetDevReserveName(ifnameRequested);

-            if (virNetDevMacVLanCreate(ifnameRequested, type, macaddress, -                                       linkdev, macvtapMode) == 0) { +            if (virNetDevMacVLanCreate(ifnameRequested, macaddress, +                                       linkdev, macvtapMode, flags) == 0) {

                   /* virNetDevMacVLanCreate() was successful - use this name */                    ifname = g_strdup(ifnameRequested); @@ -874,7 +743,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,                     * autogenerated named, so there is nothing else to                     * try - fail and return.                     */ -                virMutexUnlock(&virNetDevMacVLanCreateMutex);                    return -1;                }            } @@ -885,16 +753,19 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,             * autogenerated name, so now we look for an unused             * autogenerated name.             */ -        if (virNetDevMacVLanGenerateName(&ifname, flags) < 0 || -            virNetDevMacVLanCreate(ifname, type, macaddress, -                                   linkdev, macvtapMode) < 0) { -            virMutexUnlock(&virNetDevMacVLanCreateMutex); +        virNetDevGenNameType type; +        if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) +            type = VIR_NET_DEV_GEN_NAME_MACVTAP; +        else +            type = VIR_NET_DEV_GEN_NAME_MACVLAN; + +        if (virNetDevGenerateName(&ifname, type) < 0 || +            virNetDevMacVLanCreate(ifname, macaddress, +                                   linkdev, macvtapMode, flags) < 0)                return -1; -        }        }

       /* all done creating the device */ -    virMutexUnlock(&virNetDevMacVLanCreateMutex);

       if (virNetDevVPortProfileAssociate(ifname,                                           virtPortProfile, @@ -1050,10 +921,10 @@ bool virNetDevMacVLanIsMacvtap(const char *ifname G_GNUC_UNUSED)    }

   int virNetDevMacVLanCreate(const char *ifname G_GNUC_UNUSED, -                           const char *type G_GNUC_UNUSED,                               const virMacAddr *macaddress G_GNUC_UNUSED,                               const char *srcdev G_GNUC_UNUSED, -                           uint32_t macvlan_mode G_GNUC_UNUSED) +                           uint32_t macvlan_mode G_GNUC_UNUSED, +                           unsigned int fflags G_GNUC_UNUSED)    {        virReportSystemError(ENOSYS, "%s",                             _("Cannot create macvlan devices on this platform")); @@ -1141,10 +1012,4 @@ int virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname G_GNUC_UNUSE                             _("Cannot create macvlan devices on this platform"));        return -1;    } - -void virNetDevMacVLanReserveName(const char *name G_GNUC_UNUSED) -{ -    virReportSystemError(ENOSYS, "%s", -                         _("Cannot create macvlan devices on this platform")); -}    #endif /* ! WITH_LIBNL */ diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 48800a8f..0a013fc2 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -48,23 +48,15 @@ typedef enum {       VIR_NETDEV_MACVLAN_VNET_HDR          = 1 << 2,    } virNetDevMacVLanCreateFlags;

-/* libvirt will start macvtap/macvlan interface names with one of - * these prefixes when it auto-generates the name - */ -#define VIR_NET_GENERATED_MACVTAP_PREFIX "macvtap" -#define VIR_NET_GENERATED_MACVLAN_PREFIX "macvlan" - -void virNetDevMacVLanReserveName(const char *name); -    bool virNetDevMacVLanIsMacvtap(const char *ifname)       ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE;

   int virNetDevMacVLanCreate(const char *ifname, -                           const char *type,                               const virMacAddr *macaddress,                               const char *srcdev, -                           uint32_t macvlan_mode) -    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) +                           uint32_t macvlan_mode, +                           unsigned int flags) +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)        G_GNUC_WARN_UNUSED_RESULT;

   int virNetDevMacVLanDelete(const char *ifname)