Clean up inotify watch handling

Substantially based on a patch by Matthias Clasen <mclasen@redhat.com>
kqueue implementation by Joe Marcus Clarke <marcus@freebsd.org>

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.

4 files changed, 224 insertions, 96 deletions

diff --git a/bus/bus.c b/bus/bus.c
index 81db7d69..bfd398e6 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 f03e1bd7..f87a6347 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-kqueue.c b/bus/dir-watch-kqueue.c
index 16741dd0..7c18a3c9 100644
--- a/bus/dir-watch-kqueue.c
+++ b/bus/dir-watch-kqueue.c
@@ -37,12 +37,14 @@
 #include <dbus/dbus-watch.h>
 
 #include <dbus/dbus-internals.h>
+#include <dbus/dbus-list.h>
 #include "dir-watch.h"
 
 #define MAX_DIRS_TO_WATCH 128
 
 static int kq = -1;
 static int fds[MAX_DIRS_TO_WATCH];
+static char *dirs[MAX_DIRS_TO_WATCH];
 static int num_fds = 0;
 static DBusWatch *watch = NULL;
 static DBusLoop *loop = NULL;
@@ -90,13 +92,10 @@ _handle_kqueue_watch (DBusWatch *watch, unsigned int flags, void *data)
   return TRUE;
 }
 
-void
-bus_watch_directory (const char *dir, BusContext *context)
+static int
+_init_kqueue (BusContext *context)
 {
-  int fd;
-  struct kevent ev;
-
-  _dbus_assert (dir != NULL);
+  int ret = 0;
 
   if (kq < 0)
     {
@@ -133,49 +132,114 @@ bus_watch_directory (const char *dir, BusContext *context)
 	  }
     }
 
-  if (num_fds >= MAX_DIRS_TO_WATCH )
+  ret = 1;
+
+out:
+  return ret;
+}
+
+void
+bus_set_watched_dir (BusContext *context, DBusList **directories)
+{
+  int new_fds[MAX_DIRS_TO_WATCH];
+  char *new_dirs[MAX_DIRS_TO_WATCH];
+  DBusList *link;
+  int i, f, fd;
+
+  if (!_init_kqueue (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_fds[i] = -1;
+      new_dirs[i] = NULL;
     }
 
-  fd = open (dir, O_RDONLY);
-  if (fd < 0)
+  i = 0;
+  link = _dbus_list_get_first_link (directories);
+  while (link != NULL)
     {
-      _dbus_warn ("Cannot open directory '%s'; error '%s'\n", dir, _dbus_strerror (errno));
-      goto out;
+      new_dirs[i++] = (char *)link->data;
+      link = _dbus_list_get_next_link (directories, link);
     }
 
-  EV_SET (&ev, fd, EVFILT_VNODE, EV_ADD | EV_ENABLE | EV_CLEAR,
-          NOTE_DELETE | NOTE_EXTEND | NOTE_WRITE | NOTE_RENAME, 0, 0);
-  if (kevent (kq, &ev, 1, NULL, 0, NULL) == -1)
+  /* 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++)
     {
-      _dbus_warn ("Cannot setup a kevent for '%s'; error '%s'\n", dir, _dbus_strerror (errno));
-      close (fd);
-      goto out;
+      for (j = 0; i < num_fds; j++)
+        {
+          if (dirs[j] && strcmp (new_dirs[i], dirs[j]) == 0)
+            {
+              new_fds[i] = fds[j];
+	      new_dirs[i] = dirs[j];
+	      fds[j] = -1;
+	      dirs[j] = NULL;
+	      break;
+	    }
+	}
     }
 
-  fds[num_fds++] = fd;
-  _dbus_verbose ("Added kqueue watch on config directory '%s'\n", dir);
-
- out:
-  ;
-}
-
-void
-bus_drop_all_directory_watches (void)
-{
-  int i;
-
-  _dbus_verbose ("Dropping all watches on config directories\n");
+  /* Any directory we find in "fds" with a nonzero fd must
+   * not be in the new set, so perform cleanup now.
+   */
+  for (j = 0; j < num_fds; j++)
+    {
+      if (fds[j] != -1)
+        {
+          close (fds[j]);
+	  dbus_free (dirs[j]);
+	  fds[j] = -1;
+	  dirs[j] = NULL;
+	}
+    }
 
-  for (i = 0; i < num_fds; i++)
+  for (i = 0; new_dirs[i]; i++)
     {
-      if (close (fds[i]) != 0)
+      if (new_fds[i] == -1)
         {
-          _dbus_verbose ("Error closing fd %d for config directory watch\n", fds[i]);
+          /* FIXME - less lame error handling for failing to add a watch;
+	   * we may need to sleep.
+	   */
+          fd = open (new_dirs[i], O_RDONLY);
+	  if (fd < 0)
+            {
+              _dbus_warn ("Cannot open directory '%s'; error '%s'\n", new_dirs[i], _dbus_strerror (errno));
+	      goto out;
+	    }
+
+          EV_SET (&ev, fd, EVFILT_VNODE, EV_ADD | EV_ENABLE | EV_CLEAR,
+                  NOTE_DELETE | NOTE_EXTEND | NOTE_WRITE | NOTE_RENAME, 0, 0);
+          if (kevent (kq, &ev, 1, NULL, 0, NULL) == -1)
+            {
+              _dbus_warn ("Cannot setup a kevent for '%s'; error '%s'\n", dir, _dbus_strerror (errno));
+              close (fd);
+              goto out;
+            }
+
+	  new_fds[i] = fd;
+	  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 =) )
+	       */
+              close (fd);
+	      new_fds[i] = -1;
+	    }
 	}
     }
 
-  num_fds = 0;
+  num_fds = i;
+
+  for (i = 0; i < MAX_DIRS_TO_WATCH; i++)
+    {
+      fds[i] = new_fds[i];
+      dirs[i] = new_dirs[i];
+    }
+
+ out:
+  ;
 }
diff --git a/bus/dir-watch.h b/bus/dir-watch.h
index 8e322a6e..b44529e5 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 */