On 07/26/2017 02:44 AM, Erik Skultety wrote:

So we have a sanity check for the udev monitor fd. Theoretically, it could happen that the udev monitor fd changes (due to our own wrongdoing, hence the 'sanity' here) and if that happens it means we are handling an event from a different entity than we think, thus we should remove the handle if someone somewhere somehow hits this hypothetical case.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 9 +++++++++ 1 file changed, 9 insertions(+)

Should we perhaps "try again" either here (or in nodeStateReload if we find priv->watch == -1)? I know a separate patch, but nodedev is fairly braindead without the udev event handling. In fact, without it being set up initially, initialization fails.

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 80c346e96..ea10dc3ce 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1611,10 +1611,19 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,

udev_fd = udev_monitor_get_fd(udev_monitor); if (fd != udev_fd) { + udevPrivate *priv = driver->privateData; + virReportError(VIR_ERR_INTERNAL_ERROR, _("File descriptor returned by udev %d does not " "match node device file descriptor %d"), fd, udev_fd); + + /* this is a non-recoverable error, let's remove the handle, so that we + * don't get in here again because of some spurious behaviour and report + * the same error multiple times + */ + virEventRemoveHandle(priv->watch); +

Set priv->watch = -1 so that nodeStateCleanup doesn't retry. Although I think what ends up happening in patch 5 perhaps "resolves" some weird corner condition...

goto cleanup; }

Reviewed-by: John Ferlan <jferlan@redhat.com> since this is "better" than previously w/r/t not getting notified multiple times, fine, but I would hope we could do something to restart the event mgmt and perhaps even "re" enumerate the devices, but would need a "testable way" to make it happen. Still I wonder if comments in patch 5 end up making this go away. John

On 07/26/2017 02:44 AM, Erik Skultety wrote:

Since we only have one udev_monitor reference throughout libvirt, we should either clear the pointer we copy around to prevent invalid memory access once we unref this single reference, or start using reference counting provided by libudev. This patch follows the former and only clears the pointer.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 1 + 1 file changed, 1 insertion(+)

What's here, works, but expanding things out a bit for this one... Since we have the nodeDeviceLock, why not within: if (priv) { } do a VIR_FREE(driver->privateData); inside the if, that should set driver->privateData = NULL too. Avoids something finding driver->priv later in the instant that the nodeDeviceUnlock is run and something else gets the lock and looks @priv... IOW: See, patch 5 comments. I think if driver->priv is set to NULL, then there'd be no need to set udev_monitor to NULL, but it cannot hurt especially if this is some sort of race with udevEventHandleCallback, so... Reviewed-by: John Ferlan <jferlan@redhat.com> But I do think the VIR_FREE(priv) should move and be a VIR_FREE(driver->privateData)... Note, not @priv because that won't set @privateData = NULl which is what we'd want. You may even want to leave a comment about that indicating trying to close a race w/ event handling. John

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ea10dc3ce..cd19e79c1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1581,6 +1581,7 @@ nodeStateCleanup(void) if (udev_monitor != NULL) { udev = udev_monitor_get_udev(udev_monitor); udev_monitor_unref(udev_monitor); + priv->udev_monitor = NULL; } }

On Tue, Aug 01, 2017 at 03:46:44PM -0400, John Ferlan wrote:

On 07/26/2017 02:44 AM, Erik Skultety wrote:

...

Let this new method handle the device object we obtained from the monitor in order to enhance readability.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index cd19e79c1..7ecb330df 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1599,6 +1599,23 @@ nodeStateCleanup(void) }

+static int

Caller never checks status, so what's the point? I'd just go with void,

I didn't consider other approaches that's all, I used it to be able directly return "a call" to another function without thinking about it much - void works nicely as well.

unless at some point in time you were going to provide some sort of error/warning that the action failed or was unrecognized, similar to when the udevAddOneDevice call in udevProcessDeviceListEntry fails...

Perhaps "the next" step for this function could be:

