diff --git a/cf-reactor/reactor_transform.c b/cf-reactor/reactor_transform.c index cc0119bf70c..ce03fc86f0a 100644 --- a/cf-reactor/reactor_transform.c +++ b/cf-reactor/reactor_transform.c @@ -115,19 +115,15 @@ static void RecordEventsPromiseOutcome(EvalContext *ctx, const Promise *pp, Prom /* Returns the list of bundle references of the (expanded) 'then' attribute, * which is always a list after expansion, see ExpandBundleListReference(). - * Logs an error and returns NULL if there is no 'then' attribute or it is - * empty. */ + * Logs an error and returns NULL if it is empty. */ static const Rlist *GetThenBundles(const Promise *pp) { assert(pp != NULL); + /* A promise without 'then' is rejected by the policy checks, see + * EventsParseTreeCheck() */ const Constraint *then_constraint = PromiseGetConstraint(pp, "then"); - if (then_constraint == NULL) - { - Log(LOG_LEVEL_ERR, "Reactor events promise '%s' does not specify a 'then' bundle", pp->promiser); - return NULL; - } - + assert(then_constraint != NULL); assert(then_constraint->rval.type == RVAL_TYPE_LIST); const Rlist *bundles = RvalRlistValue(then_constraint->rval); if (bundles == NULL) diff --git a/libpromises/mod_reactor.c b/libpromises/mod_reactor.c index 520d28f03ef..b65761c42cf 100644 --- a/libpromises/mod_reactor.c +++ b/libpromises/mod_reactor.c @@ -26,6 +26,15 @@ #include #include +#include +#include + +static const char *const POLICY_ERROR_EVENTS_MISSING_ATTRIBUTE = + "Missing required attribute '%s' for promise type 'events'"; +static const char *const POLICY_ERROR_EVENTS_EMPTY_THEN = + "Empty list of bundles in attribute 'then' for promise type 'events'"; +static const char *const POLICY_ERROR_WHEN_BODY_NO_EVENT = + "Body when %s requires an event attribute, such as " REACTOR_FILE_DELETED; static const ConstraintSyntax when_constraints[] = { @@ -37,7 +46,34 @@ static const ConstraintSyntax when_constraints[] = ConstraintSyntaxNewNull() }; -static const BodySyntax when_body = BodySyntaxNew("when", when_constraints, NULL, SYNTAX_STATUS_NORMAL); +/* Checks that a 'when' body has an event attribute, i.e. any attribute other + * than the global ones (meta, inherit_from) and 'interval', which only sets + * how the event is checked for. A body inheriting from another one may get + * its event from it, and that one is checked on its own. */ +static bool WhenBodyCheck(const Body *body, Seq *errors) +{ + assert(body != NULL); + + if (BodyHasConstraint(body, "inherit_from")) + { + return true; + } + + for (size_t i = 0; i < SeqLength(body->conlist); i++) + { + const Constraint *cp = SeqAt(body->conlist, i); + if (!StringEqual(cp->lval, "meta") && !StringEqual(cp->lval, "interval")) + { + return true; + } + } + + SeqAppend(errors, PolicyErrorNew(POLICY_ELEMENT_TYPE_BODY, body, + POLICY_ERROR_WHEN_BODY_NO_EVENT, body->name)); + return false; +} + +static const BodySyntax when_body = BodySyntaxNew("when", when_constraints, &WhenBodyCheck, SYNTAX_STATUS_NORMAL); static const ConstraintSyntax CF_EVENT_BODIES[] = { @@ -48,7 +84,33 @@ static const ConstraintSyntax CF_EVENT_BODIES[] = static bool EventsParseTreeCheck(const Promise *pp, Seq *errors) { - return PromiseCheckBundleCallArity(pp, "then", errors); + bool success = true; + + if (PromiseGetConstraint(pp, "when") == NULL) + { + SeqAppend(errors, PolicyErrorNew(POLICY_ELEMENT_TYPE_PROMISE, pp, + POLICY_ERROR_EVENTS_MISSING_ATTRIBUTE, "when")); + success = false; + } + + /* Only a literally empty list can be caught here, a list made empty by an + * empty @(list) is caught on evaluation. */ + const Constraint *then = PromiseGetConstraint(pp, "then"); + if (then == NULL) + { + SeqAppend(errors, PolicyErrorNew(POLICY_ELEMENT_TYPE_PROMISE, pp, + POLICY_ERROR_EVENTS_MISSING_ATTRIBUTE, "then")); + success = false; + } + else if (then->rval.type == RVAL_TYPE_LIST && RvalRlistValue(then->rval) == NULL) + { + SeqAppend(errors, PolicyErrorNew(POLICY_ELEMENT_TYPE_CONSTRAINT, then, + POLICY_ERROR_EVENTS_EMPTY_THEN)); + success = false; + } + + success &= PromiseCheckBundleCallArity(pp, "then", errors); + return success; } const PromiseTypeSyntax CF_REACTOR_PROMISE_TYPES[] = diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index e2eb77b7098..6f5d77df9aa 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -157,6 +157,7 @@ check_PROGRAMS = \ iteration_test \ protocol_recv_overflow_test \ watcher_test \ + file_watcher_test \ stoppable_thread_test \ wakeup_channel_test @@ -236,6 +237,9 @@ watcher_test_SOURCES = watcher_test.c \ ../../cf-reactor/stoppable_thread.c \ ../../cf-reactor/wakeup_channel.c +file_watcher_test_SOURCES = file_watcher_test.c \ + ../../cf-reactor/file_watcher.c + stoppable_thread_test_SOURCES = stoppable_thread_test.c \ ../../cf-reactor/stoppable_thread.c diff --git a/tests/unit/file_watcher_test.c b/tests/unit/file_watcher_test.c new file mode 100644 index 00000000000..51bdb0ccf4f --- /dev/null +++ b/tests/unit/file_watcher_test.c @@ -0,0 +1,219 @@ +#include + +#include /* FileWatcherStateNew(), FileWatcherCheckFileDeleted() */ +#include /* RlistAppendScalar() */ +#include /* LoggingPrivContext, LoggingPrivSetContext() */ + +#include /* strstr() */ +#include /* snprintf() */ +#include /* mkdtemp() */ +#include /* close(), unlink(), rmdir(), symlink() */ +#include /* open() */ + +static int captured_err_count = 0; +static char captured_err_message[256]; + +static char *CaptureErrorLogHook(ARG_UNUSED LoggingPrivContext *pctx, LogLevel level, const char *message) +{ + if (level == LOG_LEVEL_ERR) + { + captured_err_count++; + strlcpy(captured_err_message, message, sizeof(captured_err_message)); + } + return (char *) message; +} + +/* Returns a new state of a file_deleted watcher of the file at path */ +static void *NewFileDeletedState(const char *path) +{ + Rlist *args = NULL; + RlistAppendScalar(&args, path); + void *state = FileWatcherStateNew(args); + RlistDestroy(args); + return state; +} + +/* Creates the file 'file' in a new temporary directory dir */ +static void CreateTempFile(char *dir, char *path, size_t path_size) +{ + assert_true(mkdtemp(dir) != NULL); + snprintf(path, path_size, "%s/file", dir); + int file_fd = open(path, O_CREAT | O_WRONLY, 0600); + assert_true(file_fd >= 0); + close(file_fd); +} + +/* Sets up dir/loop, a symlink to itself, so that lstat() of dir/loop/file + * fails with ELOOP (even as root), i.e. whether the file was deleted can't + * be told. Unlinking the loop makes lstat() work again (ENOENT). */ +static void SetUpSymlinkLoop(char *dir, char *loop, size_t loop_size, char *path, size_t path_size) +{ + assert_true(mkdtemp(dir) != NULL); + snprintf(loop, loop_size, "%s/loop", dir); + assert_int_equal(symlink(loop, loop), 0); + snprintf(path, path_size, "%s/file", loop); +} + +static void TearDownSymlinkLoop(const char *dir, const char *loop) +{ + unlink(loop); + assert_int_equal(rmdir(dir), 0); +} + +static void test_check_file_deleted_transitions(void) +{ + char dir[] = "/tmp/file_watcher_test.XXXXXX"; + char path[PATH_MAX]; + CreateTempFile(dir, path, sizeof(path)); + + void *state = NewFileDeletedState(path); + + /* Present (the first check is the reference) */ + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_NO_EVENT); + + /* Deleted: the event happens once, not while the file stays absent */ + assert_int_equal(unlink(path), 0); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_EVENT); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_NO_EVENT); + + /* Created again, then deleted again: the event happens again */ + int file_fd = open(path, O_CREAT | O_WRONLY, 0600); + assert_true(file_fd >= 0); + close(file_fd); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_NO_EVENT); + assert_int_equal(unlink(path), 0); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_EVENT); + + FileWatcherStateDestroy(state); + assert_int_equal(rmdir(dir), 0); +} + +static void test_check_file_deleted_absent_at_start(void) +{ + char dir[] = "/tmp/file_watcher_test.XXXXXX"; + char path[PATH_MAX]; + assert_true(mkdtemp(dir) != NULL); + snprintf(path, sizeof(path), "%s/file", dir); + + void *state = NewFileDeletedState(path); + + /* The first check is the reference, at which the file is absent */ + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_NO_EVENT); + + /* Still absent: never seen, so not deleted */ + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_NO_EVENT); + + /* Seen once created, so deleted afterwards */ + int file_fd = open(path, O_CREAT | O_WRONLY, 0600); + assert_true(file_fd >= 0); + close(file_fd); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_NO_EVENT); + assert_int_equal(unlink(path), 0); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_EVENT); + + FileWatcherStateDestroy(state); + assert_int_equal(rmdir(dir), 0); +} + +static void test_check_file_deleted_dangling_symlink(void) +{ + char dir[] = "/tmp/file_watcher_test.XXXXXX"; + char path[PATH_MAX]; + char link[PATH_MAX]; + CreateTempFile(dir, path, sizeof(path)); + snprintf(link, sizeof(link), "%s/link", dir); + assert_int_equal(symlink(path, link), 0); + + void *state = NewFileDeletedState(link); + + /* The first check is the reference */ + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_NO_EVENT); + + /* Symlinks are not followed: the link is not deleted when its target + * is, only when the link itself is */ + assert_int_equal(unlink(path), 0); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_NO_EVENT); + assert_int_equal(unlink(link), 0); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_EVENT); + + FileWatcherStateDestroy(state); + assert_int_equal(rmdir(dir), 0); +} + +static void test_check_file_deleted_error(void) +{ + char dir[] = "/tmp/file_watcher_test.XXXXXX"; + char loop[PATH_MAX]; + char path[PATH_MAX]; + SetUpSymlinkLoop(dir, loop, sizeof(loop), path, sizeof(path)); + + const LogLevel old_level = LogGetGlobalLevel(); + LogSetGlobalLevel(LOG_LEVEL_CRIT); + + void *state = NewFileDeletedState(path); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); + FileWatcherStateDestroy(state); + + LogSetGlobalLevel(old_level); + TearDownSymlinkLoop(dir, loop); +} + +static void test_check_file_deleted_logs_error_once(void) +{ + char dir[] = "/tmp/file_watcher_test.XXXXXX"; + char loop[PATH_MAX]; + char path[PATH_MAX]; + SetUpSymlinkLoop(dir, loop, sizeof(loop), path, sizeof(path)); + + captured_err_count = 0; + captured_err_message[0] = '\0'; + LoggingPrivContext log_ctx = { + .log_hook = CaptureErrorLogHook, + .force_hook_level = LOG_LEVEL_ERR, + }; + LoggingPrivSetContext(&log_ctx); + const LogLevel old_level = LogGetGlobalLevel(); + LogSetGlobalLevel(LOG_LEVEL_CRIT); + + /* The failure of the first check is logged */ + void *state = NewFileDeletedState(path); + assert_int_equal(captured_err_count, 0); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); + assert_int_equal(captured_err_count, 1); + assert_true(strstr(captured_err_message, "Unable to lstat") != NULL); + + /* The following failures are not logged again */ + for (int i = 0; i < 3; i++) + { + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); + } + assert_int_equal(captured_err_count, 1); + + /* Once lstat() works again, the next failure is logged again */ + assert_int_equal(unlink(loop), 0); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_NO_EVENT); + assert_int_equal(symlink(loop, loop), 0); + assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); + assert_int_equal(captured_err_count, 2); + + FileWatcherStateDestroy(state); + + LogSetGlobalLevel(old_level); + LoggingPrivSetContext(NULL); + TearDownSymlinkLoop(dir, loop); +} + +int main() +{ + PRINT_TEST_BANNER(); + const UnitTest tests[] = + { + unit_test(test_check_file_deleted_transitions), + unit_test(test_check_file_deleted_absent_at_start), + unit_test(test_check_file_deleted_dangling_symlink), + unit_test(test_check_file_deleted_error), + unit_test(test_check_file_deleted_logs_error_once), + }; + + return run_tests(tests); +} diff --git a/tests/unit/watcher_test.c b/tests/unit/watcher_test.c index 3573c91d709..c6fca5155eb 100644 --- a/tests/unit/watcher_test.c +++ b/tests/unit/watcher_test.c @@ -4,27 +4,13 @@ #include /* FileWatcherStateNew(), FileWatcherCheckFileDeleted() */ #include /* Promise */ #include /* RlistAppendScalar() */ -#include /* LoggingPrivContext, LoggingPrivSetContext() */ +#include /* StringEqual() */ -#include /* strstr() */ #include /* snprintf() */ #include /* mkdtemp() */ #include /* close(), unlink(), rmdir(), symlink(), sleep() */ #include /* open() */ -static int captured_err_count = 0; -static char captured_err_message[256]; - -static char *CaptureErrorLogHook(ARG_UNUSED LoggingPrivContext *pctx, LogLevel level, const char *message) -{ - if (level == LOG_LEVEL_ERR) - { - captured_err_count++; - strlcpy(captured_err_message, message, sizeof(captured_err_message)); - } - return (char *) message; -} - /* The registry only hashes the expanded promise it is given (its promiser * here, as it has no constraints, comment or parent) and hands the * unexpanded one (org_pp) back to the WatcherEventFn, so minimal promises @@ -81,14 +67,24 @@ static bool WaitForQueuedEvent(int fd, int tenths) return false; } +#define MAX_RECORDED_EVENTS 8 + static int handled_event_count = 0; static const Promise *last_event_promise = NULL; static char last_event_promise_hash[CF_HOSTKEY_STRING_SIZE]; static WatcherCheckResult last_event_check = WATCHER_CHECK_NO_EVENT; +/* Promise hashes of the first MAX_RECORDED_EVENTS handled events */ +static char event_promise_hashes[MAX_RECORDED_EVENTS][CF_HOSTKEY_STRING_SIZE]; + static void RecordEvent(ARG_UNUSED EvalContext *ctx, const Promise *pp, const char *promise_hash, WatcherCheckResult check) { + if (handled_event_count < MAX_RECORDED_EVENTS) + { + strlcpy(event_promise_hashes[handled_event_count], promise_hash, + sizeof(event_promise_hashes[handled_event_count])); + } handled_event_count++; last_event_promise = pp; strlcpy(last_event_promise_hash, promise_hash, sizeof(last_event_promise_hash)); @@ -104,12 +100,33 @@ static void AssertLastEventFor(const Promise *pp) assert_string_equal(last_event_promise_hash, promise_hash); } +/* Returns how many of the recorded events were handled for the iteration `pp` */ +static int RecordedEventCountFor(const Promise *pp) +{ + char promise_hash[CF_HOSTKEY_STRING_SIZE]; + WatcherPromiseHash(pp, promise_hash, sizeof(promise_hash)); + + int count = 0; + for (int i = 0; i < MIN(handled_event_count, MAX_RECORDED_EVENTS); i++) + { + if (StringEqual(event_promise_hashes[i], promise_hash)) + { + count++; + } + } + return count; +} + static void ResetRecordedEvents(void) { handled_event_count = 0; last_event_promise = NULL; last_event_promise_hash[0] = '\0'; last_event_check = WATCHER_CHECK_NO_EVENT; + for (int i = 0; i < MAX_RECORDED_EVENTS; i++) + { + event_promise_hashes[i][0] = '\0'; + } } static void HandleEvents(int fd) @@ -445,6 +462,45 @@ static void test_event_watcher_state_not_kept_for_other_promise(void) assert_int_equal(rmdir(dir), 0); } +static void test_event_watcher_several_events(void) +{ + char dir_a[] = "/tmp/watcher_test.XXXXXX"; + char dir_b[] = "/tmp/watcher_test.XXXXXX"; + char path_a[PATH_MAX]; + char path_b[PATH_MAX]; + CreateTempFile(dir_a, path_a, sizeof(path_a)); + CreateTempFile(dir_b, path_b, sizeof(path_b)); + + WatcherRegistryInitialize(); + const Promise iteration_a = { .promiser = "a", .org_pp = &unexpanded_promise }; + const Promise iteration_b = { .promiser = "b", .org_pp = &unexpanded_promise }; + WatcherRegister(&iteration_a, FileDeletedOptions(path_a, 1)); + WatcherRegister(&iteration_b, FileDeletedOptions(path_b, 1)); + + /* Both deleted before the first pass of the watcher thread, so both + * events are queued before the wakeup, and handled together */ + assert_int_equal(unlink(path_a), 0); + assert_int_equal(unlink(path_b), 0); + + int fd = -1; + assert_true(EventWatcherInitialize(&fd)); + + ResetRecordedEvents(); + assert_true(WaitForQueuedEvent(fd, 50)); + HandleEvents(fd); + assert_int_equal(handled_event_count, 2); + assert_int_equal(RecordedEventCountFor(&iteration_a), 1); + assert_int_equal(RecordedEventCountFor(&iteration_b), 1); + assert_int_equal(last_event_check, WATCHER_CHECK_EVENT); + + /* Each fires once */ + assert_false(WaitForQueuedEvent(fd, 20)); + + EventWatcherFinalize(); + assert_int_equal(rmdir(dir_a), 0); + assert_int_equal(rmdir(dir_b), 0); +} + /* Sets up dir/loop, a symlink to itself, so that lstat() of dir/loop/file * fails with ELOOP (even as root), i.e. whether the file was deleted can't * be told. Unlinking the loop makes lstat() work again (ENOENT). */ @@ -462,75 +518,6 @@ static void TearDownSymlinkLoop(const char *dir, const char *loop) assert_int_equal(rmdir(dir), 0); } -static void test_check_file_deleted_error(void) -{ - char dir[] = "/tmp/watcher_test.XXXXXX"; - char loop[PATH_MAX]; - char path[PATH_MAX]; - SetUpSymlinkLoop(dir, loop, sizeof(loop), path, sizeof(path)); - - const LogLevel old_level = LogGetGlobalLevel(); - LogSetGlobalLevel(LOG_LEVEL_CRIT); - - Rlist *args = NULL; - RlistAppendScalar(&args, path); - void *state = FileWatcherStateNew(args); - RlistDestroy(args); - assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); - FileWatcherStateDestroy(state); - - LogSetGlobalLevel(old_level); - TearDownSymlinkLoop(dir, loop); -} - -static void test_check_file_deleted_logs_error_once(void) -{ - char dir[] = "/tmp/watcher_test.XXXXXX"; - char loop[PATH_MAX]; - char path[PATH_MAX]; - SetUpSymlinkLoop(dir, loop, sizeof(loop), path, sizeof(path)); - - captured_err_count = 0; - captured_err_message[0] = '\0'; - LoggingPrivContext log_ctx = { - .log_hook = CaptureErrorLogHook, - .force_hook_level = LOG_LEVEL_ERR, - }; - LoggingPrivSetContext(&log_ctx); - const LogLevel old_level = LogGetGlobalLevel(); - LogSetGlobalLevel(LOG_LEVEL_CRIT); - - /* The failure of the first check is logged */ - Rlist *args = NULL; - RlistAppendScalar(&args, path); - void *state = FileWatcherStateNew(args); - RlistDestroy(args); - assert_int_equal(captured_err_count, 0); - assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); - assert_int_equal(captured_err_count, 1); - assert_true(strstr(captured_err_message, "Unable to lstat") != NULL); - - /* The following failures are not logged again */ - for (int i = 0; i < 3; i++) - { - assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); - } - assert_int_equal(captured_err_count, 1); - - /* Once lstat() works again, the next failure is logged again */ - assert_int_equal(unlink(loop), 0); - assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_NO_EVENT); - assert_int_equal(symlink(loop, loop), 0); - assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); - assert_int_equal(captured_err_count, 2); - - FileWatcherStateDestroy(state); - - LogSetGlobalLevel(old_level); - LoggingPrivSetContext(NULL); - TearDownSymlinkLoop(dir, loop); -} - static void test_event_watcher_check_failure_reported_once(void) { char dir[] = "/tmp/watcher_test.XXXXXX"; @@ -619,8 +606,7 @@ int main() unit_test(test_event_watcher_state_kept_across_pause), unit_test(test_event_watcher_state_kept_identical_ignored), unit_test(test_event_watcher_state_not_kept_for_other_promise), - unit_test(test_check_file_deleted_error), - unit_test(test_check_file_deleted_logs_error_once), + unit_test(test_event_watcher_several_events), unit_test(test_event_watcher_check_failure_reported_once), unit_test(test_watcher_register_check_failure), };