On 07/26/2017 02:44 AM, Erik Skultety wrote: 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. 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... Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; 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: 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(a)redhat.com&gt; 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

Let this new method handle the device object we obtained from the monitor in order to enhance readability. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- 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 +udevHandleOneDevice(struct udev_device *device) +{ + const char *action = udev_device_get_action(device); + + VIR_DEBUG("udev action: '%s'", action); + + if (STREQ(action, "add") || STREQ(action, "change")) + return udevAddOneDevice(device); + + if (STREQ(action, "remove")) + return udevRemoveOneDevice(device); + + return 0; +} + + static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, int fd, @@ -1607,7 +1624,6 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, { struct udev_device *device = NULL; struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver); - const char *action = NULL; int udev_fd = -1; udev_fd = udev_monitor_get_fd(udev_monitor); @@ -1635,18 +1651,7 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, goto cleanup; } - action = udev_device_get_action(device); - VIR_DEBUG("udev action: '%s'", action); - - if (STREQ(action, "add") || STREQ(action, "change")) { - udevAddOneDevice(device); - goto cleanup; - } - - if (STREQ(action, "remove")) { - udevRemoveOneDevice(device); - goto cleanup; - } + udevHandleOneDevice(device); cleanup: udev_device_unref(device); -- 2.13.3

On Tue, Aug 01, 2017 at 03:46:44PM -0400, John Ferlan wrote: 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. 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. ... 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: 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).