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

[Laine Stump]

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 at massclouds.com>
> ---
>   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;
>   }
>   
>   /**
>