7:46 a.m.
On 2020-12-08 at 07:37, Laine Stump wrote:
First - thanks for taking this on, and doing it the right way! (i.e.
refactoring my two changes down into a common set of functions to be
reused, rather than duplicating the code). I can't really say why I
didn't go the extra bit when I made the changes to virnetdevtap.c and
virnetdevmacvlan.c - I really should have cleaned it up as you've done
in patches 1 & 2 here.
On 12/4/20 2:01 AM, Shi Lei wrote:
> Extract ReserveName/GenerateName from netdevtap and netdevmacvlan as
> common helper functions. Along with them, export Lock/Unlock functions
> to protect these processes.
>
> Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
> ---
> src/libvirt_private.syms | 4 ++
> src/util/virnetdev.c | 140 +++++++++++++++++++++++++++++++++++++++
> src/util/virnetdev.h | 33 +++++++++
> 3 files changed, 177 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2f640ef1..be3148c9 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2551,6 +2551,7 @@ virNetDevDelMulti;
> virNetDevExists;
> virNetDevFeatureTypeFromString;
> virNetDevFeatureTypeToString;
> +virNetDevGenerateName;
> virNetDevGetFeatures;
> virNetDevGetIndex;
> virNetDevGetLinkInfo;
> @@ -2572,8 +2573,10 @@ virNetDevGetVLanID;
> virNetDevIfStateTypeFromString;
> virNetDevIfStateTypeToString;
> virNetDevIsVirtualFunction;
> +virNetDevLockGenName;
> virNetDevPFGetVF;
> virNetDevReadNetConfig;
> +virNetDevReserveName;
> virNetDevRunEthernetScript;
> virNetDevRxFilterFree;
> virNetDevRxFilterModeTypeFromString;
> @@ -2594,6 +2597,7 @@ virNetDevSetRcvMulti;
> virNetDevSetRootQDisc;
> virNetDevSetupControl;
> virNetDevSysfsFile;
> +virNetDevUnlockGenName;
> virNetDevValidateConfig;
> virNetDevVFInterfaceStats;
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 5104bbe7..5ff8e35f 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -17,6 +17,7 @@
> */
>
> #include <config.h>
> +#include <math.h>
>
> #include "virnetdev.h"
> #include "viralloc.h"
> @@ -95,6 +96,22 @@ VIR_LOG_INIT("util.netdev");
> (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
> #endif
>
> +VIR_ENUM_IMPL(virNetDevGenNameType,
> + VIR_NET_DEV_GEN_NAME_LAST,
> + "none",
> + "tap",
> + "macvtap",
> + "macvlan",
> +);
You've added VIR_ENUM_IMPL and VIR_ENUM_DECL, but haven't actually used
the corresponding virNetDevGenNameTypeToString() and
virNetDevGenNameTypeFromString() functions anywhere. Unless it will be
used, I'd say it would be just as well to leave it out. (and if you do
define it, probably use "vnet" for the tap device entry, since it's not
really important that the device is a tap device, just that it's a
network device whose name starts with "vnet").
Okay.
> +static virNetDevGenName
> +virNetDevGenNames[VIR_NET_DEV_GEN_NAME_LAST] = {
> + {0, NULL, VIR_MUTEX_INITIALIZER},
So you have this extra entry in the table at [0] because the first enum
value is VIR_NET_DEV_GEN_NAME_NONE. Since that value would never be
possible, I think you should just make the first value in the enum be
VIR_NET_DEV_GEN_NAME_VNET (instead of ..._TAP), and then you don't need
the unused entry in the table.
Okay.
> + {-1, VIR_NET_GENERATED_TAP_PREFIX, VIR_MUTEX_INITIALIZER},
> + {-1, VIR_NET_GENERATED_MACVTAP_PREFIX, VIR_MUTEX_INITIALIZER},
> + {-1, VIR_NET_GENERATED_MACVLAN_PREFIX, VIR_MUTEX_INITIALIZER},
> +};
> +
> typedef enum {
> VIR_MCAST_TYPE_INDEX_TOKEN,
> VIR_MCAST_TYPE_NAME_TOKEN,
> @@ -3516,3 +3533,126 @@ virNetDevSetRootQDisc(const char *ifname,
>
> return 0;
> }
> +
> +
> +/**
> + * virNetDevReserveName:
> + * @name: name of an existing network device
> + * @type: type of the network device
> + * @locked: whether this process is locked by the internal mutex
> + *
> + * Reserve a network device name, so that any new network device
> + * created with an autogenerated name will use a number higher
> + * than the number in the given device name.
> + *
> + * Returns nothing.
> + */
> +void
> +virNetDevReserveName(const char *name,
> + virNetDevGenNameType type,
> + bool locked)
> +{
> + unsigned int id;
> + const char *idstr = NULL;
> +
> + if (type && STRPREFIX(name, virNetDevGenNames[type].prefix)) {
If you're going to worry about range-checking type, make sure that it's
< ..._LAST (0 will be a valid value if you follow my recommendation
above to remove ..._NONE)
Okay.
> +
> + VIR_INFO("marking device in use: '%s'", name);
> +
> + idstr = name + strlen(virNetDevGenNames[type].prefix);
> +
> + if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
> + if (locked)
> + virMutexLock(&virNetDevGenNames[type].mutex);
> +
> + if (virNetDevGenNames[type].lastID < (int)id)
> + virNetDevGenNames[type].lastID = id;
> +
> + if (locked)
> + virMutexUnlock(&virNetDevGenNames[type].mutex);
> + }
> + }
> +}
> +
> +
> +/**
> + * virNetDevGenerateName:
> + * @ifname: pointer to pointer to string containing template
> + *
> + * generate a new (currently unused) name for a new network device based
> + * on the templace string in @ifname - replace %d with the reserved id,
> + * and keep trying new values until one is found
> + * that doesn't already exist, or we've tried 10000 different
> + * names. Once a usable name is found, replace the template with the
> + * actual name.
> + *
> + * Returns 0 on success, -1 on failure.
> + */
> +int
> +virNetDevGenerateName(char **ifname, virNetDevGenNameType type)
> +{
> + int id;
> + const char *prefix = virNetDevGenNames[type].prefix;
> + double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(prefix));
> + int maxID = INT_MAX;
> + int attempts = 0;
> +
> + if (maxIDd <= (double)INT_MAX)
> + maxID = (int)maxIDd;
> +
> + do {
> + g_autofree char *try = NULL;
> +
> + id = ++virNetDevGenNames[type].lastID;
> +
> + /* reset before overflow */
> + if (virNetDevGenNames[type].lastID >= maxID)
> + virNetDevGenNames[type].lastID = -1;
> +
> + if (*ifname)
> + try = g_strdup_printf(*ifname, id);
An interesting twist! I like the direction of this - instead of relying
on the kernel to generate the names in the case of someone defining
their device with, e.g. <target dev='myprefix%d'/>, you send it through
our generator. Sending a user-provided string to printf as the format
string makes me uneasy though. Also, what if ifname is an
unparameterized name (e.g. "mytap"), and there is already a device named
"mytap"? In that case, it will spin through the loop 10000 times trying
the same device name, although we know from the start that it's not
going to work.
How about if, instead of blindly doing this, we check to see if the
device name has a single "%" that is followed by "d"? If it's
anything
other than that (or NULL) then we just return with the string unchanged.
Okay. In the meanwhile, I should modify the explanation of this functions.
From now on, the *ifname can be a template, NULL or a user-defined
name.
> + else
> + try = g_strdup_printf("%s%d", prefix, id);
> +
> + if (!virNetDevExists(try)) {
> + g_free(*ifname);
> + *ifname = g_steal_pointer(&try);
> + return 0;
> + }
> + } while (++attempts < 10000);
> +
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("no unused %s names available"),
> + prefix);
> + return -1;
> +}
> +
> +
> +/**
> + * virNetDevLockGenName:
> + * @type: type of the network device
> + *
> + * Lock the internal mutex for creation for this network device type.
> + *
> + * Returns nothing.
> + */
> +void
> +virNetDevLockGenName(virNetDevGenNameType type)
> +{
> + virMutexLock(&virNetDevGenNames[type].mutex);
> +}
> +
> +
> +/**
> + * virNetDevUnlockGenName:
> + * @type: type of the network device
> + *
> + * Unlock the internal mutex for creation for this network device type.
> + *
> + * Returns nothing.
> + */
> +void
> +virNetDevUnlockGenName(virNetDevGenNameType type)
> +{
> + virMutexUnlock(&virNetDevGenNames[type].mutex);
> +}
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 53e606c6..19f37b61 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -40,6 +40,12 @@ typedef void virIfreq;
> */
> #define VIR_NET_GENERATED_TAP_PREFIX "vnet"
>
> +/* libvirt will start macvtap/macvlan interface names with one of
> + * these prefixes when it auto-generates the name
> + */
> +#define VIR_NET_GENERATED_MACVTAP_PREFIX "macvtap"
> +#define VIR_NET_GENERATED_MACVLAN_PREFIX "macvlan"
> +
> typedef enum {
> VIR_NETDEV_RX_FILTER_MODE_NONE = 0,
> VIR_NETDEV_RX_FILTER_MODE_NORMAL,
> @@ -145,6 +151,24 @@ struct _virNetDevCoalesce {
> uint32_t rate_sample_interval;
> };
>
> +typedef enum {
> + VIR_NET_DEV_GEN_NAME_NONE = 0,
See comment above about removing ..._NONE
Okay.
> + VIR_NET_DEV_GEN_NAME_TAP,
> + VIR_NET_DEV_GEN_NAME_MACVTAP,
> + VIR_NET_DEV_GEN_NAME_MACVLAN,
> + VIR_NET_DEV_GEN_NAME_LAST
> +} virNetDevGenNameType;
> +
> +VIR_ENUM_DECL(virNetDevGenNameType);
Also about removing VIR_ENUM_DECL() (unless we end up with a reason for
virNetDevGenNameType(To|From)String())
Okay.
> +
> +typedef struct _virNetDevGenName virNetDevGenName;
> +typedef virNetDevGenName *virNetDevGenNamePtr;
> +struct _virNetDevGenName {
> + int lastID; /* not "unsigned" because callers use %d */
> + const char *prefix;
> + virMutex mutex;
> +};
> +
>
> int virNetDevSetupControl(const char *ifname,
> virIfreq *ifr)
> @@ -321,3 +345,12 @@ int virNetDevVFInterfaceStats(virPCIDeviceAddressPtr vfAddr,
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevRxFilter, virNetDevRxFilterFree);
> +
> +void virNetDevReserveName(const char *name,
> + virNetDevGenNameType type,
> + bool locked);
> +
> +int virNetDevGenerateName(char **ifname, virNetDevGenNameType type);
> +
> +void virNetDevLockGenName(virNetDevGenNameType type);
> +void virNetDevUnlockGenName(virNetDevGenNameType type);
Otherwise this looks good!