[libvirt PATCH v4 12/25] nodedev: Refresh mdev devices when changes are detected

[Erik Skultety]

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