On 2020-12-08 at 09:23, Laine Stump wrote:
>On 12/4/20 2:01 AM, Shi Lei wrote:
>> Simplify ReserveName/GenerateName for macvlan and macvtap by using
>> common functions.
>>
>> Signed-off-by: Shi Lei <shi_lei@xxxxxxxxxxxxxx>
>> ---
>> src/util/virnetdevmacvlan.c | 107 ++++++++----------------------------
>> src/util/virnetdevmacvlan.h | 6 --
>> 2 files changed, 22 insertions(+), 91 deletions(-)
>>
>> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
>> index 72f0d670..7f58d7ca 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"
>> @@ -64,39 +63,20 @@ VIR_LOG_INIT("util.netdevmacvlan");
>> # define VIR_NET_GENERATED_PREFIX \
>> ((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \
>> VIR_NET_GENERATED_MACVTAP_PREFIX : VIR_NET_GENERATED_MACVLAN_PREFIX)
>
>^^ can't this (and the *_PATTERN #defines above it) be removed? (I
>haven't applied the patches and checked yet, but hopefully anything that
>needs those can be encapsulated in the new functions). If it's still
>lingering around, don't worry about it too much - it can be cleaned up
>after the fact, but if it can be trivially removed, then now would be a
>good time to do it.
>
>(looking into the non-patched file, it looks like
>virNetDevMacVLanCreateWithVPortProfile() is called with a flag set
>(VIR_NETDEV_MACVLAN_CREATE_WITH_TAP), and we then set char *type =
>VIR_NET_GENERATED_PREFIX, and that is just sent down unchanged to
>virNetDevMacVLanCreate(). We could instead modified
>virNetDevMacVLanCreate to accept flags and set the string inside that
>function according to the flags. Especially since the string in
>virNetDevMacVLanCreate() isn't used as a "PREFIX" for the device name -
>it is sent to netlink as the completely-unrelated-to-device-name device
>*type*, which just coincidentally is the same as our chosen name prefix
>- I think we could easily eliminate the *_PREFIX macros in this file.
Okay. I'll cleanup those stuff.
>
>
>> +# define VIR_NET_CREATE_TYPE \
>> + ((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \
>> + VIR_NET_DEV_GEN_NAME_MACVTAP : VIR_NET_DEV_GEN_NAME_MACVLAN)
>>
>>
>> -virMutex virNetDevMacVLanCreateMutex = VIR_MUTEX_INITIALIZER;
>> -static int virNetDevMacVTapLastID = -1;
>> -static int virNetDevMacVLanLastID = -1;
>> -
>> -
>> -static void
>> -virNetDevMacVLanReserveNameInternal(const char *name)
>> +static virNetDevGenNameType
>> +virNetDevMacVLanGetTypeByName(const char *name)
>
>
>I *think* once we do what I suggested above, this new function will no
>longer be needed.
Okay.
>
>
>
>> {
>> - 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;
>> - }
>> + if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX))
>> + return VIR_NET_DEV_GEN_NAME_MACVTAP;
>> + else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX))
>> + return VIR_NET_DEV_GEN_NAME_MACVLAN;
>> + else
>> + return VIR_NET_DEV_GEN_NAME_NONE;
>> }
>>
>>
>> @@ -113,9 +93,7 @@ virNetDevMacVLanReserveNameInternal(const char *name)
>> void
>> virNetDevMacVLanReserveName(const char *name)
>> {
>> - virMutexLock(&virNetDevMacVLanCreateMutex);
>> - virNetDevMacVLanReserveNameInternal(name);
>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>> + virNetDevReserveName(name, virNetDevMacVLanGetTypeByName(name), true);
>
>
>As with virnetdevtap.c - I think we should call the new common function
>directly, rather than keeping this indirection in.
Yes.
>
>
>> }
>>
>>
>> @@ -136,50 +114,7 @@ virNetDevMacVLanReserveName(const char *name)
>> 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;
>> + return virNetDevGenerateName(ifname, VIR_NET_CREATE_TYPE);
>
>Same here.
>
Okay.
>
>> }
>>
>>
>> @@ -832,7 +767,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>> return -1;
>> }
>>
>> - virMutexLock(&virNetDevMacVLanCreateMutex);
>> + virNetDevLockGenName(VIR_NET_CREATE_TYPE);
>>
>> if (ifnameRequested) {
>> int rc;
>> @@ -843,7 +778,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>> VIR_INFO("Requested macvtap device name: %s", ifnameRequested);
>>
>> if ((rc = virNetDevExists(ifnameRequested)) < 0) {
>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>> + virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
>> return -1;
>> }
>>
>> @@ -854,14 +789,16 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>> virReportSystemError(EEXIST,
>> _("Unable to create device '%s'"),
>> ifnameRequested);
>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>> + virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
>> return -1;
>> }
>> } else {
>>
>> /* ifnameRequested is available. try to open it */
>>
>> - virNetDevMacVLanReserveNameInternal(ifnameRequested);
>> + virNetDevReserveName(ifnameRequested,
>> + VIR_NET_CREATE_TYPE,
>> + false);
>>
>> if (virNetDevMacVLanCreate(ifnameRequested, type, macaddress,
>> linkdev, macvtapMode) == 0) {
>> @@ -874,7 +811,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>> * autogenerated named, so there is nothing else to
>> * try - fail and return.
>> */
>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>> + virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
>> return -1;
>> }
>> }
>> @@ -888,13 +825,13 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>> if (virNetDevMacVLanGenerateName(&ifname, flags) < 0 ||
>> virNetDevMacVLanCreate(ifname, type, macaddress,
>> linkdev, macvtapMode) < 0) {
>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>> + virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
>> return -1;
>> }
>> }
>>
>> /* all done creating the device */
>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>> + virNetDevUnlockGenName(VIR_NET_CREATE_TYPE);
>>
>> if (virNetDevVPortProfileAssociate(ifname,
>> virtPortProfile,
>> diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
>> index 48800a8f..cbdfef59 100644
>> --- a/src/util/virnetdevmacvlan.h
>> +++ b/src/util/virnetdevmacvlan.h
>> @@ -48,12 +48,6 @@ 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)
>
>
