From 68e5ca0919b91e13238c48e8f1f2cd2ec4de1d0b Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Fri, 3 Jul 2026 21:47:36 -0500 Subject: [PATCH 1/9] feat(storage): fix CFE-90 - remount NFS with correct options and update fstab entries CFE-90: NFS mount options not updating on remount. Root causes fixed: 1. FileSystemMountedCorrectly() now validates mount options against promised options 2. VerifyMountPromise() performs direct 'mount -o remount' when options differ instead of relying on 'mount -a' which skips already-mounted filesystems 3. VerifyInFstab() updates existing fstab entries when options differ, instead of only appending new entries Added helper functions GetFstabEntryOptions() and ReplaceFstabEntry() in nfs.c for parsing and updating fstab option fields. Also added StringIsSHA1Hex() stub in server_tls.c to fix community build linking against enterprise code paths. Files changed: - cf-agent/nfs.c: +97 lines (VerifyInFstab fix, GetFstabEntryOptions, ReplaceFstabEntry) - cf-agent/verify_storage.c: +65 lines (FileSystemMountedCorrectly check, remount logic) - cf-serverd/server_tls.c: +10 lines (StringIsSHA1Hex stub) --- cf-agent/nfs.c | 97 +++++++++++++++++++++++++++++++++++++++ cf-agent/verify_storage.c | 65 ++++++++++++++++++++++++-- cf-serverd/server_tls.c | 10 ++++ 3 files changed, 168 insertions(+), 4 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index 4ba7d095c7..6b7d329969 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -53,6 +53,8 @@ static void GetHostAndSource(const char *buf, char *host, char *source); static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, char *options); static bool MatchFSInFstab(char *match); static void DeleteThisItem(Item **liststart, Item *entry); +static char *GetFstabEntryOptions(char *mountpt); +static void ReplaceFstabEntry(Item **liststart, char *mountpt, char *new_entry); static const char *const VMOUNTCOMM[] = { @@ -534,6 +536,7 @@ int VerifyInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promi if (!MatchFSInFstab(mountpt)) { + /* CFE-90: Entry not in fstab - add it */ AppendItem(&FSTABLIST, fstab, NULL); FSTAB_EDITS++; cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, "Adding file system entry '%s' to '%s'", fstab, @@ -541,6 +544,22 @@ int VerifyInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promi *result = PromiseResultUpdate(*result, PROMISE_RESULT_CHANGE); changes += 1; } + else + { + /* CFE-90: Entry exists - check if options differ and update if needed */ + char *existing_opts = GetFstabEntryOptions(mountpt); + if (existing_opts != NULL && strcmp(existing_opts, opts) != 0) + { + /* Replace the entire fstab entry with the corrected options */ + ReplaceFstabEntry(&FSTABLIST, mountpt, fstab); + FSTAB_EDITS++; + cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, "Updating file system entry for '%s' in '%s' (options: '%s' -> '%s')", + mountpt, VFSTAB[VSYSTEMHARDCLASS], existing_opts, opts); + *result = PromiseResultUpdate(*result, PROMISE_RESULT_CHANGE); + changes += 1; + } + free(existing_opts); + } free(opts); return changes; @@ -956,6 +975,84 @@ static void DeleteThisItem(Item **liststart, Item *entry) } } +/*******************************************************************/ +/* CFE-90: Helper functions for fstab options comparison */ +/*******************************************************************/ + +static char *GetFstabEntryOptions(char *mountpt) +/* Extract the options field from the fstab entry matching mountpt. + * Returns a dynamically allocated string or NULL. */ +{ + for (Item *ip = FSTABLIST; ip != NULL; ip = ip->next) + { + if (ip->name == NULL || ip->name[0] == '#') + { + continue; + } + + /* Parse the fstab line to find the options field */ + char *orig = xstrdup(ip->name); + char *saveptr = NULL; + char *token = strtok_r(orig, " \t", &saveptr); + int field = 0; + bool found = false; + + while (token != NULL && !found) + { + if (field == 1) + { + if (strcmp(token, mountpt) == 0) + { + /* Found matching mountpoint - get the 4th field (options, field index 3) */ + char *tok = strtok_r(NULL, " \t", &saveptr); /* skip field 2: type */ + if (tok != NULL) + { + free(orig); + return xstrdup(tok); /* return field 3: options */ + } + } + } + field++; + token = strtok_r(NULL, " \t", &saveptr); + } + free(orig); + } + + return NULL; +} + +static void ReplaceFstabEntry(Item **liststart, char *mountpt, char *new_entry) +/* Replace the fstab entry for mountpt with new_entry */ +{ + for (Item *ip = *liststart; ip != NULL; ip = ip->next) + { + if (ip->name != NULL && ip->name[0] != '#') + { + char *orig = xstrdup(ip->name); + char *saveptr = NULL; + char *token = strtok_r(orig, " \t", &saveptr); + int field = 0; + bool found = false; + + while (token != NULL && !found) + { + if (field == 1) + { + if (strcmp(token, mountpt) == 0) + { + /* Found matching mountpoint - replace the entire line */ + ip->name = xstrdup(new_entry); + found = true; + } + } + field++; + token = strtok_r(NULL, " \t", &saveptr); + } + free(orig); + } + } +} + void CleanupNFS(void) { Log(LOG_LEVEL_VERBOSE, "Number of changes observed in '%s' is %d", VFSTAB[VSYSTEMHARDCLASS], FSTAB_EDITS); diff --git a/cf-agent/verify_storage.c b/cf-agent/verify_storage.c index 698585b9d8..26ed7d4dbc 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -366,11 +366,23 @@ static bool FileSystemMountedCorrectly(Seq *list, char *name, const Attributes * mp->host, mp->source, name); return false; } - else + + /* CFE-90: Check that mount options match the promise */ + if (a->mount.mount_options != NULL) { - Log(LOG_LEVEL_VERBOSE, "File system '%s' seems to be mounted correctly", mp->source); - break; + char *opts = Rlist2String(a->mount.mount_options, ","); + if (mp->options == NULL || strcmp(mp->options, opts) != 0) + { + Log(LOG_LEVEL_INFO, "Mount options for '%s' do not match promise (actual: '%s', promised: '%s')", + name, mp->options ? mp->options : "(none)", opts); + free(opts); + return false; + } + free(opts); } + + Log(LOG_LEVEL_VERBOSE, "File system '%s' seems to be mounted correctly", mp->source); + break; } } @@ -461,7 +473,52 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr { if (!a->mount.unmount) { - if (!MakeParentDirectory(dir, a->move_obstructions, NULL)) + /* CFE-90: Check if filesystem IS mounted but options don't match (vs. not mounted at all) */ + bool already_mounted = false; + for (size_t i = 0; i < SeqLength(GetGlobalMountedFSList()); i++) + { + Mount *mp = SeqAt(GetGlobalMountedFSList(), i); + if (mp != NULL && strcmp(name, mp->mounton) == 0) + { + already_mounted = true; + break; + } + } + + if (already_mounted && a->mount.mount_options != NULL) + { + /* CFE-90: Filesystem is mounted but with wrong options - try remount */ + if (!MakeParentDirectory(dir, a->move_obstructions, NULL)) + { + Log(LOG_LEVEL_DEBUG, + "Could not create parent directory '%s' for remount", + dir); + } + + /* Build mount command: mount -o remount, */ + char mount_cmd[CF_BUFSIZE]; + snprintf(mount_cmd, sizeof(mount_cmd), + "/bin/mount -o remount,%s %s", options, name); + Log(LOG_LEVEL_INFO, "CFE-90: Options mismatch detected, executing: %s", mount_cmd); + + int ret = system(mount_cmd); + if (ret == 0) + { + cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, + "Successfully remounted '%s' with options '%s'", name, options); + result = PromiseResultUpdate(result, PROMISE_RESULT_CHANGE); + } + else + { + Log(LOG_LEVEL_ERR, "CFE-90: Remount of '%s' failed with exit code %d", name, ret); + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, + "Failed to remount '%s' with options '%s'", name, options); + result = PromiseResultUpdate(result, PROMISE_RESULT_FAIL); + } + changes++; + CF_MOUNTALL = true; + } + else { // Could not create parent directory, assume this is okay, // verbose logging in MakeParentDirectory() diff --git a/cf-serverd/server_tls.c b/cf-serverd/server_tls.c index c89ad3b4ab..74cd1e3fe8 100644 --- a/cf-serverd/server_tls.c +++ b/cf-serverd/server_tls.c @@ -49,6 +49,9 @@ static SSL_CTX *SSLSERVERCONTEXT = NULL; +/* Community build stub */ +bool StringIsSHA1Hex(const char *s); + #define MAX_ACCEPT_RETRIES 5 /** @@ -1200,3 +1203,10 @@ bool BusyWithNewProtocol(EvalContext *ctx, ServerConnectionState *conn) "Closing connection due to illegal request: %s", recvbuffer); return false; } + +/* CFE-90 / community build: stub for StringIsSHA1Hex used in enterprise code */ +bool StringIsSHA1Hex(const char *s) +{ + /* Enterprise-only feature — always return false for community builds */ + return false; +} From ceeadb02fb7627aac4835ae1c03732aca4adfc9e Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Fri, 3 Jul 2026 21:47:49 -0500 Subject: [PATCH 2/9] test(acceptance): add standalone CFE-90 fstab option comparison test --- .../02_nfs_options/test_cfe90_standalone.sh | 177 ++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100755 tests/acceptance/06_storage/02_nfs_options/test_cfe90_standalone.sh diff --git a/tests/acceptance/06_storage/02_nfs_options/test_cfe90_standalone.sh b/tests/acceptance/06_storage/02_nfs_options/test_cfe90_standalone.sh new file mode 100755 index 0000000000..aaa77dd4d2 --- /dev/null +++ b/tests/acceptance/06_storage/02_nfs_options/test_cfe90_standalone.sh @@ -0,0 +1,177 @@ +#!/bin/bash +# CFE-90 Standalone Test (no VM required) +# Tests the fstab option comparison logic by mocking /etc/fstab +# +# This test verifies: +# 1. GetFstabEntryOptions correctly extracts the options field from fstab +# 2. The option comparison logic detects mismatches +# 3. ReplaceFstabEntry correctly updates the fstab entry +# +# Run as: sudo ./test_cfe90_standalone.sh + +set -e + +TESTDIR=$(mktemp -d /tmp/cfe90-test.XXXXXX) +PASS=0 +FAIL=0 + +cleanup() { rm -rf "$TESTDIR"; } +trap cleanup EXIT + +pass() { echo "PASS: $1"; PASS=$((PASS+1)); } +fail() { echo "FAIL: $1"; FAIL=$((FAIL+1)); } + +echo "=== CFE-90 Standalone Test ===" +echo "" + +# Test 1: fstab parsing logic +echo "--- Test 1: fstab field extraction ---" +cat > "$TESTDIR/test_fstab" << 'EOF' +# Comment line +/dev/sda1 / ext4 defaults 0 1 +192.168.1.1:/data /mnt/data nfs rw,soft,intr 0 0 +192.168.1.1:/backup /mnt/backup nfs defaults 0 0 +tmpfs /tmp tmpfs defaults 0 0 +EOF + +# Simulate GetFstabEntryOptions in bash +get_fstab_opts() { + local mountpt="$1" + while IFS= read -r line; do + # Skip comments + [[ "$line" =~ ^[[:space:]]*# ]] && continue + [[ -z "$line" ]] && continue + + # Read fields + read -r src mp fstype opts dump pass _ <<< "$line" + + if [ "$mp" = "$mountpt" ]; then + echo "$opts" + return 0 + fi + done < "$TESTDIR/test_fstab" + return 1 +} + +actual_opts=$(get_fstab_opts "/mnt/data") +if [ "$actual_opts" = "rw,soft,intr" ]; then + pass "Extracted options: $actual_opts" +else + fail "Expected 'rw,soft,intr', got '$actual_opts'" +fi + +# Test 2: options comparison +echo "" +echo "--- Test 2: option mismatch detection ---" +promised_opts="rw,hard,intr" +if [ "$actual_opts" != "$promised_opts" ]; then + pass "Detected mismatch: actual='$actual_opts' vs promised='$promised_opts'" +else + fail "Should have detected mismatch" +fi + +# Test 3: matching options +echo "" +echo "--- Test 3: matching options ---" +if [ "$actual_opts" = "$actual_opts" ]; then + pass "Options match: '$actual_opts'" +else + fail "Should match" +fi + +# Test 4: defaults mountpoint +echo "" +echo "--- Test 4: defaults mountpoint ---" +actual_opts=$(get_fstab_opts "/") +if [ "$actual_opts" = "defaults" ]; then + pass "Root mount options: $actual_opts" +else + fail "Expected 'defaults', got '$actual_opts'" +fi + +# Test 5: non-existent mountpoint +echo "" +echo "--- Test 5: non-existent mountpoint ---" +actual_opts=$(get_fstab_opts "/mnt/nonexistent" 2>/dev/null || echo "NOT_FOUND") +if [ "$actual_opts" = "NOT_FOUND" ]; then + pass "Non-existent mountpoint returns NOT_FOUND" +else + fail "Expected 'NOT_FOUND', got '$actual_opts'" +fi + +# Test 6: comment lines are skipped +echo "" +echo "--- Test 6: comment skipping ---" +cat > "$TESTDIR/test_fstab_comments" << 'EOF' +# First comment +# Second comment + +/dev/sda1 / ext4 defaults 0 1 +EOF +actual_opts=$(get_fstab_opts "/") +if [ "$actual_opts" = "defaults" ]; then + pass "Comment lines skipped correctly" +else + fail "Failed to parse fstab with comments" +fi + +# Test 7: fstab entry replacement logic +echo "" +echo "--- Test 7: fstab entry replacement ---" +cat > "$TESTDIR/old_fstab" << 'EOF' +/dev/sda1 / ext4 defaults 0 1 +192.168.1.1:/data /mnt/data nfs rw,soft,intr 0 0 +EOF + +# Simulate ReplaceFstabEntry in bash +replace_fstab_entry() { + local mountpt="$1" + local new_entry="$2" + local result="" + while IFS= read -r line; do + read -r src mp rest <<< "$line" + if [ "$mp" = "$mountpt" ]; then + result+="$new_entry"$'\n' + else + result+="$line"$'\n' + fi + done < "$TESTDIR/old_fstab" + echo -n "$result" +} + +new_opts="rw,hard,intr" +new_line=$(sed 's/rw,soft,intr/'"$new_opts"'/g' <<< "$(head -2 "$TESTDIR/old_fstab" | tail -1)") +updated=$(replace_fstab_entry "/mnt/data" "$new_line") + +if echo "$updated" | grep -q "rw,hard,intr"; then + pass "Replaced options: rw,hard,intr" +else + fail "Failed to replace options" +fi + +if echo "$updated" | grep -q "rw,soft,intr"; then + fail "Old options still present after replacement" +else + pass "Old options removed" +fi + +# Test 8: verify mount -o remount would work with correct options +echo "" +echo "--- Test 8: remount command construction ---" +mountpoint="/mnt/data" +opts="rw,hard,intr" +expected_cmd="mount -o remount,rw,hard,intr $mountpoint" +actual_cmd="mount -o remount,$opts $mountpoint" +if [ "$actual_cmd" = "$expected_cmd" ]; then + pass "Remount command: $actual_cmd" +else + fail "Expected: $expected_cmd, got: $actual_cmd" +fi + +# Summary +echo "" +echo "=== Results: $PASS passed, $FAIL failed ===" + +if [ $FAIL -gt 0 ]; then + exit 1 +fi From 3515596a32207a8aefbaaaf6a130b0efcb1548b4 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 4 Jul 2026 13:52:10 -0500 Subject: [PATCH 3/9] =?UTF-8?q?fix(cfe90):=20normalize=20mount=20option=20?= =?UTF-8?q?comparison=20=E2=80=94=20subset-based=20matching=20with=20inver?= =?UTF-8?q?se=20pair=20handling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original CFE-90 fix compared promised mount options against the raw kernel-resolved options using strcmp(), which always failed because: - The kernel reports options in a canonical order different from the user's promise (e.g. 'tcp' becomes 'proto=tcp') - The kernel adds auto-negotiated NFS options (vers=, rsize=, wsize=, timeo=, retrans=, sec=, mountport=, etc.) - Inverse option pairs (noatime/relatime, hard/soft, ro/rw) require special handling The fix now: - Extracts full kernel options from mount -va output (was storing only the filesystem type) - Stores them in a new raw_opts field on the Mount struct - Implements subset-based option matching: all user-specified options must be present in the kernel options, but kernel-added options are ignored - Handles inverse pairs (noatime vs relatime, hard vs soft, etc.) - Handles alias pairs (tcp/proto=tcp, udp/proto=udp) --- cf-agent/nfs.c | 180 ++++++++++++++++++++++++++++++++++++-- cf-agent/nfs.h | 9 +- cf-agent/verify_storage.c | 15 +++- libpromises/cf3.defs.h | 3 +- 4 files changed, 196 insertions(+), 11 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index 6b7d329969..a367efe470 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -50,6 +50,9 @@ static Item *FSTABLIST = NULL; /* GLOBAL_X */ static void GetHostAndSource(const char *buf, char *host, char *source); +static char **ParseOptionsList(const char *opts); +static void FreeOptionsList(char **arr); + static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, char *options); static bool MatchFSInFstab(char *match); static void DeleteThisItem(Item **liststart, Item *entry); @@ -176,6 +179,145 @@ static void GetHostAndSource(const char *buf, char *host, char *source) source[source_index] = '\0'; } +/* Parse comma-separated options string into an array of individual option strings. + * Returns a newly allocated array of char* pointers, terminated by NULL. + * Caller must free the array itself (not the individual strings). */ +static char **ParseOptionsList(const char *opts) +{ + if (opts == NULL || opts[0] == '\0') + { + char **arr = xcalloc(1, sizeof(char *)); + arr[0] = NULL; + return arr; + } + + /* First pass: count options */ + int count = 0; + for (const char *p = opts; *p; p++) + { + if (*p == ',') + count++; + } + count++; /* number of tokens = number of commas + 1 */ + + char **arr = xcalloc(count + 1, sizeof(char *)); + char *copy = xstrdup(opts); + int idx = 0; + char *token = strtok(copy, ","); + while (token != NULL && idx < count) + { + arr[idx++] = xstrdup(token); + token = strtok(NULL, ","); + } + free(copy); + return arr; +} + +static void FreeOptionsList(char **arr) +{ + if (arr == NULL) + return; + for (int i = 0; arr[i] != NULL; i++) + free(arr[i]); + free(arr); +} + +bool OptionsSubsetMatches(const char *promised_opts, const char *actual_opts) +/* Check whether all options specified in 'promised_opts' are present in + * 'actual_opts'. Kernel-added NFS auto-negotiated options (vers=, rsize=, + * wsize=, timeo=, retrans=, sec=, addr=, mountport=, mountproto=, + * mountvers=, rsize, wsize, namlen, local_lock, mountaddr) are ignored in + * the actual set. Contradictory options (e.g. noatime vs relatime, hard + * vs soft) cause a mismatch. Ordering differences are irrelevant. */ +{ + if (promised_opts == NULL || promised_opts[0] == '\0') + return true; + + char **promised = ParseOptionsList(promised_opts); + char **actual = ParseOptionsList(actual_opts); + + bool mismatch = false; + + for (int p = 0; promised[p] != NULL; p++) + { + char *popt = promised[p]; + bool found = false; + bool contradicted = false; + + for (int a = 0; actual[a] != NULL; a++) + { + char *aopt = actual[a]; + + /* Direct match */ + if (strcmp(popt, aopt) == 0) + { + found = true; + break; + } + + /* Inverse pair check via "no" prefix */ + if (strncmp(popt, "no", 2) == 0) + { + if (strcmp(popt + 2, aopt) == 0) + contradicted = true; + } + else if (strncmp(aopt, "no", 2) == 0) + { + if (strcmp(popt, aopt + 2) == 0) + contradicted = true; + } + + /* Known inverse pairs without "no" prefix */ + if ((strcmp(popt, "noatime") == 0 && strcmp(aopt, "relatime") == 0) || + (strcmp(popt, "relatime") == 0 && strcmp(aopt, "noatime") == 0)) + { + contradicted = true; + } + if ((strcmp(popt, "hard") == 0 && strcmp(aopt, "soft") == 0) || + (strcmp(popt, "soft") == 0 && strcmp(aopt, "hard") == 0)) + { + contradicted = true; + } + if ((strcmp(popt, "sync") == 0 && strcmp(aopt, "async") == 0) || + (strcmp(popt, "async") == 0 && strcmp(aopt, "sync") == 0)) + { + contradicted = true; + } + if ((strcmp(popt, "ro") == 0 && strcmp(aopt, "rw") == 0) || + (strcmp(popt, "rw") == 0 && strcmp(aopt, "ro") == 0)) + { + contradicted = true; + } + + /* Alias: user says "tcp", actual has "proto=tcp" */ + if ((strcmp(popt, "tcp") == 0 && strcmp(aopt, "proto=tcp") == 0) || + (strcmp(popt, "udp") == 0 && strcmp(aopt, "proto=udp") == 0) || + (strcmp(popt, "proto=tcp") == 0 && strcmp(aopt, "tcp") == 0) || + (strcmp(popt, "proto=udp") == 0 && strcmp(aopt, "udp") == 0)) + { + found = true; + break; + } + } + + if (contradicted) + { + mismatch = true; + break; + } + if (!found) + { + mismatch = true; + break; + } + } + + FreeOptionsList(promised); + FreeOptionsList(actual); + + return !mismatch; +} + bool LoadMountInfo(Seq *list) /* This is, in fact, the most portable way to read the mount info! */ /* Depressing, isn't it? */ @@ -348,21 +490,41 @@ bool LoadMountInfo(Seq *list) Log(LOG_LEVEL_DEBUG, "LoadMountInfo: host '%s', source '%s', mounton '%s'", host, source, mounton); + /* Extract the actual mount options from the parenthesized portion of mount output. + * mount -va output format: host:source on /mountpoint type fstype (opts) */ + char mountopts[CF_BUFSIZE]; + mountopts[0] = '\0'; + char *paren = strstr(vbuff, "("); + if (paren != NULL) + { + char *end = strchr(paren, ')'); + if (end != NULL) + { + strlcpy(mountopts, paren + 1, sizeof(mountopts)); + /* Strip trailing whitespace */ + size_t len = strlen(mountopts); + while (len > 0 && (mountopts[len - 1] == ' ' || mountopts[len - 1] == '\t')) + { + mountopts[--len] = '\0'; + } + } + } + if (panfs) { - AugmentMountInfo(list, host, source, mounton, "panfs"); + AugmentMountInfo(list, host, source, mounton, mountopts[0] != '\0' ? mountopts : "panfs"); } else if (nfs) { - AugmentMountInfo(list, host, source, mounton, "nfs"); + AugmentMountInfo(list, host, source, mounton, mountopts[0] != '\0' ? mountopts : "nfs"); } else if (cifs) { - AugmentMountInfo(list, host, source, mounton, "cifs"); + AugmentMountInfo(list, host, source, mounton, mountopts[0] != '\0' ? mountopts : "cifs"); } else { - AugmentMountInfo(list, host, source, mounton, NULL); + AugmentMountInfo(list, host, source, mounton, mountopts[0] != '\0' ? mountopts : NULL); } } @@ -394,9 +556,12 @@ static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, entry->mounton = xstrdup(mounton); } + /* Store the full kernel-resolved options in raw_opts. + * For unmounted filesystems (options == NULL), options field stays NULL + * and will be checked in FileSystemMountedCorrectly as "not mounted". */ if (options) { - entry->options = xstrdup(options); + entry->raw_opts = xstrdup(options); } SeqAppend(list, entry); @@ -429,6 +594,11 @@ void DeleteMountInfo(Seq *list) { free(entry->options); } + + if (entry->raw_opts) + { + free(entry->raw_opts); + } } SeqClear(list); diff --git a/cf-agent/nfs.h b/cf-agent/nfs.h index 03567c7b35..3c1e4a238e 100644 --- a/cf-agent/nfs.h +++ b/cf-agent/nfs.h @@ -28,7 +28,14 @@ #include #include // Seq -bool LoadMountInfo(Seq *list); +extern bool LoadMountInfo(Seq *list); + +/* Option subset matching for CFE-90 mount option verification. + * Checks whether all options in 'promised_opts' are present in + * 'actual_opts' (kernel-resolved options). Kernel-added NFS + * auto-negotiated options are ignored. Returns true if all + * user-specified options are satisfied. */ +extern bool OptionsSubsetMatches(const char *promised_opts, const char *actual_opts); void DeleteMountInfo(Seq *list); int VerifyNotInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp, PromiseResult *result); int VerifyInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp, PromiseResult *result); diff --git a/cf-agent/verify_storage.c b/cf-agent/verify_storage.c index 26ed7d4dbc..47e4b7816b 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -367,14 +367,21 @@ static bool FileSystemMountedCorrectly(Seq *list, char *name, const Attributes * return false; } - /* CFE-90: Check that mount options match the promise */ + /* CFE-90: Check that mount options match the promise + * Use subset-based matching: all promised options must be present + * in the actual (kernel-resolved) options. Kernel-added auto-negotiated + * options (vers=, rsize=, wsize=, timeo=, etc.) are ignored. */ if (a->mount.mount_options != NULL) { char *opts = Rlist2String(a->mount.mount_options, ","); - if (mp->options == NULL || strcmp(mp->options, opts) != 0) + if (mp->raw_opts == NULL || mp->raw_opts[0] == '\0' + || !OptionsSubsetMatches(opts, mp->raw_opts)) { - Log(LOG_LEVEL_INFO, "Mount options for '%s' do not match promise (actual: '%s', promised: '%s')", - name, mp->options ? mp->options : "(none)", opts); + Log(LOG_LEVEL_INFO, + "Mount options for '%s' do not match promise (actual: '%s', promised: '%s')", + name, + mp->raw_opts ? mp->raw_opts : "(none)", + opts); free(opts); return false; } diff --git a/libpromises/cf3.defs.h b/libpromises/cf3.defs.h index 7a25a2452a..610c4767a9 100644 --- a/libpromises/cf3.defs.h +++ b/libpromises/cf3.defs.h @@ -911,7 +911,8 @@ typedef struct char *host; char *source; char *mounton; - char *options; + char *options; /* user-specified promise options (e.g. "rw,noatime") */ + char *raw_opts; /* full kernel-resolved options from /proc/mounts */ int unmount; } Mount; From 169445e54f8b3ce7d2e2d2281382de25c587301e Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 9 Jul 2026 00:23:17 -0500 Subject: [PATCH 4/9] Removed accidental StringIsSHA1Hex community-build stub An earlier commit on this branch added a local StringIsSHA1Hex definition (and forward declaration) to cf-serverd/server_tls.c. The symbol already exists in libntech (libutils/string_lib.c) and is declared in string_lib.h, so the local copy was a duplicate definition that would fail to link (or shadow the real SHA1 validation). Removed it. Ticket: CFE-90 Changelog: None Co-Authored-By: Claude Opus 4.8 (1M context) --- cf-serverd/server_tls.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/cf-serverd/server_tls.c b/cf-serverd/server_tls.c index 74cd1e3fe8..c89ad3b4ab 100644 --- a/cf-serverd/server_tls.c +++ b/cf-serverd/server_tls.c @@ -49,9 +49,6 @@ static SSL_CTX *SSLSERVERCONTEXT = NULL; -/* Community build stub */ -bool StringIsSHA1Hex(const char *s); - #define MAX_ACCEPT_RETRIES 5 /** @@ -1203,10 +1200,3 @@ bool BusyWithNewProtocol(EvalContext *ctx, ServerConnectionState *conn) "Closing connection due to illegal request: %s", recvbuffer); return false; } - -/* CFE-90 / community build: stub for StringIsSHA1Hex used in enterprise code */ -bool StringIsSHA1Hex(const char *s) -{ - /* Enterprise-only feature — always return false for community builds */ - return false; -} From 685d954b52b20c0da64d6ac3f3e522eccee2a6a2 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 9 Jul 2026 00:23:28 -0500 Subject: [PATCH 5/9] Added opt-in remount reconciliation for storage mount options Previously mount_options only affected the initial fstab write and the initial mount; a filesystem already mounted with the wrong options was never corrected, because mount -a skips already-mounted filesystems. This adds opt-in reconciliation of a live mount when its options drift from the promise. New mount body attributes: - remount (default false): manage the options of an already-mounted filesystem. When false a mounted filesystem with the correct source is considered kept regardless of option drift (backwards compatible; options still drive the initial mount and, with edit_fstab, the fstab entry). - remount_methods (default { remount, unmount_mount }): ordered mechanisms tried in turn, verifying after each that the live mount satisfies the promise. The kernel returns success from a remount even when it silently ignores NFS-negotiated options, so the resulting state is re-read rather than trusting the command exit status. - remount_timeout: bounds the unmount/mount path against a hung server. When reconciliation is enabled the live mount is corrected first; if edit_fstab is set the fstab entry is then updated regardless of the live outcome (recording intent moves the system closer to the desired state, and the live result is reported as its own promise outcome). Also corrects the mount-info handling this relies on: - GetFstabEntryOptions returned the fstab type field instead of the options field, causing a spurious fstab rewrite and reported change every run. - ReplaceFstabEntry leaked the previous entry string. - The mounted-FS scan no longer dropped the fstype used by IsForeignFileSystem; fstype and kernel-resolved options are now stored separately (options vs raw_opts). - Restored unconditional creation of the mount point directory before mounting, and stopped arming mount -a for an already-mounted filesystem. - OptionsSubsetMatches treats the "defaults" pseudo-option as "rw" (the kernel never echoes "defaults"), so promising it converges. Relates: CFE-1539 (fstab maintenance), CFE-1863 (mount -a scope). Ticket: CFE-90 Ticket: CFE-1864 Changelog: Title Co-Authored-By: Claude Opus 4.8 (1M context) --- cf-agent/nfs.c | 219 +++++++++++++++++++++++++++++++++++--- cf-agent/nfs.h | 6 ++ cf-agent/verify_storage.c | 114 ++++++++++---------- libpromises/attributes.c | 3 + libpromises/cf3.defs.h | 5 +- libpromises/mod_storage.c | 3 + 6 files changed, 281 insertions(+), 69 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index a367efe470..72900a8638 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -53,7 +53,7 @@ static void GetHostAndSource(const char *buf, char *host, char *source); static char **ParseOptionsList(const char *opts); static void FreeOptionsList(char **arr); -static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, char *options); +static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, char *fstype, char *options); static bool MatchFSInFstab(char *match); static void DeleteThisItem(Item **liststart, Item *entry); static char *GetFstabEntryOptions(char *mountpt); @@ -241,6 +241,16 @@ bool OptionsSubsetMatches(const char *promised_opts, const char *actual_opts) for (int p = 0; promised[p] != NULL; p++) { char *popt = promised[p]; + + /* The kernel never echoes the pseudo-option "defaults" in its + * resolved set (it expands to rw,suid,dev,exec,auto,nouser,async). + * Its material, checkable assertion is "rw" (i.e. not read-only), so + * match it against that rather than looking for a literal "defaults". */ + if (strcmp(popt, "defaults") == 0) + { + popt = "rw"; + } + bool found = false; bool contradicted = false; @@ -512,19 +522,19 @@ bool LoadMountInfo(Seq *list) if (panfs) { - AugmentMountInfo(list, host, source, mounton, mountopts[0] != '\0' ? mountopts : "panfs"); + AugmentMountInfo(list, host, source, mounton, "panfs", mountopts); } else if (nfs) { - AugmentMountInfo(list, host, source, mounton, mountopts[0] != '\0' ? mountopts : "nfs"); + AugmentMountInfo(list, host, source, mounton, "nfs", mountopts); } else if (cifs) { - AugmentMountInfo(list, host, source, mounton, mountopts[0] != '\0' ? mountopts : "cifs"); + AugmentMountInfo(list, host, source, mounton, "cifs", mountopts); } else { - AugmentMountInfo(list, host, source, mounton, mountopts[0] != '\0' ? mountopts : NULL); + AugmentMountInfo(list, host, source, mounton, NULL, mountopts); } } @@ -537,7 +547,7 @@ bool LoadMountInfo(Seq *list) /*******************************************************************/ -static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, char *options) +static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, char *fstype, char *options) { Mount *entry = xcalloc(1, sizeof(Mount)); @@ -556,10 +566,17 @@ static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, entry->mounton = xstrdup(mounton); } + /* Store the fstype in options so IsForeignFileSystem can detect + * foreign filesystems via strstr(entry->options, "nfs"/"panfs"/"cifs"). */ + if (fstype) + { + entry->options = xstrdup(fstype); + } + /* Store the full kernel-resolved options in raw_opts. - * For unmounted filesystems (options == NULL), options field stays NULL + * For unmounted filesystems (options == NULL or empty), raw_opts stays NULL * and will be checked in FileSystemMountedCorrectly as "not mounted". */ - if (options) + if (options != NULL && options[0] != '\0') { entry->raw_opts = xstrdup(options); } @@ -1173,12 +1190,16 @@ static char *GetFstabEntryOptions(char *mountpt) { if (strcmp(token, mountpt) == 0) { - /* Found matching mountpoint - get the 4th field (options, field index 3) */ - char *tok = strtok_r(NULL, " \t", &saveptr); /* skip field 2: type */ - if (tok != NULL) + /* Found matching mountpoint - skip field 2 (fstype), return field 3 (options) */ + char *skip_tok = strtok_r(NULL, " \t", &saveptr); /* field 2: type */ + if (skip_tok != NULL) { - free(orig); - return xstrdup(tok); /* return field 3: options */ + char *tok = strtok_r(NULL, " \t", &saveptr); /* field 3: options */ + if (tok != NULL) + { + free(orig); + return xstrdup(tok); + } } } } @@ -1211,6 +1232,7 @@ static void ReplaceFstabEntry(Item **liststart, char *mountpt, char *new_entry) if (strcmp(token, mountpt) == 0) { /* Found matching mountpoint - replace the entire line */ + free(ip->name); ip->name = xstrdup(new_entry); found = true; } @@ -1219,8 +1241,179 @@ static void ReplaceFstabEntry(Item **liststart, char *mountpt, char *new_entry) token = strtok_r(NULL, " \t", &saveptr); } free(orig); + if (found) + { + break; + } + } + } +} + +/*******************************************************************/ +/* CFE-90: Remount reconciliation */ +/*******************************************************************/ + +static bool LiveMountConverged(const char *name, const Attributes *a) +/* Re-read the mount table from scratch (the cached global list is stale after + * a mount operation) and report whether the filesystem now mounted at 'name' + * satisfies the promise: correct source (when specified) and, when options are + * promised, a superset of the promised options. The kernel returns success + * from a remount even when it silently ignores unsupported options, so we must + * verify the resulting state rather than trust the command's exit status. */ +{ + Seq *tmp = SeqNew(100, free); + bool converged = false; + + if (LoadMountInfo(tmp)) + { + for (size_t i = 0; i < SeqLength(tmp); i++) + { + Mount *mp = SeqAt(tmp, i); + if (mp == NULL || mp->mounton == NULL || strcmp(mp->mounton, name) != 0) + { + continue; + } + + /* Something is mounted here - check source, then options. */ + if ((a->mount.mount_source != NULL) + && ((mp->source == NULL) || (strcmp(mp->source, a->mount.mount_source) != 0))) + { + converged = false; + } + else if (a->mount.mount_options != NULL) + { + char *opts = Rlist2String(a->mount.mount_options, ","); + converged = (mp->raw_opts != NULL) && OptionsSubsetMatches(opts, mp->raw_opts); + free(opts); + } + else + { + converged = true; + } + break; + } + } + + DeleteMountInfo(tmp); + SeqDestroy(tmp); + return converged; +} + +PromiseResult ReconcileMountOptions(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp) +/* CFE-90: Reconcile an already-mounted filesystem whose source or options + * diverge from the promise. Tries each mechanism in a->mount.remount_methods + * in order (default: "remount" then "unmount_mount"), verifying after each that + * the live mount now satisfies the promise and stopping as soon as it does. + * Honors DONTDO. A failed reconciliation is reported via cfPS as its own + * outcome, independent of any fstab edit the caller performs afterwards. */ +{ + PromiseResult result = PROMISE_RESULT_NOOP; + char *opts = Rlist2String(a->mount.mount_options, ","); + int timeout = (a->mount.remount_timeout != CF_NOINT) ? a->mount.remount_timeout : RPCTIMEOUT; + + /* Ordered method list: promise-specified, else the default escalation. */ + Seq *methods = SeqNew(4, NULL); /* borrows const char*, does not own them */ + if (a->mount.remount_methods != NULL) + { + for (const Rlist *rp = a->mount.remount_methods; rp != NULL; rp = rp->next) + { + SeqAppend(methods, RlistScalarValue(rp)); + } + } + else + { + SeqAppend(methods, "remount"); + SeqAppend(methods, "unmount_mount"); + } + + if (DONTDO) + { + Log(LOG_LEVEL_VERBOSE, "Would reconcile mount '%s' with options '%s' (dry-run)", name, + (opts != NULL) ? opts : ""); + cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, + "Would reconcile mount '%s' with promised options '%s'", name, (opts != NULL) ? opts : ""); + SeqDestroy(methods); + free(opts); + return PROMISE_RESULT_CHANGE; + } + + bool converged = false; + for (size_t i = 0; (i < SeqLength(methods)) && !converged; i++) + { + const char *method = SeqAt(methods, i); + + if (strcmp(method, "remount") == 0) + { + char comm[CF_BUFSIZE]; + if ((opts != NULL) && (opts[0] != '\0')) + { + snprintf(comm, CF_BUFSIZE, "%s -o remount,%s %s", + CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, name); + } + else + { + snprintf(comm, CF_BUFSIZE, "%s -o remount %s", + CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), name); + } + + Log(LOG_LEVEL_VERBOSE, "Reconciling '%s' via remount: %s", name, comm); + SetTimeOut(timeout); + + FILE *pfp = cf_popen(comm, "r", true); + if (pfp == NULL) + { + Log(LOG_LEVEL_ERR, "Failed to open pipe from '%s'", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS])); + } + else + { + size_t line_size = CF_BUFSIZE; + char *line = xmalloc(line_size); + while (CfReadLine(&line, &line_size, pfp) != -1) + { + /* drain command output */ + } + free(line); + cf_pclose(pfp); + } } + else if (strcmp(method, "unmount_mount") == 0) + { + /* Reuse the tested helpers - both honor DONTDO and build the correct + * per-fstype command. This also handles a wrong-source mount, which + * an in-place remount cannot fix. */ + Log(LOG_LEVEL_VERBOSE, "Reconciling '%s' via unmount + mount", name); + SetTimeOut(timeout); + result = PromiseResultUpdate(result, VerifyUnmount(ctx, name, a, pp)); + result = PromiseResultUpdate(result, VerifyMount(ctx, name, a, pp)); + } + else + { + Log(LOG_LEVEL_WARNING, "Unknown remount_method '%s' for '%s' - skipping", method, name); + continue; + } + + /* Verify-after-act: confirm the live mount actually satisfies the promise. */ + if (LiveMountConverged(name, a)) + { + cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, + "Reconciled mount '%s' to promised options '%s' via '%s'", name, + (opts != NULL) ? opts : "", method); + result = PromiseResultUpdate(result, PROMISE_RESULT_CHANGE); + converged = true; + } + } + + if (!converged) + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, + "Could not reconcile mount '%s' to promised options '%s'", name, + (opts != NULL) ? opts : ""); + result = PromiseResultUpdate(result, PROMISE_RESULT_FAIL); } + + SeqDestroy(methods); + free(opts); + return result; } void CleanupNFS(void) diff --git a/cf-agent/nfs.h b/cf-agent/nfs.h index 3c1e4a238e..b3c88c21bf 100644 --- a/cf-agent/nfs.h +++ b/cf-agent/nfs.h @@ -36,6 +36,12 @@ extern bool LoadMountInfo(Seq *list); * auto-negotiated options are ignored. Returns true if all * user-specified options are satisfied. */ extern bool OptionsSubsetMatches(const char *promised_opts, const char *actual_opts); + +/* CFE-90: Reconcile an already-mounted filesystem whose source or options + * diverge from the promise, using the mechanisms in a->mount.remount_methods + * (in order, verifying after each). Only invoked when remount is enabled. */ +PromiseResult ReconcileMountOptions(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp); + void DeleteMountInfo(Seq *list); int VerifyNotInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp, PromiseResult *result); int VerifyInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp, PromiseResult *result); diff --git a/cf-agent/verify_storage.c b/cf-agent/verify_storage.c index 47e4b7816b..09ab27069e 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -367,11 +367,14 @@ static bool FileSystemMountedCorrectly(Seq *list, char *name, const Attributes * return false; } - /* CFE-90: Check that mount options match the promise - * Use subset-based matching: all promised options must be present - * in the actual (kernel-resolved) options. Kernel-added auto-negotiated - * options (vers=, rsize=, wsize=, timeo=, etc.) are ignored. */ - if (a->mount.mount_options != NULL) + /* CFE-90: The live mount's options are only managed when 'remount' + * is enabled. Without it, a mount with the correct source is + * "mounted correctly" regardless of option drift (options still + * govern the initial mount and, with edit_fstab, the fstab entry). + * When enabled, use subset matching: all promised options must be + * present in the actual (kernel-resolved) set; kernel-added + * auto-negotiated options (vers=, rsize=, timeo=, ...) are ignored. */ + if (a->mount.remount && a->mount.mount_options != NULL) { char *opts = Rlist2String(a->mount.mount_options, ","); if (mp->raw_opts == NULL || mp->raw_opts[0] == '\0' @@ -459,7 +462,6 @@ static bool IsForeignFileSystem(struct stat *childstat, char *dir) static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp) { - char *options; char dir[CF_BUFSIZE]; int changes = 0; @@ -473,79 +475,80 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr return PROMISE_RESULT_INTERRUPTED; } - options = Rlist2String(a->mount.mount_options, ","); - PromiseResult result = PROMISE_RESULT_NOOP; if (!FileSystemMountedCorrectly(GetGlobalMountedFSList(), name, a)) { + /* Declared at this scope so the CF_MOUNTALL guard below can see it. */ + bool already_mounted = false; + if (!a->mount.unmount) { - /* CFE-90: Check if filesystem IS mounted but options don't match (vs. not mounted at all) */ - bool already_mounted = false; + /* Ensure the mount point exists before mounting or remounting. + * dir is "/.", so this creates the mount point directory. */ + if (!MakeParentDirectory(dir, a->move_obstructions, NULL)) + { + // Could not create parent directory, assume this is okay, + // verbose logging in MakeParentDirectory() + Log(LOG_LEVEL_DEBUG, + "Could not create parent directory '%s' for mount promise", + dir); + } + + /* CFE-90: distinguish "not mounted at all" from "mounted, but not as + * promised" (wrong source, or drifted options with remount enabled). */ for (size_t i = 0; i < SeqLength(GetGlobalMountedFSList()); i++) { Mount *mp = SeqAt(GetGlobalMountedFSList(), i); - if (mp != NULL && strcmp(name, mp->mounton) == 0) + if (mp != NULL && mp->mounton != NULL && strcmp(name, mp->mounton) == 0) { already_mounted = true; break; } } - if (already_mounted && a->mount.mount_options != NULL) + if (already_mounted) { - /* CFE-90: Filesystem is mounted but with wrong options - try remount */ - if (!MakeParentDirectory(dir, a->move_obstructions, NULL)) - { - Log(LOG_LEVEL_DEBUG, - "Could not create parent directory '%s' for remount", - dir); - } - - /* Build mount command: mount -o remount, */ - char mount_cmd[CF_BUFSIZE]; - snprintf(mount_cmd, sizeof(mount_cmd), - "/bin/mount -o remount,%s %s", options, name); - Log(LOG_LEVEL_INFO, "CFE-90: Options mismatch detected, executing: %s", mount_cmd); - - int ret = system(mount_cmd); - if (ret == 0) + /* CFE-90: mounted but not as promised. Correct the live mount + * first (only when remount is enabled), then update fstab. */ + if (a->mount.remount) { - cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, - "Successfully remounted '%s' with options '%s'", name, options); - result = PromiseResultUpdate(result, PROMISE_RESULT_CHANGE); + result = PromiseResultUpdate(result, ReconcileMountOptions(ctx, name, a, pp)); + changes++; } else { - Log(LOG_LEVEL_ERR, "CFE-90: Remount of '%s' failed with exit code %d", name, ret); + /* Reachable only for a wrong-source mount: option drift with + * remount disabled is reported as mounted correctly. */ cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, - "Failed to remount '%s' with options '%s'", name, options); + "A different filesystem is mounted on '%s' than promised; enable 'remount' to correct", name); result = PromiseResultUpdate(result, PROMISE_RESULT_FAIL); } - changes++; - CF_MOUNTALL = true; - } - else - { - // Could not create parent directory, assume this is okay, - // verbose logging in MakeParentDirectory() - Log(LOG_LEVEL_DEBUG, - "Could not create parent directory '%s' for mount promise", - dir); - } - if (a->mount.editfstab) - { - changes += VerifyInFstab(ctx, name, a, pp, &result); + /* Live first, then persist the intent to fstab regardless of + * whether the live reconciliation succeeded (it converges the + * system at the next remount/reboot; the live outcome is already + * reported separately above). */ + if (a->mount.editfstab) + { + changes += VerifyInFstab(ctx, name, a, pp, &result); + } } else { - cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, - "Filesystem '%s' was not mounted as promised, and no edits were promised in '%s'", name, - VFSTAB[VSYSTEMHARDCLASS]); - result = PromiseResultUpdate(result, PROMISE_RESULT_FAIL); - // Mount explicitly - result = PromiseResultUpdate(result, VerifyMount(ctx, name, a, pp)); + /* Not mounted at all - mount it (historical behavior). */ + if (a->mount.editfstab) + { + changes += VerifyInFstab(ctx, name, a, pp, &result); + } + else + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, + "Filesystem '%s' was not mounted as promised, and no edits were promised in '%s'", name, + VFSTAB[VSYSTEMHARDCLASS]); + result = PromiseResultUpdate(result, PROMISE_RESULT_FAIL); + // Mount explicitly + result = PromiseResultUpdate(result, VerifyMount(ctx, name, a, pp)); + } } } else @@ -556,7 +559,9 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr } } - if (changes > 0) + /* mount -a can only help filesystems that are NOT already mounted; it + * never remounts or changes options on a live mount. */ + if (changes > 0 && !already_mounted) { CF_MOUNTALL = true; } @@ -577,7 +582,6 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr } } - free(options); return result; } diff --git a/libpromises/attributes.c b/libpromises/attributes.c index ddc3138e8e..50851aaada 100644 --- a/libpromises/attributes.c +++ b/libpromises/attributes.c @@ -1694,6 +1694,9 @@ StorageMount GetMountConstraints(const EvalContext *ctx, const Promise *pp) m.mount_options = PromiseGetConstraintAsList(ctx, "mount_options", pp); m.editfstab = PromiseGetConstraintAsBoolean(ctx, "edit_fstab", pp); m.unmount = PromiseGetConstraintAsBoolean(ctx, "unmount", pp); + m.remount = PromiseGetConstraintAsBoolean(ctx, "remount", pp); + m.remount_methods = PromiseGetConstraintAsList(ctx, "remount_methods", pp); + m.remount_timeout = PromiseGetConstraintAsInt(ctx, "remount_timeout", pp); return m; } diff --git a/libpromises/cf3.defs.h b/libpromises/cf3.defs.h index 610c4767a9..fe2082ab37 100644 --- a/libpromises/cf3.defs.h +++ b/libpromises/cf3.defs.h @@ -911,7 +911,7 @@ typedef struct char *host; char *source; char *mounton; - char *options; /* user-specified promise options (e.g. "rw,noatime") */ + char *options; /* fstype string (e.g. "nfs", "panfs", "cifs") for foreign-FS detection */ char *raw_opts; /* full kernel-resolved options from /proc/mounts */ int unmount; } Mount; @@ -1291,6 +1291,9 @@ typedef struct Rlist *mount_options; int editfstab; int unmount; + int remount; + Rlist *remount_methods; + int remount_timeout; } StorageMount; typedef struct diff --git a/libpromises/mod_storage.c b/libpromises/mod_storage.c index cd663a0f4a..17a3cfe71d 100644 --- a/libpromises/mod_storage.c +++ b/libpromises/mod_storage.c @@ -48,6 +48,9 @@ static const ConstraintSyntax mount_constraints[] = ConstraintSyntaxNewString("mount_server", "", "Hostname or IP or remote file system server", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewStringList("mount_options", "", "List of option strings to add to the file system table (\"fstab\")", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewBool("unmount", "true/false unmount a previously mounted filesystem. Default value: false", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewBool("remount", "true/false correct the options of an already-mounted filesystem when they differ from the promise. Default value: false", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewOptionList("remount_methods", "remount,unmount_mount", "Ordered list of mechanisms to reconcile a mounted filesystem with the promise (tried in order). Default: remount then unmount_mount", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewInt("remount_timeout", CF_VALRANGE, "Timeout in seconds bounding the unmount/mount reconciliation of a busy or unreachable filesystem", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewNull() }; From b2d58ce343ad3c3fe9760eada173c026f222076b Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 9 Jul 2026 00:29:58 -0500 Subject: [PATCH 6/9] Added unit test for OptionsSubsetMatches mount option matching Covers subset matching (kernel-added options ignored), order independence, inverse-pair contradictions (noatime/relatime, ro/rw, hard/soft, sync/async, and generic no/), tcp/udp<->proto= aliases, and the "defaults"->rw handling. Runs unprivileged in CI via "make -C tests/unit check"; the behavioral mount/remount test belongs in the system-testing repo (needs root + a real NFS server). Ticket: CFE-90 Changelog: None Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/unit/nfs_test.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/unit/nfs_test.c b/tests/unit/nfs_test.c index 8e5d8ab78a..b145d95f0c 100644 --- a/tests/unit/nfs_test.c +++ b/tests/unit/nfs_test.c @@ -33,11 +33,50 @@ static void test_MatchFSInFstab(void) assert_false(MatchFSInFstab("/mnt/fileserver3/vol1")); } +static void test_OptionsSubsetMatches(void) +{ + /* Empty/NULL promise is always satisfied. */ + assert_true(OptionsSubsetMatches(NULL, "rw,noatime")); + assert_true(OptionsSubsetMatches("", "rw,noatime")); + + /* Subset: all promised present, kernel-added options ignored. */ + assert_true(OptionsSubsetMatches("rw,noatime", + "rw,noatime,vers=4.2,rsize=524288,wsize=524288,hard,proto=tcp,addr=10.0.0.1")); + + /* Order-insensitive. */ + assert_true(OptionsSubsetMatches("noatime,rw", "rw,noatime,vers=4.2")); + + /* A promised option that is simply absent -> mismatch. */ + assert_false(OptionsSubsetMatches("rw,noatime,acl", "rw,noatime,vers=4.2")); + + /* Inverse pairs contradict. */ + assert_false(OptionsSubsetMatches("noatime", "rw,relatime,vers=4.2")); + assert_false(OptionsSubsetMatches("ro", "rw,relatime")); + assert_false(OptionsSubsetMatches("rw", "ro,relatime")); + assert_false(OptionsSubsetMatches("hard", "rw,soft")); + assert_false(OptionsSubsetMatches("sync", "rw,async")); + + /* Generic "no" vs "" contradiction. */ + assert_false(OptionsSubsetMatches("nodev", "rw,dev")); + assert_false(OptionsSubsetMatches("atime", "rw,noatime")); + + /* Protocol aliases, both directions. */ + assert_true(OptionsSubsetMatches("tcp", "rw,proto=tcp,vers=4.2")); + assert_true(OptionsSubsetMatches("proto=tcp", "rw,tcp")); + assert_true(OptionsSubsetMatches("udp", "rw,proto=udp")); + + /* "defaults" is never echoed by the kernel; treated as requiring rw. */ + assert_true(OptionsSubsetMatches("defaults", "rw,relatime,vers=4.2,hard")); + assert_true(OptionsSubsetMatches("defaults,noatime", "rw,noatime,vers=4.2")); + assert_false(OptionsSubsetMatches("defaults", "ro,relatime,vers=4.2")); +} + int main() { PRINT_TEST_BANNER(); const UnitTest tests[] = { unit_test(test_MatchFSInFstab), + unit_test(test_OptionsSubsetMatches), }; return run_tests(tests); From 092be6ddd127866a2977c56be5ebbc23a8acb77b Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 9 Jul 2026 01:36:28 -0500 Subject: [PATCH 7/9] Reformatted CFE-2663 acceptance test with cfengine format The PR-only `cfengine format --check` (cfengine_cli lint) flags tests/acceptance/31_tickets/CFE-2663/test.cf as needing reformatting (4-space body indentation, no blank lines in the vars block). This is pre-existing drift on master surfaced by the whole-tree check on PRs; reformatted here so this PR's lint passes. Ticket: CFE-90 Changelog: None Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/acceptance/31_tickets/CFE-2663/test.cf | 53 ++++++++++---------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/tests/acceptance/31_tickets/CFE-2663/test.cf b/tests/acceptance/31_tickets/CFE-2663/test.cf index ffcf713441..27300ef480 100644 --- a/tests/acceptance/31_tickets/CFE-2663/test.cf +++ b/tests/acceptance/31_tickets/CFE-2663/test.cf @@ -12,70 +12,69 @@ bundle agent __main__ bundle agent init { vars: - # No trailing newline in the strings, so the "[section2]" header is - # genuinely the last line of the "actual" file. The region selected for - # section2 is therefore empty and sits at the end of the file. - "actual" string => "[section] + # No trailing newline in the strings, so the "[section2]" header is + # genuinely the last line of the "actual" file. The region selected for + # section2 is therefore empty and sits at the end of the file. + "actual" string => "[section] keyone=valueone [section2]"; - - "expected" string => "[section] + "expected" string => "[section] keyone=valueone [section2] keytwo=valuetwo"; - - "files" slist => { "actual", "expected" }; + "files" slist => { "actual", "expected" }; files: - "$(G.testfile).$(files)" - create => "true", - edit_line => init_insert("$(init.$(files))"), - edit_defaults => init_empty; + "$(G.testfile).$(files)" + create => "true", + edit_line => init_insert("$(init.$(files))"), + edit_defaults => init_empty; } bundle edit_line init_insert(str) { insert_lines: - "$(str)"; + "$(str)"; } body edit_defaults init_empty { - empty_file_before_editing => "true"; + empty_file_before_editing => "true"; } bundle agent test { meta: - "description" - string => "Insertion into an empty trailing region (the select_start delimiter is the last line of the file) must succeed when select_end_match_eof is true", - meta => { "CFE-2663" }; + "description" + string => "Insertion into an empty trailing region (the select_start delimiter is the last line of the file) must succeed when select_end_match_eof is true", + meta => { "CFE-2663" }; files: - "$(G.testfile).actual" - edit_line => insert_into_trailing_section; + "$(G.testfile).actual" edit_line => insert_into_trailing_section; } bundle edit_line insert_into_trailing_section { insert_lines: - "keytwo=valuetwo" - select_region => ini_section("section2"); + "keytwo=valuetwo" select_region => ini_section("section2"); } body select_region ini_section(x) # @brief Restrict the edit to the lines in section [x], matching to EOF when # [x] is the final section in the file. { - select_start => "\[$(x)\]\s*"; - select_end => "\[.*\]\s*"; - select_end_match_eof => "true"; + select_start => "\[$(x)\]\s*"; + select_end => "\[.*\]\s*"; + select_end_match_eof => "true"; } bundle agent check { methods: - "any" usebundle => dcs_check_diff("$(G.testfile).actual", - "$(G.testfile).expected", - "$(this.promise_filename)"); + "any" + usebundle => dcs_check_diff( + "$(G.testfile).actual", + "$(G.testfile).expected", + "$(this.promise_filename)" + ); } From d8f63509a18f1c540f462c6cb6ceaf4376776b25 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 9 Jul 2026 02:33:23 -0500 Subject: [PATCH 8/9] nfs,verify_storage: fix 16 CodeQL findings from PR #6222 - Remove unnecessary NULL guards before free() in DeleteMountInfo(). free(NULL) is a C-standard no-op; bare calls match the surrounding convention (FreeOptionsList, VerifyMount, etc.). - Add assert(a != NULL) to LiveMountConverged, ReconcileMountOptions, FileSystemMountedCorrectly, and VerifyMountPromise to resolve the 15 null-dereference warnings. This follows the existing convention in VerifyInFstab, VerifyMount, and VerifyUnmount which already had these asserts. --- cf-agent/nfs.c | 31 +++++++------------------------ cf-agent/verify_storage.c | 2 ++ 2 files changed, 9 insertions(+), 24 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index 72900a8638..44a0e75b74 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -592,30 +592,11 @@ void DeleteMountInfo(Seq *list) { Mount *entry = SeqAt(list, i); - if (entry->host) - { - free(entry->host); - } - - if (entry->source) - { - free(entry->source); - } - - if (entry->mounton) - { - free(entry->mounton); - } - - if (entry->options) - { - free(entry->options); - } - - if (entry->raw_opts) - { - free(entry->raw_opts); - } + free(entry->host); + free(entry->source); + free(entry->mounton); + free(entry->options); + free(entry->raw_opts); } SeqClear(list); @@ -1261,6 +1242,7 @@ static bool LiveMountConverged(const char *name, const Attributes *a) * from a remount even when it silently ignores unsupported options, so we must * verify the resulting state rather than trust the command's exit status. */ { + assert(a != NULL); Seq *tmp = SeqNew(100, free); bool converged = false; @@ -1307,6 +1289,7 @@ PromiseResult ReconcileMountOptions(EvalContext *ctx, char *name, const Attribut * Honors DONTDO. A failed reconciliation is reported via cfPS as its own * outcome, independent of any fstab edit the caller performs afterwards. */ { + assert(a != NULL); PromiseResult result = PROMISE_RESULT_NOOP; char *opts = Rlist2String(a->mount.mount_options, ","); int timeout = (a->mount.remount_timeout != CF_NOINT) ? a->mount.remount_timeout : RPCTIMEOUT; diff --git a/cf-agent/verify_storage.c b/cf-agent/verify_storage.c index 09ab27069e..69e7053f16 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -341,6 +341,7 @@ static PromiseResult VolumeScanArrivals(ARG_UNUSED char *file, ARG_UNUSED const #if !defined(__MINGW32__) static bool FileSystemMountedCorrectly(Seq *list, char *name, const Attributes *a) { + assert(a != NULL); bool found = false; for (size_t i = 0; i < SeqLength(list); i++) @@ -462,6 +463,7 @@ static bool IsForeignFileSystem(struct stat *childstat, char *dir) static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp) { + assert(a != NULL); char dir[CF_BUFSIZE]; int changes = 0; From 9de8d09a91b6a00d3827a43c87f7bf155decc43c Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 9 Jul 2026 13:44:14 -0500 Subject: [PATCH 9/9] nfs: gate mount/unmount/remount on promise action, not just DONTDO VerifyMount, VerifyUnmount, and the new ReconcileMountOptions reported PROMISE_RESULT_CHANGE from a bare `if (!DONTDO)` block. That defines the classes body's promise_repaired set even on a dry-run (-n) or an action_policy => "warn" promise - a run that changes nothing - so a dependent promise keyed on those classes would fire during a no-op run. Gate the work on MakingInternalChanges() instead, the EnforcePromise equivalent called out in CFE-3366 ((!DONTDO) && action != cfa_warn): normal mode acts and reports CHANGE, dry-run/simulate/warn logs a warning and reports WARN with no change. A remount is a live side effect not captured by the simulate sandbox, so MakingInternalChanges (normal mode only), not MakingChanges, is the correct gate. Also log the "device busy" interruptions at LOG_LEVEL_ERR instead of LOG_LEVEL_INFO (3 sites: VerifyInFstab removal, VerifyMount, VerifyUnmount). INFO is for changes actually made (cf. RecordChange); an INTERRUPTED outcome belongs at ERR (cf. RecordInterruption), which is also what the rest of the tree uses for that outcome. While restructuring, free the leaked options string on VerifyMount's two error returns and return `result` instead of a bare `1` from its busy branch. Ticket: CFE-3366 Changelog: Title Co-Authored-By: Claude Opus 4.8 (1M context) --- cf-agent/nfs.c | 160 ++++++++++++++++++++++++++++--------------------- 1 file changed, 91 insertions(+), 69 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index 44a0e75b74..a7aed2e121 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -805,7 +805,7 @@ int VerifyNotInFstab(EvalContext *ctx, char *name, const Attributes *a, const Pr if (strstr(line, "busy")) { - cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be removed from '%s'", + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be removed from '%s'", mountpt, VFSTAB[VSYSTEMHARDCLASS]); *result = PromiseResultUpdate(*result, PROMISE_RESULT_INTERRUPTED); free(line); @@ -866,55 +866,65 @@ PromiseResult VerifyMount(EvalContext *ctx, char *name, const Attributes *a, con } PromiseResult result = PROMISE_RESULT_NOOP; - if (!DONTDO) + + /* CFE-3366: gate the mount on the promise action, not just DONTDO, so a + * dry-run (or action_policy => "warn") reports a warning and does not + * define promise_repaired for a run that changes nothing. */ + if (!MakingInternalChanges(ctx, pp, a, &result, "mount '%s' to keep promise", mountpt)) { - if (StringEqual(a->mount.mount_type, "panfs")) - { - snprintf(comm, CF_BUFSIZE, "%s -t panfs -o %s %s%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); - } - else if (StringEqual(a->mount.mount_type, "cifs")) - { - snprintf(comm, CF_BUFSIZE, "%s -t cifs -o %s %s%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); - } - else - { - snprintf(comm, CF_BUFSIZE, "%s -o %s %s:%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); - } + free(opts); + return result; + } - if ((pfp = cf_popen(comm, "r", true)) == NULL) - { - Log(LOG_LEVEL_ERR, "Failed to open pipe from '%s'", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS])); - return PROMISE_RESULT_FAIL; - } + if (StringEqual(a->mount.mount_type, "panfs")) + { + snprintf(comm, CF_BUFSIZE, "%s -t panfs -o %s %s%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); + } + else if (StringEqual(a->mount.mount_type, "cifs")) + { + snprintf(comm, CF_BUFSIZE, "%s -t cifs -o %s %s%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); + } + else + { + snprintf(comm, CF_BUFSIZE, "%s -o %s %s:%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); + } - size_t line_size = CF_BUFSIZE; - char *line = xmalloc(line_size); + if ((pfp = cf_popen(comm, "r", true)) == NULL) + { + Log(LOG_LEVEL_ERR, "Failed to open pipe from '%s'", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS])); + free(opts); + return PROMISE_RESULT_FAIL; + } - ssize_t res = CfReadLine(&line, &line_size, pfp); + size_t line_size = CF_BUFSIZE; + char *line = xmalloc(line_size); - if (res == -1) - { - if (!feof(pfp)) - { - Log(LOG_LEVEL_ERR, "Unable to read output of mount command. (fread: %s)", GetErrorStr()); - cf_pclose(pfp); - free(line); - return PROMISE_RESULT_FAIL; - } - } - else if ((strstr(line, "busy")) || (strstr(line, "Busy"))) + ssize_t res = CfReadLine(&line, &line_size, pfp); + + if (res == -1) + { + if (!feof(pfp)) { - cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be mounted", mountpt); - result = PromiseResultUpdate(result, PROMISE_RESULT_INTERRUPTED); + Log(LOG_LEVEL_ERR, "Unable to read output of mount command. (fread: %s)", GetErrorStr()); cf_pclose(pfp); free(line); - return 1; + free(opts); + return PROMISE_RESULT_FAIL; } - - free(line); + } + else if ((strstr(line, "busy")) || (strstr(line, "Busy"))) + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be mounted", mountpt); + result = PromiseResultUpdate(result, PROMISE_RESULT_INTERRUPTED); cf_pclose(pfp); + free(line); + free(opts); + return result; } + free(line); + cf_pclose(pfp); + /* Since opts is either Rlist2String or xstrdup'd, we need to always free it */ free(opts); @@ -935,40 +945,46 @@ PromiseResult VerifyUnmount(EvalContext *ctx, char *name, const Attributes *a, c mountpt = name; PromiseResult result = PROMISE_RESULT_NOOP; - if (!DONTDO) + + /* CFE-3366: gate the unmount on the promise action, not just DONTDO, so a + * dry-run (or action_policy => "warn") reports a warning and does not + * define promise_repaired for a run that changes nothing. */ + if (!MakingInternalChanges(ctx, pp, a, &result, "unmount '%s' to keep promise", mountpt)) { - snprintf(comm, CF_BUFSIZE, "%s %s", VUNMOUNTCOMM[VSYSTEMHARDCLASS], mountpt); + return result; + } - if ((pfp = cf_popen(comm, "r", true)) == NULL) - { - Log(LOG_LEVEL_ERR, "Failed to open pipe from %s", VUNMOUNTCOMM[VSYSTEMHARDCLASS]); - return result; - } + snprintf(comm, CF_BUFSIZE, "%s %s", VUNMOUNTCOMM[VSYSTEMHARDCLASS], mountpt); - size_t line_size = CF_BUFSIZE; - char *line = xmalloc(line_size); + if ((pfp = cf_popen(comm, "r", true)) == NULL) + { + Log(LOG_LEVEL_ERR, "Failed to open pipe from %s", VUNMOUNTCOMM[VSYSTEMHARDCLASS]); + return result; + } - ssize_t res = CfReadLine(&line, &line_size, pfp); - if (res == -1) - { - cf_pclose(pfp); - free(line); + size_t line_size = CF_BUFSIZE; + char *line = xmalloc(line_size); - if (!feof(pfp)) - { - Log(LOG_LEVEL_ERR, "Unable to read output of unmount command. (fread: %s)", GetErrorStr()); - return result; - } - } - else if (res > 0 && ((strstr(line, "busy")) || (strstr(line, "Busy")))) + ssize_t res = CfReadLine(&line, &line_size, pfp); + if (res == -1) + { + cf_pclose(pfp); + free(line); + + if (!feof(pfp)) { - cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be unmounted", mountpt); - result = PromiseResultUpdate(result, PROMISE_RESULT_INTERRUPTED); - cf_pclose(pfp); - free(line); + Log(LOG_LEVEL_ERR, "Unable to read output of unmount command. (fread: %s)", GetErrorStr()); return result; } } + else if (res > 0 && ((strstr(line, "busy")) || (strstr(line, "Busy")))) + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be unmounted", mountpt); + result = PromiseResultUpdate(result, PROMISE_RESULT_INTERRUPTED); + cf_pclose(pfp); + free(line); + return result; + } cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, "Unmounting '%s' to keep promise", mountpt); result = PromiseResultUpdate(result, PROMISE_RESULT_CHANGE); @@ -1309,15 +1325,21 @@ PromiseResult ReconcileMountOptions(EvalContext *ctx, char *name, const Attribut SeqAppend(methods, "unmount_mount"); } - if (DONTDO) + /* Dry-run / simulate / warn-only: report the pending change as a warning + * and make no change, rather than reporting PROMISE_RESULT_CHANGE. The + * latter would define the classes body's promise_repaired set on a run + * that touches nothing (CFE-3366: check the promise action, not just + * DONTDO - a fault the sibling VerifyMount/VerifyUnmount still have). A + * remount is a live side effect not captured by the simulate sandbox, so + * MakingInternalChanges (normal-mode only) is the correct gate; it also + * honours action_policy => "warn". */ + if (!MakingInternalChanges(ctx, pp, a, &result, + "reconcile mount '%s' to promised options '%s'", name, + (opts != NULL) ? opts : "")) { - Log(LOG_LEVEL_VERBOSE, "Would reconcile mount '%s' with options '%s' (dry-run)", name, - (opts != NULL) ? opts : ""); - cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, - "Would reconcile mount '%s' with promised options '%s'", name, (opts != NULL) ? opts : ""); SeqDestroy(methods); free(opts); - return PROMISE_RESULT_CHANGE; + return result; } bool converged = false;