[PATCH 3/5] netdevmacvlan: Use helper function to create unique macvlan/macvtap name
Laine Stump
laine at laine.org
Tue Dec 8 01:23:04 UTC 2020
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 at massclouds.com>
> ---
> 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.
> +# 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.
> {
> - 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.
> }
>
>
> @@ -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.
> }
>
>
> @@ -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)
More information about the libvir-list
mailing list