Attachment 32882 Details for Bug 21597 – updated patch

[patch] updated patch

0001-Clean-up-inotify-watch-handling.patch (text/plain), 8.55 KB, created by Colin Walters on 2010-01-28 13:55:35 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Colin Walters

Created: 2010-01-28 13:55:35 UTC

Size: 8.55 KB

patch

obsolete

>From af9e29ff862a260fff3997ae694e56295de21b48 Mon Sep 17 00:00:00 2001
>From: Colin Walters <walters@verbum.org>
>Date: Thu, 28 Jan 2010 16:26:39 -0500
>Subject: [PATCH] Clean up inotify watch handling
>
>Substantially based on a patch by Matthias Clasen <mclasen@redhat.com>
>
>Previously, when we detected a configuration change (which included
>the set of config directories to monitor for changes), we would
>simply drop all watches, then readd them.
>
>The problem with this is that it introduced a race condition where
>we might not be watching one of the config directories for changes.
>
>Rather than dropping and readding, change the OS-dependent monitoring
>API to simply take a new set of directories to monitor.  Implicit
>in this is that the OS-specific layer needs to keep track of the
>previously monitored set.
>
>This change is only currently implemented for inotify (Linux).
>---
> bus/bus.c               |   19 +++---
> bus/dir-watch-inotify.c |  150 ++++++++++++++++++++++++++++++++--------------
> bus/dir-watch.h         |   15 +++--
> 3 files changed, 124 insertions(+), 60 deletions(-)
>
>diff --git a/bus/bus.c b/bus/bus.c
>index 81db7d6..bfd398e 100644
>--- a/bus/bus.c
>+++ b/bus/bus.c
>@@ -516,11 +516,6 @@ process_config_every_time (BusContext      *context,
>       goto failed;
>     }
> 
>-  /* Drop existing conf-dir watches (if applicable) */
>-
>-  if (is_reload)
>-    bus_drop_all_directory_watches ();
>-
>   _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>   retval = TRUE;
> 
>@@ -551,13 +546,17 @@ process_config_postinit (BusContext      *context,
>   _dbus_hash_table_unref (service_context_table);
> 
>   /* Watch all conf directories */
>-  _dbus_list_foreach (bus_config_parser_get_conf_dirs (parser),
>-		      (DBusForeachFunction) bus_watch_directory,
>-		      context);
>+  bus_set_watched_dirs (context, bus_config_parser_get_conf_dirs (parser));
> 
>   return TRUE;
> }
> 
>+static void
>+bus_shutdown_all_directory_watches (void *data)
>+{
>+  bus_set_watched_dirs ((BusContext *) data, NULL);
>+}
>+
> BusContext*
> bus_context_new (const DBusString *config_file,
>                  ForceForkSetting  force_fork,
>@@ -588,7 +587,9 @@ bus_context_new (const DBusString *config_file,
>   context->refcount = 1;
> 
>   _dbus_generate_uuid (&context->uuid);
>-  
>+
>+  _dbus_register_shutdown_func (bus_shutdown_all_directory_watches, context);
>+
>   if (!_dbus_string_copy_data (config_file, &context->config_file))
>     {
>       BUS_SET_OOM (error);
>diff --git a/bus/dir-watch-inotify.c b/bus/dir-watch-inotify.c
>index f03e1bd..f87a634 100644
>--- a/bus/dir-watch-inotify.c
>+++ b/bus/dir-watch-inotify.c
>@@ -34,6 +34,7 @@
> #include <errno.h>
> 
> #include <dbus/dbus-internals.h>
>+#include <dbus/dbus-list.h>
> #include <dbus/dbus-watch.h>
> #include "dir-watch.h"
> 
>@@ -43,6 +44,7 @@
> 
> /* use a static array to avoid handling OOM */
> static int wds[MAX_DIRS_TO_WATCH];
>+static char *dirs[MAX_DIRS_TO_WATCH];
> static int num_wds = 0;
> static int inotify_fd = -1;
> static DBusWatch *watch = NULL;
>@@ -90,12 +92,10 @@ _handle_inotify_watch (DBusWatch *passed_watch, unsigned int flags, void *data)
>   return TRUE;
> }
> 
>-void
>-bus_watch_directory (const char *dir, BusContext *context)
>+static int
>+_init_inotify (BusContext *context)
> {
>-  int wd;
>-
>-  _dbus_assert (dir != NULL);
>+  int ret = 0;
> 
>   if (inotify_fd == -1) {
> #ifdef HAVE_INOTIFY_INIT1
>@@ -112,59 +112,117 @@ bus_watch_directory (const char *dir, BusContext *context)
>      watch = _dbus_watch_new (inotify_fd, DBUS_WATCH_READABLE, TRUE,
>                               _handle_inotify_watch, NULL, NULL);
> 
>-	if (watch == NULL)
>-          {
>-            _dbus_warn ("Unable to create inotify watch\n");
>-	    goto out;
>-	  }
>-
>-	if (!_dbus_loop_add_watch (loop, watch, _inotify_watch_callback,
>-                                   NULL, NULL))
>-          {
>-            _dbus_warn ("Unable to add reload watch to main loop");
>-	    _dbus_watch_unref (watch);
>-	    watch = NULL;
>-            goto out;
>-	  }
>+     if (watch == NULL)
>+       {
>+         _dbus_warn ("Unable to create inotify watch\n");
>+         goto out;
>+       }
>+
>+     if (!_dbus_loop_add_watch (loop, watch, _inotify_watch_callback,
>+                                NULL, NULL))
>+       {
>+         _dbus_warn ("Unable to add reload watch to main loop");
>+	 _dbus_watch_unref (watch);
>+	 watch = NULL;
>+         goto out;
>+       }
>   }
> 
>-  if (num_wds >= MAX_DIRS_TO_WATCH )
>+  ret = 1;
>+
>+out:
>+  return ret;
>+}
>+
>+void
>+bus_set_watched_dirs (BusContext *context, DBusList **directories)
>+{
>+  int new_wds[MAX_DIRS_TO_WATCH];
>+  char *new_dirs[MAX_DIRS_TO_WATCH];
>+  DBusList *link;
>+  int i, j, wd;
>+
>+  if (!_init_inotify (context))
>+    goto out;
>+
>+  for (i = 0; i < MAX_DIRS_TO_WATCH; i++)
>     {
>-      _dbus_warn ("Cannot watch config directory '%s'. Already watching %d directories\n", dir, MAX_DIRS_TO_WATCH);
>-      goto out;
>+      new_wds[i] = -1;
>+      new_dirs[i] = NULL;
>     }
> 
>-  wd = inotify_add_watch (inotify_fd, dir, IN_CLOSE_WRITE | IN_DELETE | IN_MOVED_TO | IN_MOVED_FROM);
>-  if (wd < 0)
>+  i = 0;
>+  link = _dbus_list_get_first_link (directories);
>+  while (link != NULL)
>     {
>-      _dbus_warn ("Cannot setup inotify for '%s'; error '%s'\n", dir, _dbus_strerror (errno));
>-      goto out;
>+      new_dirs[i++] = (char *)link->data;
>+      link = _dbus_list_get_next_link (directories, link);
>     }
> 
>-  wds[num_wds++] = wd;
>-  _dbus_verbose ("Added watch on config directory '%s'\n", dir);
>-
>- out:
>-  ;
>-}
>+  /* Look for directories in both the old and new sets, if
>+   * we find one, move its data into the new set.
>+   */
>+  for (i = 0; new_dirs[i]; i++)
>+    {
>+      for (j = 0; j < num_wds; j++)
>+        {
>+          if (dirs[j] && strcmp (new_dirs[i], dirs[j]) == 0)
>+            {
>+              new_wds[i] = wds[j];
>+              new_dirs[i] = dirs[j];
>+              wds[j] = -1;
>+              dirs[j] = NULL;
>+              break;
>+            }
>+        }
>+    }
> 
>-void 
>-bus_drop_all_directory_watches (void)
>-{
>-  int ret;
>+  /* Any directories we find in "wds" with a nonzero fd must
>+   * not be in the new set, so perform cleanup now.
>+   */
>+  for (j = 0; j < num_wds; j++)
>+    {
>+      if (wds[j] != -1)
>+        {
>+          inotify_rm_watch (inotify_fd, wds[j]);
>+          dbus_free (dirs[j]);
>+          wds[j] = -1;
>+          dirs[j] = NULL;
>+        }
>+    }
> 
>-  if (watch != NULL)
>+  for (i = 0; new_dirs[i]; i++)
>     {
>-      _dbus_loop_remove_watch (loop, watch, _inotify_watch_callback, NULL);
>-      _dbus_watch_unref (watch);
>-      watch = NULL;
>+      if (new_wds[i] == -1)
>+        {
>+          /* FIXME - less lame error handling for failing to add a watch; we may need to sleep. */
>+          wd = inotify_add_watch (inotify_fd, new_dirs[i], IN_CLOSE_WRITE | IN_DELETE | IN_MOVED_TO | IN_MOVED_FROM);
>+          if (wd < 0)
>+            {
>+              _dbus_warn ("Cannot setup inotify for '%s'; error '%s'\n", new_dirs[i], _dbus_strerror (errno));
>+              goto out;
>+            }
>+          new_wds[i] = wd;
>+          new_dirs[i] = _dbus_strdup (new_dirs[i]);
>+          if (!new_dirs[i])
>+            {
>+              /* FIXME have less lame handling for OOM, we just silently fail to
>+               * watch.  (In reality though, the whole OOM handling in dbus is stupid
>+               * but we won't go into that in this comment =) )
>+               */
>+              inotify_rm_watch (inotify_fd, wd);
>+              new_wds[i] = -1;
>+            }
>+        }
>     }
> 
>-  _dbus_verbose ("Dropping all watches on config directories\n");
>-  ret = close (inotify_fd);
>-  if (ret)
>-    _dbus_verbose ("Error dropping watches: '%s'\n",  _dbus_strerror(errno));
>+  num_wds = i;
>+
>+  for (i = 0; i < MAX_DIRS_TO_WATCH; i++)
>+    {
>+      wds[i] = new_wds[i];
>+      dirs[i] = new_dirs[i];
>+    }
> 
>-  num_wds = 0;
>-  inotify_fd = -1;
>+ out:;
> }
>diff --git a/bus/dir-watch.h b/bus/dir-watch.h
>index 8e322a6..b44529e 100644
>--- a/bus/dir-watch.h
>+++ b/bus/dir-watch.h
>@@ -26,10 +26,15 @@
> #ifndef DIR_WATCH_H
> #define DIR_WATCH_H
> 
>-/* setup a watch on a directory (OS dependent, may be a NOP) */
>-void bus_watch_directory (const char *directory, BusContext *context);
>-
>-/* drop all the watches previously set up by bus_config_watch_directory (OS dependent, may be a NOP) */
>-void bus_drop_all_directory_watches (void);
>+/**
>+ * Update the set of directories to monitor for changes.  The
>+ * operating-system-specific implementation of this function should
>+ * avoid creating a window where a directory in both the
>+ * old and new set isn't monitored.
>+ *
>+ * @param context The bus context
>+ * @param dirs List of strings which are directory paths
>+ */
>+void bus_set_watched_dirs (BusContext *context, DBusList **dirs);
> 
> #endif /* DIR_WATCH_H */
>-- 
>1.6.6
>

Actions: View | Splinter Review

Attachments on bug 21597: 32166 | 32881 | 32882

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.