From 02482d88bab1ed38496ee4597255d8b8d0777e4a Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Tue, 18 Feb 2025 15:44:59 +0100 Subject: [PATCH] Use isc_buffer_t for read/write ops To support materialization, each value stored inside cfgmgr must have an extra flag telling in which mode the value was inserted/updated (i.e. was it in builtin mode, user mode or running mode). Instead of playing with pointers arithmetic to squeeze this extra info in the value being serialized/deserialized into LMDB, uses isc_buffer_t. --- lib/isc/cfgmgr.c | 142 ++++++++++++++++++++++++++--------- lib/isc/include/isc/cfgmgr.h | 4 +- tests/isc/cfgmgr_test.c | 11 +-- 3 files changed, 113 insertions(+), 44 deletions(-) diff --git a/lib/isc/cfgmgr.c b/lib/isc/cfgmgr.c index d50b60e141..fd50fad5c7 100644 --- a/lib/isc/cfgmgr.c +++ b/lib/isc/cfgmgr.c @@ -169,6 +169,13 @@ isc_cfgmgr_init(isc_mem_t *mctx, const char *dbpath) { REQUIRE(mctx != NULL); REQUIRE(dbpath != NULL); + /* + * Validate assumptions used when reading/writting data. + */ + REQUIRE(sizeof(bool) == sizeof(uint8_t)); + REQUIRE(sizeof(isc_cfgmgr_type_t) == sizeof(uint8_t)); + REQUIRE(sizeof(isc_cfgmgr_mode_t) == sizeof(uint8_t)); + isc_mem_attach(mctx, &isc__cfgmgr_mctx); INSIST(isc__cfgmgr_mctx != NULL); @@ -655,12 +662,43 @@ isc_cfgmgr_delnode(void) { isc_cfgmgr_close(); } -isc_result_t -isc_cfgmgr_read(const char *name, isc_cfgmgr_val_t *value) { +static size_t +isc__cfgmgr_valuesz(const isc_cfgmgr_val_t *value) { + size_t sz = sizeof(value->type); + + sz += sizeof(isc_cfgmgr_mode_t); + switch (value->type) { + case ISC_CFGMGR_UNDEFINED: + UNREACHABLE(); + case ISC_CFGMGR_STRING: + sz += strlen(value->string) + 1; + break; + case ISC_CFGMGR_BOOLEAN: + sz += sizeof(value->boolean); + break; + case ISC_CFGMGR_NONE: + break; + case ISC_CFGMGR_SOCKADDR: + sz += sizeof(value->sockaddr); + break; + case ISC_CFGMGR_UINT32: + sz += sizeof(value->uint32); + break; + } + + return sz; +} + +static isc_result_t +isc__cfgmgr_read(MDB_cursor *cursor, const char *name, isc_cfgmgr_val_t *value, + isc_cfgmgr_mode_t *modep) { isc_result_t result = ISC_R_SUCCESS; MDB_val dbkey; MDB_val dbval; + isc_cfgmgr_mode_t mode; char key[BUFLEN]; + isc_buffer_t b; + isc_region_t r; isc__cfgmgr_checkopenednodes(); REQUIRE(name != NULL); @@ -672,34 +710,59 @@ isc_cfgmgr_read(const char *name, isc_cfgmgr_val_t *value) { dbkey = (MDB_val){ .mv_size = isc__cfgmgr_keylen(key) + 1, .mv_data = key }; - if (mdb_cursor_get(isc__cfgmgr_lmdbcursor(), &dbkey, &dbval, MDB_SET) != - 0) - { + if (mdb_cursor_get(cursor, &dbkey, &dbval, MDB_SET) != 0) { result = ISC_R_NOTFOUND; goto out; } - const isc_cfgmgr_type_t strtype = ISC_CFGMGR_STRING; - if (memcmp(dbval.mv_data, &strtype, sizeof(strtype)) == 0) { - value->type = strtype; - value->string = (char *)dbval.mv_data + sizeof(strtype); - } else { - REQUIRE(sizeof(*value) == dbval.mv_size); - memmove(value, dbval.mv_data, sizeof(*value)); + isc_buffer_init(&b, dbval.mv_data, dbval.mv_size); + isc_buffer_add(&b, dbval.mv_size); + + value->type = isc_buffer_getuint8(&b); + mode = isc_buffer_getuint8(&b); + if (modep) { + *modep = mode; } - INSIST(value->type != ISC_CFGMGR_UNDEFINED); + switch (value->type) { + case ISC_CFGMGR_UNDEFINED: + UNREACHABLE(); + case ISC_CFGMGR_STRING: + isc_buffer_remainingregion(&b, &r); + value->string = (const char *)r.base; + break; + case ISC_CFGMGR_BOOLEAN: + value->boolean = isc_buffer_getuint8(&b); + break; + case ISC_CFGMGR_NONE: + break; + case ISC_CFGMGR_SOCKADDR: + isc_buffer_remainingregion(&b, &r); + memmove(&value->sockaddr, r.base, r.length); + break; + case ISC_CFGMGR_UINT32: + value->uint32 = isc_buffer_getuint32(&b); + break; + } out: return result; } +isc_result_t +isc_cfgmgr_read(const char *name, isc_cfgmgr_val_t *value) { + return isc__cfgmgr_read(isc__cfgmgr_lmdbcursor(), name, value, NULL); +} + isc_result_t isc_cfgmgr_write(const char *name, const isc_cfgmgr_val_t *value) { isc_result_t result = ISC_R_SUCCESS; MDB_val dbkey; MDB_val dbval; char key[BUFLEN]; + isc_buffer_t b; + isc_region_t r; + char *bdata; isc__cfgmgr_checkopenednodes(); REQUIRE(isc__cfgmgr_ctx.readonly == false); @@ -725,34 +788,39 @@ isc_cfgmgr_write(const char *name, const isc_cfgmgr_val_t *value) { goto out; } - /* - * String is a specific case as it needs an allocation, so the size is - * just the size of the type and the string length + 1. For the other - * cases, in order to keep the flow unconditional and simple there is a - * slight memory waste, i.e. storing a uint32_t will take the size of - * isc_cfgmgr_val_t. - */ - if (value->type == ISC_CFGMGR_STRING) { - dbval.mv_size = sizeof(value->type) + strlen(value->string) + 1; - dbval.mv_data = isc_mem_allocate(isc__cfgmgr_mctx, - dbval.mv_size); - memmove(dbval.mv_data, value, sizeof(value->type)); - strcpy((char *)dbval.mv_data + sizeof(value->type), - value->string); - } else { - dbval = (MDB_val){ .mv_size = sizeof(*value), - /* - * LMDB won't modify the mv_data buffer but - * its API is designed w/o the const buffer. - */ - .mv_data = (void *)value }; + dbval.mv_size = isc__cfgmgr_valuesz(value); + bdata = isc_mem_allocate(isc__cfgmgr_mctx, dbval.mv_size); + isc_buffer_init(&b, bdata, dbval.mv_size); + dbval.mv_data = isc_buffer_base(&b); + + isc_buffer_putuint8(&b, value->type); + isc_buffer_putuint8(&b, isc__cfgmgr_ctx.mode); + switch (value->type) { + case ISC_CFGMGR_UNDEFINED: + UNREACHABLE(); + case ISC_CFGMGR_STRING: + r.base = (unsigned char *)value->string; + r.length = strlen(value->string) + 1; + isc_buffer_copyregion(&b, &r); + break; + case ISC_CFGMGR_BOOLEAN: + isc_buffer_putuint8(&b, value->boolean); + break; + case ISC_CFGMGR_NONE: + break; + case ISC_CFGMGR_SOCKADDR: + r.base = (unsigned char *)&value->sockaddr; + r.length = sizeof(value->sockaddr); + isc_buffer_copyregion(&b, &r); + break; + case ISC_CFGMGR_UINT32: + isc_buffer_putuint32(&b, value->uint32); + break; } REQUIRE(mdb_cursor_put(isc__cfgmgr_lmdbcursor(), &dbkey, &dbval, 0) == 0); - if (value->type == ISC_CFGMGR_STRING) { - isc_mem_free(isc__cfgmgr_mctx, dbval.mv_data); - } + isc_mem_free(isc__cfgmgr_mctx, dbval.mv_data); out: return result; diff --git a/lib/isc/include/isc/cfgmgr.h b/lib/isc/include/isc/cfgmgr.h index ffd147a465..d444b45555 100644 --- a/lib/isc/include/isc/cfgmgr.h +++ b/lib/isc/include/isc/cfgmgr.h @@ -64,7 +64,7 @@ enum isc_cfgmgr_mode { ISC_CFGMGR_MODEBUILTIN, ISC_CFGMGR_MODEUSER, ISC_CFGMGR_MODERUNNING -}; +} __attribute__((__packed__)); /* * Supported data types for read/write operations from/to cfgmgr. @@ -82,7 +82,7 @@ typedef enum isc_cfgmgr_type { * Generic value holding the actual value and type value for * read/write from/to cfgmgr. * - * cfgmgr_type_t::NONE doesn't have associated value, + * cfgmgr_type_t::NONE doesn't have associated value. */ typedef struct isc_cfgmgr_val { isc_cfgmgr_type_t type; diff --git a/tests/isc/cfgmgr_test.c b/tests/isc/cfgmgr_test.c index 99217f4dd0..3f7ebe6c8d 100644 --- a/tests/isc/cfgmgr_test.c +++ b/tests/isc/cfgmgr_test.c @@ -1895,9 +1895,9 @@ ISC_RUN_TEST_IMPL(isc_cfgmgr_dumptests) { // ISC_CFGMGR_UINT32 }; const isc_cfgmgr_prop_t prop2bis = { "prop2bis", // false, ISC_CFGMGR_UINT32 }; // const isc_cfgmgr_prop_t *foo_props[] = { &prop1, &prop2, &prop2bis, -//NULL }; const isc_cfgmgr_clause_t *foo_clauses[] = { &bar, NULL }; -//const isc_cfgmgr_clause_t foo = { "foo", false, false, foo_clauses, -//foo_props }; +// NULL }; const isc_cfgmgr_clause_t *foo_clauses[] = { &bar, NULL }; +// const isc_cfgmgr_clause_t foo = { "foo", false, false, foo_clauses, +// foo_props }; // // const isc_cfgmgr_prop_t prop5 = { "prop5", false, // ISC_CFGMGR_STRING }; const isc_cfgmgr_prop_t *baz_props[] = { &prop5, @@ -1975,8 +1975,9 @@ ISC_RUN_TEST_IMPL(isc_cfgmgr_dumptests) { // cfgmgr_dumptests_clausestarts, // .clauseends = // cfgmgr_dumptests_clauseends, .property = -// cfgmgr_dumptests_property, .state = &buffer, .indent = 8, .dumpmode = -//ISC_CFGMGR_MODEUSER }; +// cfgmgr_dumptests_property, .state = &buffer, +// .indent = 8, .dumpmode = +// ISC_CFGMGR_MODEUSER }; // // isc_buffer_init(&buffer, data, sizeof(data)); // result = isc_cfgmgr_dump(&cfg);