From aabe86ecbbcb101f242815333c57b4dfb6d6e3b6 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 17 Feb 2017 15:03:02 +0000 Subject: [PATCH 15/15] config-parser: Store service directories in structs This lets us give them a flags word, which we immediately use to track whether this directory should be watched with inotify or equivalent. The struct name is unfortunately a bit odd, because I had aimed to use BusServiceDir, but activation.c already has BusServiceDirectory so that would have been too confusing. Signed-off-by: Simon McVittie --- bus/activation.c | 14 +++- bus/config-parser.c | 202 +++++++++++++++++++++++++++++++++++++++++++++------- bus/config-parser.h | 18 +++++ 3 files changed, 205 insertions(+), 29 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index b3b5510a..8e477067 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -26,6 +26,7 @@ #include #include "activation.h" #include "activation-exit-codes.h" +#include "config-parser.h" #include "desktop-file.h" #include "dispatch.h" #include "services.h" @@ -61,6 +62,7 @@ typedef struct { int refcount; char *dir_c; + BusServiceDirFlags flags; DBusHashTable *entries; } BusServiceDirectory; @@ -821,9 +823,12 @@ bus_activation_reload (BusActivation *activation, link = _dbus_list_get_first_link (directories); while (link != NULL) { + BusConfigServiceDir *config = link->data; BusServiceDirectory *s_dir; - dir = _dbus_strdup ((const char *) link->data); + _dbus_assert (config->path != NULL); + + dir = _dbus_strdup (config->path); if (!dir) { BUS_SET_OOM (error); @@ -840,6 +845,7 @@ bus_activation_reload (BusActivation *activation, s_dir->refcount = 1; s_dir->dir_c = dir; + s_dir->flags = config->flags; s_dir->entries = _dbus_hash_table_new (DBUS_HASH_STRING, NULL, (DBusFreeFunction)bus_activation_entry_unref); @@ -2519,6 +2525,7 @@ static dbus_bool_t do_service_reload_test (DBusString *dir, dbus_bool_t oom_test) { BusActivation *activation; + BusConfigServiceDir config; DBusString address; DBusList *directories; CheckData d; @@ -2526,7 +2533,10 @@ do_service_reload_test (DBusString *dir, dbus_bool_t oom_test) directories = NULL; _dbus_string_init_const (&address, ""); - if (!_dbus_list_append (&directories, _dbus_string_get_data (dir))) + config.path = _dbus_string_get_data (dir); + config.flags = BUS_SERVICE_DIR_FLAGS_NONE; + + if (!_dbus_list_append (&directories, &config)) return FALSE; activation = bus_activation_new (NULL, &address, &directories, NULL); diff --git a/bus/config-parser.c b/bus/config-parser.c index cec992df..ad165c0f 100644 --- a/bus/config-parser.c +++ b/bus/config-parser.c @@ -100,7 +100,7 @@ struct BusConfigParser DBusList *mechanisms; /**< Auth mechanisms */ - DBusList *service_dirs; /**< Directories to look for session services in */ + DBusList *service_dirs; /**< BusConfigServiceDirs to look for session services in */ DBusList *conf_dirs; /**< Directories to look for policy configuration in */ @@ -232,7 +232,33 @@ merge_service_context_hash (DBusHashTable *dest, return FALSE; } -static dbus_bool_t +/* + * Create a new BusConfigServiceDir. Takes ownership of path if #TRUE is returned. + * + * @returns #FALSE on OOM + */ +static BusConfigServiceDir * +bus_config_service_dir_new_take (char *path, + BusServiceDirFlags flags) +{ + BusConfigServiceDir *self = dbus_new0 (BusConfigServiceDir, 1); + + if (self == NULL) + return NULL; + + self->path = path; /* take ownership */ + self->flags = flags; + return self; +} + +static void +bus_config_service_dir_free (BusConfigServiceDir *self) +{ + dbus_free (self->path); + dbus_free (self); +} + +static BusConfigServiceDir * service_dirs_find_dir (DBusList **service_dirs, const char *dir) { @@ -242,31 +268,81 @@ service_dirs_find_dir (DBusList **service_dirs, for (link = *service_dirs; link; link = _dbus_list_get_next_link(service_dirs, link)) { - const char *link_dir; - - link_dir = (const char *)link->data; - if (strcmp (dir, link_dir) == 0) - return TRUE; + BusConfigServiceDir *link_dir = link->data; + + if (strcmp (dir, link_dir->path) == 0) + return link_dir; } - return FALSE; + return NULL; } -static void +static void service_dirs_append_link_unique_or_free (DBusList **service_dirs, DBusList *dir_link) { - if (!service_dirs_find_dir (service_dirs, dir_link->data)) + BusConfigServiceDir *dir = dir_link->data; + BusConfigServiceDir *already = service_dirs_find_dir (service_dirs, + dir->path); + + if (already == NULL) { _dbus_list_append_link (service_dirs, dir_link); } else { - dbus_free (dir_link->data); + /* BusServiceDirFlags are chosen such that the compatible thing to do + * is to "and" the flags. For example, if a directory is explicitly + * added as a (which is watched with inotify) and is also + * the transient service directory (which should not be watched), + * the compatible thing to do is to watch it. */ + already->flags &= dir->flags; + bus_config_service_dir_free (dir_link->data); _dbus_list_free_link (dir_link); } } +/* + * Consume links from dirs (a list of paths), converting them into links in + * service_dirs (a list of unique BusServiceDir). + * + * On success, return TRUE. dirs will be empty, and every original entry in + * dirs will have a corresponding service_dirs entry. + * + * On OOM, return FALSE. Each original entry of dirs has either been + * appended to service_dirs or left in dirs. + */ +static dbus_bool_t +service_dirs_absorb_string_list (DBusList **service_dirs, + DBusList **dirs, + BusServiceDirFlags flags) +{ + DBusList *link; + + _dbus_assert (service_dirs != NULL); + _dbus_assert (dirs != NULL); + + while ((link = _dbus_list_pop_first_link (dirs))) + { + char *path = link->data; + BusConfigServiceDir *dir = bus_config_service_dir_new_take (path, flags); + + if (dir == NULL) + { + /* OOM - roll back (this does not need to allocate memory) */ + _dbus_list_prepend_link (service_dirs, link); + return FALSE; + } + + /* Ownership of path has been taken by dir */ + link->data = dir; + service_dirs_append_link_unique_or_free (service_dirs, link); + } + + _dbus_assert (*dirs == NULL); + return TRUE; +} + static dbus_bool_t merge_included (BusConfigParser *parser, BusConfigParser *included, @@ -504,7 +580,7 @@ bus_config_parser_unref (BusConfigParser *parser) _dbus_list_clear (&parser->listen_on); _dbus_list_foreach (&parser->service_dirs, - (DBusForeachFunction) dbus_free, + (DBusForeachFunction) bus_config_service_dir_free, NULL); _dbus_list_clear (&parser->service_dirs); @@ -821,7 +897,6 @@ start_busconfig_child (BusConfigParser *parser, } else if (element_type == ELEMENT_STANDARD_SESSION_SERVICEDIRS) { - DBusList *link; DBusList *dirs; dirs = NULL; @@ -840,14 +915,23 @@ start_busconfig_child (BusConfigParser *parser, return FALSE; } - while ((link = _dbus_list_pop_first_link (&dirs))) - service_dirs_append_link_unique_or_free (&parser->service_dirs, link); + /* We have traditionally watched the standard session service + * directories with inotify, and allowed service files whose names do not + * match the bus name */ + if (!service_dirs_absorb_string_list (&parser->service_dirs, &dirs, + BUS_SERVICE_DIR_FLAGS_NONE)) + { + BUS_SET_OOM (error); + _dbus_list_foreach (&dirs, (DBusForeachFunction) dbus_free, + NULL); + _dbus_list_clear (&dirs); + return FALSE; + } return TRUE; } else if (element_type == ELEMENT_STANDARD_SYSTEM_SERVICEDIRS) { - DBusList *link; DBusList *dirs; dirs = NULL; @@ -866,8 +950,19 @@ start_busconfig_child (BusConfigParser *parser, return FALSE; } - while ((link = _dbus_list_pop_first_link (&dirs))) - service_dirs_append_link_unique_or_free (&parser->service_dirs, link); + /* We have traditionally watched the standard system service + * directories with inotify, and allowed service files whose names do not + * match the bus name (the servicehelper won't successfully activate + * them, but we do still parse them) */ + if (!service_dirs_absorb_string_list (&parser->service_dirs, &dirs, + BUS_SERVICE_DIR_FLAGS_NONE)) + { + BUS_SET_OOM (error); + _dbus_list_foreach (&dirs, (DBusForeachFunction) dbus_free, + NULL); + _dbus_list_clear (&dirs); + return FALSE; + } return TRUE; } @@ -2589,6 +2684,7 @@ bus_config_parser_content (BusConfigParser *parser, case ELEMENT_SERVICEDIR: { char *s; + BusConfigServiceDir *dir; DBusString full_path; DBusList *link; @@ -2609,15 +2705,27 @@ bus_config_parser_content (BusConfigParser *parser, goto nomem; } - link = _dbus_list_alloc_link (s); + /* has traditionally implied that we watch the + * directory with inotify, and allow service files whose names do not + * match the bus name */ + dir = bus_config_service_dir_new_take (s, BUS_SERVICE_DIR_FLAGS_NONE); - if (link == NULL) + if (dir == NULL) { _dbus_string_free (&full_path); dbus_free (s); goto nomem; } + link = _dbus_list_alloc_link (dir); + + if (link == NULL) + { + _dbus_string_free (&full_path); + bus_config_service_dir_free (dir); + goto nomem; + } + /* cannot fail */ service_dirs_append_link_unique_or_free (&parser->service_dirs, link); _dbus_string_free (&full_path); @@ -2817,7 +2925,12 @@ bus_config_parser_get_watched_dirs (BusConfigParser *parser, link != NULL; link = _dbus_list_get_next_link (&parser->service_dirs, link)) { - if (!_dbus_list_append (watched_dirs, link->data)) + BusConfigServiceDir *dir = link->data; + + if (dir->flags & BUS_SERVICE_DIR_FLAGS_NO_WATCH) + continue; + + if (!_dbus_list_append (watched_dirs, dir->path)) goto oom; } @@ -3178,6 +3291,34 @@ lists_of_c_strings_equal (DBusList *a, } static dbus_bool_t +lists_of_service_dirs_equal (DBusList *a, + DBusList *b) +{ + DBusList *ia; + DBusList *ib; + + ia = a; + ib = b; + + while (ia != NULL && ib != NULL) + { + BusConfigServiceDir *da = ia->data; + BusConfigServiceDir *db = ib->data; + + if (strcmp (da->path, db->path)) + return FALSE; + + if (da->flags != db->flags) + return FALSE; + + ia = _dbus_list_get_next_link (&a, ia); + ib = _dbus_list_get_next_link (&b, ib); + } + + return ia == NULL && ib == NULL; +} + +static dbus_bool_t limits_equal (const BusLimits *a, const BusLimits *b) { @@ -3220,7 +3361,7 @@ config_parsers_equal (const BusConfigParser *a, if (!lists_of_c_strings_equal (a->mechanisms, b->mechanisms)) return FALSE; - if (!lists_of_c_strings_equal (a->service_dirs, b->service_dirs)) + if (!lists_of_service_dirs_equal (a->service_dirs, b->service_dirs)) return FALSE; /* FIXME: compare policy */ @@ -3549,7 +3690,9 @@ test_default_session_servicedirs (const DBusString *test_base_dir) link != NULL; link = _dbus_list_get_next_link (dirs, link), i++) { - printf (" test service dir: '%s'\n", (char *)link->data); + BusConfigServiceDir *dir = link->data; + + printf (" test service dir: '%s'\n", dir->path); printf (" current standard service dir: '%s'\n", test_session_service_dir_matches[i]); if (test_session_service_dir_matches[i] == NULL) { @@ -3557,12 +3700,17 @@ test_default_session_servicedirs (const DBusString *test_base_dir) goto out; } - if (strcmp (test_session_service_dir_matches[i], - (char *)link->data) != 0) + if (strcmp (test_session_service_dir_matches[i], dir->path) != 0) { printf ("'%s' directory does not match '%s' in the match set\n", - (char *)link->data, - test_session_service_dir_matches[i]); + dir->path, test_session_service_dir_matches[i]); + goto out; + } + + if (dir->flags != BUS_SERVICE_DIR_FLAGS_NONE) + { + printf ("'%s' directory has flags 0x%x, should be 0x%x\n", + dir->path, dir->flags, BUS_SERVICE_DIR_FLAGS_NONE); goto out; } } diff --git a/bus/config-parser.h b/bus/config-parser.h index 2361e22f..b3dfa91e 100644 --- a/bus/config-parser.h +++ b/bus/config-parser.h @@ -86,4 +86,22 @@ BusConfigParser* bus_config_load (const DBusString *file, const BusConfigParser *parent, DBusError *error); +/* + * These are chosen such that if we configure a directory twice with different + * flags, we have to do an "and" operation on the flags - the compatible + * thing to do is to have no flags. + */ +typedef enum +{ + BUS_SERVICE_DIR_FLAGS_NO_WATCH = (1 << 0), + /* Keep this one at the end to reduce diffs when adding new entries */ + BUS_SERVICE_DIR_FLAGS_NONE = 0 +} BusServiceDirFlags; + +typedef struct +{ + BusServiceDirFlags flags; + char *path; +} BusConfigServiceDir; + #endif /* BUS_CONFIG_PARSER_H */ -- 2.11.0