On 2020-12-08 at 09:42, Laine Stump wrote:
>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@xxxxxxxxxxxxxx>
>> ---
>> 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.
>
Okay.
>
>>
>> - 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())
Okay. I'm going to commit this series before that.
Regards,
Shi Lei
>
