From af9e29ff862a260fff3997ae694e56295de21b48 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 28 Jan 2010 16:26:39 -0500 Subject: [PATCH] Clean up inotify watch handling Substantially based on a patch by Matthias Clasen 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 #include +#include #include #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