Re: Re: [PATCH 2/5] netdevtap: Use common helper function to create unique tap name

On 12/4/20 2:01 AM, Shi Lei wrote: > Simplify GenerateName/ReserveName for netdevtap by using common > functions. > > Signed-off-by: Shi Lei <shi_lei(a)massclouds.com&gt; > --- >   src/util/virnetdevtap.c | 58 +++-------------------------------------- >   1 file changed, 4 insertions(+), 54 deletions(-) > > diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c > index 198607b5..38b2f171 100644 > --- a/src/util/virnetdevtap.c > +++ b/src/util/virnetdevtap.c > @@ -55,9 +55,6 @@ >   >   VIR_LOG_INIT("util.netdevtap"); >   > -virMutex virNetDevTapCreateMutex = VIR_MUTEX_INITIALIZER; > -static int virNetDevTapLastID = -1; /* not "unsigned" because callers use %d */ > - >   >   /** >    * virNetDevTapReserveName: > @@ -72,25 +69,7 @@ static int virNetDevTapLastID = -1; /* not "unsigned" because callers use %d */ >   void >   virNetDevTapReserveName(const char *name) >   { > -    unsigned int id; > -    const char *idstr = NULL; > - > - > -    if (STRPREFIX(name, VIR_NET_GENERATED_TAP_PREFIX)) { > - > -        VIR_INFO("marking device in use: '%s'", name); > - > -        idstr = name + strlen(VIR_NET_GENERATED_TAP_PREFIX); > - > -        if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) { > -            virMutexLock(&virNetDevTapCreateMutex); > - > -            if (virNetDevTapLastID < (int)id) > -                virNetDevTapLastID = id; > - > -            virMutexUnlock(&virNetDevTapCreateMutex); > -        } > -    } > +    virNetDevReserveName(name, VIR_NET_DEV_GEN_NAME_TAP, true); I think we can just call virNetDevReserveName() directly, rather than keeping virNetDevTapReserveName() as a go-between...

>   } >   >   > @@ -199,36 +178,7 @@ virNetDevTapGetRealDeviceName(char *ifname G_GNUC_UNUSED) >   static int >   virNetDevTapGenerateName(char **ifname) >   { > -    int id; > -    double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(VIR_NET_GENERATED_TAP_PREFIX)); > -    int maxID = INT_MAX; > -    int attempts = 0; > - > -    if (maxIDd <= (double)INT_MAX) > -        maxID = (int)maxIDd; > - > -    do { > -        g_autofree char *try = NULL; > - > -        id = ++virNetDevTapLastID; > - > -        /* reset before overflow */ > -        if (virNetDevTapLastID >= maxID) > -            virNetDevTapLastID = -1; > - > -        try = g_strdup_printf(*ifname, 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"), > -                   VIR_NET_GENERATED_TAP_PREFIX); > -    return -1; > +    return virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_TAP); Same here.

>   } >   >   > @@ -263,7 +213,7 @@ int virNetDevTapCreate(char **ifname, >       int ret = -1; >       int fd = -1; >   > -    virMutexLock(&virNetDevTapCreateMutex); > +    virNetDevLockGenName(VIR_NET_DEV_GEN_NAME_TAP); >   >       /* if ifname is "vnet%d", then auto-generate a name for the new >        * device (the kernel could do this for us, but has a bad habit of The code around here is going to need to be updated to take advantage of the "g_strdup_printf(*ifname, id)" in your new Generate function. (as a matter of fact, the functions that call virNetDevTapCreate() should probably also be updated to stop them from adding in "vnet%d" when ifname is empty - we can just let it remain empty until the call to virNetDevGenerateName().

> @@ -333,7 +283,7 @@ int virNetDevTapCreate(char **ifname, >       ret = 0; >   >    cleanup: > -    virMutexUnlock(&virNetDevTapCreateMutex); > +    virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_TAP); >       if (ret < 0) { >           VIR_FORCE_CLOSE(fd); >           while (i--)