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@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; }
/**