12:51 a.m.
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(a)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.
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;
> }