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

On 12/4/20 2:01 AM, Shi Lei wrote: > Simplify virNetDevVethCreate by using common GenerateName/ReserveName > functions. > Signed-off-by: Shi Lei <shi_lei(a)massclouds.com> --- >   src/lxc/lxc_process.c    |   3 + >   src/util/virnetdevveth.c | 146 +++++++++++---------------------------- >   2 files changed, 43 insertions(+), 106 deletions(-) > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index 7e07f49f..5f277a22 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -287,6 +287,9 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm, >   >       VIR_DEBUG("calling vethCreate()"); >       parentVeth = net->ifname; > +    if (parentVeth) > +        virNetDevReserveName(parentVeth, VIR_NET_DEV_GEN_NAME_VETH, true); > + >       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..2e3d0c82 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,53 @@ 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; > -        } > +    virNetDevLockGenName(VIR_NET_DEV_GEN_NAME_VETH); >   > -        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; > +    if (!*veth1) { > +        if (virNetDevGenerateName(&veth1auto, VIR_NET_DEV_GEN_NAME_VETH) < 0) { > +            virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH); > +            return -1; > +        } > +    } > +    if (!*veth2) { > +        if (virNetDevGenerateName(&veth2auto, VIR_NET_DEV_GEN_NAME_VETH) < 0) { > +            virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH); > +            return -1; >           } > +    } > + > +    virNetDevUnlockGenName(VIR_NET_DEV_GEN_NAME_VETH); I can't think of a scenario where it would actually solve any race condition, but in the tap and macvlan uses, the lock is acquired before generating the new name, and not cleared until after the new device is created. I guess really since the libvirtd process will never use the same name twice, and since all the locking we could ever put within libvirtd will not prevent some *other* process from using the name we've just chosen, that it's unnecessary to hold the lock for so long in the other cases - i.e., while typing this I've convinced myself not that we should move this unlock down further, but that we can shorten the section where we hold the lock in the tap and macvlan cases.

>   > -        VIR_DEBUG("Failed to create veth host: %s guest: %s: %d", > -                  *veth1 ? *veth1 : veth1auto, > -                  *veth2 ? *veth2 : veth2auto, > -                  status); > +    cmd = virCommandNew("ip"); > +    virCommandAddArgList(cmd, "link", "add", > +                         *veth1 ? *veth1 : veth1auto, > +                         "type", "veth", "peer", "name", > +                         *veth2 ? *veth2 : veth2auto, > +                         NULL); > + > +    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; >   } >   >   /** (Definitely after going through these patches, I think that this series should be applied *before* the "use netlink to create veth devices" series, and that that series should get rid of the "status" arg to VethCreate())