From f795cf22120ac0b2652cfe5879527936ab434106 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 1 Aug 2026 01:16:47 -0500 Subject: [PATCH 1/5] Removed the fixed size limit on the ts_key measurement name file Nova_DumpSlots() appended to a 16 KB stack buffer with strlcat(), which truncates silently. Line length depends on the name, description and units a measurement promise gives the slot, so there is no useful bound to size against; at CF_OBSERVABLES=300 the file is already 8474 bytes with only the built-in observables in it. It is now built in a Writer that grows. Once a record is written only through the highest slot in use, a truncated ts_key costs the measurements in the lost slots, not just their names. Ticket: ENT-14329 Changelog: cf-monitord no longer silently truncates the ts_key measurement name file Co-Authored-By: Claude Opus 5 (1M context) --- cf-monitord/monitoring.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/cf-monitord/monitoring.c b/cf-monitord/monitoring.c index 52d3b4fc36..497faf64af 100644 --- a/cf-monitord/monitoring.c +++ b/cf-monitord/monitoring.c @@ -93,14 +93,14 @@ void NovaNamedEvent(const char *eventname, double value) static void Nova_DumpSlots(void) { -#define MAX_KEY_FILE_SIZE 16384 /* usually around 4000, cannot grow much */ - char filename[CF_BUFSIZE]; int i; snprintf(filename, CF_BUFSIZE - 1, "%s%cts_key", GetStateDir(), FILE_SEPARATOR); - char file_contents_new[MAX_KEY_FILE_SIZE] = {0}; + /* Line length depends on the name, description and units a measurement + * promise gives the slot, so there is no useful bound to size a buffer to. */ + Writer *contents = StringWriter(); for (i = 0; i < CF_OBSERVABLES; i++) { @@ -120,12 +120,15 @@ static void Nova_DumpSlots(void) snprintf(line, sizeof(line), "%d,spare,unused\n", i); } - strlcat(file_contents_new, line, sizeof(file_contents_new)); + WriterWrite(contents, line); } + char *file_contents_new = StringWriterClose(contents); + bool contents_changed = true; - Writer *w = FileRead(filename, MAX_KEY_FILE_SIZE, NULL); + /* One byte more than we generated: a longer file differs anyway. */ + Writer *w = FileRead(filename, strlen(file_contents_new) + 1, NULL); if (w) { if(strcmp(StringWriterData(w), file_contents_new) == 0) @@ -145,6 +148,8 @@ static void Nova_DumpSlots(void) GetErrorStr()); } } + + free(file_contents_new); } void GetObservable(int i, char *name, size_t name_size, char *desc, size_t desc_size) From 6c8b1446ad2404a6f334f4e8bfb0b3914264aa80 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Mon, 20 Jul 2026 09:44:51 -0500 Subject: [PATCH 2/5] Reduced cf-monitord record size to the measurement slots in use AveragesUsedSize() gives the length through the highest slot in use, and the two WriteDB() call sites now write that much. GetRecordForTime() zeroes its buffer and DBPrivRead() clamps the copy, so unstored slots read back as zero -- which is also how a record from a smaller-CF_OBSERVABLES agent already reads. On Linux, where the memory and I/O probes register nine slots, a record is 2600 bytes rather than 9608: about 22.8 MB per host across both DBs instead of 79 MB. Ticket: ENT-14329 Changelog: cf-monitord stores only in-use measurement slots Co-Authored-By: Claude Opus 5 (1M context) --- cf-check/dump.c | 12 +- cf-monitord/env_monitor.c | 3 +- cf-monitord/history.c | 3 +- libpromises/monitoring_read.c | 24 ++++ libpromises/monitoring_read.h | 1 + tests/unit/Makefile.am | 4 + tests/unit/observations_storage_test.c | 191 +++++++++++++++++++++++++ 7 files changed, 231 insertions(+), 7 deletions(-) create mode 100644 tests/unit/observations_storage_test.c diff --git a/cf-check/dump.c b/cf-check/dump.c index f0a920629a..bf52dfca1c 100644 --- a/cf-check/dump.c +++ b/cf-check/dump.c @@ -162,18 +162,20 @@ static void print_struct_lock_data( static void print_struct_averages( const MDB_val value, const bool strip_strings, const char *tskey_filename) { - assert(sizeof(Averages) == value.mv_size); - if (sizeof(Averages) != value.mv_size) + // Records hold only the in-use slots (AveragesUsedSize), so a short record + // is normal. Copy into a zeroed struct so unstored slots read as zero. + assert(value.mv_size <= sizeof(Averages)); + if (value.mv_size > sizeof(Averages)) { - // Fall back to simple printing in release builds: + // Larger than the struct: fall back to simple printing. print_json_string(value.mv_data, value.mv_size, strip_strings); } else { // TODO: clean up Averages char **obnames = NULL; - Averages averages; - memcpy(&averages, value.mv_data, sizeof(averages)); + Averages averages = {0}; + memcpy(&averages, value.mv_data, value.mv_size); const time_t last_seen = averages.last_seen; obnames = GetObservableNames(tskey_filename); diff --git a/cf-monitord/env_monitor.c b/cf-monitord/env_monitor.c index fa84e92115..7328bf5088 100644 --- a/cf-monitord/env_monitor.c +++ b/cf-monitord/env_monitor.c @@ -50,6 +50,7 @@ #include /* MonOtherInit,MonOtherGatherData */ #include /* HistoryUpdate */ #include /* GetObservable */ +#include /* AveragesUsedSize */ #include @@ -773,7 +774,7 @@ static void UpdateAverages(EvalContext *ctx, char *timekey, const Averages *cons Log(LOG_LEVEL_INFO, "Updated averages at '%s'", timekey); - WriteDB(dbp, timekey, newvals, sizeof(Averages)); + WriteDB(dbp, timekey, newvals, AveragesUsedSize()); WriteDB(dbp, "DATABASE_AGE", &AGE, sizeof(double)); CloseDB(dbp); diff --git a/cf-monitord/history.c b/cf-monitord/history.c index e0375b2a1b..d41f617300 100644 --- a/cf-monitord/history.c +++ b/cf-monitord/history.c @@ -26,6 +26,7 @@ #include #include /* MakeTimekey */ +#include /* AveragesUsedSize */ #include #include #include @@ -71,7 +72,7 @@ static void PutRecordForTime(CF_DB *db, time_t time, const Averages *values) MakeTimekey(time, timekey); - WriteDB(db, timekey, values, sizeof(Averages)); + WriteDB(db, timekey, values, AveragesUsedSize()); } static void Nova_SaveFilePosition(const char *handle, const char *name, long fileptr) diff --git a/libpromises/monitoring_read.c b/libpromises/monitoring_read.c index de0aae753b..363ce02784 100644 --- a/libpromises/monitoring_read.c +++ b/libpromises/monitoring_read.c @@ -25,6 +25,7 @@ #include +#include /* offsetof */ #include /* FILE_SEPARATOR */ #include @@ -225,6 +226,27 @@ bool NovaHasSlot(int idx) return idx < ob_spare || SLOTS[idx - ob_spare]; } +/** + * Bytes of an Averages record in use: the built-in observables plus every slot + * through the highest one a measurement has registered. cf-monitord writes only + * this much, so unused slots cost no disk. + */ +size_t AveragesUsedSize(void) +{ + Nova_LoadSlots(); + + size_t slots = ob_spare; + for (int i = CF_OBSERVABLES - 1; i >= ob_spare; i--) + { + if (SLOTS[i - ob_spare] != NULL) + { + slots = i + 1; + break; + } + } + return offsetof(Averages, Q) + slots * sizeof(QPoint); +} + const char *NovaGetSlotName(int idx) { Nova_LoadSlots(); @@ -324,6 +346,8 @@ bool GetRecordForTime(CF_DB *db, time_t time, Averages *result) MakeTimekey(time, timekey); + /* Records may be short (AveragesUsedSize); unstored slots must read zero. */ + memset(result, 0, sizeof(Averages)); return ReadDB(db, timekey, result, sizeof(Averages)); } diff --git a/libpromises/monitoring_read.h b/libpromises/monitoring_read.h index 0acad5630d..6ceb24b50c 100644 --- a/libpromises/monitoring_read.h +++ b/libpromises/monitoring_read.h @@ -40,6 +40,7 @@ MonitoringSlot *Nova_MakeSlot(const char *name, const char *description, bool consolidable); void Nova_LoadSlots(void); bool NovaHasSlot(int idx); +size_t AveragesUsedSize(void); const char *NovaGetSlotName(int idx); const char *NovaGetSlotDescription(int index); const char *NovaGetSlotUnits(int index); diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index 3864b67876..659c20219c 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -105,6 +105,7 @@ check_PROGRAMS = \ lastseen_migration_test \ changes_migration_test \ observations_migration_test \ + observations_storage_test \ db_test \ db_concurrent_test \ item_lib_test \ @@ -266,6 +267,9 @@ db_concurrent_test_LDADD = libtest.la libdb.la observations_migration_test_SOURCES = observations_migration_test.c observations_migration_test_LDADD = libtest.la libdb.la +observations_storage_test_SOURCES = observations_storage_test.c +observations_storage_test_LDADD = libtest.la ../../libpromises/libpromises.la + lastseen_test_SOURCES = lastseen_test.c \ ../../libntech/libutils/statistics.c #lastseen_test_CPPFLAGS = $(libdb_la_CPPFLAGS) diff --git a/tests/unit/observations_storage_test.c b/tests/unit/observations_storage_test.c new file mode 100644 index 0000000000..3630b08a74 --- /dev/null +++ b/tests/unit/observations_storage_test.c @@ -0,0 +1,191 @@ +/* + Copyright 2026 Northern.tech AS + + This file is part of CFEngine 3 - written and maintained by Northern.tech AS. + + This program is free software; you can redistribute it and/or modify it + under the terms of the GNU General Public License as published by the + Free Software Foundation; version 3. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA + + To the extent this program is licensed as part of the Enterprise + versions of CFEngine, the applicable Commercial Open Source License + (COSL) may apply to this file if you as a licensee so wish it. See + included file COSL.txt. +*/ + +#include +#include /* offsetof */ +#include +#include +#include + +#include +#include +#include +#include /* DeleteDirectoryTree */ +#include /* xsnprintf */ + +/* The custom slot this test registers, offset from the first spare slot. */ +#define TEST_CUSTOM_SLOT_OFFSET 9 + +/* Stale read-buffer contents, distinct from zero and from any stored value. */ +#define STALE 12345.0 + +char CFWORKDIR[CF_BUFSIZE]; + +/* The size of a record holding slots 0..last_slot inclusive. */ +static size_t size_through_slot(int last_slot) +{ + return offsetof(Averages, Q) + (size_t) (last_slot + 1) * sizeof(QPoint); +} + +void tests_setup(void) +{ + static char env[] = /* Needs to be static for putenv() */ + "CFENGINE_TEST_OVERRIDE_WORKDIR=/tmp/observations_storage_test.XXXXXX"; + + char *workdir = strchr(env, '=') + 1; /* start of the path */ + assert(workdir - 1 && workdir[0] == '/'); + + mkdtemp(workdir); + strlcpy(CFWORKDIR, workdir, CF_BUFSIZE); + putenv(env); + mkdir(GetStateDir(), (S_IRWXU | S_IRWXG | S_IRWXO)); +} + +static void tests_teardown(void) +{ + /* DeleteDirectoryTree() keeps the top directory, so rmdir() it too. */ + DeleteDirectoryTree(CFWORKDIR); + rmdir(CFWORKDIR); +} + +/** + * Writes a ts_key as Nova_DumpSlots() does, registering one custom measurement + * so the in-use range reaches past the built-in observables. + */ +static void write_ts_key_with_one_custom_slot(void) +{ + char filename[CF_BUFSIZE]; + xsnprintf(filename, CF_BUFSIZE, "%s%cts_key", GetStateDir(), FILE_SEPARATOR); + + FILE *f = safe_fopen(filename, "w"); + assert_true(f != NULL); + + for (int i = 0; i < CF_OBSERVABLES; i++) + { + if (i == ob_spare + TEST_CUSTOM_SLOT_OFFSET) + { + fprintf(f, "%d,test_measurement,A test measurement,units,0.000,100.000,1\n", i); + } + else + { + fprintf(f, "%d,spare,unused\n", i); + } + } + + fclose(f); +} + +static void test_used_size_without_custom_slots(void) +{ + /* No ts_key yet, so only the built-in observables are in use. */ + assert_int_equal(AveragesUsedSize(), size_through_slot(ob_spare - 1)); + + /* The point of the change: less than a full-size record. */ + assert_true(AveragesUsedSize() < sizeof(Averages)); +} + +static void test_used_size_covers_highest_registered_slot(void) +{ + write_ts_key_with_one_custom_slot(); + + /* Through the registered slot and no further. */ + assert_int_equal( + AveragesUsedSize(), + size_through_slot(ob_spare + TEST_CUSTOM_SLOT_OFFSET)); + assert_true(AveragesUsedSize() < sizeof(Averages)); +} + +static void test_short_record_reads_back_zero_extended(void) +{ + /* Stored slots must read back intact and the unstored tail as zero, even + * when the caller's buffer held something else first. */ + const time_t when = 1234567890; + const int custom_slot = ob_spare + TEST_CUSTOM_SLOT_OFFSET; + const size_t used = AveragesUsedSize(); + + CF_DB *db; + assert_true(OpenDB(&db, dbid_observations)); + + Averages written; + memset(&written, 0, sizeof(written)); + written.last_seen = when; + written.Q[0].q = 1.0; + written.Q[0].expect = 2.0; + written.Q[0].var = 3.0; + written.Q[0].dq = 4.0; + written.Q[custom_slot].q = 5.0; + written.Q[custom_slot].dq = 6.0; + + char timekey[CF_MAXVARSIZE]; + MakeTimekey(when, timekey); + assert_true(WriteDB(db, timekey, &written, used)); + + /* Stale doubles, not a memset byte pattern: 0xAA-filled bytes read back as + * ~1e-103, which assert_double_close() cannot tell from zero. */ + Averages read_back; + read_back.last_seen = -1; + for (int i = 0; i < CF_OBSERVABLES; i++) + { + read_back.Q[i].q = STALE; + read_back.Q[i].expect = STALE; + read_back.Q[i].var = STALE; + read_back.Q[i].dq = STALE; + } + assert_true(GetRecordForTime(db, when, &read_back)); + + assert_int_equal(read_back.last_seen, when); + assert_double_close(read_back.Q[0].q, 1.0); + assert_double_close(read_back.Q[0].expect, 2.0); + assert_double_close(read_back.Q[0].var, 3.0); + assert_double_close(read_back.Q[0].dq, 4.0); + assert_double_close(read_back.Q[custom_slot].q, 5.0); + assert_double_close(read_back.Q[custom_slot].dq, 6.0); + + /* Never stored. */ + assert_double_close(read_back.Q[custom_slot + 1].q, 0.0); + assert_double_close(read_back.Q[CF_OBSERVABLES - 1].q, 0.0); + assert_double_close(read_back.Q[CF_OBSERVABLES - 1].dq, 0.0); + + CloseDB(db); +} + +int main(void) +{ + tests_setup(); + + /* Order matters: the first test needs no ts_key, the rest need the custom + * slot the second one registers. */ + const UnitTest tests[] = + { + unit_test(test_used_size_without_custom_slots), + unit_test(test_used_size_covers_highest_registered_slot), + unit_test(test_short_record_reads_back_zero_extended), + }; + + PRINT_TEST_BANNER(); + int ret = run_tests(tests); + + tests_teardown(); + return ret; +} From 823362745e88a6c9eb28a858a79a797a7018ba13 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Mon, 20 Jul 2026 10:46:36 -0500 Subject: [PATCH 3/5] Reported unregistered observables as spare instead of padding the name table GetObservable() copied from OBSERVABLES for slots the table does not name, so raising CF_OBSERVABLES meant adding a {"spare", "unused"} row per added slot to keep strncpy() off a NULL. It now reports those slots directly, and the rows are gone. Ticket: ENT-14329 Changelog: None Co-Authored-By: Claude Opus 5 (1M context) --- cf-monitord/monitoring.c | 5 +- libpromises/constants.c | 233 +-------------------------------------- 2 files changed, 4 insertions(+), 234 deletions(-) diff --git a/cf-monitord/monitoring.c b/cf-monitord/monitoring.c index 497faf64af..f58dccffe2 100644 --- a/cf-monitord/monitoring.c +++ b/cf-monitord/monitoring.c @@ -170,8 +170,9 @@ void GetObservable(int i, char *name, size_t name_size, char *desc, size_t desc_ } else { - strncpy(name, OBSERVABLES[i][0], name_size - 1); - strncpy(desc, OBSERVABLES[i][1], desc_size - 1); + /* OBSERVABLES has no rows at or above ob_spare. */ + strncpy(name, "spare", name_size - 1); + strncpy(desc, "unused", desc_size - 1); } } } diff --git a/libpromises/constants.c b/libpromises/constants.c index 8ade1ced25..3844ede6b0 100644 --- a/libpromises/constants.c +++ b/libpromises/constants.c @@ -151,236 +151,5 @@ const char *const OBSERVABLES[CF_OBSERVABLES][2] = {"postgres_out", "PostgreSQL database client sessions (out)"}, {"ipp_in", "Internet Printer Protocol (in)"}, {"ipp_out", "Internet Printer Protocol (out)"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - /* ENT-6511: slots 100-299, added when CF_OBSERVABLES was raised - 100->300. Every slot must have a name entry: cf-monitord's - GetObservable() falls back to OBSERVABLES[i] for unregistered - slots, so a table shorter than CF_OBSERVABLES => NULL deref. */ - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, - {"spare", "unused"}, + /* Slots at/after ob_spare are reported "spare" by GetObservable(). */ }; From bdb8c1adb526a192c2c4ece601a5bd6521ed85a1 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Fri, 31 Jul 2026 23:45:32 -0500 Subject: [PATCH 4/5] Removed the Averages DB migration, made unnecessary by variable-size records The migration (ENT-6511) rewrote pre-ENT-6511 records to the full Averages size so every record on disk shared one layout. Short records are now the normal case and readers handle them, so the migration would only work against this ticket: on upgrade it expands ~4392 records in each of two DBs to full size, the ~56 MB per host this change removes. It never shipped in a release, so only hosts tracking master carry the "version" key it wrote, and that key is inert once the plan is unregistered. Ticket: ENT-14329 Changelog: None Co-Authored-By: Claude Opus 5 (1M context) --- cf-check/dump.c | 13 +- libpromises/Makefile.am | 1 - libpromises/dbm_migration.c | 7 +- libpromises/dbm_migration_observations.c | 113 ----------- tests/unit/Makefile.am | 6 +- tests/unit/observations_migration_test.c | 242 ----------------------- 6 files changed, 3 insertions(+), 379 deletions(-) delete mode 100644 libpromises/dbm_migration_observations.c delete mode 100644 tests/unit/observations_migration_test.c diff --git a/cf-check/dump.c b/cf-check/dump.c index bf52dfca1c..e1f8e2cd42 100644 --- a/cf-check/dump.c +++ b/cf-check/dump.c @@ -290,18 +290,7 @@ static void print_struct_or_string( { if (structs) { - if ((StringContains(file, "cf_observations.lmdb") - || StringContains(file, "history.lmdb")) - && StringEqual(key.mv_data, "version")) - { - // After the CF_OBSERVABLES migration (dbm_migration_observations.c) - // these DBs hold a "version" bookkeeping key whose value is a short - // version string, not an Averages struct. Print it as a string; - // otherwise it would reach print_struct_averages() and trip its - // struct-size assertion. - print_json_string(value.mv_data, value.mv_size, strip_strings); - } - else if (StringContains(file, "cf_lastseen.lmdb") + if (StringContains(file, "cf_lastseen.lmdb") && StringStartsWith(key.mv_data, "q")) { print_struct_lastseen_quality(value, strip_strings); diff --git a/libpromises/Makefile.am b/libpromises/Makefile.am index 4cef498d15..8f60e0f501 100644 --- a/libpromises/Makefile.am +++ b/libpromises/Makefile.am @@ -84,7 +84,6 @@ libpromises_la_SOURCES = \ dbm_api.c dbm_api.h dbm_api_types.h dbm_priv.h \ dbm_migration.c dbm_migration.h \ dbm_migration_lastseen.c \ - dbm_migration_observations.c \ dbm_lmdb.c \ dbm_quick.c \ dbm_tokyocab.c \ diff --git a/libpromises/dbm_migration.c b/libpromises/dbm_migration.c index a833d1cf89..6066483533 100644 --- a/libpromises/dbm_migration.c +++ b/libpromises/dbm_migration.c @@ -29,7 +29,6 @@ #include extern const DBMigrationFunction dbm_migration_plan_lastseen[]; -extern const DBMigrationFunction dbm_migration_plan_observations[]; #ifndef LMDB @@ -52,11 +51,7 @@ static size_t DBVersion(DBHandle *db) } static const DBMigrationFunction *const dbm_migration_plans[dbid_max] = { - [dbid_lastseen] = dbm_migration_plan_lastseen, - /* Both DBs store the same fixed-size Averages records, so they share the - * plan that expands them when CF_OBSERVABLES grows. */ - [dbid_observations] = dbm_migration_plan_observations, - [dbid_history] = dbm_migration_plan_observations + [dbid_lastseen] = dbm_migration_plan_lastseen }; bool DBMigrate(DBHandle *db, dbid id) diff --git a/libpromises/dbm_migration_observations.c b/libpromises/dbm_migration_observations.c deleted file mode 100644 index d4f7fb0a30..0000000000 --- a/libpromises/dbm_migration_observations.c +++ /dev/null @@ -1,113 +0,0 @@ -/* - Copyright 2026 Northern.tech AS - - This file is part of CFEngine 3 - written and maintained by Northern.tech AS. - - This program is free software; you can redistribute it and/or modify it - under the terms of the GNU General Public License as published by the - Free Software Foundation; version 3. - - This program is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU General Public License for more details. - - You should have received a copy of the GNU General Public License - along with this program; if not, write to the Free Software - Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA - - To the extent this program is licensed as part of the Enterprise - versions of CFEngine, the applicable Commercial Open Source License - (COSL) may apply to this file if you as a licensee so wish it. See - included file COSL.txt. -*/ - -#include - -#include -#include - -/* Number of measurement slots (CF_OBSERVABLES) before ENT-6511 raised it from - * 100 to 300. A record written by an agent from before that change is this many - * slots long. */ -#define CF_OBSERVABLES_BEFORE_ENT_6511 100 - -typedef struct -{ - time_t last_seen; - QPoint Q[CF_OBSERVABLES_BEFORE_ENT_6511]; -} AveragesBeforeEnt6511; - -/* - * The observations (cf_observations.lmdb) and history (history.lmdb) databases - * store fixed-size Averages records keyed by time. Raising CF_OBSERVABLES grows - * that struct, so a record written by an older agent is shorter than the current - * struct. cf-monitord's own read path zero-extends short records, and it - * overwrites the observations records on its next cycle, but the history records - * are never rewritten. Migrate every old-size record to the current size, - * zero-filling the added slots, so all records on disk share one layout. - */ -static bool MeasurementsMigrationVersion0(DBHandle *db) -{ - DBCursor *cursor; - if (!NewDBCursor(db, &cursor)) - { - Log(LOG_LEVEL_ERR, - "Unable to create database cursor during measurement DB migration"); - return false; - } - - char *key; - void *value; - int key_size, value_size; - - while (NextDB(cursor, &key, &key_size, &value, &value_size)) - { - /* Only fixed-size Averages records written before CF_OBSERVABLES was - * raised need expanding. Scalar bookkeeping keys (e.g. "DATABASE_AGE" - * and "version") are a different size and are left untouched. */ - if (value_size != (int) sizeof(AveragesBeforeEnt6511)) - { - continue; - } - - /* Copy the old, shorter record into a full-size, zeroed struct so the - * slots added by the larger CF_OBSERVABLES read back as zero. */ - Averages expanded; - memset(&expanded, 0, sizeof(expanded)); - memcpy(&expanded, value, sizeof(AveragesBeforeEnt6511)); - - // This will overwrite the entry - if (!DBCursorWriteEntry(cursor, &expanded, sizeof(expanded))) - { - Log(LOG_LEVEL_ERR, - "Unable to expand measurement record for key '%s' during migration", - key); - DeleteDBCursor(cursor); - return false; - } - } - - if (!DeleteDBCursor(cursor)) - { - Log(LOG_LEVEL_ERR, - "Unable to close cursor during measurement DB migration"); - return false; - } - - if (!WriteDB(db, "version", "1", sizeof("1"))) - { - Log(LOG_LEVEL_ERR, - "Failed to update version number during measurement DB migration"); - return false; - } - - Log(LOG_LEVEL_INFO, "Migrated measurement database to version 1"); - return true; -} - -const DBMigrationFunction dbm_migration_plan_observations[] = -{ - MeasurementsMigrationVersion0, - NULL -}; diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index 659c20219c..719fa37366 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -68,7 +68,6 @@ libdb_la_SOURCES = db_stubs.c \ ../../libpromises/dbm_lmdb.c \ ../../libpromises/dbm_migration.c \ ../../libpromises/dbm_migration_lastseen.c \ - ../../libpromises/dbm_migration_observations.c \ ../../libpromises/global_mutex.c \ ../../cf-check/backup.c \ ../../cf-check/diagnose.c \ @@ -104,7 +103,6 @@ check_PROGRAMS = \ lastseen_test \ lastseen_migration_test \ changes_migration_test \ - observations_migration_test \ observations_storage_test \ db_test \ db_concurrent_test \ @@ -264,12 +262,10 @@ db_concurrent_test_SOURCES = db_concurrent_test.c #db_concurrent_test_CPPFLAGS = $(libdb_la_CPPFLAGS) db_concurrent_test_LDADD = libtest.la libdb.la -observations_migration_test_SOURCES = observations_migration_test.c -observations_migration_test_LDADD = libtest.la libdb.la - observations_storage_test_SOURCES = observations_storage_test.c observations_storage_test_LDADD = libtest.la ../../libpromises/libpromises.la + lastseen_test_SOURCES = lastseen_test.c \ ../../libntech/libutils/statistics.c #lastseen_test_CPPFLAGS = $(libdb_la_CPPFLAGS) diff --git a/tests/unit/observations_migration_test.c b/tests/unit/observations_migration_test.c deleted file mode 100644 index 02f2565c79..0000000000 --- a/tests/unit/observations_migration_test.c +++ /dev/null @@ -1,242 +0,0 @@ -#include -#include -#include -#include - -#include -#include -#include /* StringEqual */ -#include /* DeleteDirectoryTree */ - -#ifndef LMDB - -/* The measurement DB migration only exists for the LMDB backend (see - * dbm_migration.c); with other backends DBMigrate() is a no-op. */ -int main(void) -{ - return 0; -} - -#else - -/* The Averages layout from before ENT-6511 raised CF_OBSERVABLES from 100 to - * 300. A record written by such an agent is this size on disk. */ -#define CF_OBSERVABLES_BEFORE_ENT_6511 100 - -typedef struct -{ - time_t last_seen; - QPoint Q[CF_OBSERVABLES_BEFORE_ENT_6511]; -} AveragesBeforeEnt6511; - -char CFWORKDIR[CF_BUFSIZE]; - -void tests_setup(void) -{ - static char env[] = /* Needs to be static for putenv() */ - "CFENGINE_TEST_OVERRIDE_WORKDIR=/tmp/observations_migration_test.XXXXXX"; - - char *workdir = strchr(env, '=') + 1; /* start of the path */ - assert(workdir - 1 && workdir[0] == '/'); - - mkdtemp(workdir); - strlcpy(CFWORKDIR, workdir, CF_BUFSIZE); - putenv(env); - mkdir(GetStateDir(), (S_IRWXU | S_IRWXG | S_IRWXO)); -} - -static void tests_teardown(void) -{ - /* DeleteDirectoryTree() empties the tree but keeps the top directory, so - * rmdir() the now-empty workdir afterwards. */ - DeleteDirectoryTree(CFWORKDIR); - rmdir(CFWORKDIR); -} - -/* - * Provides an observations DB in a pre-migration (version 0) state: OpenDB() - * runs the migration and writes a "version" marker, so wipe every entry to get - * back to how an old, never-migrated database looks on disk. - */ -static DBHandle *setup_unversioned(void) -{ - /* Empties the state dir (keeps the dir itself) so OpenDB() starts fresh. */ - assert_true(DeleteDirectoryTree(GetStateDir())); - - DBHandle *db; - assert_true(OpenDB(&db, dbid_observations)); - - DBCursor *cursor; - assert_true(NewDBCursor(db, &cursor)); - - char *key; - void *value; - int ksize, vsize; - while (NextDB(cursor, &key, &ksize, &value, &vsize)) - { - DBCursorDeleteEntry(cursor); - } - assert_true(DeleteDBCursor(cursor)); - - return db; -} - -/* Returns the on-disk size of a record, or -1 if the key is absent. */ -static int record_size(DBHandle *db, const char *want_key) -{ - DBCursor *cursor; - assert_true(NewDBCursor(db, &cursor)); - - char *key; - void *value; - int ksize, vsize; - int found = -1; - while (NextDB(cursor, &key, &ksize, &value, &vsize)) - { - if (StringEqual(key, want_key)) - { - found = vsize; - } - } - assert_true(DeleteDBCursor(cursor)); - return found; -} - -static void test_migrate_expands_old_record(void) -{ - DBHandle *db = setup_unversioned(); - - /* A measurement record in the old, shorter (100-slot) layout ... */ - AveragesBeforeEnt6511 old; - memset(&old, 0, sizeof(old)); - old.last_seen = 1234567; - old.Q[0].q = 1.0; - old.Q[0].expect = 2.0; - old.Q[0].var = 3.0; - old.Q[0].dq = 4.0; - old.Q[CF_OBSERVABLES_BEFORE_ENT_6511 - 1].q = 5.0; - old.Q[CF_OBSERVABLES_BEFORE_ENT_6511 - 1].expect = 6.0; - old.Q[CF_OBSERVABLES_BEFORE_ENT_6511 - 1].var = 7.0; - old.Q[CF_OBSERVABLES_BEFORE_ENT_6511 - 1].dq = 8.0; - assert_true(WriteDB(db, "Mon_Hr12_Q1", &old, sizeof(old))); - - /* ... and a scalar bookkeeping record that must NOT be touched. */ - double age = 42.0; - assert_true(WriteDB(db, "DATABASE_AGE", &age, sizeof(age))); - - CloseDB(db); - - /* Reopening runs the migration (no version marker present yet). */ - assert_true(OpenDB(&db, dbid_observations)); - - /* The measurement record is now the full, current size ... */ - assert_int_equal(record_size(db, "Mon_Hr12_Q1"), (int) sizeof(Averages)); - /* ... the scalar record is left at its original size ... */ - assert_int_equal(record_size(db, "DATABASE_AGE"), (int) sizeof(double)); - /* ... and the version marker was written. */ - assert_true(HasKeyDB(db, "version", strlen("version") + 1)); - - /* The original slots are preserved and the new slots read back as zero. */ - Averages migrated; - memset(&migrated, 0, sizeof(migrated)); - assert_true(ReadDB(db, "Mon_Hr12_Q1", &migrated, sizeof(migrated))); - assert_int_equal(migrated.last_seen, 1234567); - assert_double_close(migrated.Q[0].q, 1.0); - assert_double_close(migrated.Q[0].dq, 4.0); - assert_double_close(migrated.Q[CF_OBSERVABLES_BEFORE_ENT_6511 - 1].q, 5.0); - assert_double_close(migrated.Q[CF_OBSERVABLES_BEFORE_ENT_6511 - 1].dq, 8.0); - assert_double_close(migrated.Q[CF_OBSERVABLES_BEFORE_ENT_6511].q, 0.0); - assert_double_close(migrated.Q[CF_OBSERVABLES - 1].var, 0.0); - - double read_age = 0.0; - assert_true(ReadDB(db, "DATABASE_AGE", &read_age, sizeof(read_age))); - assert_double_close(read_age, 42.0); - - CloseDB(db); -} - -static void test_current_record_untouched(void) -{ - /* A record already at the current size must not be altered by the - * migration (the size-exact check must ignore it). */ - DBHandle *db = setup_unversioned(); - - Averages current; - memset(¤t, 0, sizeof(current)); - current.last_seen = 7654321; - current.Q[CF_OBSERVABLES - 1].q = 99.0; - assert_true(WriteDB(db, "Tue_Hr06_Q2", ¤t, sizeof(current))); - CloseDB(db); - - assert_true(OpenDB(&db, dbid_observations)); - - assert_int_equal(record_size(db, "Tue_Hr06_Q2"), (int) sizeof(Averages)); - - Averages read; - memset(&read, 0, sizeof(read)); - assert_true(ReadDB(db, "Tue_Hr06_Q2", &read, sizeof(read))); - assert_int_equal(read.last_seen, 7654321); - assert_double_close(read.Q[CF_OBSERVABLES - 1].q, 99.0); - - CloseDB(db); -} - -static void test_migration_is_idempotent(void) -{ - /* Once migrated, reopening must not run the migration again nor disturb - * the data. */ - DBHandle *db = setup_unversioned(); - - AveragesBeforeEnt6511 old; - memset(&old, 0, sizeof(old)); - old.last_seen = 111; - assert_true(WriteDB(db, "Wed_Hr18_Q3", &old, sizeof(old))); - CloseDB(db); - - /* First open migrates. */ - assert_true(OpenDB(&db, dbid_observations)); - assert_int_equal(record_size(db, "Wed_Hr18_Q3"), (int) sizeof(Averages)); - CloseDB(db); - - /* Second open is a no-op. */ - assert_true(OpenDB(&db, dbid_observations)); - assert_int_equal(record_size(db, "Wed_Hr18_Q3"), (int) sizeof(Averages)); - - char version[8]; - assert_true(ReadDB(db, "version", version, sizeof(version))); - assert_string_equal(version, "1"); - - CloseDB(db); -} - -int main(void) -{ - tests_setup(); - - const UnitTest tests[] = - { - unit_test(test_migrate_expands_old_record), - unit_test(test_current_record_untouched), - unit_test(test_migration_is_idempotent), - }; - - PRINT_TEST_BANNER(); - int ret = run_tests(tests); - - tests_teardown(); - return ret; -} - -/* STUBS */ - -/* OpenDB() pulls in libpromises DB code that references FatalError(), but the - * real implementation lives in the daemons, not the test binary. Provide a stub - * so the test links; the migration paths under test never call it, so reaching - * it is itself a test failure. */ -void FatalError(ARG_UNUSED char *s, ...) -{ - fail(); - exit(42); -} - -#endif // LMDB From 78f76aa64729e3ed279c7c08dc3d061934dd99d5 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 1 Aug 2026 01:12:39 -0500 Subject: [PATCH 5/5] Fixed cf-check dump labelling every named measurement as a spare slot GetObservableNames() had its two cases the wrong way round: named slots came back as "spare[]", and every spare slot shared the single name "spare" so the whole tail collapsed into one key of the JSON object. A malformed line also reached that comparison with name possibly untouched by sscanf(); it now falls back to the numbered placeholder. Ticket: ENT-14329 Changelog: cf-check dump reports measurement names instead of labelling them spare Co-Authored-By: Claude Opus 5 (1M context) --- cf-check/observables.c | 11 +- tests/unit/Makefile.am | 3 + tests/unit/observables_names_test.c | 230 ++++++++++++++++++++++++++++ 3 files changed, 240 insertions(+), 4 deletions(-) create mode 100644 tests/unit/observables_names_test.c diff --git a/cf-check/observables.c b/cf-check/observables.c index 2619ad1dce..d58968bfe4 100644 --- a/cf-check/observables.c +++ b/cf-check/observables.c @@ -5,7 +5,7 @@ #include // FILE_SEPARATOR #include // xstrdup() #include // observable_strings -#include // StringEqual +#include // StringEqual(), StringCopy() #include /** @@ -100,16 +100,19 @@ char **GetObservableNames(const char *ts_key_path) if ((fields != 2) && (fields != 6)) { Log(LOG_LEVEL_ERR, "Wrong line format in ts_key: %s", line); + /* sscanf() may have left name untouched. */ + StringCopy("spare", name, sizeof(name)); } if (StringEqual(name, "spare")) { - temp[i] = xstrdup(name); + /* Numbered, not a shared "spare": these become JSON keys. */ + snprintf(buf, CF_MAXVARSIZE, "spare[%d]", i); + temp[i] = xstrdup(buf); } else { - snprintf(buf, CF_MAXVARSIZE, "spare[%d]", i); - temp[i] = xstrdup(buf); + temp[i] = xstrdup(name); } } fclose(f); diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index 719fa37366..024e25f78b 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -104,6 +104,7 @@ check_PROGRAMS = \ lastseen_migration_test \ changes_migration_test \ observations_storage_test \ + observables_names_test \ db_test \ db_concurrent_test \ item_lib_test \ @@ -265,6 +266,8 @@ db_concurrent_test_LDADD = libtest.la libdb.la observations_storage_test_SOURCES = observations_storage_test.c observations_storage_test_LDADD = libtest.la ../../libpromises/libpromises.la +observables_names_test_SOURCES = observables_names_test.c \ + ../../cf-check/observables.c lastseen_test_SOURCES = lastseen_test.c \ ../../libntech/libutils/statistics.c diff --git a/tests/unit/observables_names_test.c b/tests/unit/observables_names_test.c new file mode 100644 index 0000000000..43b2ebbc98 --- /dev/null +++ b/tests/unit/observables_names_test.c @@ -0,0 +1,230 @@ +/* + Copyright 2026 Northern.tech AS + + This file is part of CFEngine 3 - written and maintained by Northern.tech AS. + + This program is free software; you can redistribute it and/or modify it + under the terms of the GNU General Public License as published by the + Free Software Foundation; version 3. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA + + To the extent this program is licensed as part of the Enterprise + versions of CFEngine, the applicable Commercial Open Source License + (COSL) may apply to this file if you as a licensee so wish it. See + included file COSL.txt. +*/ + +#include +#include + +#include /* CF_BUFSIZE, CF_MAXVARSIZE */ +#include /* observables_max, observable_strings */ +#include /* CF_OBSERVABLES, GetObservableNames */ +#include /* DeleteDirectoryTree, safe_fopen */ +#include /* StringEqual */ +#include /* xsnprintf */ + +/* cf-check's built-in list ends with a pseudo-entry named "spare", so + * observables_max is one more than the real named count. */ +#define LAST_NAMED_OBSERVABLE (observables_max - 2) +#define FIRST_SPARE_SLOT (observables_max - 1) + +/* Offsets from the first spare slot: the slot the test ts_key names, and the + * one whose line it corrupts. */ +#define CUSTOM_SLOT_OFFSET 8 +#define MALFORMED_SLOT_OFFSET 20 + +static char TESTDIR[CF_BUFSIZE]; + +/** + * dump.c indexes and free()s every entry, so a hole is an invalid free, not just + * a missing name. + */ +static void assert_all_names_present(char **names) +{ + assert_true(names != NULL); + for (int i = 0; i < CF_OBSERVABLES; i++) + { + assert_true(names[i] != NULL); + } +} + +static void free_names(char **names) +{ + for (int i = 0; i < CF_OBSERVABLES; i++) + { + free(names[i]); + } + free(names); +} + +/** + * Writes a ts_key as Nova_DumpSlots() does. Fewer than CF_OBSERVABLES lines + * gives the short file an older, smaller-CF_OBSERVABLES agent left behind. + */ +static void write_ts_key( + char *path, size_t path_size, const char *name, int lines, bool with_malformed) +{ + xsnprintf(path, path_size, "%s/%s", TESTDIR, name); + + FILE *f = safe_fopen(path, "w"); + assert_true(f != NULL); + + for (int i = 0; i < lines; i++) + { + if (i < observables_max) + { + fprintf(f, "%d,%s,Built-in observable %d\n", i, observable_strings[i], i); + } + else if (i == FIRST_SPARE_SLOT + CUSTOM_SLOT_OFFSET) + { + fprintf(f, "%d,test_measurement,A test measurement,units,0.000,100.000,1\n", i); + } + else if (with_malformed && i == FIRST_SPARE_SLOT + MALFORMED_SLOT_OFFSET) + { + fprintf(f, "%d no commas here at all\n", i); + } + else + { + fprintf(f, "%d,spare,unused\n", i); + } + } + + fclose(f); +} + +/* Before the fix, named observables came back as "spare[]" and every + * spare slot shared the name "spare", collapsing the tail into one JSON key. */ +static void test_named_slots_report_their_name(void) +{ + char path[CF_BUFSIZE]; + write_ts_key(path, sizeof(path), "ts_key_full", CF_OBSERVABLES, false); + char **names = GetObservableNames(path); + assert_all_names_present(names); + + /* Built-ins report the name ts_key gives them. */ + assert_true(StringEqual(names[0], observable_strings[0])); + assert_true(StringEqual(names[LAST_NAMED_OBSERVABLE], + observable_strings[LAST_NAMED_OBSERVABLE])); + + /* A custom measurement reports its name, not a placeholder. */ + const int custom = FIRST_SPARE_SLOT + CUSTOM_SLOT_OFFSET; + assert_true(StringEqual(names[custom], "test_measurement")); + + /* Nothing keeps the bare "spare" that used to collapse them. */ + for (int i = 0; i < CF_OBSERVABLES; i++) + { + assert_false(StringEqual(names[i], "spare")); + } + + free_names(names); +} + +/* The names become JSON keys, so two spares sharing one lose a slot. */ +static void test_spare_slots_are_numbered_and_distinct(void) +{ + char path[CF_BUFSIZE]; + write_ts_key(path, sizeof(path), "ts_key_spares", CF_OBSERVABLES, false); + char **names = GetObservableNames(path); + assert_all_names_present(names); + + char expected[CF_MAXVARSIZE]; + const int first_spare = FIRST_SPARE_SLOT; + const int last_spare = CF_OBSERVABLES - 1; + + xsnprintf(expected, sizeof(expected), "spare[%d]", first_spare); + assert_true(StringEqual(names[first_spare], expected)); + + xsnprintf(expected, sizeof(expected), "spare[%d]", last_spare); + assert_true(StringEqual(names[last_spare], expected)); + + assert_false(StringEqual(names[first_spare], names[last_spare])); + + free_names(names); +} + +/* sscanf() may leave name untouched, so a bad line must not be read as a name. */ +static void test_malformed_line_falls_back_to_numbered_spare(void) +{ + char path[CF_BUFSIZE]; + write_ts_key(path, sizeof(path), "ts_key_malformed", CF_OBSERVABLES, true); + char **names = GetObservableNames(path); + assert_all_names_present(names); + + const int bad = FIRST_SPARE_SLOT + MALFORMED_SLOT_OFFSET; + char expected[CF_MAXVARSIZE]; + xsnprintf(expected, sizeof(expected), "spare[%d]", bad); + assert_true(StringEqual(names[bad], expected)); + + free_names(names); +} + +/* A short ts_key must still fill every slot, or the caller free()s garbage. */ +static void test_short_ts_key_fills_the_remaining_slots(void) +{ + const int short_lines = 100; + char path[CF_BUFSIZE]; + write_ts_key(path, sizeof(path), "ts_key_short", short_lines, false); + char **names = GetObservableNames(path); + assert_all_names_present(names); + + char expected[CF_MAXVARSIZE]; + xsnprintf(expected, sizeof(expected), "spare[%d]", short_lines); + assert_true(StringEqual(names[short_lines], expected)); + + xsnprintf(expected, sizeof(expected), "spare[%d]", CF_OBSERVABLES - 1); + assert_true(StringEqual(names[CF_OBSERVABLES - 1], expected)); + + free_names(names); +} + +/* With no ts_key, names come from the built-in table. */ +static void test_missing_ts_key_uses_the_builtin_table(void) +{ + char path[CF_BUFSIZE]; + xsnprintf(path, CF_BUFSIZE, "%s/does_not_exist", TESTDIR); + + char **names = GetObservableNames(path); + assert_all_names_present(names); + + assert_true(StringEqual(names[0], observable_strings[0])); + + char expected[CF_MAXVARSIZE]; + xsnprintf(expected, sizeof(expected), "observable[%d]", CF_OBSERVABLES - 1); + assert_true(StringEqual(names[CF_OBSERVABLES - 1], expected)); + + free_names(names); +} + +int main(void) +{ + char template[] = "/tmp/observables_names_test.XXXXXX"; + assert_true(mkdtemp(template) != NULL); + strlcpy(TESTDIR, template, CF_BUFSIZE); + + const UnitTest tests[] = + { + unit_test(test_named_slots_report_their_name), + unit_test(test_spare_slots_are_numbered_and_distinct), + unit_test(test_malformed_line_falls_back_to_numbered_spare), + unit_test(test_short_ts_key_fills_the_remaining_slots), + unit_test(test_missing_ts_key_uses_the_builtin_table), + }; + + PRINT_TEST_BANNER(); + int ret = run_tests(tests); + + /* DeleteDirectoryTree() keeps the top directory, so rmdir() it too. */ + DeleteDirectoryTree(TESTDIR); + rmdir(TESTDIR); + + return ret; +}