Re: Re: [PATCHv3 4/5] netdevveth: Simplify virNetDevVethCreate by using virNetDevGenerateName

On 12/13/20 8:50 PM, Shi Lei wrote: > Simplify virNetDevVethCreate by using common GenerateName/ReserveName > functions. > > Signed-off-by: Shi Lei <shi_lei(a)massclouds.com&gt; > --- >   src/lxc/lxc_process.c    |   3 + >   src/util/virnetdevveth.c | 140 +++++++++------------------------------ >   2 files changed, 36 insertions(+), 107 deletions(-) > > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index eb29431e..2e8ae706 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -307,6 +307,9 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm, >   >       VIR_DEBUG("calling vethCreate()"); >       parentVeth = net->ifname; > +    if (parentVeth) > +        virNetDevReserveName(parentVeth); > + I think this is unnecessary (since a user-provided name shouldn't be using the auto-generate pattern, and would have been deleted by the parser anyway (see src/conf/domain_conf.c:12038, and comments in my reply to patch 5/5). So the only possible string here would be a string that would pass through virNetDevReserveName() with no action taken anyway.

>       if (virNetDevVethCreate(&parentVeth, &containerVeth) < 0) >           return NULL; >       VIR_DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth); > diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c > index b3eee1af..d6932a2e 100644 > --- a/src/util/virnetdevveth.c > +++ b/src/util/virnetdevveth.c > @@ -32,48 +32,6 @@ >   >   VIR_LOG_INIT("util.netdevveth"); >   > -/* Functions */ > - > -virMutex virNetDevVethCreateMutex = VIR_MUTEX_INITIALIZER; > - > -static int virNetDevVethExists(int devNum) > -{ > -    int ret; > -    g_autofree char *path = NULL; > - > -    path = g_strdup_printf(SYSFS_NET_DIR "vnet%d/", devNum); > -    ret = virFileExists(path) ? 1 : 0; > -    VIR_DEBUG("Checked dev vnet%d usage: %d", devNum, ret); > -    return ret; > -} > - > -/** > - * virNetDevVethGetFreeNum: > - * @startDev: device number to start at (x in vethx) > - * > - * Looks in /sys/class/net/ to find the first available veth device > - * name. > - * > - * Returns non-negative device number on success or -1 in case of error > - */ > -static int virNetDevVethGetFreeNum(int startDev) > -{ > -    int devNum; > - > -#define MAX_DEV_NUM 65536 > - > -    for (devNum = startDev; devNum < MAX_DEV_NUM; devNum++) { > -        int ret = virNetDevVethExists(devNum); > -        if (ret < 0) > -            return -1; > -        if (ret == 0) > -            return devNum; > -    } > - > -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > -                   _("No free veth devices available")); > -    return -1; > -} >   >   /** >    * virNetDevVethCreate: > @@ -102,77 +60,45 @@ static int virNetDevVethGetFreeNum(int startDev) >    */ >   int virNetDevVethCreate(char** veth1, char** veth2) >   { > -    int ret = -1; > -    int vethNum = 0; > -    size_t i; > - > -    /* > -     * We might race with other containers, but this is reasonably > -     * unlikely, so don't do too many retries for device creation > -     */ > -    virMutexLock(&virNetDevVethCreateMutex); > -#define MAX_VETH_RETRIES 10 > - > -    for (i = 0; i < MAX_VETH_RETRIES; i++) { > -        g_autofree char *veth1auto = NULL; > -        g_autofree char *veth2auto = NULL; > -        g_autoptr(virCommand) cmd = NULL; > - > -        int status; > -        if (!*veth1) { > -            int veth1num; > -            if ((veth1num = virNetDevVethGetFreeNum(vethNum)) < 0) > -                goto cleanup; > - > -            veth1auto = g_strdup_printf("vnet%d", veth1num); > -            vethNum = veth1num + 1; > -        } > -        if (!*veth2) { > -            int veth2num; > -            if ((veth2num = virNetDevVethGetFreeNum(vethNum)) < 0) > -                goto cleanup; > +    int status; > +    g_autofree char *veth1auto = NULL; > +    g_autofree char *veth2auto = NULL; > +    g_autoptr(virCommand) cmd = NULL; >   > -            veth2auto = g_strdup_printf("vnet%d", veth2num); > -            vethNum = veth2num + 1; > -        } > +    if (!*veth1) { > +        if (virNetDevGenerateName(&veth1auto, VIR_NET_DEV_GEN_NAME_VNET) < 0) > +            return -1; > +    } > +    if (!*veth2) { > +        if (virNetDevGenerateName(&veth2auto, VIR_NET_DEV_GEN_NAME_VNET) < 0) > +            return -1; > +    } Both of these don't need the "if (!*vethN) { ... }. Remember that virNetDevGenerateName() is a NOP if the input ifname != NULL. Otherwise good. I can squash in these two changes if you approve.

>   > -        cmd = virCommandNew("ip"); > -        virCommandAddArgList(cmd, "link", "add", > -                             *veth1 ? *veth1 : veth1auto, > -                             "type", "veth", "peer", "name", > -                             *veth2 ? *veth2 : veth2auto, > -                             NULL); > - > -        if (virCommandRun(cmd, &status) < 0) > -            goto cleanup; > - > -        if (status == 0) { > -            if (veth1auto) { > -                *veth1 = veth1auto; > -                veth1auto = NULL; > -            } > -            if (veth2auto) { > -                *veth2 = veth2auto; > -                veth2auto = NULL; > -            } > -            VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2); > -            ret = 0; > -            goto cleanup; > -        } > +    cmd = virCommandNew("ip"); > +    virCommandAddArgList(cmd, "link", "add", > +                         *veth1 ? *veth1 : veth1auto, > +                         "type", "veth", "peer", "name", > +                         *veth2 ? *veth2 : veth2auto, > +                         NULL); >   > -        VIR_DEBUG("Failed to create veth host: %s guest: %s: %d", > -                  *veth1 ? *veth1 : veth1auto, > -                  *veth2 ? *veth2 : veth2auto, > -                  status); > +    if (virCommandRun(cmd, &status) < 0 || status) { > +        virReportError(VIR_ERR_INTERNAL_ERROR, > +                       "%s", _("Failed to allocate free veth pair")); > +        return -1; >       } >   > -    virReportError(VIR_ERR_INTERNAL_ERROR, > -                   _("Failed to allocate free veth pair after %d attempts"), > -                   MAX_VETH_RETRIES); > +    VIR_DEBUG("create veth host: %s guest: %s: %d", > +              *veth1 ? *veth1 : veth1auto, > +              *veth2 ? *veth2 : veth2auto, > +              status); > + > +    if (veth1auto) > +        *veth1 = g_steal_pointer(&veth1auto); > +    if (veth2auto) > +        *veth2 = g_steal_pointer(&veth2auto); >   > - cleanup: > -    virMutexUnlock(&virNetDevVethCreateMutex); > -    return ret; > +    VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2); > +    return 0; >   } >   >   /** >