From 76270a165d0557ac5c2ded7beda61c17cf7ae752 Mon Sep 17 00:00:00 2001 From: Victor Moene Date: Thu, 15 Oct 2026 11:12:38 +0200 Subject: [PATCH 1/6] Refactored file watcher to share code between file event types Ticket: ENT-14692 Signed-off-by: Victor Moene Co-Authored-By: Claude Opus 5.5 --- cf-reactor/file_watcher.c | 159 +++++++++++++++++++-------------- cf-reactor/file_watcher.h | 2 +- cf-reactor/reactor_transform.c | 2 +- tests/unit/watcher_test.c | 6 +- 4 files changed, 98 insertions(+), 71 deletions(-) diff --git a/cf-reactor/file_watcher.c b/cf-reactor/file_watcher.c index 8a82d06dff1..205ad14da52 100644 --- a/cf-reactor/file_watcher.c +++ b/cf-reactor/file_watcher.c @@ -30,114 +30,141 @@ #include #include -// =========== code for EVENT_FILE_DELETED event type =========== - -/* Minimum time between two logs of a persisting lstat() failure, since the - * file is checked on every poll */ -#define LSTAT_ERROR_LOG_INTERVAL_SECS 60 +/* Minimum time between two logs of a persisting failure to check the file, + * since the file is checked on every poll */ +#define CHECK_ERROR_LOG_INTERVAL_SECS 60 typedef struct { char *path; - time_t last_seen; /* Time the file was last observed present, 0 if absent */ - time_t lstat_error_logged; /* Time an lstat() failure was last logged, 0 if lstat() works */ + time_t last_checked; /* Time of the last successful check, 0 if none yet */ + time_t last_error_logged; /* Time the last error was logged, 0 if no errors */ + + /* What the last successful check observed */ + bool last_exists; } FileWatcherState; -/* Sets *deleted to whether the path of the watcher exists. Only - * ENOENT/ENOTDIR mean the path genuinely doesn't exist. Any other lstat() - * failure (e.g. EACCES, ESTALE) is a transient/permission error, in which case - * we can't tell whether the file was deleted, so false is returned and - * *deleted is left untouched. A persisting failure is logged at most once - * every LSTAT_ERROR_LOG_INTERVAL_SECS. - * - * Doesn't follow symlinks, i.e. a dangling symlink is not considered deleted */ -static bool FileDeleted(FileWatcherState *fws, bool *deleted) + +void *FileWatcherStateNew(const char *path) { - assert(fws != NULL); - assert(deleted != NULL); + FileWatcherState *fws = (FileWatcherState *) xcalloc(1, sizeof(FileWatcherState)); + fws->path = xstrdup(path); - struct stat sb; - if (lstat(fws->path, &sb) == 0) - { - *deleted = false; - fws->lstat_error_logged = 0; - return true; - } + /* Check once right away, as the reference for the next checks */ + FileWatcherCheckFileDeleted(fws); - if (errno == ENOENT || errno == ENOTDIR) + return fws; +} + +void FileWatcherStateDestroy(void *state) +{ + FileWatcherState *fws = state; + if (fws != NULL) { - *deleted = true; - fws->lstat_error_logged = 0; - return true; + free(fws->path); + free(fws); } +} + +// =========== code shared by the file event types =========== + +/* Logs the failure of operation op (with errno err) on the file of the + * watcher. A persisting failure is logged at most once every + * CHECK_ERROR_LOG_INTERVAL_SECS. */ +static void LogCheckError(FileWatcherState *fws, const char *op, int err) +{ + assert(fws != NULL); - const int lstat_errno = errno; const time_t now = time(NULL); - if (fws->lstat_error_logged == 0 || - now - fws->lstat_error_logged >= LSTAT_ERROR_LOG_INTERVAL_SECS) + if (fws->last_error_logged == 0 || + now - fws->last_error_logged >= CHECK_ERROR_LOG_INTERVAL_SECS) { - Log(LOG_LEVEL_ERR, "Unable to lstat '%s' while checking for file deletion: %s", - fws->path, GetErrorStrFromCode(lstat_errno)); - fws->lstat_error_logged = now; + Log(LOG_LEVEL_ERR, "Unable to %s '%s' while checking for file events: %s", + op, fws->path, GetErrorStrFromCode(err)); + fws->last_error_logged = now; } - return false; } -void *FileWatcherStateNew(const char *path) +/* Checks whether the file of the watcher exists, with lstat(). On success, + * returns true and sets *exists, as well as *sb if the file exists and sb is + * not NULL. Only ENOENT/ENOTDIR mean the file doesn't exist. On any other + * lstat() failure (e.g. EACCES, ESTALE), whether the file exists is unknown, + * so the failure is logged (see LogCheckError()) and false is returned. + * + * Doesn't follow symlinks, e.g. a dangling symlink exists */ +static bool FileLstat(FileWatcherState *fws, bool *exists, struct stat *sb) { - FileWatcherState *fws = (FileWatcherState *) xmalloc(sizeof(FileWatcherState)); + assert(fws != NULL); + assert(exists != NULL); - fws->path = xstrdup(path); - fws->lstat_error_logged = 0; + struct stat buf; + if (lstat(fws->path, (sb != NULL) ? sb : &buf) == 0) + { + *exists = true; + fws->last_error_logged = 0; + return true; + } - /* If the file's presence can't be determined, treat it as not yet seen */ - bool deleted; - fws->last_seen = (FileDeleted(fws, &deleted) && !deleted) ? time(NULL) : 0; + if (errno == ENOENT || errno == ENOTDIR) + { + *exists = false; + fws->last_error_logged = 0; + return true; + } - return (void *) fws; + LogCheckError(fws, "lstat", errno); + return false; } -WatcherCheckResult FileWatcherCheckFileDeleted(void *state) +/* Records a successful check, at which the file existed or not and the event + * (e.g. "deletion") happened or not, and returns its result. The first + * successful check is only the reference for the next ones, so it can't + * fire. */ +static WatcherCheckResult FileWatcherRecordObservation(FileWatcherState *fws, bool exists, + bool happened, const char *event) { - assert(state != NULL); - FileWatcherState *fws = state; - - bool deleted_now; - if (!FileDeleted(fws, &deleted_now)) - { - /* Unknown state, keep last_seen as is */ - return WATCHER_CHECK_ERROR; - } + assert(fws != NULL); + assert(event != NULL); const time_t now = time(NULL); - const bool deleted = (fws->last_seen != 0) && deleted_now; + happened = happened && (fws->last_checked != 0); - if (deleted) + if (happened) { - char last_seen_str[26]; - const char *last_seen_ts = cf_strtimestamp_local(fws->last_seen, last_seen_str); + char last_checked_str[26]; + const char *last_checked_ts = cf_strtimestamp_local(fws->last_checked, last_checked_str); char now_str[26]; const char *now_ts = cf_strtimestamp_local(now, now_str); Log(LOG_LEVEL_VERBOSE, - "Observed deletion of file '%s' at %s (last seen present at %s)", + "Observed %s of file '%s' at %s (last checked at %s)", + event, fws->path, (now_ts != NULL) ? now_ts : "unknown time", - (last_seen_ts != NULL) ? last_seen_ts : "unknown time"); + (last_checked_ts != NULL) ? last_checked_ts : "unknown time"); } - fws->last_seen = deleted_now ? 0 : now; - return deleted ? WATCHER_CHECK_EVENT : WATCHER_CHECK_NO_EVENT; + fws->last_checked = now; + fws->last_exists = exists; + return happened ? WATCHER_CHECK_EVENT : WATCHER_CHECK_NO_EVENT; } -void DestroyFileWatcherState(void *state) +// =========== code for the file_deleted event =========== + +WatcherCheckResult FileWatcherCheckFileDeleted(void *state) { + assert(state != NULL); FileWatcherState *fws = state; - if (fws != NULL) + + bool exists; + if (!FileLstat(fws, &exists, NULL)) { - free(fws->path); - free(fws); + /* Unknown state, keep the last observation as is */ + return WATCHER_CHECK_ERROR; } + + bool deleted = fws->last_exists && !exists; + return FileWatcherRecordObservation(fws, exists, deleted, "deletion"); } diff --git a/cf-reactor/file_watcher.h b/cf-reactor/file_watcher.h index f96ea8daae5..3cbf211bc6e 100644 --- a/cf-reactor/file_watcher.h +++ b/cf-reactor/file_watcher.h @@ -31,6 +31,6 @@ void *FileWatcherStateNew(const char *path); WatcherCheckResult FileWatcherCheckFileDeleted(void *state); -void DestroyFileWatcherState(void *state); +void FileWatcherStateDestroy(void *state); #endif diff --git a/cf-reactor/reactor_transform.c b/cf-reactor/reactor_transform.c index 27c725a46fb..8f5f2426073 100644 --- a/cf-reactor/reactor_transform.c +++ b/cf-reactor/reactor_transform.c @@ -167,7 +167,7 @@ static PromiseResult KeepEventsPromise(EvalContext *ctx, const Promise *pp) WatcherOptions opt = { .state = FileWatcherStateNew(path), .check_callback = FileWatcherCheckFileDeleted, - .destroy_state = DestroyFileWatcherState, + .destroy_state = FileWatcherStateDestroy, }; // register watcher diff --git a/tests/unit/watcher_test.c b/tests/unit/watcher_test.c index c8255195317..5eebaafde56 100644 --- a/tests/unit/watcher_test.c +++ b/tests/unit/watcher_test.c @@ -39,7 +39,7 @@ static WatcherOptions FileDeletedOptions(const char *path) WatcherOptions opt = { .state = FileWatcherStateNew(path), .check_callback = FileWatcherCheckFileDeleted, - .destroy_state = DestroyFileWatcherState, + .destroy_state = FileWatcherStateDestroy, }; return opt; } @@ -465,7 +465,7 @@ static void test_check_file_deleted_error(void) void *state = FileWatcherStateNew(path); assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); - DestroyFileWatcherState(state); + FileWatcherStateDestroy(state); LogSetGlobalLevel(old_level); TearDownSymlinkLoop(dir, loop); @@ -507,7 +507,7 @@ static void test_check_file_deleted_logs_error_once(void) assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); assert_int_equal(captured_err_count, 2); - DestroyFileWatcherState(state); + FileWatcherStateDestroy(state); LogSetGlobalLevel(old_level); LoggingPrivSetContext(NULL); From 36ba57171791230e38ff764c3d6d884d16a57d66 Mon Sep 17 00:00:00 2001 From: Victor Moene Date: Thu, 15 Oct 2026 11:13:00 +0200 Subject: [PATCH 2/6] Check watchers once at registration The reference check of a new watcher is now done by WatcherRegister(), without holding the registry lock as it may block. If it fails, the watcher is not registered and WatcherRegister() returns false, so that the events promise fails. Ticket: ENT-14692 Signed-off-by: Victor Moene Co-Authored-By: Claude Opus 5.5 --- cf-reactor/file_watcher.c | 3 -- cf-reactor/reactor_transform.c | 5 ++- cf-reactor/watcher.c | 61 ++++++++++++++++++++++------------ cf-reactor/watcher.h | 8 +++-- tests/unit/watcher_test.c | 42 ++++++++++++++++++++--- 5 files changed, 87 insertions(+), 32 deletions(-) diff --git a/cf-reactor/file_watcher.c b/cf-reactor/file_watcher.c index 205ad14da52..68c36d3c63e 100644 --- a/cf-reactor/file_watcher.c +++ b/cf-reactor/file_watcher.c @@ -50,9 +50,6 @@ void *FileWatcherStateNew(const char *path) FileWatcherState *fws = (FileWatcherState *) xcalloc(1, sizeof(FileWatcherState)); fws->path = xstrdup(path); - /* Check once right away, as the reference for the next checks */ - FileWatcherCheckFileDeleted(fws); - return fws; } diff --git a/cf-reactor/reactor_transform.c b/cf-reactor/reactor_transform.c index 8f5f2426073..5442b73e907 100644 --- a/cf-reactor/reactor_transform.c +++ b/cf-reactor/reactor_transform.c @@ -172,7 +172,10 @@ static PromiseResult KeepEventsPromise(EvalContext *ctx, const Promise *pp) // register watcher Log(LOG_LEVEL_INFO, "Registering a file_deleted watcher for events promise '%s', on file '%s'", pp->promiser, path); - WatcherRegister(pp, opt, 1); + if (!WatcherRegister(pp, opt, 1)) + { + return PROMISE_RESULT_FAIL; + } return PROMISE_RESULT_NOOP; } diff --git a/cf-reactor/watcher.c b/cf-reactor/watcher.c index dd2bc22436f..c94774604d5 100644 --- a/cf-reactor/watcher.c +++ b/cf-reactor/watcher.c @@ -119,7 +119,7 @@ void WatcherRegistryFinalize(void) } // Expects interval to be strictly greater than 0, otherwise the watcher thread will busy spin -void WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval) +bool WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval) { assert(pp != NULL); assert(pp->promiser != NULL); @@ -128,18 +128,48 @@ void WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval) assert(opt.state != NULL); assert(watchers != NULL); assert(interval > 0); - - ThreadLock(&watchers_mutex); + char promise_hash[CF_HOSTKEY_STRING_SIZE]; WatcherPromiseHash(pp, promise_hash, sizeof(promise_hash)); - Watcher *prev = MapGet(watchers, promise_hash); - if (prev != NULL) + ThreadLock(&watchers_mutex); + + if (MapHasKey(watchers, promise_hash)) { // ignore duplicates ThreadUnlock(&watchers_mutex); opt.destroy_state(opt.state); - return; + return true; + } + + // Take over the state of an identical watcher + Watcher *prev = (old_watchers != NULL) ? MapGet(old_watchers, promise_hash) : NULL; + const bool taken_over = (prev != NULL); + void *state = opt.state; + if (taken_over) + { + state = prev->state; + prev->state = NULL; /* so not destroyed with prev */ + } + + ThreadUnlock(&watchers_mutex); + + if (taken_over) + { + Log(LOG_LEVEL_DEBUG, "Reactor watcher '%s' takes over the state of an identical watcher", pp->promiser); + opt.destroy_state(opt.state); + } + else + { + /* Without holding watchers_mutex, as the check may block (e.g. + * lstat() on an unresponsive NFS mount). The watcher isn't registered + * yet, so the watcher thread can't check it concurrently. */ + if (opt.check_callback(state) == WATCHER_CHECK_ERROR) + { + /* No reference for the next checks, so not registered */ + opt.destroy_state(state); + return false; + } } Watcher *w = xmalloc(sizeof(Watcher)); @@ -151,24 +181,13 @@ void WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval) w->last_check = WATCHER_CHECK_NO_EVENT; w->check_callback = opt.check_callback; w->destroy_state = opt.destroy_state; + w->state = state; - // Take over the state of an identical watcher - prev = (old_watchers != NULL) ? MapGet(old_watchers, promise_hash) : NULL; - if (prev == NULL) - { - w->state = opt.state; - } - else - { - Log(LOG_LEVEL_DEBUG, "Reactor watcher '%s' takes over the state of an identical watcher", w->promiser); - w->state = prev->state; - prev->state = NULL; /* so not destroyed with prev */ - opt.destroy_state(opt.state); - } - + ThreadLock(&watchers_mutex); MapInsert(watchers, w->promise_hash, w); - ThreadUnlock(&watchers_mutex); + + return true; } static void WatcherDestroy(void *item) diff --git a/cf-reactor/watcher.h b/cf-reactor/watcher.h index 4f14efb14d3..b4472ebd7b9 100644 --- a/cf-reactor/watcher.h +++ b/cf-reactor/watcher.h @@ -86,7 +86,9 @@ void WatcherRegistryFinalize(void); * If a watcher of the previous policy (see EventWatcherPause()) was * registered for an identical expanded promise, the new watcher takes over * its state instead of opt.state, so that the events happening while paused - * are detected. + * are detected. Otherwise opt.state is checked once right away, as the + * reference for the next checks, so that only what happens from now on + * fires. If that check fails, the watcher is not registered. * * @param pp the expanded events promise. Its hash and its unexpanded * promise (pp->org_pp) are passed back to the WatcherEventFn on @@ -95,8 +97,10 @@ void WatcherRegistryFinalize(void); * @param opt what the watcher checks for, see WatcherOptions. opt.state is * owned by the registry. * @param interval interval between runs + * @return false if the check of opt.state failed (WATCHER_CHECK_ERROR), in + * which case the watcher is not registered, true otherwise. */ -void WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval); +bool WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval); bool EventWatcherInitialize(int *fd); void EventWatcherHandleEvents(EvalContext *ctx, WatcherEventFn on_event, int fd, fd_set *readfds); diff --git a/tests/unit/watcher_test.c b/tests/unit/watcher_test.c index 5eebaafde56..5c149cf0685 100644 --- a/tests/unit/watcher_test.c +++ b/tests/unit/watcher_test.c @@ -121,7 +121,7 @@ static void test_watcher_register_single(void) { WatcherRegistryInitialize(); - WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event"), 5); + assert_true(WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event"), 5)); WatcherRegistryFinalize(); } @@ -157,8 +157,8 @@ static void test_watcher_register_identical_ignored(void) .promiser = "test-event", .org_pp = &other_unexpanded_promise, }; - WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1); - WatcherRegister(&other_expanded_promise, FileDeletedOptions(path), 1); + assert_true(WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1)); + assert_true(WatcherRegister(&other_expanded_promise, FileDeletedOptions(path), 1)); int fd = -1; assert_true(EventWatcherInitialize(&fd)); @@ -488,8 +488,10 @@ static void test_check_file_deleted_logs_error_once(void) const LogLevel old_level = LogGetGlobalLevel(); LogSetGlobalLevel(LOG_LEVEL_CRIT); - /* The failure at registration is logged */ + /* The failure of the first check is logged */ void *state = FileWatcherStateNew(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); @@ -524,8 +526,11 @@ static void test_event_watcher_check_failure_reported_once(void) const LogLevel old_level = LogGetGlobalLevel(); LogSetGlobalLevel(LOG_LEVEL_CRIT); + /* Registered while lstat() works, failing from then on */ WatcherRegistryInitialize(); - WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1); + assert_int_equal(unlink(loop), 0); + assert_true(WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1)); + assert_int_equal(symlink(loop, loop), 0); int fd = -1; assert_true(EventWatcherInitialize(&fd)); @@ -556,6 +561,32 @@ static void test_event_watcher_check_failure_reported_once(void) TearDownSymlinkLoop(dir, loop); } +static void test_watcher_register_check_failure(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); + + /* Without a reference, the watcher is not registered (its state is + * destroyed by the registry, checked by valgrind/ASan) */ + WatcherRegistryInitialize(); + assert_false(WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1)); + + /* So the watcher thread doesn't check it, and doesn't report its failure */ + int fd = -1; + assert_true(EventWatcherInitialize(&fd)); + assert_false(WaitForQueuedEvent(fd, 20)); + + EventWatcherFinalize(); + + LogSetGlobalLevel(old_level); + TearDownSymlinkLoop(dir, loop); +} + int main() { PRINT_TEST_BANNER(); @@ -576,6 +607,7 @@ int main() unit_test(test_check_file_deleted_error), unit_test(test_check_file_deleted_logs_error_once), unit_test(test_event_watcher_check_failure_reported_once), + unit_test(test_watcher_register_check_failure), }; return run_tests(tests); From 6da19af0c2a44669b4391d41a850365aed0bb9cd Mon Sep 17 00:00:00 2001 From: Victor Moene Date: Thu, 15 Oct 2026 11:13:17 +0200 Subject: [PATCH 3/6] Moved the watcher interval into WatcherOptions WatcherRegister() now takes a pointer to the options, which include the interval between two checks. Ticket: ENT-14692 Signed-off-by: Victor Moene Co-Authored-By: Claude Opus 5.5 --- cf-reactor/reactor_transform.c | 3 +- cf-reactor/watcher.c | 26 ++++++++--------- cf-reactor/watcher.h | 16 +++++----- tests/unit/watcher_test.c | 53 +++++++++++++++++++--------------- 4 files changed, 53 insertions(+), 45 deletions(-) diff --git a/cf-reactor/reactor_transform.c b/cf-reactor/reactor_transform.c index 5442b73e907..dcf3ed13123 100644 --- a/cf-reactor/reactor_transform.c +++ b/cf-reactor/reactor_transform.c @@ -168,11 +168,12 @@ static PromiseResult KeepEventsPromise(EvalContext *ctx, const Promise *pp) .state = FileWatcherStateNew(path), .check_callback = FileWatcherCheckFileDeleted, .destroy_state = FileWatcherStateDestroy, + .interval = 1, }; // register watcher Log(LOG_LEVEL_INFO, "Registering a file_deleted watcher for events promise '%s', on file '%s'", pp->promiser, path); - if (!WatcherRegister(pp, opt, 1)) + if (!WatcherRegister(pp, &opt)) { return PROMISE_RESULT_FAIL; } diff --git a/cf-reactor/watcher.c b/cf-reactor/watcher.c index c94774604d5..87f516315a2 100644 --- a/cf-reactor/watcher.c +++ b/cf-reactor/watcher.c @@ -119,15 +119,15 @@ void WatcherRegistryFinalize(void) } // Expects interval to be strictly greater than 0, otherwise the watcher thread will busy spin -bool WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval) +bool WatcherRegister(const Promise *pp, WatcherOptions *opt) { assert(pp != NULL); assert(pp->promiser != NULL); - assert(opt.check_callback != NULL); - assert(opt.destroy_state != NULL); - assert(opt.state != NULL); + assert(opt != NULL); + assert(opt->check_callback != NULL); + assert(opt->destroy_state != NULL); + assert(opt->interval > 0); assert(watchers != NULL); - assert(interval > 0); char promise_hash[CF_HOSTKEY_STRING_SIZE]; WatcherPromiseHash(pp, promise_hash, sizeof(promise_hash)); @@ -138,14 +138,14 @@ bool WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval) { // ignore duplicates ThreadUnlock(&watchers_mutex); - opt.destroy_state(opt.state); + opt->destroy_state(opt->state); return true; } // Take over the state of an identical watcher Watcher *prev = (old_watchers != NULL) ? MapGet(old_watchers, promise_hash) : NULL; const bool taken_over = (prev != NULL); - void *state = opt.state; + void *state = opt->state; if (taken_over) { state = prev->state; @@ -157,17 +157,17 @@ bool WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval) if (taken_over) { Log(LOG_LEVEL_DEBUG, "Reactor watcher '%s' takes over the state of an identical watcher", pp->promiser); - opt.destroy_state(opt.state); + opt->destroy_state(opt->state); } else { /* Without holding watchers_mutex, as the check may block (e.g. * lstat() on an unresponsive NFS mount). The watcher isn't registered * yet, so the watcher thread can't check it concurrently. */ - if (opt.check_callback(state) == WATCHER_CHECK_ERROR) + if (opt->check_callback(state) == WATCHER_CHECK_ERROR) { /* No reference for the next checks, so not registered */ - opt.destroy_state(state); + opt->destroy_state(state); return false; } } @@ -176,11 +176,11 @@ bool WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval) w->promise_hash = xstrdup(promise_hash); w->promise = pp->org_pp; w->promiser = xstrdup(pp->promiser); - w->poll_interval_secs = interval; + w->poll_interval_secs = opt->interval; w->next_due = 0; /* due immediately on the watcher thread's first pass */ w->last_check = WATCHER_CHECK_NO_EVENT; - w->check_callback = opt.check_callback; - w->destroy_state = opt.destroy_state; + w->check_callback = opt->check_callback; + w->destroy_state = opt->destroy_state; w->state = state; ThreadLock(&watchers_mutex); diff --git a/cf-reactor/watcher.h b/cf-reactor/watcher.h index b4472ebd7b9..efe3c75fcfe 100644 --- a/cf-reactor/watcher.h +++ b/cf-reactor/watcher.h @@ -46,6 +46,7 @@ typedef struct WatcherCheckFn check_callback; /* whether the event happened, updating state */ WatcherStateDestroyFn destroy_state; void *state; /* owned by the registry once registered */ + time_t interval; /* seconds between two checks, > 0 */ } WatcherOptions; /** @@ -85,8 +86,8 @@ void WatcherRegistryFinalize(void); * * If a watcher of the previous policy (see EventWatcherPause()) was * registered for an identical expanded promise, the new watcher takes over - * its state instead of opt.state, so that the events happening while paused - * are detected. Otherwise opt.state is checked once right away, as the + * its state instead of opt->state, so that the events happening while paused + * are detected. Otherwise opt->state is checked once right away, as the * reference for the next checks, so that only what happens from now on * fires. If that check fails, the watcher is not registered. * @@ -94,13 +95,14 @@ void WatcherRegistryFinalize(void); * promise (pp->org_pp) are passed back to the WatcherEventFn on * event. The unexpanded promise is not owned, so the policy * holding it must not be destroyed before EventWatcherPause(). - * @param opt what the watcher checks for, see WatcherOptions. opt.state is - * owned by the registry. - * @param interval interval between runs - * @return false if the check of opt.state failed (WATCHER_CHECK_ERROR), in + * @param opt what the watcher checks for, see WatcherOptions. opt->state is + * owned by the registry, which destroys it right away if not + * used, so it must not be used by the caller afterwards. + * opt->interval must be strictly greater than 0. + * @return false if the check of opt->state failed (WATCHER_CHECK_ERROR), in * which case the watcher is not registered, true otherwise. */ -bool WatcherRegister(const Promise *pp, WatcherOptions opt, time_t interval); +bool WatcherRegister(const Promise *pp, WatcherOptions *opt); bool EventWatcherInitialize(int *fd); void EventWatcherHandleEvents(EvalContext *ctx, WatcherEventFn on_event, int fd, fd_set *readfds); diff --git a/tests/unit/watcher_test.c b/tests/unit/watcher_test.c index 5c149cf0685..c280c485b77 100644 --- a/tests/unit/watcher_test.c +++ b/tests/unit/watcher_test.c @@ -34,14 +34,19 @@ static const Promise expanded_promise = { .org_pp = &unexpanded_promise, }; -static WatcherOptions FileDeletedOptions(const char *path) +/* Returns the options of a file_deleted watcher, valid until the next call + * (WatcherRegister() copies them) */ +static WatcherOptions *FileDeletedOptions(const char *path, time_t interval) { - WatcherOptions opt = { + static WatcherOptions opt; + + opt = (WatcherOptions) { .state = FileWatcherStateNew(path), .check_callback = FileWatcherCheckFileDeleted, .destroy_state = FileWatcherStateDestroy, + .interval = interval, }; - return opt; + return &opt; } /* Creates the file 'file' in a new temporary directory dir */ @@ -121,7 +126,7 @@ static void test_watcher_register_single(void) { WatcherRegistryInitialize(); - assert_true(WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event"), 5)); + assert_true(WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event", 5))); WatcherRegistryFinalize(); } @@ -134,8 +139,8 @@ static void test_watcher_register_iterations(void) * different watchers */ const Promise iteration_a = { .promiser = "/a", .org_pp = &unexpanded_promise }; const Promise iteration_b = { .promiser = "/b", .org_pp = &unexpanded_promise }; - WatcherRegister(&iteration_a, FileDeletedOptions("/nonexistent/a"), 5); - WatcherRegister(&iteration_b, FileDeletedOptions("/nonexistent/b"), 5); + WatcherRegister(&iteration_a, FileDeletedOptions("/nonexistent/a", 5)); + WatcherRegister(&iteration_b, FileDeletedOptions("/nonexistent/b", 5)); WatcherRegistryFinalize(); } @@ -157,8 +162,8 @@ static void test_watcher_register_identical_ignored(void) .promiser = "test-event", .org_pp = &other_unexpanded_promise, }; - assert_true(WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1)); - assert_true(WatcherRegister(&other_expanded_promise, FileDeletedOptions(path), 1)); + assert_true(WatcherRegister(&expanded_promise, FileDeletedOptions(path, 1))); + assert_true(WatcherRegister(&other_expanded_promise, FileDeletedOptions(path, 1))); int fd = -1; assert_true(EventWatcherInitialize(&fd)); @@ -185,18 +190,18 @@ static void test_watcher_register_again_after_pause(void) * registered again for the new policy, also without the event watcher * being initialized */ ResetRecordedEvents(); - WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event"), 5); + WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event", 5)); EventWatcherPause(NULL, RecordEvent); - WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event"), 5); + WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event", 5)); EventWatcherResume(); /* Paused twice in a row (e.g. a failed policy reload in between), the * watchers set aside the first time are replaced by those registered * since, without leaking (checked by valgrind/ASan) */ EventWatcherPause(NULL, RecordEvent); - WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event"), 5); + WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event", 5)); EventWatcherPause(NULL, RecordEvent); - WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event"), 5); + WatcherRegister(&expanded_promise, FileDeletedOptions("/nonexistent/test-event", 5)); EventWatcherResume(); assert_int_equal(handled_event_count, 0); @@ -238,7 +243,7 @@ static void test_event_watcher_pause_handles_queued_events(void) CreateTempFile(dir, path, sizeof(path)); WatcherRegistryInitialize(); - WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1); + WatcherRegister(&expanded_promise, FileDeletedOptions(path, 1)); int fd = -1; assert_true(EventWatcherInitialize(&fd)); @@ -276,7 +281,7 @@ static void test_event_watcher_idle_woken_by_wake_up(void) sleep(1); /* Registering a watcher doesn't wake it up, EventWatcherWakeUp() must */ - WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1); + WatcherRegister(&expanded_promise, FileDeletedOptions(path, 1)); assert_int_equal(unlink(path), 0); assert_false(WaitForQueuedEvent(fd, 20)); EventWatcherWakeUp(); @@ -304,7 +309,7 @@ static void test_event_watcher_idle_woken_by_resume(void) /* Registering doesn't wake the watcher thread up, resuming must */ EventWatcherPause(NULL, RecordEvent); - WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1); + WatcherRegister(&expanded_promise, FileDeletedOptions(path, 1)); assert_int_equal(unlink(path), 0); assert_false(WaitForQueuedEvent(fd, 20)); EventWatcherResume(); @@ -326,7 +331,7 @@ static void test_event_watcher_state_kept_across_pause(void) CreateTempFile(dir, path, sizeof(path)); WatcherRegistryInitialize(); - WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1); + WatcherRegister(&expanded_promise, FileDeletedOptions(path, 1)); int fd = -1; assert_true(EventWatcherInitialize(&fd)); @@ -346,7 +351,7 @@ static void test_event_watcher_state_kept_across_pause(void) .promiser = "test-event", .org_pp = &new_unexpanded_promise, }; - WatcherRegister(&new_expanded_promise, FileDeletedOptions(path), 1); + WatcherRegister(&new_expanded_promise, FileDeletedOptions(path, 1)); EventWatcherResume(); assert_true(WaitForQueuedEvent(fd, 50)); @@ -367,7 +372,7 @@ static void test_event_watcher_state_kept_identical_ignored(void) CreateTempFile(dir, path, sizeof(path)); WatcherRegistryInitialize(); - WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1); + WatcherRegister(&expanded_promise, FileDeletedOptions(path, 1)); int fd = -1; assert_true(EventWatcherInitialize(&fd)); @@ -389,8 +394,8 @@ static void test_event_watcher_state_kept_identical_ignored(void) .promiser = "test-event", .org_pp = &other_unexpanded_promise, }; - WatcherRegister(&new_expanded_promise, FileDeletedOptions(path), 1); - WatcherRegister(&other_expanded_promise, FileDeletedOptions(path), 1); + WatcherRegister(&new_expanded_promise, FileDeletedOptions(path, 1)); + WatcherRegister(&other_expanded_promise, FileDeletedOptions(path, 1)); EventWatcherResume(); assert_true(WaitForQueuedEvent(fd, 50)); @@ -410,7 +415,7 @@ static void test_event_watcher_state_not_kept_for_other_promise(void) CreateTempFile(dir, path, sizeof(path)); WatcherRegistryInitialize(); - WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1); + WatcherRegister(&expanded_promise, FileDeletedOptions(path, 1)); int fd = -1; assert_true(EventWatcherInitialize(&fd)); @@ -425,7 +430,7 @@ static void test_event_watcher_state_not_kept_for_other_promise(void) .promiser = "other-event", .org_pp = &unexpanded_promise, }; - WatcherRegister(&other_expanded_promise, FileDeletedOptions(path), 1); + WatcherRegister(&other_expanded_promise, FileDeletedOptions(path, 1)); EventWatcherResume(); /* The watcher thread checks at least every second */ @@ -529,7 +534,7 @@ static void test_event_watcher_check_failure_reported_once(void) /* Registered while lstat() works, failing from then on */ WatcherRegistryInitialize(); assert_int_equal(unlink(loop), 0); - assert_true(WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1)); + assert_true(WatcherRegister(&expanded_promise, FileDeletedOptions(path, 1))); assert_int_equal(symlink(loop, loop), 0); int fd = -1; @@ -574,7 +579,7 @@ static void test_watcher_register_check_failure(void) /* Without a reference, the watcher is not registered (its state is * destroyed by the registry, checked by valgrind/ASan) */ WatcherRegistryInitialize(); - assert_false(WatcherRegister(&expanded_promise, FileDeletedOptions(path), 1)); + assert_false(WatcherRegister(&expanded_promise, FileDeletedOptions(path, 1))); /* So the watcher thread doesn't check it, and doesn't report its failure */ int fd = -1; From c967ff548ebea455b3c95fe8426e9e0960c72af1 Mon Sep 17 00:00:00 2001 From: Victor Moene Date: Thu, 15 Oct 2026 11:13:21 +0200 Subject: [PATCH 4/6] Reduced cf-reactor logging of watcher registration and firing Ticket: ENT-14692 Signed-off-by: Victor Moene Co-Authored-By: Claude Opus 5.5 --- cf-reactor/reactor_transform.c | 2 -- cf-reactor/watcher.c | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/cf-reactor/reactor_transform.c b/cf-reactor/reactor_transform.c index dcf3ed13123..5d7611c73e6 100644 --- a/cf-reactor/reactor_transform.c +++ b/cf-reactor/reactor_transform.c @@ -171,8 +171,6 @@ static PromiseResult KeepEventsPromise(EvalContext *ctx, const Promise *pp) .interval = 1, }; - // register watcher - Log(LOG_LEVEL_INFO, "Registering a file_deleted watcher for events promise '%s', on file '%s'", pp->promiser, path); if (!WatcherRegister(pp, &opt)) { return PROMISE_RESULT_FAIL; diff --git a/cf-reactor/watcher.c b/cf-reactor/watcher.c index 87f516315a2..c72bd8ffb9c 100644 --- a/cf-reactor/watcher.c +++ b/cf-reactor/watcher.c @@ -323,7 +323,7 @@ static void HandleQueuedEvents(EvalContext *ctx, WatcherEventFn on_event) } else { - Log(LOG_LEVEL_NOTICE, "Reactor watcher '%s' fired", w->promiser); + Log(LOG_LEVEL_VERBOSE, "Reactor watcher '%s' fired", w->promiser); } on_event(ctx, w->promise, w->promise_hash, event->check); free(event); From 62724bef4610cad99dad91eeac0e6d2662515bd9 Mon Sep 17 00:00:00 2001 From: Victor Moene Date: Thu, 15 Oct 2026 11:13:32 +0200 Subject: [PATCH 5/6] Added interval attribute to reactor events promises It sets the seconds between two checks for the event, 1 by default. The parser only checks it if it doesn't depend on variables or body arguments, so it is checked again once expanded, and the events promise fails if it is invalid. Ticket: ENT-14692 Signed-off-by: Victor Moene Co-Authored-By: Claude Opus 5.5 --- cf-reactor/reactor_transform.c | 52 +++++++++++++++++++++++++++++++++- libpromises/mod_reactor.c | 1 + libpromises/mod_reactor.h | 3 ++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/cf-reactor/reactor_transform.c b/cf-reactor/reactor_transform.c index 5d7611c73e6..3bfe7cfe399 100644 --- a/cf-reactor/reactor_transform.c +++ b/cf-reactor/reactor_transform.c @@ -40,6 +40,9 @@ #include // Initialize/FinalizeCustomPromises() #include // SetReferenceTime() #include // UpdateTimeClasses() +#include // REACTOR_INTERVAL_RANGE +#include // CheckConstraintTypeMatch() +#include /* Promise types evaluated within `bundle reactor NAME { ... }`. */ static const char *const REACTOR_TYPESEQUENCE[] = @@ -134,6 +137,47 @@ static const Rlist *GetThenBundles(const Promise *pp) return bundles; } +#define DEFAULT_INTERVAL 1 + +/* Sets *interval to the seconds between two checks for the event of the + * (expanded) events promise. The parser and cf-promises only check it if it + * doesn't depend on variables or body arguments, so it is checked again here + * the same way: logs an error and returns false if it is invalid. */ +static bool EventsPromiseGetInterval(const Promise *pp, time_t *interval) +{ + assert(pp != NULL); + assert(interval != NULL); + + const Constraint *interval_con = PromiseGetConstraint(pp, "interval"); + if (interval_con == NULL) + { + *interval = DEFAULT_INTERVAL; + return true; + } + + if (interval_con->rval.type != RVAL_TYPE_SCALAR) + { + Log(LOG_LEVEL_ERR, + "Reactor events promise '%s' has an invalid interval, expected a number of seconds", + pp->promiser); + return false; + } + + const char *value = RvalScalarValue(interval_con->rval); + const SyntaxTypeMatch err = CheckConstraintTypeMatch( + interval_con->lval, interval_con->rval, CF_DATA_TYPE_INT, REACTOR_INTERVAL_RANGE, 0); + if (err != SYNTAX_TYPE_MATCH_OK) + { + Log(LOG_LEVEL_ERR, + "Reactor events promise '%s' has an invalid interval '%s': %s", + pp->promiser, value, SyntaxTypeMatchToString(err)); + return false; + } + + *interval = (time_t) IntFromString(value); + return true; +} + // Temporary limitations: // - 'when' body: only a single constraint, 'file_deleted', is supported (no OR-ing of constraints yet) // TODO: support multiple 'when' constraints @@ -164,11 +208,17 @@ static PromiseResult KeepEventsPromise(EvalContext *ctx, const Promise *pp) } } + time_t interval; + if (!EventsPromiseGetInterval(pp, &interval)) + { + return PROMISE_RESULT_FAIL; + } + WatcherOptions opt = { .state = FileWatcherStateNew(path), .check_callback = FileWatcherCheckFileDeleted, .destroy_state = FileWatcherStateDestroy, - .interval = 1, + .interval = interval, }; if (!WatcherRegister(pp, &opt)) diff --git a/libpromises/mod_reactor.c b/libpromises/mod_reactor.c index 113a3e9a1a8..34c1b67b042 100644 --- a/libpromises/mod_reactor.c +++ b/libpromises/mod_reactor.c @@ -33,6 +33,7 @@ static const ConstraintSyntax when_constraints[] = /* Row models */ ConstraintSyntaxNewString("file_deleted", CF_ANYSTRING, "File to react for on deletion", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewInt("interval", REACTOR_INTERVAL_RANGE, "Seconds between two checks for the event", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewNull() }; diff --git a/libpromises/mod_reactor.h b/libpromises/mod_reactor.h index 72842f35045..2cc79db1135 100644 --- a/libpromises/mod_reactor.h +++ b/libpromises/mod_reactor.h @@ -27,6 +27,9 @@ #include +/* Range of the 'interval' attribute of the 'when' body of events promises */ +#define REACTOR_INTERVAL_RANGE "1,99999999999" + extern const PromiseTypeSyntax CF_REACTOR_PROMISE_TYPES[]; #endif From 29fb11ce7e6cafd083cf19460a677faeea7e27f1 Mon Sep 17 00:00:00 2001 From: Victor Moene Date: Thu, 15 Oct 2026 11:13:50 +0200 Subject: [PATCH 6/6] Added modular interface for event types Moved from a hardcoded and single `file_deleted` event type, to a table with all the event types and their necessary code, so that cf-reactor can easily be extended with new event types. Ticket: ENT-14692 Signed-off-by: Victor Moene Co-Authored-By: Claude Opus 5.5 --- cf-reactor/file_watcher.c | 5 +- cf-reactor/file_watcher.h | 2 +- cf-reactor/reactor_transform.c | 187 +++++++++++++++++++++++++++++---- cf-reactor/watcher.h | 1 + libpromises/mod_reactor.c | 2 +- libpromises/mod_reactor.h | 2 + tests/unit/watcher_test.c | 16 ++- 7 files changed, 187 insertions(+), 28 deletions(-) diff --git a/cf-reactor/file_watcher.c b/cf-reactor/file_watcher.c index 68c36d3c63e..869df2d7cab 100644 --- a/cf-reactor/file_watcher.c +++ b/cf-reactor/file_watcher.c @@ -45,8 +45,11 @@ typedef struct } FileWatcherState; -void *FileWatcherStateNew(const char *path) +void *FileWatcherStateNew(const Rlist *args) { + assert(args != NULL); + const char *path = RlistScalarValue(args); + FileWatcherState *fws = (FileWatcherState *) xcalloc(1, sizeof(FileWatcherState)); fws->path = xstrdup(path); diff --git a/cf-reactor/file_watcher.h b/cf-reactor/file_watcher.h index 3cbf211bc6e..2ab0eaa5cd5 100644 --- a/cf-reactor/file_watcher.h +++ b/cf-reactor/file_watcher.h @@ -29,7 +29,7 @@ #include /* WatcherCheckResult */ -void *FileWatcherStateNew(const char *path); +void *FileWatcherStateNew(const Rlist *args); WatcherCheckResult FileWatcherCheckFileDeleted(void *state); void FileWatcherStateDestroy(void *state); diff --git a/cf-reactor/reactor_transform.c b/cf-reactor/reactor_transform.c index 3bfe7cfe399..cc0119bf70c 100644 --- a/cf-reactor/reactor_transform.c +++ b/cf-reactor/reactor_transform.c @@ -33,15 +33,15 @@ #include #include #include -#include #include // ScheduleAgentOperations() #include // GetClassContextAttributes() #include // ConnCache_Init(), ConnCache_Destroy() #include // Initialize/FinalizeCustomPromises() #include // SetReferenceTime() #include // UpdateTimeClasses() -#include // REACTOR_INTERVAL_RANGE -#include // CheckConstraintTypeMatch() +#include // REACTOR_INTERVAL_RANGE, REACTOR_FILE_DELETED +#include // CheckConstraintTypeMatch(), PromiseTypeSyntaxGet(), BodySyntaxGetConstraintSyntax() +#include #include /* Promise types evaluated within `bundle reactor NAME { ... }`. */ @@ -137,6 +137,87 @@ static const Rlist *GetThenBundles(const Promise *pp) return bundles; } +/*****************************************************************************/ + +/* A constraint of the 'when' body passed to the watcher of an event, with the + * type of value it accepts */ +typedef struct +{ + const char *name; + RvalType type; +} EventArg; + +typedef struct +{ + const char *name; + RvalType type; /* type of value the 'name' constraint accepts */ + const EventArg *extra_args; /* terminated by an EventArg with a NULL name */ + WatcherStateNewFn new_state; + WatcherStateDestroyFn destroy_state; + WatcherCheckFn check; +} EventType; + +static const EventArg NO_EXTRA_ARGS[] = +{ + { NULL, RVAL_TYPE_NOPROMISEE } +}; + +#define EventTypeNew(name, type, args, new_state, destroy_state, check) \ + { name, type, args, new_state, destroy_state, check } +#define EventTypeNewNull() EventTypeNew(NULL, RVAL_TYPE_NOPROMISEE, NULL, NULL, NULL, NULL) + +/* A file event, about the file at the path of its constraint */ +#define FileEventTypeNew(name, check) \ + EventTypeNew(name, RVAL_TYPE_SCALAR, NO_EXTRA_ARGS, FileWatcherStateNew, FileWatcherStateDestroy, check) + +static const EventType EVENT_TABLE[] = +{ + FileEventTypeNew(REACTOR_FILE_DELETED, FileWatcherCheckFileDeleted), + EventTypeNewNull() +}; + +/* Appends the value of constraint con of the events promise to args if it is + * of the given type, otherwise logs an error and returns false. */ +static bool EventArgAppend(const Promise *pp, const Constraint *con, RvalType type, Rlist **args) +{ + assert(pp != NULL); + assert(con != NULL); + assert(args != NULL); + + if (con->rval.type != type) + { + Log(LOG_LEVEL_ERR, + "Reactor events promise '%s' has an invalid '%s', expected a %s value, got a %s value", + pp->promiser, con->lval, RvalTypeToString(type), RvalTypeToString(con->rval.type)); + return false; + } + RlistAppend(args, con->rval.item, con->rval.type); + return true; +} + +static const EventType *EventsPromiseGetFirstEvent(const Promise *pp, Rlist **args) +{ + assert(pp != NULL); + assert(args != NULL); + + for (size_t i = 0; EVENT_TABLE[i].name != NULL; i++) + { + const Constraint *con = PromiseGetConstraint(pp, EVENT_TABLE[i].name); + if (con != NULL) + { + if (!EventArgAppend(pp, con, EVENT_TABLE[i].type, args)) + { + return NULL; + } + return &EVENT_TABLE[i]; + } + } + Log(LOG_LEVEL_ERR, + "Reactor events promise '%s' does not specify an event to watch in its 'when' body", + pp->promiser); + return NULL; +} + #define DEFAULT_INTERVAL 1 /* Sets *interval to the seconds between two checks for the event of the @@ -178,22 +259,91 @@ static bool EventsPromiseGetInterval(const Promise *pp, time_t *interval) return true; } -// Temporary limitations: -// - 'when' body: only a single constraint, 'file_deleted', is supported (no OR-ing of constraints yet) -// TODO: support multiple 'when' constraints -static PromiseResult KeepEventsPromise(EvalContext *ctx, const Promise *pp) +static bool ResolveWhenBody(const Promise *pp, WatcherOptions *opt) { assert(pp != NULL); + assert(opt != NULL); + // Get the event and its arg + Rlist *args = NULL; + const EventType *event = EventsPromiseGetFirstEvent(pp, &args); - const char *path = PromiseGetConstraintAsRval(pp, "file_deleted", RVAL_TYPE_SCALAR); - if (path == NULL) + if (event == NULL) { - Log(LOG_LEVEL_WARNING, - "Reactor events promise '%s' must specify exactly one file to watch in its 'when' body, ignoring", - pp->promiser); - return PROMISE_RESULT_FAIL; + RlistDestroy(args); + return false; + } + + // If the event has additional args, add them to the list of args + for (const EventArg *extra_arg = event->extra_args; extra_arg->name != NULL; extra_arg++) + { + const Constraint *con = PromiseGetConstraint(pp, extra_arg->name); + if (con == NULL) + { + Log(LOG_LEVEL_ERR, + "Constraint '%s' is required for event '%s' in events promise '%s'", + extra_arg->name, event->name, pp->promiser); + RlistDestroy(args); + return false; + } + if (!EventArgAppend(pp, con, extra_arg->type, &args)) + { + RlistDestroy(args); + return false; + } } + // Check that other constraints are not defined in the promise, that + // do not belong here + const PromiseTypeSyntax *events_syntax = PromiseTypeSyntaxGet("reactor", "events"); + assert(events_syntax != NULL); + const ConstraintSyntax *when_syntax = + PromiseTypeSyntaxGetConstraintSyntax(events_syntax, "when")->range.body_type_syntax->constraints; + + for (size_t i = 0; i < SeqLength(pp->conlist); i++) + { + const Constraint *cp = SeqAt(pp->conlist, i); + if (BodySyntaxGetConstraintSyntax(when_syntax, cp->lval) == NULL || /* not a 'when' constraint */ + StringEqual(cp->lval, event->name) || + StringEqual(cp->lval, "interval") || + StringEqual(cp->lval, "meta")) /* also a promise attribute */ + { + continue; + } + + bool own_arg = false; + for (const EventArg *extra_arg = event->extra_args; extra_arg->name != NULL; extra_arg++) + { + own_arg = own_arg || StringEqual(extra_arg->name, cp->lval); + } + if (!own_arg) + { + Log(LOG_LEVEL_ERR, + "Reactor events promise '%s' cannot have both '%s' and '%s' in its 'when' body, only one event can be watched per promise", + pp->promiser, event->name, cp->lval); + RlistDestroy(args); + return false; + } + } + + if (!EventsPromiseGetInterval(pp, &opt->interval)) + { + RlistDestroy(args); + return false; + } + + opt->state = event->new_state(args); + opt->check_callback = event->check; + opt->destroy_state = event->destroy_state; + RlistDestroy(args); + return true; +} + +/*****************************************************************************/ + +static PromiseResult KeepEventsPromise(EvalContext *ctx, const Promise *pp) +{ + assert(pp != NULL); + const Rlist *then_bundles = GetThenBundles(pp); if (then_bundles == NULL) { @@ -208,19 +358,12 @@ static PromiseResult KeepEventsPromise(EvalContext *ctx, const Promise *pp) } } - time_t interval; - if (!EventsPromiseGetInterval(pp, &interval)) + WatcherOptions opt; + if (!ResolveWhenBody(pp, &opt)) { return PROMISE_RESULT_FAIL; } - WatcherOptions opt = { - .state = FileWatcherStateNew(path), - .check_callback = FileWatcherCheckFileDeleted, - .destroy_state = FileWatcherStateDestroy, - .interval = interval, - }; - if (!WatcherRegister(pp, &opt)) { return PROMISE_RESULT_FAIL; diff --git a/cf-reactor/watcher.h b/cf-reactor/watcher.h index efe3c75fcfe..60134096091 100644 --- a/cf-reactor/watcher.h +++ b/cf-reactor/watcher.h @@ -35,6 +35,7 @@ typedef enum WATCHER_CHECK_ERROR, /* unable to tell whether the event happened */ } WatcherCheckResult; +typedef void *(*WatcherStateNewFn)(const Rlist *args); typedef WatcherCheckResult (*WatcherCheckFn)(void *state); typedef void (*WatcherStateDestroyFn)(void *state); diff --git a/libpromises/mod_reactor.c b/libpromises/mod_reactor.c index 34c1b67b042..520d28f03ef 100644 --- a/libpromises/mod_reactor.c +++ b/libpromises/mod_reactor.c @@ -32,7 +32,7 @@ static const ConstraintSyntax when_constraints[] = CONSTRAINT_SYNTAX_GLOBAL, /* Row models */ - ConstraintSyntaxNewString("file_deleted", CF_ANYSTRING, "File to react for on deletion", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewString(REACTOR_FILE_DELETED, CF_ANYSTRING, "File to react for on deletion", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewInt("interval", REACTOR_INTERVAL_RANGE, "Seconds between two checks for the event", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewNull() }; diff --git a/libpromises/mod_reactor.h b/libpromises/mod_reactor.h index 2cc79db1135..d84ed22643c 100644 --- a/libpromises/mod_reactor.h +++ b/libpromises/mod_reactor.h @@ -27,6 +27,8 @@ #include +#define REACTOR_FILE_DELETED "file_deleted" + /* Range of the 'interval' attribute of the 'when' body of events promises */ #define REACTOR_INTERVAL_RANGE "1,99999999999" diff --git a/tests/unit/watcher_test.c b/tests/unit/watcher_test.c index c280c485b77..3573c91d709 100644 --- a/tests/unit/watcher_test.c +++ b/tests/unit/watcher_test.c @@ -3,6 +3,7 @@ #include #include /* FileWatcherStateNew(), FileWatcherCheckFileDeleted() */ #include /* Promise */ +#include /* RlistAppendScalar() */ #include /* LoggingPrivContext, LoggingPrivSetContext() */ #include /* strstr() */ @@ -40,12 +41,15 @@ static WatcherOptions *FileDeletedOptions(const char *path, time_t interval) { static WatcherOptions opt; + Rlist *args = NULL; + RlistAppendScalar(&args, path); opt = (WatcherOptions) { - .state = FileWatcherStateNew(path), + .state = FileWatcherStateNew(args), .check_callback = FileWatcherCheckFileDeleted, .destroy_state = FileWatcherStateDestroy, .interval = interval, }; + RlistDestroy(args); return &opt; } @@ -468,7 +472,10 @@ static void test_check_file_deleted_error(void) const LogLevel old_level = LogGetGlobalLevel(); LogSetGlobalLevel(LOG_LEVEL_CRIT); - void *state = FileWatcherStateNew(path); + Rlist *args = NULL; + RlistAppendScalar(&args, path); + void *state = FileWatcherStateNew(args); + RlistDestroy(args); assert_int_equal(FileWatcherCheckFileDeleted(state), WATCHER_CHECK_ERROR); FileWatcherStateDestroy(state); @@ -494,7 +501,10 @@ static void test_check_file_deleted_logs_error_once(void) LogSetGlobalLevel(LOG_LEVEL_CRIT); /* The failure of the first check is logged */ - void *state = FileWatcherStateNew(path); + 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);