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@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