From 1d09631df09eb8304f12ae844a7485eacf9b3198 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 25 Jul 2026 09:06:24 +0000 Subject: [PATCH] feat(config-upgrade-snapshot): add configcompare tool, extend snapverify (task 8.4) Build configcompare_paths(), an offline sibling to snapdiff/walcompare that recomputes each member's active-config sha256 fresh from its own on-disk workdir/name/conf bytes and reports identical vs. divergent (Req 4.2). Wire it as a cmd_configcompare CLI subcommand in test.c, matching its siblings, after review caught it shipped as an unreachable library function in round 1. Extend snapverify to match the current on-disk checkpoint format: bump SNAP_FORMAT_VERSION to 2, replace the stale raw queue-frame decode with the capnp length-prefixed walk, add host-frame and config-bytes-section structural walks, and decode (not just presence-check) the header's cfgref tag/sha. Without this fix snapverify rejected every checkpoint written by the current codebase. --- .kiro/specs/config-upgrade-snapshot/tasks.md | 4 +- src/controller/meson.build | 8 +- src/controller/testsuite/configcompare.c | 219 ++++++++++++++ src/controller/testsuite/configcompare.h | 74 +++++ src/controller/testsuite/controller_test.c | 2 + src/controller/testsuite/snapverify.c | 179 ++++++++++-- src/controller/testsuite/snapverify.h | 44 ++- src/controller/testsuite/test.c | 25 ++ src/controller/testsuite/test_configcompare.c | 217 ++++++++++++++ src/controller/testsuite/test_snapverify.c | 272 ++++++++++++++++-- 10 files changed, 997 insertions(+), 47 deletions(-) create mode 100644 src/controller/testsuite/configcompare.c create mode 100644 src/controller/testsuite/configcompare.h create mode 100644 src/controller/testsuite/test_configcompare.c diff --git a/.kiro/specs/config-upgrade-snapshot/tasks.md b/.kiro/specs/config-upgrade-snapshot/tasks.md index bb558910..475cf03b 100644 --- a/.kiro/specs/config-upgrade-snapshot/tasks.md +++ b/.kiro/specs/config-upgrade-snapshot/tasks.md @@ -106,7 +106,7 @@ - _Depends: 6.2_ - _Boundary: Activation Fence (test_upgrade_activation_fence.c)_ -- [ ] 8.4 (P) Build a cross-member active-configuration comparator and extend checkpoint verification tooling +- [x] 8.4 (P) Build a cross-member active-configuration comparator and extend checkpoint verification tooling - Build a new offline, file-based tool that compares each member's active configuration content hash, recomputed fresh from each member's own on-disk configuration file, mirroring the existing checkpoint-comparison tool's shape - Extend the existing checkpoint-structure verification tool to decode and assert the embedded configuration reference's tag and hash (not merely report their presence), and to report host-frame count and configuration-bytes-section presence - Observable: the comparator correctly reports two members with matching configuration as identical and two with differing configuration as divergent; the extended verification tool reports a concrete tag/hash value and section counts for a real checkpoint file @@ -149,6 +149,8 @@ ## Implementation Notes +- **Task 8.4 round-1 rejection (real gap, caught by review):** the first pass built `configcompare_paths()` (correct, well-tested against `persist.c`/`config_repl.c`'s real `workdir/name/conf/current`+`conf/cluster.bin.` resolution convention) and linked `configcompare.c` into both `xce_test_src` and `controller_unit_test_src` in meson, but never registered it as a `cmd_configcompare` subcommand in `testsuite/test.c` — unlike its siblings `walcompare`/`snapverify`/`snapdiff`, each of which has a `cmd_*` wrapper in BOTH `top_commands[]` and `client_commands[]`. Left as delivered, the tool was a tested library function with no shell entry point — unusable by task 8.6/8.9's later HA soak scenarios, which design.md explicitly says will invoke `configcompare` directly against each node's data directory. Fixed with a 25-line, `test.c`-only remediation mirroring `cmd_snapdiff`'s exact shape; verified end-to-end via `echo 'configcompare ' | xce_test` (checked `$?` for both IDENTICAL/exit-0 and DIVERGENT/exit-1 cases, not just printed text). **Heads-up for any future task in this spec (or elsewhere) that adds a new offline testsuite tool in this family**: linking the `.c` file into the meson source lists is NOT sufficient — it must also get a `cmd_*` wrapper + `top_commands[]`/`client_commands[]` entries in `test.c`, or it silently has no way to be invoked outside the unit test binary. + - **Task 8.3: no new tests needed** — the 10 tests already in `test_config_repl.c` from tasks 6.1/6.2 fully satisfy both bullets and Requirements 3.2/3.3/3.4/4.3/5.5 (verified independently by implementer and reviewer; zero-diff task). The two "fence sets" tests genuinely toggle different halves of `upgrade_materialization_verify`'s condition (materialized vs. bytes_verified), not duplicates. Retry-trampoline automatic-firing (delaymgr actually running) is untested, matching the exact precedent depth of `config_activate_retry_fn`/`config_schedule_activate_retry` (also untested at that depth anywhere in this codebase) — judged out of scope, not a gap. - **Task 8.2: no new tests needed** — task 3's `test_upgrade_snapshot_restore.c` already satisfies all 3 bullets and Requirements 1.10/1.11/5.8/7.1 (verified independently by both implementer and reviewer; zero-diff task). Requirement 7.1's full scenario is compositionally covered across two files (`test_upgrade_snapshot_restore.c`'s real-rejoin test + `test_persist_snapshot_entities.c`'s `test_snapent_load_snapshot_restores_hosts_and_queues_end_to_end`'s real checkpoint-decode test), not by one continuous test — judged adequate since `restore_host_state` is a single invocation-site-independent function. design.md's Requirements Traceability table (row 7.1) under-credits this and should eventually be corrected to cite both files, not just `test_upgrade_snapshot_restore.c` — docs-only, not a testing gap. diff --git a/src/controller/meson.build b/src/controller/meson.build index 6f689f63..52204b59 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -54,11 +54,14 @@ controller = executable('xce_controller', link_args: wrap_link_args, install: true) -xce_test_src = [ 'testsuite/test.c', 'testsuite/waldump.c', 'testsuite/walcompare.c', 'testsuite/snapverify.c', 'testsuite/snapdiff.c'] +xce_test_src = [ 'testsuite/test.c', 'testsuite/waldump.c', 'testsuite/walcompare.c', 'testsuite/snapverify.c', 'testsuite/snapdiff.c', 'testsuite/configcompare.c'] readline_dep = dependency('readline') ncurses_dep = dependency('ncurses') history_dep = dependency('history') +ssl_dep = dependency('openssl') # also needed by configcompare.c (SHA256()); + # controller_unit_test_deps below re-assigns + # the same dependency() result, harmless xce_test_deps = [ libxce_dep, @@ -70,6 +73,7 @@ xce_test_deps = [ tinylog_dep, zlib_dep, # waldump: crc32 WAL validation c_capnproto_dep, # waldump: decode JOB_SUBMIT jobid + ssl_dep, # configcompare: SHA256() active-config hashing ] xce_test = executable('xce_test', xce_test_src, @@ -157,6 +161,8 @@ controller_unit_test_src = [ 'testsuite/snapverify.c', 'testsuite/test_snapdiff.c', 'testsuite/snapdiff.c', + 'testsuite/test_configcompare.c', + 'testsuite/configcompare.c', 'testsuite/test_persist_snapshot_entities.c', 'testsuite/test_apply_seam.c', 'testsuite/test_apply_committed_entry.c', diff --git a/src/controller/testsuite/configcompare.c b/src/controller/testsuite/configcompare.c new file mode 100644 index 00000000..dec172df --- /dev/null +++ b/src/controller/testsuite/configcompare.c @@ -0,0 +1,219 @@ +#define _GNU_SOURCE +#include "configcompare.h" + +#include +#include +#include +#include +#include +#include + +/* Bound on a member's config bytes this tool will read -- mirrors + * persist.c's PERSIST_CONFIG_BYTES_MAX_SIZE / config_repl.h's CONF_MAX_SIZE + * (1 MiB) exactly. Duplicated as its own local constant rather than + * including either header, for the same reason the resolve-then-read logic + * below is duplicated -- see read_member_config_sha256()'s doc comment. */ +#define CONFIGCOMPARE_CONFIG_MAX_SIZE (1u * 1024u * 1024u) + +/* Resolve `/conf/current` -> `/conf/`, + * read its raw bytes, and hash them into `out_sha`. + * + * DELIBERATE DUPLICATION -- this is the FOURTH independent copy of this + * exact resolve-then-read-then-hash logic in this codebase: + * 1. config_repl.c's conf_read_current_filename() + conf_read_raw() + * (the production implementation -- config replication). + * 2. config_repl.c's local_config_sha256() (activation-fence hash check; + * an inline copy for the same "no cross-file call needed here" + * reason as #4 below). + * 3. persist.c's read_active_config_bytes() (checkpoint config-bytes + * embedding; see its own doc comment for the full "why duplicate, + * not call" rationale -- avoiding a persist.c -> config_repl.c + * layering dependency). + * 4. This function. configcompare.c is compiled into the standalone + * xce_test CLI (meson.build's xce_test_src), which links ONLY + * libxce_dep / librtti_dep / struct2json_dep / readline_dep / + * history_dep / ncurses_dep / tinylog_dep / zlib_dep / + * c_capnproto_dep -- deliberately NOT config_repl.c, persist.c, + * cluster.c, or any other heavy controller object file. This is a + * lightweight, dependency-light offline CLI tool family + * (walcompare/snapdiff/snapverify all share the same constraint; no + * live RPC to any cluster member, consistent with every other tool + * in this family -- design.md "Regression Tooling"). Adding a + * config_repl.c dependency to link in one function would violate + * that boundary, so the ~25-line resolve+read is duplicated inline + * here instead, exactly as persist.c's read_active_config_bytes() + * already chose to do for the identical reason. + * + * Returns 0 with `out_sha` filled on success. Returns a negative errno on + * any failure to resolve or read: -ENOENT (no conf/current pointer, or it + * names a versioned file that is not present -- benign "not provisioned + * yet" as well as genuine absence, both surfaced identically to the + * caller as "unreadable"), -EINVAL (malformed pointer file: empty, + * embedded '/', wrong prefix, wrong hex length), -EIO (short + * read/zero-byte versioned file), -EFBIG (versioned file exceeds + * CONFIGCOMPARE_CONFIG_MAX_SIZE), or -ENOMEM. This is the caller's + * "unreadable/absent" signal -- distinct from and never conflated with a + * mismatched hash. */ +static int read_member_config_sha256(const char* conf_dir, uint8_t out_sha[32]) +{ + if (!conf_dir || !*conf_dir) + return -EINVAL; + + char head_path[4160]; + snprintf(head_path, sizeof(head_path), "%s/conf/current", conf_dir); + + FILE* hf = fopen(head_path, "r"); + if (hf == NULL) + return -ENOENT; /* no config pushed to this member yet */ + + struct stat hst; + if (fstat(fileno(hf), &hst) == 0 && hst.st_size == 0) { + fclose(hf); + return -ENOENT; /* interrupted-write pointer file -- treat as absent */ + } + + char fname[256]; + if (fgets(fname, (int)sizeof(fname), hf) == NULL) { + fclose(hf); + return -EIO; + } + fclose(hf); + fname[strcspn(fname, "\n")] = '\0'; + + if (strchr(fname, '/') != NULL) + return -EINVAL; /* corrupted pointer file -- embedded path separator */ + if (strncmp(fname, "cluster.bin.", 12) != 0) + return -EINVAL; + const char* hex = fname + 12; + if (strlen(hex) != 12 || strspn(hex, "0123456789abcdef") != 12) + return -EINVAL; + + char cfg_path[4416]; + snprintf(cfg_path, sizeof(cfg_path), "%s/conf/%s", conf_dir, fname); + + FILE* cf = fopen(cfg_path, "rb"); + if (cf == NULL) + return -ENOENT; /* pointer references a file that is not there */ + + if (fseek(cf, 0, SEEK_END) != 0) { fclose(cf); return -EIO; } + long sz = ftell(cf); + if (sz < 0) { fclose(cf); return -EIO; } + if (fseek(cf, 0, SEEK_SET) != 0) { fclose(cf); return -EIO; } + + if (sz == 0) { + fclose(cf); + return -EIO; /* zero-byte versioned config file: genuine corruption */ + } + if ((size_t)sz > CONFIGCOMPARE_CONFIG_MAX_SIZE) { + fclose(cf); + return -EFBIG; + } + + uint8_t* buf = (uint8_t*)malloc((size_t)sz); + if (buf == NULL) { + fclose(cf); + return -ENOMEM; + } + if (fread(buf, 1, (size_t)sz, cf) != (size_t)sz) { + free(buf); + fclose(cf); + return -EIO; + } + fclose(cf); + + SHA256(buf, (size_t)sz, out_sha); + free(buf); + return 0; +} + +static const char* sha256_hex(const uint8_t sha[32], char out[65]) +{ + static const char digits[] = "0123456789abcdef"; + for (int i = 0; i < 32; i++) { + out[i * 2] = digits[(sha[i] >> 4) & 0xF]; + out[i * 2 + 1] = digits[sha[i] & 0xF]; + } + out[64] = '\0'; + return out; +} + +int configcompare_paths(const char* const* conf_paths, int n, FILE* out, + configcompare_report_t* rep) +{ + configcompare_report_t local; + memset(&local, 0, sizeof(local)); + local.first_divergent_index = -1; + + if (!conf_paths || n < 2) { + if (rep) + *rep = local; + if (out) + fprintf(out, + "configcompare: nothing to compare (need >= 2 members, " + "got %d)\n", n); + return -EINVAL; + } + if (n > CONFIGCOMPARE_MAX_MEMBERS) { + if (rep) + *rep = local; + if (out) + fprintf(out, "configcompare: too many members (%d > max %d)\n", n, + CONFIGCOMPARE_MAX_MEMBERS); + return -EINVAL; + } + + local.n = n; + int first_ok = -1; + char hexbuf[65]; + + for (int i = 0; i < n; i++) { + int rc = read_member_config_sha256(conf_paths[i], local.sha256[i]); + local.read_rc[i] = rc; + + if (rc != 0) { + if (out) + fprintf(out, + "configcompare: member %d (%s): UNREADABLE: %s\n", i, + conf_paths[i] ? conf_paths[i] : "(null)", + strerror(-rc)); + if (rep) + *rep = local; + /* Short-circuit on the first unreadable member, mirroring + * snapdiff_paths's early-return-on-open-failure style: a + * member we could not read is not evidence of matching OR + * mismatching configuration, so there is no point continuing + * to accumulate a verdict that will be discarded anyway. */ + return rc; + } + + if (out) + fprintf(out, "configcompare: member %d (%s): sha256=%s\n", i, + conf_paths[i], sha256_hex(local.sha256[i], hexbuf)); + + if (first_ok < 0) { + first_ok = i; + } else if (local.first_divergent_index < 0 && + memcmp(local.sha256[i], local.sha256[first_ok], 32) != 0) { + local.first_divergent_index = i; + } + } + + local.all_present = 1; + local.all_identical = (local.first_divergent_index < 0); + + if (rep) + *rep = local; + + if (!local.all_identical) { + if (out) + fprintf(out, + "configcompare: DIVERGENT -- member %d differs from " + "member %d\n", + local.first_divergent_index, first_ok); + return 1; + } + + if (out) + fprintf(out, "configcompare: IDENTICAL across %d members\n", n); + return 0; +} diff --git a/src/controller/testsuite/configcompare.h b/src/controller/testsuite/configcompare.h new file mode 100644 index 00000000..f4783699 --- /dev/null +++ b/src/controller/testsuite/configcompare.h @@ -0,0 +1,74 @@ +#pragma once +/* + * configcompare.h — cross-member active-configuration comparator + * (config-upgrade-snapshot task 8.4, Requirement 4.2). + * + * Compares each cluster member's locally-held ACTIVE configuration content + * hash, recomputed FRESH from that member's own on-disk configuration file + * (never trusted from a cached/embedded tag), and reports whether every + * member agrees. + * + * A new sibling tool to walcompare, NOT an extension of it: walcompare's + * internal model is WAL-entry-indexed (lsn -> term, pcrc) -- an awkward fit + * for a single (tag, sha256) value per member with no LSN dimension + * (design.md "Regression Tooling"). Mirrors snapdiff_paths's exact + * signature/reporting shape instead. + * + * Offline/file-based: no live RPC to any cluster member, consistent with + * every other tool in this family (walcompare, snapdiff, snapverify). + * + * Strictly read-only: opens files O_RDONLY, mutates nothing. + */ +#include +#include + +/* Bound on the number of members a single call can compare -- generous for + * any real cluster size this codebase targets; a caller with more members + * than this is almost certainly passing a malformed argument list. */ +#define CONFIGCOMPARE_MAX_MEMBERS 32 + +typedef struct { + int n; /* member count passed in */ + int read_rc[CONFIGCOMPARE_MAX_MEMBERS]; /* 0 = resolved+hashed ok, + * else -errno; entries at + * or past a short-circuit + * failure are unset (0) + * and must be ignored -- + * see all_present */ + uint8_t sha256[CONFIGCOMPARE_MAX_MEMBERS][32]; /* valid only where + * read_rc[i] == 0 */ + int all_present; /* 1 iff every member (0..n-1) was resolved and + * hashed successfully */ + int all_identical; /* 1 iff all_present && every member's sha256 + * equals the first member's */ + int first_divergent_index; /* -1 if all_identical or !all_present; + * else the first index (after the + * reference, member 0) whose hash + * differs from member 0's */ +} configcompare_report_t; + +/* Compare the active-configuration content hash of `n` members. Each + * `conf_paths[i]` is a member's `workdir/name` directory -- the SAME + * directory persist.c's `read_active_config_bytes()` and config_repl.c's + * `local_config_sha256()` both resolve `conf/current` (and the versioned + * config file it points at) under, matching where this spec's snapshot-embed + * step also reads from. NOT the raw `conf/current` file itself -- this + * function resolves `/conf/current` and `/conf/` + * internally, exactly like those two functions do. + * + * Prints a human-readable report to `out` (may be NULL) and fills `*rep` + * (may be NULL). Returns: + * 0 if every member's config was readable and all hashes are identical + * 1 if every member's config was readable but at least one diverges + * (see `*rep->first_divergent_index` for which) + * <0 (negative errno) if `n < 2` (nothing to compare), `n` exceeds + * CONFIGCOMPARE_MAX_MEMBERS, or any member's config could not be + * resolved/read -- mirrors snapdiff_paths's early-return-on-open- + * failure style: the first unreadable member short-circuits the scan, + * and `*rep` is still filled with whatever was learned about the + * members checked before the failure, so the report can say which + * member and why. A member that could not be read is NOT evidence of + * matching OR mismatching configuration -- it is a distinct third + * outcome from "identical" and "divergent". */ +int configcompare_paths(const char* const* conf_paths, int n, FILE* out, + configcompare_report_t* rep); diff --git a/src/controller/testsuite/controller_test.c b/src/controller/testsuite/controller_test.c index 9d3b173a..13d56d6c 100644 --- a/src/controller/testsuite/controller_test.c +++ b/src/controller/testsuite/controller_test.c @@ -69,6 +69,7 @@ extern void test_waldump(void); extern void test_walcompare(void); extern void test_snapverify(void); extern void test_snapdiff(void); +extern void test_configcompare(void); extern void test_drive_async_config(void); extern void test_slowlane(void); extern void test_commit_admission_pool(void); @@ -228,6 +229,7 @@ int main(int argc, char* argv[]) TEST(test_walcompare); TEST(test_snapverify); TEST(test_snapdiff); + TEST(test_configcompare); TEST(test_drive_async_config); TEST(test_slowlane); TEST(test_commit_admission_pool); diff --git a/src/controller/testsuite/snapverify.c b/src/controller/testsuite/snapverify.c index cd2bc2e7..d801ebb2 100644 --- a/src/controller/testsuite/snapverify.c +++ b/src/controller/testsuite/snapverify.c @@ -9,8 +9,12 @@ #include /* Mirrors persist.c's SNAPSHOT_FORMAT_VERSION / header layout exactly — see - * persist.c write_snapshot() (~line 1758) and load_snapshot() (~line 2325). */ -#define SNAP_FORMAT_VERSION 1u + * persist.c write_snapshot() (~line 1758) and load_snapshot() (~line 2325). + * config-upgrade-snapshot task 1 bumped persist.c's SNAPSHOT_FORMAT_VERSION + * 1->2 (host/queue admin-state frames migrated to capnp, Req 1.9/5.7) -- + * this MUST track that bump, or this tool hard-rejects every real + * checkpoint the current codebase writes (task 8.4 finding). */ +#define SNAP_FORMAT_VERSION 2u #define SNAP_HEADER_MAX (16 + 1 + 8 + 32) /* magic+lsn + optional cfgref tail */ static void fail(snapverify_report_t* rep, int rc, off_t off, const char* why) @@ -78,8 +82,17 @@ int snapverify_path(const char* path, FILE* out, snapverify_report_t* rep) } memcpy(&local.checkpoint_lsn, header + 8, 8); - if (n >= SNAP_HEADER_MAX && header[16] != 0) - local.has_cfgref = 1; + if (n >= SNAP_HEADER_MAX) { + /* Decode the tag/hash UNCONDITIONALLY whenever the header extends + * this far, regardless of the presence byte -- so a caller can + * inspect the raw bytes even in edge cases. has_cfgref keeps its + * existing meaning (whether those bytes are semantically + * meaningful) untouched. */ + memcpy(&local.cfgref_tag, header + 17, 8); + memcpy(local.cfgref_sha, header + 25, 32); + if (header[16] != 0) + local.has_cfgref = 1; + } off_t consumed = (off_t)n; @@ -200,11 +213,11 @@ int snapverify_path(const char* path, FILE* out, snapverify_report_t* rep) local.queue_count = queue_count; for (uint32_t i = 0; i < queue_count; i++) { - off_t len_off = consumed; - uint32_t name_len = 0; - if (read(fd, &name_len, sizeof(name_len)) != (ssize_t)sizeof(name_len)) { + off_t len_off = consumed; + uint32_t plen = 0; + if (read(fd, &plen, sizeof(plen)) != (ssize_t)sizeof(plen)) { close(fd); - fail(&local, -EIO, len_off, "short read on queue name_len"); + fail(&local, -EIO, len_off, "short read on queue payload_len"); if (rep) *rep = local; if (out) @@ -212,13 +225,13 @@ int snapverify_path(const char* path, FILE* out, snapverify_report_t* rep) path, i, local.fail_reason, (intmax_t)len_off); return -EIO; } - consumed += (off_t)sizeof(name_len); + consumed += (off_t)sizeof(plen); - if (name_len == 0 || (off_t)name_len > st.st_size - consumed) { + if (plen == 0 || (off_t)plen > st.st_size - consumed) { close(fd); char why[160]; snprintf(why, sizeof(why), - "bad name_len=%u (remaining=%jd)", name_len, + "bad payload_len=%u (remaining=%jd)", plen, (intmax_t)(st.st_size - consumed)); fail(&local, -EIO, len_off, why); if (rep) @@ -230,9 +243,9 @@ int snapverify_path(const char* path, FILE* out, snapverify_report_t* rep) return -EIO; } - if (lseek(fd, name_len, SEEK_CUR) < 0) { + if (lseek(fd, plen, SEEK_CUR) < 0) { close(fd); - fail(&local, -EIO, consumed, "seek past queue name failed"); + fail(&local, -EIO, consumed, "seek past queue payload failed"); if (rep) *rep = local; if (out) @@ -240,21 +253,138 @@ int snapverify_path(const char* path, FILE* out, snapverify_report_t* rep) local.fail_reason); return -EIO; } - consumed += (off_t)name_len; + consumed += (off_t)plen; + local.queues_ok++; + } - uint32_t status = 0; - if (read(fd, &status, sizeof(status)) != (ssize_t)sizeof(status)) { + /* Host section (config-upgrade-snapshot task 4/8.4, SNAPSHOT_FORMAT_ + * VERSION=2): new after the queue section (design.md "Checkpoint File + * Layout", persist.c load_snapshot_entities()/write_snapshot()). Same + * [payload_len][bytes] per-frame structural shape as the job/queue + * loops above -- this tool validates structure only, never decodes the + * capnp payload. */ + uint32_t host_count = 0; + if (read(fd, &host_count, sizeof(host_count)) != + (ssize_t)sizeof(host_count)) { + close(fd); + fail(&local, -EIO, consumed, "short read on host_count"); + if (rep) + *rep = local; + if (out) + fprintf(out, "snapverify: %s: %s (offset %jd)\n", path, + local.fail_reason, (intmax_t)consumed); + return -EIO; + } + consumed += (off_t)sizeof(host_count); + local.host_count = host_count; + + for (uint32_t i = 0; i < host_count; i++) { + off_t len_off = consumed; + uint32_t plen = 0; + if (read(fd, &plen, sizeof(plen)) != (ssize_t)sizeof(plen)) { close(fd); - fail(&local, -EIO, consumed, "short read on queue status"); + fail(&local, -EIO, len_off, "short read on host payload_len"); if (rep) *rep = local; if (out) - fprintf(out, "snapverify: %s: queue %u: %s (offset %jd)\n", - path, i, local.fail_reason, (intmax_t)consumed); + fprintf(out, "snapverify: %s: host %u: %s (offset %jd)\n", + path, i, local.fail_reason, (intmax_t)len_off); return -EIO; } - consumed += (off_t)sizeof(status); - local.queues_ok++; + consumed += (off_t)sizeof(plen); + + if (plen == 0 || (off_t)plen > st.st_size - consumed) { + close(fd); + char why[160]; + snprintf(why, sizeof(why), + "bad payload_len=%u (remaining=%jd)", plen, + (intmax_t)(st.st_size - consumed)); + fail(&local, -EIO, len_off, why); + if (rep) + *rep = local; + if (out) + fprintf(out, "snapverify: %s: host %u: %s (length field at " + "offset %jd)\n", + path, i, local.fail_reason, (intmax_t)len_off); + return -EIO; + } + + if (lseek(fd, plen, SEEK_CUR) < 0) { + close(fd); + fail(&local, -EIO, consumed, "seek past host payload failed"); + if (rep) + *rep = local; + if (out) + fprintf(out, "snapverify: %s: host %u: %s\n", path, i, + local.fail_reason); + return -EIO; + } + consumed += (off_t)plen; + local.hosts_ok++; + } + + /* Config-bytes section (config-upgrade-snapshot task 2.3/8.4): a + * single flat length-prefixed blob appended LAST, after the host + * section -- NOT a [count][frame...] shape like job/queue/host above, + * since there is exactly one active configuration, never a set + * (persist.c write_snapshot()). The length field itself may be + * legitimately ABSENT (a file that ends right after the host section + * is still a valid, if slightly older, fmt=2-consistent shape) -- + * distinguish that from the length field being present-but-zero + * (persist.c explicitly writes an empty-but-present section when no + * local config bytes exist yet) and from a genuinely truncated length + * field (a short, non-zero partial read -- real corruption). */ + uint32_t cfg_bytes_len = 0; + ssize_t cfg_len_n = read(fd, &cfg_bytes_len, sizeof(cfg_bytes_len)); + if (cfg_len_n == 0) { + /* Clean EOF right after the host section -- no config-bytes + * section present at all. Not a defect. */ + local.has_config_bytes_section = 0; + } else if (cfg_len_n != (ssize_t)sizeof(cfg_bytes_len)) { + close(fd); + fail(&local, -EIO, consumed, "short read on config_bytes_len"); + if (rep) + *rep = local; + if (out) + fprintf(out, "snapverify: %s: %s (offset %jd)\n", path, + local.fail_reason, (intmax_t)consumed); + return -EIO; + } else { + off_t len_off = consumed; + consumed += (off_t)sizeof(cfg_bytes_len); + local.has_config_bytes_section = 1; + local.config_bytes_len = cfg_bytes_len; + + if ((off_t)cfg_bytes_len > st.st_size - consumed) { + close(fd); + char why[160]; + snprintf(why, sizeof(why), + "bad config_bytes_len=%u (remaining=%jd)", cfg_bytes_len, + (intmax_t)(st.st_size - consumed)); + fail(&local, -EIO, len_off, why); + if (rep) + *rep = local; + if (out) + fprintf(out, "snapverify: %s: %s (length field at offset " + "%jd)\n", + path, local.fail_reason, (intmax_t)len_off); + return -EIO; + } + + if (cfg_bytes_len > 0) { + if (lseek(fd, cfg_bytes_len, SEEK_CUR) < 0) { + close(fd); + fail(&local, -EIO, consumed, + "seek past config bytes failed"); + if (rep) + *rep = local; + if (out) + fprintf(out, "snapverify: %s: %s\n", path, + local.fail_reason); + return -EIO; + } + consumed += (off_t)cfg_bytes_len; + } } close(fd); @@ -277,8 +407,11 @@ int snapverify_path(const char* path, FILE* out, snapverify_report_t* rep) if (out) fprintf(out, "snapverify: %s: OK (lsn=%" PRIu64 " fmt=%u jobs=%u queues=%u " - "size=%jd)\n", + "hosts=%u cfgref_tag=%016" PRIx64 " cfg_bytes_len=%u " + "has_cfg_bytes=%d size=%jd)\n", path, local.checkpoint_lsn, local.format_version, - local.job_count, local.queue_count, (intmax_t)local.file_size); + local.job_count, local.queue_count, local.host_count, + local.cfgref_tag, local.config_bytes_len, + local.has_config_bytes_section, (intmax_t)local.file_size); return 0; } diff --git a/src/controller/testsuite/snapverify.h b/src/controller/testsuite/snapverify.h index 3855e341..ca00eee1 100644 --- a/src/controller/testsuite/snapverify.h +++ b/src/controller/testsuite/snapverify.h @@ -1,14 +1,20 @@ #pragma once /* * snapverify.h — read-only checkpoint-file structural inspector/validator - * (bug-1389 triage: OPCode_snapshotTransfer chunk-content corruption). + * (bug-1389 triage: OPCode_snapshotTransfer chunk-content corruption; + * extended by config-upgrade-snapshot task 8.4, Requirement 4.2, for the + * SNAPSHOT_FORMAT_VERSION=2 host + config-bytes sections and to decode -- + * not merely report the presence of -- the header's cfgref tag/hash). * * Parses the on-disk checkpoint layout persist.c's load_snapshot() / * load_snapshot_entities() use (design.md "Checkpoint File Layout") WITHOUT - * restoring any job/queue into a live cluster_t — a pure structural walk, so - * it needs no jobmgr/qmgr context and is safe to run standalone against a - * snap-*.capnp (locally generated) or snap-*.bin (received via - * ha_handle_snapshot_chunk, ha.c) file on disk. + * restoring any job/queue/host into a live cluster_t — a pure structural + * walk, so it needs no jobmgr/qmgr/workermgr context and is safe to run + * standalone against a snap-*.capnp (locally generated) or snap-*.bin + * (received via ha_handle_snapshot_chunk, ha.c) file on disk. This tool + * validates STRUCTURE only, never content: job/queue/host frame payloads + * and the config-bytes section are walked past via their length prefix, + * never capnp-decoded or interpreted. * * Strictly read-only: opens O_RDONLY, mutates nothing. */ @@ -18,7 +24,17 @@ typedef struct { uint64_t checkpoint_lsn; - int has_cfgref; /* config-ref tail present in the header */ + int has_cfgref; /* config-ref tail present in the header + * (header[16] != 0) -- semantic presence, + * distinct from cfgref_tag/cfgref_sha below + * being decoded */ + uint64_t cfgref_tag; /* header cfgref tag, decoded whenever the + * header extends to SNAP_HEADER_MAX bytes, + * REGARDLESS of has_cfgref -- meaningful + * only when has_cfgref is also 1 */ + uint8_t cfgref_sha[32]; /* header cfgref sha256, same decode- + * unconditionally-but-gate-meaning-on- + * has_cfgref discipline as cfgref_tag */ int has_entity_section; /* fmt + job_count present past the header; * 0 = legacy position-only checkpoint */ uint32_t format_version; @@ -27,6 +43,22 @@ typedef struct { uint32_t queue_count; /* as declared in the file (0 if legacy, or if the * job-frame walk failed before reaching it) */ uint32_t queues_ok; /* queue frames structurally walked before any failure */ + uint32_t host_count; /* as declared in the file (0 if legacy, or if an + * earlier section's walk failed before reaching + * it) */ + uint32_t hosts_ok; /* host frames structurally walked before any failure */ + int has_config_bytes_section; /* 1 iff the trailing config-bytes + * section's 4-byte length field + * itself was present and readable + * (i.e. the file extends past the + * host section at all) -- TRUE even + * when that length decodes to 0, and + * FALSE (not a failure) when the + * file simply ends right after the + * host section */ + uint32_t config_bytes_len; /* decoded length of the config-bytes + * section; valid only when + * has_config_bytes_section is 1 */ off_t file_size; off_t consumed; /* bytes structurally accounted for */ int ok; /* 1 = well-formed, consumed == file_size */ diff --git a/src/controller/testsuite/test.c b/src/controller/testsuite/test.c index 1a329fcc..0161b0e6 100644 --- a/src/controller/testsuite/test.c +++ b/src/controller/testsuite/test.c @@ -27,6 +27,7 @@ #include "waldump.h" /* offline WAL inspector (cmd_waldump) */ #include "snapdiff.h" /* byte-level checkpoint-file comparator (cmd_snapdiff) */ #include "snapverify.h" /* offline checkpoint-file structural validator (cmd_snapverify) */ +#include "configcompare.h" /* cross-member active-config comparator (cmd_configcompare) */ #include "xce/cluster.h" /* xce_parse_peers, xce_cluster_config_t */ #include "xce/types.h" @@ -133,6 +134,7 @@ extern int cmd_waldump(int, char*[]); extern int cmd_walcompare(int, char*[]); extern int cmd_snapverify(int, char*[]); extern int cmd_snapdiff(int, char*[]); +extern int cmd_configcompare(int, char*[]); static int worker_house_keeping(void*); /* Session verdict / loop state (xce-test-e2e-primitives task 1.1). @@ -194,6 +196,7 @@ static command_t top_commands[] = { { "walcompare", cmd_walcompare, "Diff member committed WAL prefixes" }, { "snapverify", cmd_snapverify, "Verify checkpoint file structure" }, { "snapdiff", cmd_snapdiff, "Byte-diff two checkpoint files" }, + { "configcompare", cmd_configcompare, "Compare active config hash across members" }, { "quit", cmd_quit, "Quit xce test" }, { (char*)NULL, (cmd_func_t)NULL, (char*)NULL } }; @@ -237,6 +240,7 @@ static command_t client_commands[] = { { "walcompare", cmd_walcompare, "Diff member committed WAL prefixes" }, { "snapverify", cmd_snapverify, "Verify checkpoint file structure" }, { "snapdiff", cmd_snapdiff, "Byte-diff two checkpoint files" }, + { "configcompare", cmd_configcompare, "Compare active config hash across members" }, { "shutdown", cmd_shutdown, "shutdown xce controller" }, { "jobrun", cmd_jobrun, "run a job" }, { "jobsub", cmd_jobsub, "submit a job" }, @@ -856,6 +860,27 @@ int cmd_snapdiff(int argc, char* argv[]) return (rc == 0) ? 0 : -1; } +/* configcompare [more...] — cross-member + * active-configuration comparator (config-upgrade-snapshot task 8.4, Req + * 4.2). Each path is a member's `workdir/name` conf-root directory (the + * same directory persist.c's read_active_config_bytes() and config_repl.c's + * local_config_sha256() resolve `conf/current` under) — NOT a raw config + * file path. Recomputes each member's active-config sha256 fresh from its + * own on-disk bytes and reports whether all members match. Returns 0 if + * all members are present and identical, nonzero if any member is + * unreadable or divergent. */ +int cmd_configcompare(int argc, char* argv[]) +{ + if (argc < 3) { + fprintf(stderr, "usage: configcompare [more...]\n"); + return -1; + } + + configcompare_report_t rep; + int rc = configcompare_paths((const char* const*)&argv[1], argc - 1, stdout, &rep); + return (rc == 0) ? 0 : -1; +} + int cmd_up(int argc, char* argv[]) { if (active_ctx->parent != NULL) { diff --git a/src/controller/testsuite/test_configcompare.c b/src/controller/testsuite/test_configcompare.c new file mode 100644 index 00000000..e9eff213 --- /dev/null +++ b/src/controller/testsuite/test_configcompare.c @@ -0,0 +1,217 @@ +#define _GNU_SOURCE +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "configcompare.h" + +static char g_dir[256]; + +static void fresh_dir(void) +{ + strcpy(g_dir, "/tmp/xce_configcompare_XXXXXX"); + TEST_ASSERT_NOT_NULL(mkdtemp(g_dir)); +} + +static void cleanup_dir(void) +{ + char cmd[512]; + snprintf(cmd, sizeof(cmd), "rm -rf %s", g_dir); + system(cmd); +} + +/* Creates //conf/ (a member's `workdir/name` directory, mirroring + * conf_dir = "%s/%s", myconf->workdir, myconf->name in persist.c / + * config_repl.c) and writes the resulting path into out_path. Does NOT touch + * config_repl.c or persist.c -- these are hand-written fixture files so the + * test exercises configcompare.c's OWN resolve-then-read logic, not a + * different code path's. */ +static void make_member_dir(const char* root, const char* name, + char out_path[512]) +{ + snprintf(out_path, 512, "%s/%s", root, name); + TEST_ASSERT_EQUAL_INT(0, mkdir(out_path, 0700)); + char conf_dir[560]; + snprintf(conf_dir, sizeof(conf_dir), "%s/conf", out_path); + TEST_ASSERT_EQUAL_INT(0, mkdir(conf_dir, 0700)); +} + +static void write_conf_current(const char* member_path, const char* hexsuffix) +{ + char ptr_path[600]; + snprintf(ptr_path, sizeof(ptr_path), "%s/conf/current", member_path); + FILE* f = fopen(ptr_path, "w"); + TEST_ASSERT_NOT_NULL(f); + fprintf(f, "cluster.bin.%s\n", hexsuffix); + fclose(f); +} + +static void write_conf_blob(const char* member_path, const char* hexsuffix, + const uint8_t* bytes, size_t len) +{ + char blob_path[600]; + snprintf(blob_path, sizeof(blob_path), "%s/conf/cluster.bin.%s", + member_path, hexsuffix); + FILE* f = fopen(blob_path, "wb"); + TEST_ASSERT_NOT_NULL(f); + if (len > 0) + TEST_ASSERT_EQUAL_INT((int)len, (int)fwrite(bytes, 1, len, f)); + fclose(f); +} + +/* Full happy-path member: dir + conf/current + the versioned blob it + * points at. */ +static void setup_member_ok(const char* root, const char* name, + const char* hexsuffix, const uint8_t* bytes, + size_t len, char out_path[512]) +{ + make_member_dir(root, name, out_path); + write_conf_current(out_path, hexsuffix); + write_conf_blob(out_path, hexsuffix, bytes, len); +} + +static void test_two_members_identical_config_reports_identical(void) +{ + fresh_dir(); + const uint8_t data[] = "identical config bytes for both members"; + + char m0[512], m1[512]; + setup_member_ok(g_dir, "m0", "aaaaaaaaaaaa", data, sizeof(data) - 1, m0); + setup_member_ok(g_dir, "m1", "bbbbbbbbbbbb", data, sizeof(data) - 1, m1); + + const char* paths[2] = { m0, m1 }; + configcompare_report_t rep; + int rc = configcompare_paths(paths, 2, stdout, &rep); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_INT(1, rep.all_present); + TEST_ASSERT_EQUAL_INT(1, rep.all_identical); + TEST_ASSERT_EQUAL_INT(-1, rep.first_divergent_index); + cleanup_dir(); +} + +static void test_two_members_differing_config_reports_divergent(void) +{ + fresh_dir(); + const uint8_t data0[] = "config bytes version A"; + const uint8_t data1[] = "config bytes version B, quite different"; + + char m0[512], m1[512]; + setup_member_ok(g_dir, "m0", "aaaaaaaaaaaa", data0, sizeof(data0) - 1, m0); + setup_member_ok(g_dir, "m1", "bbbbbbbbbbbb", data1, sizeof(data1) - 1, m1); + + const char* paths[2] = { m0, m1 }; + configcompare_report_t rep; + int rc = configcompare_paths(paths, 2, stdout, &rep); + + TEST_ASSERT_EQUAL_INT(1, rc); + TEST_ASSERT_EQUAL_INT(1, rep.all_present); + TEST_ASSERT_EQUAL_INT(0, rep.all_identical); + TEST_ASSERT_EQUAL_INT(1, rep.first_divergent_index); + cleanup_dir(); +} + +/* A member with no conf/current pointer file at all must be reported as + * unreadable (read_rc != 0), never silently folded into "identical" or + * "divergent" against the two members that DO agree. */ +static void test_missing_conf_current_reported_unreadable_not_divergent(void) +{ + fresh_dir(); + const uint8_t data[] = "shared config bytes across the healthy members"; + + char m0[512], m1[512], m2[512]; + setup_member_ok(g_dir, "m0", "aaaaaaaaaaaa", data, sizeof(data) - 1, m0); + /* m1: conf/ dir exists but conf/current was never written. */ + make_member_dir(g_dir, "m1", m1); + setup_member_ok(g_dir, "m2", "cccccccccccc", data, sizeof(data) - 1, m2); + + const char* paths[3] = { m0, m1, m2 }; + configcompare_report_t rep; + int rc = configcompare_paths(paths, 3, stdout, &rep); + + TEST_ASSERT_LESS_THAN_INT(0, rc); + TEST_ASSERT_EQUAL_INT(0, rep.read_rc[0]); + TEST_ASSERT_NOT_EQUAL(0, rep.read_rc[1]); + TEST_ASSERT_EQUAL_INT(0, rep.all_present); + cleanup_dir(); +} + +/* A member whose conf/current points at a versioned file that does not + * exist on disk gets the same "unreadable" treatment as a missing pointer + * file -- neither is evidence of matching or mismatching configuration. */ +static void test_current_points_to_missing_versioned_file_reported_unreadable(void) +{ + fresh_dir(); + const uint8_t data[] = "shared config bytes, dangling pointer case"; + + char m0[512], m1[512]; + setup_member_ok(g_dir, "m0", "aaaaaaaaaaaa", data, sizeof(data) - 1, m0); + /* m1: conf/current points at cluster.bin.dddddddddddd, but that + * versioned file was never written. */ + make_member_dir(g_dir, "m1", m1); + write_conf_current(m1, "dddddddddddd"); + + const char* paths[2] = { m0, m1 }; + configcompare_report_t rep; + int rc = configcompare_paths(paths, 2, stdout, &rep); + + TEST_ASSERT_LESS_THAN_INT(0, rc); + TEST_ASSERT_EQUAL_INT(0, rep.read_rc[0]); + TEST_ASSERT_NOT_EQUAL(0, rep.read_rc[1]); + cleanup_dir(); +} + +/* Basic determinism sanity check: comparing the same two byte-identical + * files twice in a row yields the same hash both times. */ +static void test_repeated_comparison_of_identical_files_is_deterministic(void) +{ + fresh_dir(); + const uint8_t data[] = "deterministic hash check bytes, fixed content"; + + char m0[512], m1[512]; + setup_member_ok(g_dir, "m0", "aaaaaaaaaaaa", data, sizeof(data) - 1, m0); + setup_member_ok(g_dir, "m1", "bbbbbbbbbbbb", data, sizeof(data) - 1, m1); + + const char* paths[2] = { m0, m1 }; + configcompare_report_t rep1, rep2; + int rc1 = configcompare_paths(paths, 2, NULL, &rep1); + int rc2 = configcompare_paths(paths, 2, NULL, &rep2); + + TEST_ASSERT_EQUAL_INT(0, rc1); + TEST_ASSERT_EQUAL_INT(0, rc2); + TEST_ASSERT_EQUAL_INT(0, memcmp(rep1.sha256[0], rep2.sha256[0], 32)); + TEST_ASSERT_EQUAL_INT(0, memcmp(rep1.sha256[1], rep2.sha256[1], 32)); + cleanup_dir(); +} + +/* Fewer than two members means there is nothing to compare -- a negative + * errno, not a degenerate "identical" verdict. */ +static void test_n_less_than_2_returns_negative_errno(void) +{ + const char* one_path[1] = { "/tmp/xce_configcompare_irrelevant" }; + configcompare_report_t rep; + + int rc0 = configcompare_paths(NULL, 0, NULL, &rep); + TEST_ASSERT_LESS_THAN_INT(0, rc0); + + int rc1 = configcompare_paths(one_path, 1, NULL, &rep); + TEST_ASSERT_LESS_THAN_INT(0, rc1); +} + +void test_configcompare(void) +{ + UnitySetTestFile(__FILE__); + RUN_TEST(test_two_members_identical_config_reports_identical); + RUN_TEST(test_two_members_differing_config_reports_divergent); + RUN_TEST(test_missing_conf_current_reported_unreadable_not_divergent); + RUN_TEST(test_current_points_to_missing_versioned_file_reported_unreadable); + RUN_TEST(test_repeated_comparison_of_identical_files_is_deterministic); + RUN_TEST(test_n_less_than_2_returns_negative_errno); +} diff --git a/src/controller/testsuite/test_snapverify.c b/src/controller/testsuite/test_snapverify.c index 73b66adf..84651aea 100644 --- a/src/controller/testsuite/test_snapverify.c +++ b/src/controller/testsuite/test_snapverify.c @@ -26,15 +26,41 @@ static void cleanup_dir(void) } /* Writes a checkpoint file matching persist.c write_snapshot()'s exact - * on-disk layout (magic+lsn+cfgref(41, all-zero/absent) + fmt + job_count + - * per-job[len+buf] + queue_count + per-queue[namelen+name+status]). - * job_lens/job_count and queue_names/queue_count are caller-controlled so - * tests can inject a specific defect. `truncate_last_bytes` drops that many - * bytes off the END of the fully-written file (0 = write it whole). */ -static void write_snap(const char* path, uint64_t lsn, uint32_t fmt, - const uint32_t* job_lens, uint32_t job_count, - const char** queue_names, const uint32_t* queue_status, - uint32_t queue_count, int truncate_last_bytes) + * on-disk layout for SNAPSHOT_FORMAT_VERSION=2 (magic+lsn+cfgref(41) + fmt + + * job_count + per-job[payload_len+buf] + queue_count + per-queue + * [payload_len+opaque_bytes] + host_count + per-host[payload_len+buf] + + * cfg_bytes_len + cfg_bytes). + * + * Queue/host frame CONTENT is irrelevant here -- snapverify validates + * structure only (length-prefix walk), never capnp-decodes a payload -- so + * queue frames are written as an opaque [payload_len][name_len+name+status] + * blob (convenient filler reusing the queue_names/queue_status test + * parameters, NOT a real capnp QueueAdminState encoding) and host frames as + * an opaque [payload_len][0xCD-filled bytes] blob. + * + * job_lens/job_count, queue_names/queue_status/queue_count, and + * host_lens/host_count are all caller-controlled so tests can inject a + * specific defect or size at a specific section. cfg_bytes_len controls the + * trailing config-bytes section's declared length (content is 0xEF filler); + * pass 0 for "section present but empty" -- the section's 4-byte length + * field is ALWAYS written by this helper (mirrors persist.c writing it on + * every checkpoint, even when there are no local config bytes yet). Tests + * that need to inject a real header cfgref, or a file with NO config-bytes + * length field at all, use write_snap_full()/a hand-written file instead + * (see below). + * + * `truncate_last_bytes` drops that many bytes off the END of the fully- + * written file (0 = write it whole). */ +static void write_snap_full(const char* path, uint64_t lsn, + int cfgref_present, uint64_t cfgref_tag, + const uint8_t* cfgref_sha /* 32 bytes, or NULL + * for all-zero */, + uint32_t fmt, + const uint32_t* job_lens, uint32_t job_count, + const char** queue_names, + const uint32_t* queue_status, uint32_t queue_count, + const uint32_t* host_lens, uint32_t host_count, + uint32_t cfg_bytes_len, int truncate_last_bytes) { FILE* fp = fopen(path, "wb"); TEST_ASSERT_NOT_NULL(fp); @@ -43,6 +69,10 @@ static void write_snap(const char* path, uint64_t lsn, uint32_t fmt, memset(header, 0, sizeof(header)); memcpy(header, "XCESNAP\0", 8); memcpy(header + 8, &lsn, 8); + header[16] = (char)(cfgref_present ? 1 : 0); + memcpy(header + 17, &cfgref_tag, 8); + if (cfgref_sha) + memcpy(header + 25, cfgref_sha, 32); fwrite(header, 1, sizeof(header), fp); fwrite(&fmt, sizeof(fmt), 1, fp); @@ -57,11 +87,31 @@ static void write_snap(const char* path, uint64_t lsn, uint32_t fmt, fwrite(&queue_count, sizeof(queue_count), 1, fp); for (uint32_t i = 0; i < queue_count; i++) { - uint32_t nlen = (uint32_t)strlen(queue_names[i]); + uint32_t nlen = (uint32_t)strlen(queue_names[i]); + uint32_t payload_len = (uint32_t)(sizeof(nlen) + nlen + + sizeof(queue_status[i])); + fwrite(&payload_len, sizeof(payload_len), 1, fp); fwrite(&nlen, sizeof(nlen), 1, fp); fwrite(queue_names[i], 1, nlen, fp); fwrite(&queue_status[i], sizeof(queue_status[i]), 1, fp); } + + fwrite(&host_count, sizeof(host_count), 1, fp); + for (uint32_t i = 0; i < host_count; i++) { + fwrite(&host_lens[i], sizeof(host_lens[i]), 1, fp); + uint8_t* buf = malloc(host_lens[i]); + memset(buf, 0xCD, host_lens[i]); + fwrite(buf, 1, host_lens[i], fp); + free(buf); + } + + fwrite(&cfg_bytes_len, sizeof(cfg_bytes_len), 1, fp); + if (cfg_bytes_len > 0) { + uint8_t* buf = malloc(cfg_bytes_len); + memset(buf, 0xEF, cfg_bytes_len); + fwrite(buf, 1, cfg_bytes_len, fp); + free(buf); + } fclose(fp); if (truncate_last_bytes > 0) { @@ -74,6 +124,21 @@ static void write_snap(const char* path, uint64_t lsn, uint32_t fmt, } } +/* Convenience wrapper for the (common) case of no header cfgref -- mirrors + * every pre-task-8.4 call site's shape, just with the new host_lens/ + * host_count/cfg_bytes_len parameters appended. */ +static void write_snap(const char* path, uint64_t lsn, uint32_t fmt, + const uint32_t* job_lens, uint32_t job_count, + const char** queue_names, const uint32_t* queue_status, + uint32_t queue_count, + const uint32_t* host_lens, uint32_t host_count, + uint32_t cfg_bytes_len, int truncate_last_bytes) +{ + write_snap_full(path, lsn, 0, 0, NULL, fmt, job_lens, job_count, + queue_names, queue_status, queue_count, host_lens, + host_count, cfg_bytes_len, truncate_last_bytes); +} + static void test_legacy_position_only_file_is_ok(void) { fresh_dir(); @@ -104,21 +169,29 @@ static void test_entity_bearing_well_formed_file_is_ok(void) char path[512]; snprintf(path, sizeof(path), "%s/snap-0000000000000064.capnp", g_dir); - uint32_t job_lens[3] = { 100, 200, 50 }; + uint32_t job_lens[3] = { 100, 200, 50 }; const char* qnames[2] = { "gpu", "cpu" }; - uint32_t qstatus[2] = { 1, 0 }; - write_snap(path, 100, 1, job_lens, 3, qnames, qstatus, 2, 0); + uint32_t qstatus[2] = { 1, 0 }; + /* fmt=2 (SNAPSHOT_FORMAT_VERSION), no hosts, cfg-bytes section present + * but empty -- must still be fully consumed to EOF by the fmt-2-aware + * parser (task 8.4 fix: this file shape is what a real checkpoint + * written by the current codebase looks like). */ + write_snap(path, 100, 2, job_lens, 3, qnames, qstatus, 2, NULL, 0, 0, 0); snapverify_report_t rep; int rc = snapverify_path(path, stdout, &rep); TEST_ASSERT_EQUAL_INT(0, rc); TEST_ASSERT_EQUAL_INT(1, rep.ok); TEST_ASSERT_EQUAL_INT(1, rep.has_entity_section); - TEST_ASSERT_EQUAL_UINT32(1, rep.format_version); + TEST_ASSERT_EQUAL_UINT32(2, rep.format_version); TEST_ASSERT_EQUAL_UINT32(3, rep.job_count); TEST_ASSERT_EQUAL_UINT32(3, rep.jobs_ok); TEST_ASSERT_EQUAL_UINT32(2, rep.queue_count); TEST_ASSERT_EQUAL_UINT32(2, rep.queues_ok); + TEST_ASSERT_EQUAL_UINT32(0, rep.host_count); + TEST_ASSERT_EQUAL_UINT32(0, rep.hosts_ok); + TEST_ASSERT_EQUAL_INT(1, rep.has_config_bytes_section); + TEST_ASSERT_EQUAL_UINT32(0, rep.config_bytes_len); TEST_ASSERT_EQUAL_INT(-1, (int)rep.fail_offset); cleanup_dir(); } @@ -165,7 +238,8 @@ static void test_unrecognized_format_version_is_einval(void) char path[512]; snprintf(path, sizeof(path), "%s/snap-0000000000000001.capnp", g_dir); uint32_t job_lens[1] = { 10 }; - write_snap(path, 1, 99 /* bogus version */, job_lens, 1, NULL, NULL, 0, 0); + write_snap(path, 1, 99 /* bogus version */, job_lens, 1, NULL, NULL, 0, + NULL, 0, 0, 0); snapverify_report_t rep; int rc = snapverify_path(path, stdout, &rep); @@ -187,7 +261,7 @@ static void test_corrupt_job_payload_len_reports_exact_failure_point(void) snprintf(path, sizeof(path), "%s/snap-0000000000000002.capnp", g_dir); uint32_t job_lens[4] = { 50, 60, 70, 80 }; - write_snap(path, 2, 1, job_lens, 4, NULL, NULL, 0, 0); + write_snap(path, 2, 2, job_lens, 4, NULL, NULL, 0, NULL, 0, 0, 0); /* Corrupt job 2's length field (3rd job, after header(57)+fmt(4)+ * job_count(4)+job0(4+50)+job1(4+60) = 57+4+4+54+64 = 183) to a value @@ -217,6 +291,166 @@ static void test_missing_path_errors(void) TEST_ASSERT_LESS_THAN_INT(0, rc); } +/* task 8.4: the header's cfgref tag/sha must be DECODED (not merely + * reported present) -- a genuine round-trip assertion against concrete + * injected values, not just "non-zero". */ +static void test_cfgref_tag_and_sha_decode_roundtrip(void) +{ + fresh_dir(); + char path[512]; + snprintf(path, sizeof(path), "%s/snap-00000000000000c8.capnp", g_dir); + + uint64_t tag = 0x1122334455667788ULL; + uint8_t sha[32]; + memset(sha, 0xAA, 16); + memset(sha + 16, 0xBB, 16); + + write_snap_full(path, 200, 1 /* cfgref_present */, tag, sha, 2, NULL, 0, + NULL, NULL, 0, NULL, 0, 0, 0); + + snapverify_report_t rep; + int rc = snapverify_path(path, stdout, &rep); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_INT(1, rep.ok); + TEST_ASSERT_EQUAL_INT(1, rep.has_cfgref); + TEST_ASSERT_EQUAL_UINT64(tag, rep.cfgref_tag); + TEST_ASSERT_EQUAL_MEMORY(sha, rep.cfgref_sha, 32); + cleanup_dir(); +} + +/* task 8.4: host frames get their own structural walk + counts, same shape + * as the job/queue loops. */ +static void test_host_frames_are_walked_and_counted(void) +{ + fresh_dir(); + char path[512]; + snprintf(path, sizeof(path), "%s/snap-00000000000000c9.capnp", g_dir); + + uint32_t host_lens[3] = { 30, 45, 12 }; + write_snap(path, 201, 2, NULL, 0, NULL, NULL, 0, host_lens, 3, 0, 0); + + snapverify_report_t rep; + int rc = snapverify_path(path, stdout, &rep); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_INT(1, rep.ok); + TEST_ASSERT_EQUAL_UINT32(3, rep.host_count); + TEST_ASSERT_EQUAL_UINT32(3, rep.hosts_ok); + cleanup_dir(); +} + +/* task 8.4: a non-empty trailing config-bytes section is reported with its + * concrete declared length. */ +static void test_nonempty_config_bytes_section_is_reported(void) +{ + fresh_dir(); + char path[512]; + snprintf(path, sizeof(path), "%s/snap-00000000000000ca.capnp", g_dir); + + write_snap(path, 202, 2, NULL, 0, NULL, NULL, 0, NULL, 0, 42, 0); + + snapverify_report_t rep; + int rc = snapverify_path(path, stdout, &rep); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_INT(1, rep.ok); + TEST_ASSERT_EQUAL_INT(1, rep.has_config_bytes_section); + TEST_ASSERT_EQUAL_UINT32(42, rep.config_bytes_len); + cleanup_dir(); +} + +/* task 8.4: an EXPLICIT zero-length config-bytes section (the 4-byte length + * field IS present and reads 0, immediately followed by EOF) must be + * distinguishable from a file that has no config-bytes section at all -- + * both are ok==1, but only the explicit-zero case has + * has_config_bytes_section==1. */ +static void test_explicit_zero_length_config_bytes_section_is_ok(void) +{ + fresh_dir(); + char path[512]; + snprintf(path, sizeof(path), "%s/snap-00000000000000cb.capnp", g_dir); + + write_snap(path, 203, 2, NULL, 0, NULL, NULL, 0, NULL, 0, 0, 0); + + snapverify_report_t rep; + int rc = snapverify_path(path, stdout, &rep); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_INT(1, rep.ok); + TEST_ASSERT_EQUAL_INT(1, rep.has_config_bytes_section); + TEST_ASSERT_EQUAL_UINT32(0, rep.config_bytes_len); + cleanup_dir(); +} + +/* task 8.4: a file that simply ENDS right after the host section (no + * config_bytes_len field at all -- a slightly-older-but-still-fmt-2- + * consistent shape) is still ok==1, but has_config_bytes_section==0 -- + * the counterpart to the explicit-zero-length case above, proving the two + * are genuinely distinguishable rather than collapsed into one "empty" + * bucket. Hand-written (not via write_snap_full, which always writes the + * length field) to produce exactly this shape. */ +static void test_no_config_bytes_section_at_all_is_still_ok(void) +{ + fresh_dir(); + char path[512]; + snprintf(path, sizeof(path), "%s/snap-00000000000000cc.capnp", g_dir); + + FILE* fp = fopen(path, "wb"); + TEST_ASSERT_NOT_NULL(fp); + + char header[16 + 1 + 8 + 32]; + memset(header, 0, sizeof(header)); + memcpy(header, "XCESNAP\0", 8); + uint64_t lsn = 204; + memcpy(header + 8, &lsn, 8); + fwrite(header, 1, sizeof(header), fp); + + uint32_t fmt = 2; + uint32_t zero = 0; + fwrite(&fmt, sizeof(fmt), 1, fp); /* format_version */ + fwrite(&zero, sizeof(zero), 1, fp); /* job_count = 0 */ + fwrite(&zero, sizeof(zero), 1, fp); /* queue_count = 0 */ + fwrite(&zero, sizeof(zero), 1, fp); /* host_count = 0 */ + /* No config_bytes_len field at all -- file ends right here. */ + fclose(fp); + + snapverify_report_t rep; + int rc = snapverify_path(path, stdout, &rep); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_INT(1, rep.ok); + TEST_ASSERT_EQUAL_INT(0, rep.has_config_bytes_section); + cleanup_dir(); +} + +/* task 8.4: same corrupt-length-field diagnostic as + * test_corrupt_job_payload_len_reports_exact_failure_point, in the host + * section instead of the job section. */ +static void test_corrupt_host_payload_len_reports_exact_failure_point(void) +{ + fresh_dir(); + char path[512]; + snprintf(path, sizeof(path), "%s/snap-00000000000000cd.capnp", g_dir); + + uint32_t host_lens[3] = { 20, 30, 40 }; + write_snap(path, 205, 2, NULL, 0, NULL, NULL, 0, host_lens, 3, 0, 0); + + /* header(57) + fmt(4) + job_count(4) + [0 jobs] + queue_count(4) + + * [0 queues] + host_count(4) + host0(4+20) + host1(4+30) = offset of + * host2's payload_len field. */ + long host2_len_off = 57 + 4 + 4 + 4 + 4 + (4 + 20) + (4 + 30); + FILE* fp = fopen(path, "r+b"); + TEST_ASSERT_NOT_NULL(fp); + fseek(fp, host2_len_off, SEEK_SET); + uint32_t garbage = 0x7FFFFFFF; + fwrite(&garbage, sizeof(garbage), 1, fp); + fclose(fp); + + snapverify_report_t rep; + int rc = snapverify_path(path, stdout, &rep); + TEST_ASSERT_EQUAL_INT(-EIO, rc); + TEST_ASSERT_EQUAL_INT(0, rep.ok); + TEST_ASSERT_EQUAL_UINT32(2, rep.hosts_ok); /* host 0, host 1 parsed fine */ + TEST_ASSERT_EQUAL_INT(host2_len_off, (int)rep.fail_offset); + cleanup_dir(); +} + void test_snapverify(void) { UnitySetTestFile(__FILE__); @@ -227,4 +461,10 @@ void test_snapverify(void) RUN_TEST(test_unrecognized_format_version_is_einval); RUN_TEST(test_corrupt_job_payload_len_reports_exact_failure_point); RUN_TEST(test_missing_path_errors); + RUN_TEST(test_cfgref_tag_and_sha_decode_roundtrip); + RUN_TEST(test_host_frames_are_walked_and_counted); + RUN_TEST(test_nonempty_config_bytes_section_is_reported); + RUN_TEST(test_explicit_zero_length_config_bytes_section_is_ok); + RUN_TEST(test_no_config_bytes_section_at_all_is_still_ok); + RUN_TEST(test_corrupt_host_payload_len_reports_exact_failure_point); }