From 2ed7e34e259432db5458bac259cda9c381aaa24a Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 21 May 2026 08:09:02 +0000 Subject: [PATCH] fix(config-repl): conf_save_config() must use workdir/name not workdir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit load_primary_config() reads from workdir/name/conf — conf_save_config() was writing to workdir/conf. Latent bug: would have corrupted upgrade restart cycle once Task 11 wired the upgrade path. Update all conf_read_current_filename, conf_get_saved_tag, and conf_check_stale calls in the test suite to use the corrected effective_dir (workdir/name) path structure. Add new test test_conf_save_config_path_consistency that explicitly verifies the written path matches what load_primary_config() reads. --- src/controller/config_repl.c | 8 +- src/controller/testsuite/test_config_repl.c | 102 +++++++++++++++++--- src/docs/config-replication-design.md | 2 + 3 files changed, 96 insertions(+), 16 deletions(-) diff --git a/src/controller/config_repl.c b/src/controller/config_repl.c index abf2750b..0b48bd4d 100644 --- a/src/controller/config_repl.c +++ b/src/controller/config_repl.c @@ -231,8 +231,12 @@ int conf_read_current_filename(const char *workdir, char *out, size_t outsz) int conf_save_config(myconf_t *myconf, const uint8_t *bytes, size_t len, const uint8_t sha256[32]) { + /* effective_dir = workdir/name — must match load_primary_config() path */ + char effective_dir[4096]; + snprintf(effective_dir, sizeof(effective_dir), "%s/%s", + myconf->workdir, myconf->name); char conf_dir[4096]; - make_conf_dir(myconf->workdir, conf_dir, sizeof(conf_dir)); + make_conf_dir(effective_dir, conf_dir, sizeof(conf_dir)); if (mkdir(conf_dir, 0700) != 0 && errno != EEXIST) return -errno; @@ -303,7 +307,7 @@ int conf_save_config(myconf_t *myconf, const uint8_t *bytes, size_t len, } /* Step 3: trim old versions */ - conf_trim_old_versions(myconf->workdir, myconf->conf_keep_n); + conf_trim_old_versions(effective_dir, myconf->conf_keep_n); /* Update conffile to the new versioned path (heap-allocated) */ free(myconf->conffile); diff --git a/src/controller/testsuite/test_config_repl.c b/src/controller/testsuite/test_config_repl.c index 33d2edf9..8b44d018 100644 --- a/src/controller/testsuite/test_config_repl.c +++ b/src/controller/testsuite/test_config_repl.c @@ -23,7 +23,9 @@ * at the top and cr_teardown() via the label before returning. */ static char g_tmpdir[256]; -static char g_conf_dir[300]; +/* g_instdir = g_tmpdir/ctrl0 — the effective_dir used by conf_save_config() */ +static char g_instdir[300]; +static char g_conf_dir[356]; /* tmpdir string for load_primary_config tests (no conf/current) */ static char tmpdir[256]; @@ -33,7 +35,10 @@ static void cr_setup(void) snprintf(g_tmpdir, sizeof(g_tmpdir), "/tmp/test_cr_%d_%ld", (int)getpid(), (long)time(NULL)); mkdir(g_tmpdir, 0700); - snprintf(g_conf_dir, sizeof(g_conf_dir), "%s/conf", g_tmpdir); + /* conf_save_config() writes to workdir/name/conf — mirror that here */ + snprintf(g_instdir, sizeof(g_instdir), "%s/ctrl0", g_tmpdir); + mkdir(g_instdir, 0700); + snprintf(g_conf_dir, sizeof(g_conf_dir), "%s/conf", g_instdir); mkdir(g_conf_dir, 0700); } @@ -68,7 +73,8 @@ static void test_conf_read_current_filename_empty_file(void) close(fd); char out[256] = {0}; - int rc = conf_read_current_filename(g_tmpdir, out, sizeof(out)); + /* conf/current lives under g_instdir/conf — pass g_instdir as base */ + int rc = conf_read_current_filename(g_instdir, out, sizeof(out)); TEST_ASSERT_EQUAL_INT(-ENODATA, rc); cr_teardown(); printf("test_conf_read_current_filename_empty_file: PASS\n"); @@ -84,7 +90,8 @@ static void test_conf_read_current_filename_path_separator(void) fclose(f); char out[256] = {0}; - int rc = conf_read_current_filename(g_tmpdir, out, sizeof(out)); + /* conf/current lives under g_instdir/conf — pass g_instdir as base */ + int rc = conf_read_current_filename(g_instdir, out, sizeof(out)); TEST_ASSERT_EQUAL_INT(-EINVAL, rc); cr_teardown(); printf("test_conf_read_current_filename_path_separator: PASS\n"); @@ -100,7 +107,8 @@ static void test_conf_read_current_filename_bad_prefix(void) fclose(f); char out[256] = {0}; - int rc = conf_read_current_filename(g_tmpdir, out, sizeof(out)); + /* conf/current lives under g_instdir/conf — pass g_instdir as base */ + int rc = conf_read_current_filename(g_instdir, out, sizeof(out)); TEST_ASSERT_EQUAL_INT(-EINVAL, rc); cr_teardown(); printf("test_conf_read_current_filename_bad_prefix: PASS\n"); @@ -116,7 +124,8 @@ static void test_conf_read_current_filename_wrong_hex_length(void) fclose(f); char out[256] = {0}; - int rc = conf_read_current_filename(g_tmpdir, out, sizeof(out)); + /* conf/current lives under g_instdir/conf — pass g_instdir as base */ + int rc = conf_read_current_filename(g_instdir, out, sizeof(out)); TEST_ASSERT_EQUAL_INT(-EINVAL, rc); cr_teardown(); printf("test_conf_read_current_filename_wrong_hex_length: PASS\n"); @@ -170,7 +179,8 @@ static void test_conf_save_filename_has_12_hex_suffix(void) conf_save_config(&myconf, data, len, sha256); char fname[256] = {0}; - int rc = conf_read_current_filename(g_tmpdir, fname, sizeof(fname)); + /* conf_save_config() writes to workdir/name/conf — read from g_instdir */ + int rc = conf_read_current_filename(g_instdir, fname, sizeof(fname)); TEST_ASSERT_EQUAL_INT(0, rc); TEST_ASSERT_EQUAL_INT(0, strncmp(fname, "cluster.bin.", 12)); const char *hex = fname + 12; @@ -228,7 +238,9 @@ static void test_conf_trim_keeps_only_n_versions(void) static void test_conf_get_saved_tag_no_config(void) { cr_setup(); - uint64_t tag = conf_get_saved_tag(g_tmpdir); + /* conf_get_saved_tag looks for conf/current under its argument; + * after the path fix, conf lives under workdir/name = g_instdir */ + uint64_t tag = conf_get_saved_tag(g_instdir); TEST_ASSERT_EQUAL_UINT64(0, tag); cr_teardown(); printf("test_conf_get_saved_tag_no_config: PASS\n"); @@ -252,7 +264,7 @@ static void test_conf_get_saved_tag_after_save(void) conf_save_config(&myconf, data, len, sha256); free(myconf.conffile); - uint64_t tag = conf_get_saved_tag(g_tmpdir); + uint64_t tag = conf_get_saved_tag(g_instdir); TEST_ASSERT_NOT_EQUAL(0, tag); /* Tag must equal first 8 bytes of sha256 in host order */ @@ -274,7 +286,7 @@ static void test_conf_check_stale_disabled(void) { cr_setup(); /* timeout 0 = disabled, never stale */ - TEST_ASSERT_EQUAL_INT(0, conf_check_stale(g_tmpdir, 0)); + TEST_ASSERT_EQUAL_INT(0, conf_check_stale(g_instdir, 0)); cr_teardown(); printf("test_conf_check_stale_disabled: PASS\n"); } @@ -296,7 +308,7 @@ static void test_conf_check_stale_fresh(void) free(myconf.conffile); /* Just saved — not stale with 24h timeout */ - TEST_ASSERT_EQUAL_INT(0, conf_check_stale(g_tmpdir, 86400)); + TEST_ASSERT_EQUAL_INT(0, conf_check_stale(g_instdir, 86400)); cr_teardown(); printf("test_conf_check_stale_fresh: PASS\n"); } @@ -319,7 +331,8 @@ static void test_conf_check_stale_aged(void) /* Age the versioned file by 20 seconds via utime */ char fname[256] = {0}; - conf_read_current_filename(g_tmpdir, fname, sizeof(fname)); + /* conf_save_config() writes to workdir/name/conf — read from g_instdir */ + conf_read_current_filename(g_instdir, fname, sizeof(fname)); char ver_path[512]; snprintf(ver_path, sizeof(ver_path), "%s/%s", g_conf_dir, fname); @@ -329,9 +342,9 @@ static void test_conf_check_stale_aged(void) utime(ver_path, &old_time); /* 5-second threshold → stale */ - TEST_ASSERT_EQUAL_INT(1, conf_check_stale(g_tmpdir, 5)); + TEST_ASSERT_EQUAL_INT(1, conf_check_stale(g_instdir, 5)); /* 30-second threshold → not stale */ - TEST_ASSERT_EQUAL_INT(0, conf_check_stale(g_tmpdir, 30)); + TEST_ASSERT_EQUAL_INT(0, conf_check_stale(g_instdir, 30)); cr_teardown(); printf("test_conf_check_stale_aged: PASS\n"); @@ -409,6 +422,66 @@ static void test_load_primary_config_missing_returns_error(void) } #endif +/* ── test_conf_save_config_path_consistency ─────── */ + +static void test_conf_save_config_path_consistency(void) +{ + /* Verify conf_save_config() writes to workdir/name/conf, not workdir/conf. + * This mirrors the path that load_primary_config() reads from. */ + char workdir[256]; + snprintf(workdir, sizeof(workdir), "/tmp/test_pathbug_%d", (int)getpid()); + char instdir[300]; + snprintf(instdir, sizeof(instdir), "%s/ctrl01", workdir); + char conf_dir[356]; + snprintf(conf_dir, sizeof(conf_dir), "%s/conf", instdir); + + mkdir(workdir, 0700); + /* conf_save_config() builds effective_dir = workdir/name internally, + * then mkdirs effective_dir/conf — but effective_dir itself must exist. */ + mkdir(instdir, 0700); + + /* Fake 32-byte sha256 and dummy payload */ + uint8_t sha256[32]; + memset(sha256, 0xab, sizeof(sha256)); + const uint8_t payload[] = "dummy config bytes for path test"; + size_t plen = sizeof(payload) - 1; + + myconf_t myconf; + memset(&myconf, 0, sizeof(myconf)); + myconf.workdir = workdir; + myconf.name = "ctrl01"; + myconf.conf_keep_n = 2; + myconf.conffile = NULL; + + int rc = conf_save_config(&myconf, payload, plen, sha256); + TEST_ASSERT_EQUAL_INT(0, rc); + + /* Versioned file must be under workdir/name/conf/, not workdir/conf/ */ + TEST_ASSERT_NOT_NULL(myconf.conffile); + TEST_ASSERT_EQUAL_INT(0, strncmp(myconf.conffile, conf_dir, + strlen(conf_dir))); + + /* conf/current pointer file must exist at workdir/name/conf/current */ + char ptr_path[400]; + snprintf(ptr_path, sizeof(ptr_path), "%s/current", conf_dir); + TEST_ASSERT_EQUAL_INT(0, access(ptr_path, F_OK)); + + /* workdir/conf/ must NOT exist (wrong location) */ + char wrong_conf[300]; + snprintf(wrong_conf, sizeof(wrong_conf), "%s/conf", workdir); + struct stat st; + TEST_ASSERT_NOT_EQUAL(0, stat(wrong_conf, &st)); + + free(myconf.conffile); + + /* Cleanup */ + char cmd[512]; + snprintf(cmd, sizeof(cmd), "rm -rf %s", workdir); + system(cmd); + + printf("test_conf_save_config_path_consistency: PASS\n"); +} + /* ── dispatch ───────────────────────────────────── */ void test_config_repl(void) @@ -430,6 +503,7 @@ void test_config_repl(void) RUN_TEST(test_sanitise_cn_printable); RUN_TEST(test_sanitise_cn_control_chars); RUN_TEST(test_sanitise_cn_truncated); + RUN_TEST(test_conf_save_config_path_consistency); #ifdef XCE_TESTING RUN_TEST(test_load_primary_config_missing_returns_error); #endif diff --git a/src/docs/config-replication-design.md b/src/docs/config-replication-design.md index 589b0b88..80a26398 100644 --- a/src/docs/config-replication-design.md +++ b/src/docs/config-replication-design.md @@ -255,6 +255,8 @@ running identical configs without parsing any binary content. ### 3.2 Write procedure (standby receiving new config) +**`effective_dir`** throughout this section denotes `${workdir}/${name}` — the controller instance's base directory. All conf/ paths are relative to it. + ```c // 0. Ensure conf/ directory exists with restrictive permissions snprintf(conf_dir, ..., "%s/conf", effective_dir);