diff --git a/lib/isc/cfgmgr.c b/lib/isc/cfgmgr.c index f5c01477e5..b7fc77060c 100644 --- a/lib/isc/cfgmgr.c +++ b/lib/isc/cfgmgr.c @@ -30,6 +30,11 @@ */ #define BUFLEN 511 +/* + * The builtin prefix name + */ +#define BUILTIN_PREFIX "builtin" + typedef struct openedclause openedclause_t; struct openedclause { char *name; @@ -45,8 +50,10 @@ typedef struct { MDB_cursor *cursor; MDB_txn *txn; bool readonly; + bool builtinon; } context_t; +static const char *isc__cfgmgr_lasterror = ""; static isc_mem_t *isc__cfgmgr_mctx = NULL; static MDB_env *isc__cfgmgr_env = NULL; static thread_local context_t isc__cfgmgr_ctx = @@ -55,7 +62,8 @@ static thread_local context_t isc__cfgmgr_ctx = .buffer = NULL, .cursor = NULL, .txn = NULL, - .readonly = false }; + .readonly = false, + .builtinon = false }; static unsigned long isc__cfgmgr_parseid(const char *dbkey) { @@ -219,12 +227,12 @@ static isc_result_t isc__cfgmgr_findclause(const char *name, unsigned long *id) { isc_result_t result = ISC_R_SUCCESS; MDB_val dbkey; - size_t dotpos = 0; + size_t prefixlen = 0; REQUIRE(name != NULL); isc__cfgmgr_buildkey(name, true); - dotpos = strlen(isc__cfgmgr_ctx.buffer) - 1; + prefixlen = strlen(isc__cfgmgr_ctx.buffer); dbkey = (MDB_val){ .mv_size = strlen(isc__cfgmgr_ctx.buffer) + 1, .mv_data = (char *)isc__cfgmgr_ctx.buffer }; @@ -241,11 +249,13 @@ isc__cfgmgr_findclause(const char *name, unsigned long *id) { } /* - * LMDB found a key which starts by "prefix", so let's make + * LMDB found a key with "prefix", so let's make * sure it's actually the same prefix by checking the found - * key has an immediate leading dot + * key has the exact same prefix (including the leading dot) */ - if (dbkey.mv_size <= dotpos || ((char *)dbkey.mv_data)[dotpos] != '.') { + if (dbkey.mv_size <= prefixlen || + strncmp(dbkey.mv_data, isc__cfgmgr_ctx.buffer, prefixlen)) + { result = ISC_R_NOTFOUND; goto out; } @@ -304,6 +314,7 @@ isc__cfgmgr_freectx(void) { isc_mem_free(isc__cfgmgr_mctx, isc__cfgmgr_ctx.prefix); isc__cfgmgr_ctx.txn = NULL; isc__cfgmgr_ctx.cursor = NULL; + isc__cfgmgr_ctx.builtinon = false; } static isc_result_t @@ -363,14 +374,46 @@ isc_cfgmgr_rwtransaction(void) { return isc__cfgmgr_starttransaction(false); } -static void -popclause(void) { - openedclause_t *clause = ISC_LIST_HEAD(isc__cfgmgr_ctx.openedclauses); +const char * +isc_cfgmgr_lasterror(void) { + return isc__cfgmgr_lasterror; +} - 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)); - isc__cfgmgr_updateprefix(); +static isc_result_t +isc__cfgmgr_validate(void) { + isc_result_t result = ISC_R_SUCCESS; + + return result; +} + +void +isc_cfgmgr_builtinon(void) { + REQUIRE(isc__cfgmgr_env != NULL); + REQUIRE(isc__cfgmgr_ctx.txn != NULL); + REQUIRE(isc__cfgmgr_ctx.cursor != NULL); + REQUIRE(isc__cfgmgr_ctx.builtinon == false); + + isc__cfgmgr_ctx.builtinon = true; + + strcpy(isc__cfgmgr_ctx.buffer, isc__cfgmgr_ctx.prefix); + REQUIRE(snprintf(isc__cfgmgr_ctx.prefix, BUFLEN, BUILTIN_PREFIX ".%s", + isc__cfgmgr_ctx.buffer) < BUFLEN); +} + +void +isc_cfgmgr_builtinoff(void) { + REQUIRE(isc__cfgmgr_env != NULL); + REQUIRE(isc__cfgmgr_ctx.txn != NULL); + REQUIRE(isc__cfgmgr_ctx.cursor != NULL); + REQUIRE(isc__cfgmgr_ctx.builtinon == true); + REQUIRE(strncmp(isc__cfgmgr_ctx.prefix, BUILTIN_PREFIX, + strlen(BUILTIN_PREFIX)) == 0); + + REQUIRE(snprintf(isc__cfgmgr_ctx.buffer, BUFLEN, "%s", + isc__cfgmgr_ctx.prefix + strlen(BUILTIN_PREFIX) + 1) < + BUFLEN); + strcpy(isc__cfgmgr_ctx.prefix, isc__cfgmgr_ctx.buffer); + isc__cfgmgr_ctx.builtinon = false; } isc_result_t @@ -382,14 +425,21 @@ isc_cfgmgr_commit(void) { REQUIRE(isc__cfgmgr_ctx.cursor != NULL); while (ISC_LIST_EMPTY(isc__cfgmgr_ctx.openedclauses) == false) { - popclause(); + isc_cfgmgr_close(); } - mdb_cursor_close(isc__cfgmgr_ctx.cursor); - if (mdb_txn_commit(isc__cfgmgr_ctx.txn) != 0) { - result = ISC_R_FAILURE; + result = isc__cfgmgr_validate(); + + if (result == ISC_R_SUCCESS) { + mdb_cursor_close(isc__cfgmgr_ctx.cursor); + if (mdb_txn_commit(isc__cfgmgr_ctx.txn) != 0) { + isc__cfgmgr_lasterror = "configuration database error"; + result = ISC_R_FAILURE; + } + isc__cfgmgr_freectx(); + } else { + isc_cfgmgr_rollback(); } - isc__cfgmgr_freectx(); return result; } @@ -401,7 +451,7 @@ isc_cfgmgr_rollback(void) { REQUIRE(isc__cfgmgr_ctx.cursor != NULL); while (ISC_LIST_EMPTY(isc__cfgmgr_ctx.openedclauses) == false) { - popclause(); + isc_cfgmgr_close(); } mdb_cursor_close(isc__cfgmgr_ctx.cursor); @@ -429,6 +479,8 @@ isc_cfgmgr_open(const char *name) { void isc_cfgmgr_close(void) { + openedclause_t *clause; + REQUIRE(isc__cfgmgr_env != NULL); REQUIRE(ISC_LIST_EMPTY(isc__cfgmgr_ctx.openedclauses) == false); REQUIRE(isc__cfgmgr_ctx.prefix != NULL); @@ -436,7 +488,11 @@ isc_cfgmgr_close(void) { REQUIRE(isc__cfgmgr_ctx.txn != NULL); REQUIRE(isc__cfgmgr_ctx.cursor != NULL); - popclause(); + 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)); + isc__cfgmgr_updateprefix(); } void @@ -577,6 +633,60 @@ out: return result; } +static isc_result_t +isc__cfgmgr_getval(const char *name, isc_cfgmgr_val_t *value); + +static isc_result_t +isc__cfgmgr_getvalparent(const char *name, isc_cfgmgr_val_t *value) { + isc_result_t result = ISC_R_NOTFOUND; + openedclause_t *clause; + + REQUIRE(ISC_LIST_EMPTY(isc__cfgmgr_ctx.openedclauses) == false); + clause = ISC_LIST_HEAD(isc__cfgmgr_ctx.openedclauses); + + if (isc__cfgmgr_ctx.builtinon) { + goto out; + } + + if (clause->link.next == NULL) { + /* + * We were already at a top-level clause, so let's enable the + * builtin mode to attempt to find name in the defaults. + */ + isc_cfgmgr_builtinon(); + result = isc__cfgmgr_getval(name, value); + isc_cfgmgr_builtinoff(); + } else { + /* + * Not at the top-level clause, so let's recursively attempt in + * the parent clauses until we reach the top-level (and attempt + * the builtin mode above). + * + * isc_cfgmgr_open/close could be used instead of directly + * tweaking the openedclauses stack, but this would require + * extra LMDB lookup to retreive the current clause id once + * we're done. + */ + ISC_LIST_UNLINK(isc__cfgmgr_ctx.openedclauses, clause, link); + isc__cfgmgr_updateprefix(); + + /* + * Recursively attempt to get a value in the parents then + * restore the current opened clause state + */ + result = isc__cfgmgr_getval(name, value); + + /* + * Restore the previous opened clause + */ + ISC_LIST_PREPEND(isc__cfgmgr_ctx.openedclauses, clause, link); + isc__cfgmgr_updateprefix(); + } + +out: + return result; +} + static isc_result_t isc__cfgmgr_getval(const char *name, isc_cfgmgr_val_t *value) { isc_result_t result = ISC_R_SUCCESS; @@ -602,6 +712,10 @@ isc__cfgmgr_getval(const char *name, isc_cfgmgr_val_t *value) { }; if (mdb_cursor_get(isc__cfgmgr_ctx.cursor, &dbkey, &dbval, opt) != 0) { result = opt == MDB_NEXT_DUP ? ISC_R_NOMORE : ISC_R_NOTFOUND; + + if (result == ISC_R_NOTFOUND) { + result = isc__cfgmgr_getvalparent(name, value); + } goto out; } diff --git a/lib/isc/include/isc/cfgmgr.h b/lib/isc/include/isc/cfgmgr.h index 09fd62d167..9d731b611f 100644 --- a/lib/isc/include/isc/cfgmgr.h +++ b/lib/isc/include/isc/cfgmgr.h @@ -44,11 +44,31 @@ typedef struct isc_cfgmgr_val { }; } isc_cfgmgr_val_t; +/* + * Thoe both function enable or disable the "builtin" mode. (disabled by + * default, and automatically disabled anytime a transaction finished). This + * implicitely namespacing the clause name when reading or + * writting values in cfgmgr. Must be called under a transaction. + */ +void +isc_cfgmgr_builtinon(void); + +void +isc_cfgmgr_builtinoff(void); + /* * Get the property "name" in the opened clause into the caller * allocated "value" and returns ISC_R_SUCCESS. Returns ISC_R_NOTFOUND * and "*value" is not mutated if "name" is not found. If "name" is a list * property, get its head. + * + * If a value is not found and builtin mode is off, and before returning + * ISC_R_NOTFOUND, cfgmgr internally attempts to lookup for such value in the + * parent clauses. If the name is found, the parent value is returned. When the + * top-level parent is reached (or if the current clause is a top-level one), a + * last attempt is made by trying the builtin mode namespacing of the clause + * name (which is how isc_cfgmgr_setval() store values when + * isc_cfgmgr_builtinon() is set). */ isc_result_t isc_cfgmgr_getval(const char *name, isc_cfgmgr_val_t *value); @@ -150,15 +170,31 @@ isc_result_t isc_cfgmgr_rwtransaction(void); /* - * Makes all changes applied during this transaction visible to all transaction - * started after this call returns. If something goes wrong while applying the - * changes, ISC_R_FAILURE is returned and the changes made during the - * transaction are discareded. ISC_R_SUCCESS is returned otherwise. If a clause - * is opened when this function is called, it is automatically closed. + * Validate the new configuration and makes all changes applied during this + * transaction visible to all transaction started after this call returns. If + * something goes wrong while applying the changes, ISC_R_FAILURE is returned + * and the changes made during the transaction are discareded. ISC_R_SUCCESS is + * returned otherwise. If a clause is opened when this function is called, it is + * automatically closed. + * + * If the validation of the new configuration fails, the transaction is + * rolled-backed and all the changes are discarded. It is possible to get + * details about the error by calling `isc_cfgmgr_lasterror()`. */ isc_result_t isc_cfgmgr_commit(void); +/* + * Return a NULL-terminated string explaining why the last commit fails, most + * likely a configuration validation error. If no error occured so far, return + * an empty string. + * + * This is a thread-local string and it is valid until the next usage of cfgmgr + * on this thread. + */ +const char * +isc_cfgmgr_lasterror(void); + /* * Discard all the changes made during transaction and terminate the * transaction. If a clause is opened when this function is called, it is diff --git a/tests/isc/cfgmgr_test.c b/tests/isc/cfgmgr_test.c index b3619525db..6dc382b8d2 100644 --- a/tests/isc/cfgmgr_test.c +++ b/tests/isc/cfgmgr_test.c @@ -373,6 +373,11 @@ ISC_RUN_TEST_IMPL(isc_cfgmgr_rw) { result = isc_cfgmgr_commit(); assert_int_equal(result, ISC_R_SUCCESS); + /* + * No errors, so empty last error message + */ + assert_string_equal(isc_cfgmgr_lasterror(), ""); + isc_cfgmgr_deinit(); } @@ -1299,6 +1304,233 @@ ISC_RUN_TEST_IMPL(isc_cfgmgr_threads) { isc_cfgmgr_deinit(); } +ISC_RUN_TEST_IMPL(isc_cfgmgr_validation_error) { + isc_result_t result; + + result = isc_cfgmgr_init(mctx, TEST_DBPATH); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * To be done - but first it needs a clear pattern to walk accross the + * whole configuration. + */ + + isc_cfgmgr_deinit(); +} + +ISC_RUN_TEST_IMPL(isc_cfgmgr_getparent) { + isc_result_t result; + isc_cfgmgr_val_t val; + + result = isc_cfgmgr_init(mctx, TEST_DBPATH); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_rwtransaction(); + assert_int_equal(result, ISC_R_SUCCESS); + + assert_int_equal(isc_cfgmgr_open("foo"), ISC_R_NOTFOUND); + assert_int_equal(isc_cfgmgr_open("bar"), ISC_R_NOTFOUND); + assert_int_equal(isc_cfgmgr_open("baz"), ISC_R_NOTFOUND); + + isc_cfgmgr_newclause("foo"); + + val = (isc_cfgmgr_val_t){ .type = ISC_CFGMGR_UINT32, .uint32 = 42 }; + result = isc_cfgmgr_setval("prop1", &val); + assert_int_equal(result, ISC_R_SUCCESS); + + isc_cfgmgr_newclause("bar"); + val = (isc_cfgmgr_val_t){ .type = ISC_CFGMGR_UINT32, .uint32 = 24 }; + result = isc_cfgmgr_setval("prop2", &val); + assert_int_equal(result, ISC_R_SUCCESS); + + isc_cfgmgr_newclause("baz"); + val = (isc_cfgmgr_val_t){ .type = ISC_CFGMGR_UINT32, .uint32 = 999 }; + result = isc_cfgmgr_setval("prop3", &val); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_commit(); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_transaction(); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_open("foo"); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_getval("prop1", &val); + assert_int_equal(result, ISC_R_SUCCESS); + assert_int_equal(val.type, ISC_CFGMGR_UINT32); + assert_int_equal(val.uint32, 42); + + /* + * prop2 and prop3 are in child clauses, can't be found + */ + result = isc_cfgmgr_getval("prop2", &val); + assert_int_equal(result, ISC_R_NOTFOUND); + result = isc_cfgmgr_getval("prop3", &val); + assert_int_equal(result, ISC_R_NOTFOUND); + + /* + * open the child bar clause and it gets possible to read prop2 _and_ + * prop1 (thanks to getvalparent flow) but still not prop3 as it's in a + * child clause + */ + result = isc_cfgmgr_open("bar"); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_getval("prop2", &val); + assert_int_equal(result, ISC_R_SUCCESS); + assert_int_equal(val.type, ISC_CFGMGR_UINT32); + assert_int_equal(val.uint32, 24); + + result = isc_cfgmgr_getval("prop1", &val); + assert_int_equal(result, ISC_R_SUCCESS); + assert_int_equal(val.type, ISC_CFGMGR_UINT32); + assert_int_equal(val.uint32, 42); + + result = isc_cfgmgr_getval("prop3", &val); + assert_int_equal(result, ISC_R_NOTFOUND); + + /* + * finally open the baz clause (nested in bar), all properties can be + * read + */ + result = isc_cfgmgr_open("baz"); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_getval("prop3", &val); + assert_int_equal(result, ISC_R_SUCCESS); + assert_int_equal(val.type, ISC_CFGMGR_UINT32); + assert_int_equal(val.uint32, 999); + + result = isc_cfgmgr_getval("prop2", &val); + assert_int_equal(result, ISC_R_SUCCESS); + assert_int_equal(val.type, ISC_CFGMGR_UINT32); + assert_int_equal(val.uint32, 24); + + result = isc_cfgmgr_getval("prop1", &val); + assert_int_equal(result, ISC_R_SUCCESS); + assert_int_equal(val.type, ISC_CFGMGR_UINT32); + assert_int_equal(val.uint32, 42); + + result = isc_cfgmgr_commit(); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * bar override prop1, so it has its own prop1 (and this doesn't affect + * the one in foo, but baz now see the overriden one) + */ + result = isc_cfgmgr_rwtransaction(); + + assert_int_equal(result, ISC_R_SUCCESS); + result = isc_cfgmgr_open("foo"); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_open("bar"); + assert_int_equal(result, ISC_R_SUCCESS); + + val.uint32 = 42424242; + result = isc_cfgmgr_setval("prop1", &val); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_open("baz"); + val.uint32 = 0; + result = isc_cfgmgr_getval("prop1", &val); + assert_int_equal(result, ISC_R_SUCCESS); + assert_int_equal(val.uint32, 42424242); + + /* + * Let's go back to foo and make sure we still have 42 for prop1 + * there... + */ + isc_cfgmgr_close(); + isc_cfgmgr_close(); + + result = isc_cfgmgr_getval("prop1", &val); + assert_int_equal(result, ISC_R_SUCCESS); + assert_int_equal(val.uint32, 42); + + isc_cfgmgr_commit(); + isc_cfgmgr_deinit(); +} + +ISC_RUN_TEST_IMPL(isc_cfgmgr_opentests) { + isc_result_t result; + isc_cfgmgr_val_t val = { .type = ISC_CFGMGR_UINT32, .uint32 = 1234 }; + + result = isc_cfgmgr_init(mctx, TEST_DBPATH); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_rwtransaction(); + assert_int_equal(result, ISC_R_SUCCESS); + + assert_int_equal(isc_cfgmgr_open("foo"), ISC_R_NOTFOUND); + assert_int_equal(isc_cfgmgr_open("bar"), ISC_R_NOTFOUND); + assert_int_equal(isc_cfgmgr_open("baz"), ISC_R_NOTFOUND); + + isc_cfgmgr_newclause("foo"); + isc_cfgmgr_setval("prop", &val); + assert_int_equal(result, ISC_R_SUCCESS); + + isc_cfgmgr_newclause("bar"); + isc_cfgmgr_setval("prop", &val); + assert_int_equal(result, ISC_R_SUCCESS); + + isc_cfgmgr_newclause("baz"); + isc_cfgmgr_setval("prop", &val); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_commit(); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_rwtransaction(); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * Can't open directly bar or baz! + */ + result = isc_cfgmgr_open("bar"); + assert_int_equal(result, ISC_R_NOTFOUND); + + result = isc_cfgmgr_open("baz"); + assert_int_equal(result, ISC_R_NOTFOUND); + + result = isc_cfgmgr_open("foo"); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * And can't open directly baz (not open foo, which doesn't exists there + * either)! + */ + result = isc_cfgmgr_open("baz"); + assert_int_equal(result, ISC_R_NOTFOUND); + + result = isc_cfgmgr_open("foo"); + assert_int_equal(result, ISC_R_NOTFOUND); + + result = isc_cfgmgr_open("bar"); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * Finally open baz (but then can't open bar or foo already opened + * anyway, but the reason is because they doesn't exists here) + */ + result = isc_cfgmgr_open("baz"); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_cfgmgr_open("foo"); + assert_int_equal(result, ISC_R_NOTFOUND); + + result = isc_cfgmgr_open("bar"); + assert_int_equal(result, ISC_R_NOTFOUND); + + result = isc_cfgmgr_open("baz"); + assert_int_equal(result, ISC_R_NOTFOUND); + + (void)isc_cfgmgr_commit(); + isc_cfgmgr_deinit(); +} + ISC_TEST_LIST_START ISC_TEST_ENTRY(isc_cfgmgr_assertions) ISC_TEST_ENTRY(isc_cfgmgr_rw) @@ -1311,5 +1543,8 @@ ISC_TEST_ENTRY(isc_cfgmgr_repeatable_clauses) ISC_TEST_ENTRY(isc_cfgmgr_nested_clauses) ISC_TEST_ENTRY(isc_cfgmgr_threads) ISC_TEST_ENTRY(isc_cfgmgr_parseid) +ISC_TEST_ENTRY(isc_cfgmgr_validation_error) +ISC_TEST_ENTRY(isc_cfgmgr_getparent) +ISC_TEST_ENTRY(isc_cfgmgr_opentests) ISC_TEST_LIST_END ISC_TEST_MAIN