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