[PATCH 1/2] node_device_udev: handle move events

[Mark Asselstine]

On Saturday, April 18, 2020 9:25:57 A.M. EDT Martin Kletzander wrote:
> On Thu, Apr 16, 2020 at 11:57:45AM -0400, Mark Asselstine wrote:
> >It is possible and common to rename some devices, this is especially
> >true for ethernet devices such as veth pairs.
> >
> >In the udevEventHandleThread() we will be notified of this change but
> >currently we only process "add", "change" and "remove"
> >events. Renaming a device such as above results in a "move" event, not
> >a "remove" followed by and "add" or vise versa. This change will add
> >the new/destination device to our records but unfortunately there is
> >no usable mechanism to identify the old/source device to remove it
> >from the records. So this is admittedly only a partial fix.
> 
> There is, but it is only internal.  We should ask for the function
> `udev_device_get_devpath_old()` to be publicized, if possible.  That should
> give us the old name.
> 
> When I checked this using `udevadm monitor --property` I got two events for
> each rename, one kernel event (where the old path pointed to the previous
> name) and one udev event where the old path referred to the original name
> the device was created with.
> 
> For example when I created a virtual network device with the name "fdsa",
> then renamed it bunch of times, started udev monitoring and then renamed it
> from "first" to "second" this was the output of udevadm for that one
> particular rename:
> 
> monitor will print the received events for:
> UDEV - the event which udev sends out after rule processing
> KERNEL - the kernel uevent
> 
> KERNEL[36343.246068] move     /devices/virtual/net/second (net)
> ACTION=move
> DEVPATH=/devices/virtual/net/second
> DEVPATH_OLD=/devices/virtual/net/first
> IFINDEX=3
> INTERFACE=second
> SEQNUM=4609
> SUBSYSTEM=net
> 
> UDEV  [36343.246785] move     /devices/virtual/net/fdsa (net)
> ACTION=move
> DEVPATH=/devices/virtual/net/fdsa
> DEVPATH_OLD=/devices/virtual/net/first
> IFINDEX=3
> INTERFACE=fdsa
> SEQNUM=4609
> SUBSYSTEM=net
> USEC_INITIALIZED=136864589
> 
> Thank you for a patch like this, I did not want to change the error just
> because "it's probably fine" until I can be sure that we do not lose the
> device (even though someone else is clearly managing it).
> 
> So I think the patches are great, and as much as I would like to add support
> for removing the old device if possible, probably using the function above,
> it will need some extra work.  At least we should file a request for the
> udev function to be exposed.
> 
> Anyway, I am not against these patches and I would even be fine with
> changing the warning to info in 2/2, but I will wait for someone else to
> see this and comment (and I'd like to know your opinion on what I wrote as
> well).  If that does not happen, I will retest them against (at that point)
> current master and push them.
> 
> Reviewed-by: Martin Kletzander <mkletzan at redhat.com>

Thanks Martin, I am in agreement with your analysis and assessment, and am 
happy that you are okay with the approach I took, I had my concerns. As you 
say, this is ideally something that needs to be taken up with the udev folks, 
as that is the proper place to start to correct this, they really do need to 
expose the src and dst in a move action. I tried several workarounds such as 
invalidating the device list and re-enumerating the devices again, and as you 
can see I opted to be blunt about this as there was no good workaround.

Thanks for considering these patches. As always I will keep an eye out to 
ensure they don't create new problems.

Mark

> 
> >Signed-off-by: Mark Asselstine <mark.asselstine at windriver.com>
> >---
> >
> > src/node_device/node_device_udev.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> >diff --git a/src/node_device/node_device_udev.c
> >b/src/node_device/node_device_udev.c index 8451903e8a..3149de8321 100644
> >--- a/src/node_device/node_device_udev.c
> >+++ b/src/node_device/node_device_udev.c
> >@@ -1499,6 +1499,11 @@ udevHandleOneDevice(struct udev_device *device)
> >
> >     if (STREQ(action, "remove"))
> >     
> >         return udevRemoveOneDevice(device);
> >
> >+    if (STREQ(action, "move")) {
> >+        /* TODO: implement a way of finding and removing the old device */
> >+        return udevAddOneDevice(device);
> >+    }
> >+
> >
> >     return 0;
> > 
> > }