7:35 a.m.
On Wed, Feb 03, 2021 at 11:38:56AM -0600, Jonathon Jongsma wrote:
We need to query mdevctl for changes to device definitions since an
administrator can define new devices by executing mdevctl outside of
libvirt.
In the future, mdevctl may add a way to signal device add/remove via
events, but for now we resort to a bit of a workaround: monitoring the
mdevctl config directory for changes to files. When a change is
detected, we query mdevctl and update our device list. The mdevctl
querying is handled in a throwaway thread, and these threads are
synchronized with a mutex.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_udev.c | 158 +++++++++++++++++++++++++++++
1 file changed, 158 insertions(+)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 038941ec51..8a1210cffb 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -19,6 +19,7 @@
*/
#include <config.h>
+#include <gio/gio.h>
#include <libudev.h>
#include <pciaccess.h>
#include <scsi/scsi.h>
@@ -66,6 +67,10 @@ struct _udevEventData {
virCond threadCond;
bool threadQuit;
bool dataReady;
+
+ GList *mdevctlMonitors;
+ virMutex mdevctlLock;
+ int mdevctlTimeout;
};
Sorry for the delay in review, it took me a while to wrap my head around the
amount of GLib black magic going on in this patch.
...
+
+
+static void
+mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED,
+ GFile *file,
+ GFile *other_file G_GNUC_UNUSED,
+ GFileMonitorEvent event_type,
+ gpointer user_data);
+
+
+/* Recursively monitors a directory and its subdirectories for file changes and
+ * returns a GList of GFileMonitor objects */
+static GList*
+monitorFileRecursively(udevEventDataPtr udev,
+ GFile *file)
+{
+ GList *monitors = NULL;
+ g_autoptr(GError) error = NULL;
+ g_autoptr(GFileEnumerator) children = NULL;
+ GFileMonitor *mon;
+
+ if (!(children = g_file_enumerate_children(file, "standard::*",
+ G_FILE_QUERY_INFO_NONE, NULL,
&error)))
+ goto error;
+
+ if (!(mon = g_file_monitor(file, G_FILE_MONITOR_NONE, NULL, &error)))
+ goto error;
+
+ g_signal_connect(mon, "changed",
+ G_CALLBACK(mdevctlEventHandleCallback), udev);
newline here^
+ monitors = g_list_append(monitors, mon);
+
+ while (true) {
+ GFileInfo *info;
+ GFile *child;
We should initialize all the pointers - coverity might find creative ways of
complaining about it.
+ GList *child_monitors = NULL;
+
+ if (!g_file_enumerator_iterate(children, &info, &child, NULL,
&error))
I hope that the fact that ^this can block will never pose a problem for us.
+ goto error;
newline here
+ if (!info)
+ break;
+
+ if (g_file_query_file_type(child, G_FILE_QUERY_INFO_NONE, NULL) ==
+ G_FILE_TYPE_DIRECTORY) {
+
+ child_monitors = monitorFileRecursively(udev, child);
+ if (child_monitors)
+ monitors = g_list_concat(monitors, child_monitors);
+ }
+ }
+
+ return monitors;
+
+ error:
+ g_list_free_full(monitors, g_object_unref);
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to monitor directory: %s"), error->message);
+ g_clear_error(&error);
+ return NULL;
+}
+
+
+static void
+mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED,
+ GFile *file,
+ GFile *other_file G_GNUC_UNUSED,
+ GFileMonitorEvent event_type,
+ gpointer user_data)
+{
+ udevEventDataPtr priv = user_data;
+ /* if a new directory appears, monitor that directory for changes */
+ if (event_type == G_FILE_MONITOR_EVENT_CREATED &&
+ g_file_query_file_type(file, G_FILE_QUERY_INFO_NONE, NULL) ==
+ G_FILE_TYPE_DIRECTORY) {
+ GList *newmonitors = monitorFileRecursively(priv, file);
+ priv->mdevctlMonitors = g_list_concat(priv->mdevctlMonitors, newmonitors);
priv->mdevctlMonitors is shared among threads, but you only acquire the
mutex just before exec'ing mdevctl in the mdevctlHandler thread, so ^this can
yield unexpected results in terms of directories monitored.
+ }
+
+ /* When mdevctl creates a device, it can result in multiple notify events
+ * emitted for a single logical change (e.g. several CHANGED events, or a
+ * CREATED and CHANGED event followed by CHANGES_DONE_HINT). To avoid
+ * spawning a mdevctl thread multiple times for a single logical
+ * configuration change, try to coalesce these changes by waiting for the
+ * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the
+ * signal if that event never comes */
So there's no guaranteed pattern to monitor, is that so? IOW Even the
CHANGES_DONE_HINT may not actually arrive? That's a very poor design. I'd like
to ditch the timer as much as possible.
+ if (event_type != G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) {
+ if (priv->mdevctlTimeout > 0)
+ virEventRemoveTimeout(priv->mdevctlTimeout);
+ priv->mdevctlTimeout = virEventAddTimeout(100, scheduleMdevctlHandler,
+ priv, NULL);
+ return;
+ }
+
+ scheduleMdevctlHandler(-1, priv);
+}
+
+
static int
nodeStateInitialize(bool privileged,
const char *root,
@@ -2007,6 +2147,8 @@ nodeStateInitialize(bool privileged,
udevEventDataPtr priv = NULL;
struct udev *udev = NULL;
virThread enumThread;
+ g_autoptr(GFile) mdevctlConfigDir =
g_file_new_for_path("/etc/mdevctl.d");
+ GError *error = NULL;
^This error isn't passed anywhere...
if (root != NULL) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -2108,6 +2250,22 @@ nodeStateInitialize(bool privileged,
if (priv->watch == -1)
goto unlock;
+ /* mdevctl may add notification events in the future:
+ * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to
+ * monitoring the mdevctl configuration directory for changes.
+ * mdevctl configuration is stored in a directory tree within
+ * /etc/mdevctl.d/. There is a directory for each parent device, which
+ * contains a file defining each mediated device */
One security aspect with this proposal that needs to mentioned is that we
should not make the directory to be monitored configurable, otherwise it's so
easy to deplete resources - simply because this patch doesn't introduce any
limit on number of spawned threads (like we have for thread pools).
+ if (!(priv->mdevctlMonitors = monitorFileRecursively(priv,
+ mdevctlConfigDir))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to monitor mdevctl config directory: %s"),
+ error->message);
This error message is redundant, because monitorFileRecursively already reports
errors appending the respective GError, not to mentioned you didn't pass error
anywhere, so it would always be NULL.
Erik