On 2020-12-15 at 11:09, Laine Stump wrote:
>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.
Okay.
>
>
>> 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.
>
Okay.
Shi Lei
>
>>
>> - 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;
>> }
>>
>> /**
>>
>
