Re: Re: [PATCH 3/5] netdevmacvlan: Use helper function to create unique macvlan/macvtap name

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(a)massclouds.com&gt; > --- >   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)