diff --git a/lib/isccfg/cfgmgr.c b/lib/isccfg/cfgmgr.c index 8e2e8d732f..2d2e9481e2 100644 --- a/lib/isccfg/cfgmgr.c +++ b/lib/isccfg/cfgmgr.c @@ -112,6 +112,7 @@ isc_cfgmgr_init(isc_mem_t *mctx, const char *dbpath) { REQUIRE(isc__cfgmgr_mctx == NULL); REQUIRE(isc__cfgmgr_env == NULL); REQUIRE(mctx != NULL); + REQUIRE(dbpath != NULL); isc_mem_attach(mctx, &isc__cfgmgr_mctx); INSIST(isc__cfgmgr_mctx != NULL); @@ -124,6 +125,12 @@ isc_cfgmgr_init(isc_mem_t *mctx, const char *dbpath) { INSIST(isc__cfgmgr_env != NULL); random = isc_random32(); + + /* + * Because LMDB also create a -lock file, we also need + * to build it (even if not passed to LMDB itself) so we can + * immediately delete it + */ REQUIRE(snprintf(dbname, BUFLEN, "%s-%u", dbpath, random) < BUFLEN); REQUIRE(snprintf(dblockname, BUFLEN, "%s-%u-lock", dbpath, random) < BUFLEN); @@ -160,14 +167,15 @@ out: void isc_cfgmgr_deinit(void) { /* - * Well, I'm on the fence about those context checks... It's - * good to have, but because they thread specific, it doesn't - * means there isn't a thread somewhere which haven't released - * its opened clauses and not the one calling cfgmgr_deinit, - * then we'll leak those - even if unlikely as this function - * should be call late in shutdown flow. (That said it's just - * an extra clue, because destroying the context will assert - * anyway, as some memory would not be released yet). + * Well, I'm on the fence about those context checks for the + * deinit function: it's good to have, but because the context + * data is thread specific, it doesn't means there isn't a + * thread somewhere which haven't released its opened clauses + * and not the one calling isc_cfgmgr_deinit, then we'll leak + * those - even if unlikely as this function should be call + * late in shutdown flow. (That said it's just an extra clue, + * because destroying the context will assert anyway, as some + * memory would not be released yet). */ REQUIRE(ISC_LIST_EMPTY(isc__cfgmgr_ctx.openedclauses)); REQUIRE(isc__cfgmgr_ctx.prefix == NULL); @@ -185,12 +193,17 @@ isc_cfgmgr_deinit(void) { static void isc__cfgmgr_buildkey(const char *name, bool trailingdot) { size_t written; - const char *prefix = ISC_LIST_EMPTY(isc__cfgmgr_ctx.openedclauses) - ? "" - : isc__cfgmgr_ctx.prefix; + const char *prefix = ""; const char *dot = trailingdot ? "." : ""; + REQUIRE(name != NULL); REQUIRE(isc__cfgmgr_ctx.buffer != NULL); + REQUIRE(isc__cfgmgr_ctx.prefix != NULL); + + if (ISC_LIST_EMPTY(isc__cfgmgr_ctx.openedclauses) == false) { + prefix = isc__cfgmgr_ctx.prefix; + } + written = snprintf(isc__cfgmgr_ctx.buffer, BUFLEN, "%s%s%s", prefix, name, dot); INSIST(written <= BUFLEN); @@ -203,9 +216,6 @@ isc__cfgmgr_findclause(const char *name, unsigned long *id) { size_t dotpos = 0; REQUIRE(name != NULL); - REQUIRE(isc__cfgmgr_ctx.buffer != NULL); - REQUIRE(isc__cfgmgr_ctx.txn != NULL); - REQUIRE(isc__cfgmgr_ctx.cursor != NULL); isc__cfgmgr_buildkey(name, true); dotpos = strlen(isc__cfgmgr_ctx.buffer) - 1; @@ -294,12 +304,6 @@ static isc_result_t isc__cfgmgr_starttransaction(bool readonly) { MDB_dbi dbi; - REQUIRE(isc__cfgmgr_env != NULL); - REQUIRE(isc__cfgmgr_ctx.prefix == NULL); - REQUIRE(isc__cfgmgr_ctx.buffer == NULL); - REQUIRE(isc__cfgmgr_ctx.txn == NULL); - REQUIRE(isc__cfgmgr_ctx.cursor == NULL); - if (mdb_txn_begin(isc__cfgmgr_env, NULL, readonly ? MDB_RDONLY : 0, &isc__cfgmgr_ctx.txn) != 0) { @@ -423,10 +427,8 @@ isc_cfgmgr_open(const char *name) { static void popclause(void) { - REQUIRE(isc__cfgmgr_env != NULL); - REQUIRE(ISC_LIST_EMPTY(isc__cfgmgr_ctx.openedclauses) == false); - openedclause_t *clause = ISC_LIST_HEAD(isc__cfgmgr_ctx.openedclauses); + ISC_LIST_UNLINK(isc__cfgmgr_ctx.openedclauses, clause, link); isc_mem_free(isc__cfgmgr_mctx, clause->name); isc_mem_put(isc__cfgmgr_mctx, clause, sizeof(*clause)); @@ -510,6 +512,10 @@ isc_cfgmgr_newclause(const char *name) { REQUIRE(isc__cfgmgr_env != NULL); if (isc__cfgmgr_ctx.txn == NULL || isc__cfgmgr_ctx.cursor == NULL) { + REQUIRE(isc__cfgmgr_ctx.prefix == NULL); + REQUIRE(isc__cfgmgr_ctx.buffer == NULL); + REQUIRE(isc__cfgmgr_ctx.txn == NULL); + REQUIRE(isc__cfgmgr_ctx.cursor == NULL); result = isc__cfgmgr_starttransaction(false); } diff --git a/tests/isccfg/cfgmgr_test.c b/tests/isccfg/cfgmgr_test.c index 3a9118cd4d..1802585d92 100644 --- a/tests/isccfg/cfgmgr_test.c +++ b/tests/isccfg/cfgmgr_test.c @@ -22,10 +22,116 @@ #include +#include "../../lib/isccfg/cfgmgr.c" + #include #define TEST_DBPATH "/tmp/named-cfgmgr-lmdb" +ISC_RUN_TEST_IMPL(isc_cfgmgr_assertions) { + isc_cfgmgr_val_t dummyval; + + /* + * Invalid cfgmgr initialization + */ + expect_assert_failure(isc_cfgmgr_init(NULL, TEST_DBPATH)); + expect_assert_failure(isc_cfgmgr_init(mctx, NULL)); + expect_assert_failure(isc_cfgmgr_close()); + expect_assert_failure(isc_cfgmgr_getval("foo", &dummyval)); + expect_assert_failure(isc_cfgmgr_getnextlistval(&dummyval)); + expect_assert_failure(isc_cfgmgr_setval("foo", &dummyval)); + expect_assert_failure(isc_cfgmgr_setnextlistval("foo", &dummyval)); + expect_assert_failure(isc_cfgmgr_nextclause()); + expect_assert_failure(isc_cfgmgr_open("foo")); + expect_assert_failure(isc_cfgmgr_openrw("foo")); + expect_assert_failure(isc_cfgmgr_newclause("foo")); + expect_assert_failure(isc_cfgmgr_delclause()); + expect_assert_failure(isc_cfgmgr_deinit()); + + /* + * operations which must be used under transaction + */ + (void)isc_cfgmgr_init(mctx, TEST_DBPATH); + (void)isc_cfgmgr_newclause("foo"); + (void)isc_cfgmgr_setval("bar", + &(isc_cfgmgr_val_t){ .type = ISC_CFGMGR_NONE }); + (void)isc_cfgmgr_close(); + expect_assert_failure(isc_cfgmgr_delclause()); + expect_assert_failure(isc_cfgmgr_getval("foo", &dummyval)); + expect_assert_failure(isc_cfgmgr_getnextlistval(&dummyval)); + expect_assert_failure(isc_cfgmgr_setval("foo", &dummyval)); + expect_assert_failure(isc_cfgmgr_setnextlistval("foo", &dummyval)); + expect_assert_failure(isc_cfgmgr_nextclause()); + isc_cfgmgr_deinit(); + + /* + * One close for one open + */ + (void)isc_cfgmgr_init(mctx, TEST_DBPATH); + (void)isc_cfgmgr_newclause("foo"); + (void)isc_cfgmgr_close(); + expect_assert_failure(isc_cfgmgr_close()); + isc_cfgmgr_deinit(); + + /* + * nothing is open in case of top-level open failure + */ + (void)isc_cfgmgr_init(mctx, TEST_DBPATH); + (void)isc_cfgmgr_open("foo"); + expect_assert_failure(isc_cfgmgr_close()); + (void)isc_cfgmgr_openrw("foo"); + expect_assert_failure(isc_cfgmgr_close()); + isc_cfgmgr_deinit(); + + /* + * can't write on non-write open clause nor open a subclause as write + */ + (void)isc_cfgmgr_init(mctx, TEST_DBPATH); + (void)isc_cfgmgr_newclause("foo"); + (void)isc_cfgmgr_setval("bar", + &(isc_cfgmgr_val_t){ .type = ISC_CFGMGR_NONE }); + (void)isc_cfgmgr_newclause("subfoo"); + (void)isc_cfgmgr_setval("subfoobar", + &(isc_cfgmgr_val_t){ .type = ISC_CFGMGR_NONE }); + (void)isc_cfgmgr_close(); + (void)isc_cfgmgr_close(); + (void)isc_cfgmgr_open("foo"); + expect_assert_failure(isc_cfgmgr_setval( + "bar", &(isc_cfgmgr_val_t){ .type = ISC_CFGMGR_NONE })); + expect_assert_failure(isc_cfgmgr_setval( + "baz", &(isc_cfgmgr_val_t){ .type = ISC_CFGMGR_NONE })); + expect_assert_failure(isc_cfgmgr_newclause("gee")); + expect_assert_failure(isc_cfgmgr_delclause()); + expect_assert_failure(isc_cfgmgr_openrw("subfoo")); + (void)isc_cfgmgr_close(); + isc_cfgmgr_deinit(); + + /* + * reading values parameters + */ + (void)isc_cfgmgr_init(mctx, TEST_DBPATH); + (void)isc_cfgmgr_newclause("foo"); + (void)isc_cfgmgr_setval("bar", + &(isc_cfgmgr_val_t){ .type = ISC_CFGMGR_NONE }); + expect_assert_failure(isc_cfgmgr_getval("bar", NULL)); + (void)isc_cfgmgr_close(); + isc_cfgmgr_deinit(); + + /* + * can't de-init while having opened clause + */ + (void)isc_cfgmgr_init(mctx, TEST_DBPATH); + (void)isc_cfgmgr_newclause("foo"); + expect_assert_failure(isc_cfgmgr_deinit()); + (void)isc_cfgmgr_close(); + (void)isc_cfgmgr_deinit(); + + /* + * can't deinit twice + */ + expect_assert_failure(isc_cfgmgr_deinit()); +} + ISC_RUN_TEST_IMPL(isc_cfgmgr_rw) { isc_result_t result; isc_cfgmgr_val_t val1; @@ -792,7 +898,8 @@ ISC_RUN_TEST_IMPL(isc_cfgmgr_nested_clauses) { result = isc_cfgmgr_close(); assert_int_equal(result, ISC_R_SUCCESS); - val = (isc_cfgmgr_val_t){ .type = ISC_CFGMGR_UNDEFINED, .string = NULL }; + val = (isc_cfgmgr_val_t){ .type = ISC_CFGMGR_UNDEFINED, + .string = NULL }; result = isc_cfgmgr_open("foo"); assert_int_equal(result, ISC_R_SUCCESS); @@ -813,7 +920,8 @@ ISC_RUN_TEST_IMPL(isc_cfgmgr_nested_clauses) { result = isc_cfgmgr_nextclause(); assert_int_equal(result, ISC_R_SUCCESS); - val = (isc_cfgmgr_val_t){ .type = ISC_CFGMGR_UNDEFINED, .string = NULL }; + val = (isc_cfgmgr_val_t){ .type = ISC_CFGMGR_UNDEFINED, + .string = NULL }; result = isc_cfgmgr_getval("propsubclause", &val); assert_int_equal(result, ISC_R_SUCCESS); assert_int_equal(val.type, ISC_CFGMGR_STRING); @@ -1102,6 +1210,7 @@ ISC_RUN_TEST_IMPL(isc_cfgmgr_threads) { } ISC_TEST_LIST_START +ISC_TEST_ENTRY(isc_cfgmgr_assertions) ISC_TEST_ENTRY(isc_cfgmgr_rw) ISC_TEST_ENTRY(isc_cfgmgr_override) ISC_TEST_ENTRY(isc_cfgmgr_rw_string)