[PATCH 0/3] udevHandleOneDevice: Remove old instance of device on "move"
Follow up to: https://www.redhat.com/archives/libvir-list/2020-April/msg00817.html I'd appreciate if somebody with systemd udev could test these and check if they are working as expected. Michal Prívozník (3): udevRemoveOneDevice: Unlock node device obj upon return node_device_udev: Split udevRemoveOneDevice() into two udevHandleOneDevice: Remove old instance of device on "move" src/node_device/node_device_udev.c | 34 +++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 10 deletions(-) -- 2.25.3
When removing a node device object from the internal list the udevRemoveOneDevice() function does plain unref over the object. This is not sufficient. If there is another thread that's waiting for the object lock it will wait forever. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 3149de8321..0166a13b0f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1244,7 +1244,7 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); virNodeDeviceObjListRemove(driver->devs, obj); - virObjectUnref(obj); + virNodeDeviceObjEndAPI(&obj); virObjectEventStateQueue(driver->nodeDeviceEventState, event); return 0; -- 2.25.3
On a Monday in 2020, Michal Privoznik wrote:
When removing a node device object from the internal list the udevRemoveOneDevice() function does plain unref over the object. This is not sufficient. If there is another thread that's waiting for the object lock it will wait forever.
I presume nobody wanted to lock that device before - this seems to have been there even before the conversion to virObjectLockable in commit dae23ec3456011f86086db76d45d8d0d266f7b9f
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Move internals of udevRemoveOneDevice() into a separate function which accepts sysfs path as an argument and actually removes the device from the internal list. It will be reused later. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 0166a13b0f..e9a76a7b01 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1222,17 +1222,15 @@ udevGetDeviceDetails(struct udev_device *device, static int -udevRemoveOneDevice(struct udev_device *device) +udevRemoveOneDeviceSysPath(const char *path) { virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; virObjectEventPtr event = NULL; - const char *name = NULL; - name = udev_device_get_syspath(device); - if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, name))) { - VIR_DEBUG("Failed to find device to remove that has udev name '%s'", - name); + if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, path))) { + VIR_DEBUG("Failed to find device to remove that has udev path '%s'", + path); return -1; } def = virNodeDeviceObjGetDef(obj); @@ -1242,7 +1240,7 @@ udevRemoveOneDevice(struct udev_device *device) 0); VIR_DEBUG("Removing device '%s' with sysfs path '%s'", - def->name, name); + def->name, path); virNodeDeviceObjListRemove(driver->devs, obj); virNodeDeviceObjEndAPI(&obj); @@ -1251,6 +1249,15 @@ udevRemoveOneDevice(struct udev_device *device) } +static int +udevRemoveOneDevice(struct udev_device *device) +{ + const char *path = udev_device_get_syspath(device); + + return udevRemoveOneDeviceSysPath(path); +} + + static int udevSetParent(struct udev_device *device, virNodeDeviceDefPtr def) -- 2.25.3
On Mon, Apr 20, 2020 at 04:20:40PM +0200, Michal Privoznik wrote:
Move internals of udevRemoveOneDevice() into a separate function which accepts sysfs path as an argument and actually removes the device from the internal list. It will be reused later.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
When a device is "move"-d (this basically means it was renamed), we add the new device onto our list but keep the old there too. Fortunately, udev sets this DEVPATH_OLD property which points to the old device path. We can use it to remove the old instance. To test this try renaming an interface, for instance: # ip link set tunl0 name tunl1 # ip link set tunl1 name tunl0 One problem with udev is that it sends old ifname in INTERFACE property, which creates a problem for us, the property is where we get the ifname from and use it then to query all kind of info about the interface. Well, if it is non-existent then we can't query anything. This happens if ifname rename is suppressed (net.ifnames=0 on kernel cmd line for instance). Fortunately, we can use "kernel" source for udev events which has always the fresh info. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e9a76a7b01..386f23ef3a 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1507,7 +1507,14 @@ udevHandleOneDevice(struct udev_device *device) return udevRemoveOneDevice(device); if (STREQ(action, "move")) { - /* TODO: implement a way of finding and removing the old device */ + const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); + + if (devpath_old) { + g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); + + udevRemoveOneDeviceSysPath(devpath_old_fixed); + } + return udevAddOneDevice(device); } @@ -1872,7 +1879,7 @@ nodeStateInitialize(bool privileged, virObjectLock(priv); - priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); + priv->udev_monitor = udev_monitor_new_from_netlink(udev, "kernel"); if (!priv->udev_monitor) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("udev_monitor_new_from_netlink returned NULL")); -- 2.25.3
On Mon, Apr 20, 2020 at 04:20:41PM +0200, Michal Privoznik wrote:
When a device is "move"-d (this basically means it was renamed), we add the new device onto our list but keep the old there too. Fortunately, udev sets this DEVPATH_OLD property which points to the old device path. We can use it to remove the old instance.
To test this try renaming an interface, for instance:
# ip link set tunl0 name tunl1 # ip link set tunl1 name tunl0
One problem with udev is that it sends old ifname in INTERFACE property, which creates a problem for us, the property is where we get the ifname from and use it then to query all kind of info about the interface. Well, if it is non-existent then we can't query anything. This happens if ifname rename is suppressed (net.ifnames=0 on kernel cmd line for instance). Fortunately, we can use "kernel" source for udev events which has always the fresh info.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
participants (3)
-
Ján Tomko -
Martin Kletzander -
Michal Privoznik