wdgzyjx/myxce
1
0
Fork 0
forked from lei.chen/myxce
Code Pull requests Activity

fix(config-repl): conf_save_config() must use workdir/name not workdir

Browse source 
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.
This commit is contained in:
Rongsong Shen 2026-05-21 08:09:02 +00:00
parent 86fe35e671
commit 2ed7e34e25
3 changed files with 96 additions and 16 deletions
Download patch file Download diff file Expand all files Collapse all files

8
src/controller/config_repl.c
View file

@ -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);

102
src/controller/testsuite/test_config_repl.c
View file

@ -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

2
src/docs/config-replication-design.md
View file

@ -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);