On 2020-12-14 at 10:10, Laine Stump wrote:
>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@xxxxxxxxxxxxxx>
>> ---
>> 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.
Okay. I agree with your change. Thanks!
>
>
>> + 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;
>> }
>
>
