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@xxxxxxxxxxxxxx>
>> ---
>> 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!
>
>