if ((STREQ("add") || STREQ("change)) && udevAdd() == 0) return;

if (STREQ("remove") && udevRemove() == 0) return;

VIR_WARN(), using something like:

warning is the default log level and there are a number of devices withing the system which we don't care about and if debug is enabled you'd see messages like "Discarding device...", so by using warnings here, you'd just pollute the logs the same way, not sure whether that's desirable, since 1) you can't tell whether we discarded the device on purpose (which is the case most of the time) or 2) there's a real problem with the device. ...

Not everyone likes the ignore_value();, so since we're not deciding to bail if the handling fails, I think using void for the function is fine.

Still for pure code motion... You have my:

Reviewed-by: John Ferlan <jferlan@redhat.com>

Although I'm not sure (yet) what value this would provide.

I added a handle thread in a follow-up series where the thread routine invokes this function and I wanted to make the thread routine a few lines shorter and "cleaner", no additional value added. Erik

On 07/26/2017 02:44 AM, Erik Skultety wrote:

Since @udev_monitor isn't immutable within the driver, we need to protect every access to it by locking the driver, so that no spurious action changing the pointer while one thread is actively using it might occur. This patch just takes some precaution measures.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 7ecb330df..0643a5845 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1623,9 +1623,13 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, void *data ATTRIBUTE_UNUSED) { struct udev_device *device = NULL; - struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver); + struct udev_monitor *udev_monitor = NULL; int udev_fd = -1;

+ nodeDeviceLock(); + if (!(udev_monitor = DRV_STATE_UDEV_MONITOR(driver))) + goto error; +

Originally I thought maybe we should provide some sort of error here, but I convinced myself perhaps not - although I wouldn't be opposed to a VIR_WARN or something... My second thought was should we add some sort of EventRemoveHandle type operation? But once we get the lock, the only way udev_monitor would be NULL is if nodeStateCleanup is run which should already have removed the handle, so probably not. But then it struck me, if that's the case, then the nodeDeviceLock could fail rather miserably because driver could be NULL... (e.g. virMutexLock(&driver->lock);) Still not quite clear how a path such as this could be triggered other than perhaps the last dying breaths of the daemon - unless something that the *unref does causes any events to be flushed. Maybe a "delayed" event - who knows. Still, if we wait on the lock, we could end up in a state where @driver doesn't exist or anything it was referencing has been freed... So I think we need : if (!driver) return; to follow how nodeStateCleanup will bail... then (considering my thoughts in patch 2) Add to node_device_udev.h (and use it in Cleanup too, so we don't have to create a local @priv variable): #define DRV_STATE_WATCH(ds) (((udevPrivate *)((ds)->privateData))->watch) nodeDeviceLock(); if (!driver || !driver->privateData || DRV_STATE_WATCH(driver) == -1 || !(udev_monitor = DRV_STATE_UDEV_MONITOR(driver))) { nodeDeviceUnlock(); return; } then the rest of the code. If you're "waiting" to get the lock while Cleanup is running, then remove the chance that something that Cleanup does could cause awful things to happen in this code. In cleanup, once nodeDeviceUnlock() is run, but before virMutexDestroy, we could grab the lock. That would cause virMutexDestroy (pthread_mutex_destroy) to fail, but we don't check that. That just becomes a side effect of using an event function and taking out the mutex I suppose. Oh and I should point out that it's possible that virMutexDestroy does run, thus our getting the lock does nothing, but one would think that the subsequent condition checks wouldn't be racing too hard against the VIR_FREE(driver) in the other thread. The impossible part of this race is that how does one tell if something is waiting on the mutex other than getting a failure when we destroy it, but since we don't manage failures for destroy, we have no way to be sure. Still in the long run, both threads are heading to a very quick death, so does it really matter. John (muttering to myself how much I hate lock races).

udev_fd = udev_monitor_get_fd(udev_monitor); if (fd != udev_fd) { udevPrivate *priv = driver->privateData; @@ -1640,22 +1644,26 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, * the same error multiple times */ virEventRemoveHandle(priv->watch); - - goto cleanup; + goto error; }

device = udev_monitor_receive_device(udev_monitor); if (device == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("udev_monitor_receive_device returned NULL")); - goto cleanup; + goto error; }

+ nodeDeviceUnlock(); udevHandleOneDevice(device);

cleanup: udev_device_unref(device); return; + + error: + nodeDeviceUnlock(); + goto cleanup; }