10:03 a.m.
On 08/24/2017 07:23 AM, Erik Skultety wrote:
We need to perform some sanity checks on the udev monitor before
every
use so that we know nothing changed in the meantime. The reason for
moving the code to a separate function is to be able to perform the same
check from a worker thread that will replace the udevEventHandleCallback
in terms of poking udev for device events.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/node_device/node_device_udev.c | 57 ++++++++++++++++++++++++++------------
1 file changed, 39 insertions(+), 18 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index f4177455c..465d272ba 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1615,47 +1615,68 @@ udevHandleOneDevice(struct udev_device *device)
}
-static void
-udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
- int fd,
- int events ATTRIBUTE_UNUSED,
- void *data ATTRIBUTE_UNUSED)
+/* the caller must be holding the driver lock prior to calling this function */
+static bool
+udevCheckMonitorFD(struct udev_monitor *udev_monitor, int fd)
One line for each argument...
I think in keeping with other functions - this should be named
'udevEventCheckMonitorFD'
{
- struct udev_device *device = NULL;
- struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
- int udev_fd = -1;
+ int real_fd = -1;
- udev_fd = udev_monitor_get_fd(udev_monitor);
- if (fd != udev_fd) {
+ /* sanity check that the monitor socket hasn't changed */
+ real_fd = udev_monitor_get_fd(udev_monitor);
+
+ if (real_fd != 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);
+ real_fd, 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
+ /* this is a non-recoverable error, thus the event handle has to be
+ * removed, so that we don't get into the handler again because of some
+ * spurious behaviour
*/
virEventRemoveHandle(priv->watch);
priv->watch = -1;
Hmmm... thinking a couple patches later - this would seem to be a good
candidate for something that udevEventThreadDataFree could do (as long
as it held the appropriate lock of course).
It's almost too bad we couldn't somehow "pretend" to have a restart...
different problem though!
- goto cleanup;
+ return false;
}
- device = udev_monitor_receive_device(udev_monitor);
- if (device == NULL) {
+ return true;
+}
+
+
+static void
+udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
+ int fd,
+ int events ATTRIBUTE_UNUSED,
+ void *data ATTRIBUTE_UNUSED)
+{
+ struct udev_device *device = NULL;
+ struct udev_monitor *udev_monitor = NULL;
+
+ nodeDeviceLock();
+ udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
Technically there's a couple of things going on here - including the
addition of the nodeDevice{Lock|Unlock}.
Probably would have been best to make the split first, then add the
Lock/Unlock afterwards (or vice versa).
Still once I get to patch 3, I began wondering how the udev interaction
works.
+
+ if (!udevCheckMonitorFD(udev_monitor, fd))
+ goto unlock;> +
+ if (!(device = udev_monitor_receive_device(udev_monitor))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("udev_monitor_receive_device returned NULL"));
I almost wonder if it would be better to have this be a
ReportSystemError w/ errno... Not that udev docs give any guidance
there, but maybe it'd help.
- goto cleanup;
+ goto unlock;
I know this is "existing"; however, if !device, then what's the purpose
of calling udev_device_unref(NULL)? In fact there's one path in
udevGetDMIData which actually checks != NULL before calling - although
it has no reason to since I see no way for it to be NULL at cleanup
(again a different issue).
Also, wouldn't nodeDeviceLock/Unlock wrapping only be necessary within
udevCheckMonitorFD? Why would the udev call need to be wrapped as well?
John
}
+ nodeDeviceUnlock();
udevHandleOneDevice(device);
cleanup:
udev_device_unref(device);
return;
+
+ unlock:
+ nodeDeviceUnlock();
+ goto cleanup;
}