lib/rules,cache: use transactions, improve assertions

When inserting rules from a config file, process everything
in a single transaction to avoid using inconsistent sets of rules,
especially in a different instance and/or in case some error happens.

Also fix some over-eager assertions (CHECK_RET).

diff --git a/daemon/bindings/cache.c b/daemon/bindings/cache.c
index d42ff627696bd43cc1f56dca988ba6a19a740ecc..7247076795340ecf09a6a9dc70b69402f93a1573 100644 (file)
--- a/daemon/bindings/cache.c
+++ b/daemon/bindings/cache.c
@@ -190,8 +190,9 @@ static int cache_open(lua_State *L)
 
        /* Reopen cache */
        struct kr_cdb_opts opts = {
-               (conf && strlen(conf)) ? conf : ".",
-               cache_size
+               .is_cache = true,
+               .path = (conf && strlen(conf)) ? conf : ".",
+               .maxsize = cache_size,
        };
        int ret = kr_cache_open(&engine->resolver.cache, api, &opts, engine->pool);
        if (ret != 0) {

diff --git a/daemon/main.c b/daemon/main.c
index 7de6f42b7b4de0de29b2a7c1d0a7c3df0eaf1655..6b8ca064bedc1fd7a1d934910560eb53f9c874e4 100644 (file)
--- a/daemon/main.c
+++ b/daemon/main.c
@@ -612,12 +612,16 @@ int main(int argc, char **argv)
                goto cleanup;
        }
 
+       /* Starting everything succeeded, so commit rule DB changes. */
+       kr_rules_commit(true);
+
        /* Run the event loop */
        ret = run_worker(loop, &engine, fork_id == 0, the_args);
 
 cleanup:/* Cleanup. */
        engine_deinit(&engine);
        worker_deinit();
+       kr_rules_commit(false);
        kr_rules_deinit();
        if (loop != NULL) {
                uv_loop_close(loop);

diff --git a/lib/cache/api.c b/lib/cache/api.c
index 4b4a8c593a5be4f5614f54684b0b6732683e3e63..37dff26eda09875acfc2ac7549d6c3222980c017 100644 (file)
--- a/lib/cache/api.c
+++ b/lib/cache/api.c
@@ -175,7 +175,7 @@ int kr_cache_commit(struct kr_cache *cache)
                return kr_error(EINVAL);
        }
        if (cache->api->commit) {
-               return cache_op(cache, commit);
+               return cache_op(cache, commit, true);
        }
        return kr_ok();
 }

diff --git a/lib/cache/cdb_api.h b/lib/cache/cdb_api.h
index 43d1433f50a198e510b3e792b2acc9c5461094b8..41130b621cd436ffc31274ab89943bfbf46b696f 100644 (file)
--- a/lib/cache/cdb_api.h
+++ b/lib/cache/cdb_api.h
@@ -57,8 +57,11 @@ struct kr_cdb_api {
        int (*count)(kr_cdb_pt db, struct kr_cdb_stats *stat);
        int (*clear)(kr_cdb_pt db, struct kr_cdb_stats *stat);
 
-       /** Run after a row of operations to release transaction/lock if needed. */
-       int (*commit)(kr_cdb_pt db, struct kr_cdb_stats *stat);
+       /** Run after a row of operations to release transaction/lock if needed.
+        * \param accept true=commit / false=abort
+        * \return error code - accepting RW transactions can fail with LMDB.
+        */
+       int (*commit)(kr_cdb_pt db, struct kr_cdb_stats *stat, bool accept);
 
        /* Data access */
 

diff --git a/lib/cache/cdb_lmdb.c b/lib/cache/cdb_lmdb.c
index 76570f1928c08fe4ada96a4758757da1ef769109..742744bbeddc6daee83424097ab537b0cd5b0f5d 100644 (file)
--- a/lib/cache/cdb_lmdb.c
+++ b/lib/cache/cdb_lmdb.c
@@ -72,7 +72,8 @@ static inline kr_cdb_pt env2db(struct lmdb_env *env)
        return (kr_cdb_pt)env;
 }
 
-static int cdb_commit(kr_cdb_pt db, struct kr_cdb_stats *stats);
+static int cdb_commit(kr_cdb_pt db, struct kr_cdb_stats *stats, bool accept);
+static void txn_abort(struct lmdb_env *env);
 
 /** @brief Convert LMDB error code. */
 static int lmdb_error(int error)
@@ -106,7 +107,7 @@ static inline MDB_val val_knot2mdb(knot_db_val_t v)
  * It's much lighter than reopen_env(). */
 static int refresh_mapsize(struct lmdb_env *env)
 {
-       int ret = cdb_commit(env2db(env), NULL);
+       int ret = cdb_commit(env2db(env), NULL, true);
        if (!ret) ret = lmdb_error(mdb_env_set_mapsize(env->env, 0));
        if (ret) return ret;
 
@@ -215,9 +216,14 @@ static int txn_get(struct lmdb_env *env, MDB_txn **txn, bool rdonly)
        return kr_ok();
 }
 
-static int cdb_commit(kr_cdb_pt db, struct kr_cdb_stats *stats)
+static int cdb_commit(kr_cdb_pt db, struct kr_cdb_stats *stats, bool accept)
 {
        struct lmdb_env *env = db2env(db);
+       if (!accept) {
+               txn_abort(env);
+               return kr_ok();
+       }
+
        int ret = kr_ok();
        if (env->txn.rw) {
                if (stats) stats->commit++;
@@ -238,9 +244,11 @@ static int txn_curs_get(struct lmdb_env *env, MDB_cursor **curs, struct kr_cdb_s
                return kr_error(EINVAL);
        if (env->txn.ro_curs_active)
                goto success;
-       /* Only in a read-only txn; TODO: it's a bit messy/coupled */
+       /* Only in a read-only txn; TODO: it's a bit messy/coupled
+        * At least for rules we don't do the auto-commit feature. */
        if (env->txn.rw) {
-               int ret = cdb_commit(env2db(env), stats);
+               if (!env->is_cache) return kr_error(EINPROGRESS);
+               int ret = cdb_commit(env2db(env), stats, true);
                if (ret) return ret;
        }
        MDB_txn *txn = NULL;
@@ -296,7 +304,7 @@ static void cdb_close_env(struct lmdb_env *env, struct kr_cdb_stats *stats)
 
        /* Get rid of any transactions. */
        txn_free_ro(env);
-       cdb_commit(env2db(env), stats);
+       cdb_commit(env2db(env), stats, env->is_cache);
 
        mdb_env_sync(env->env, 1);
        stats->close++;
@@ -562,8 +570,8 @@ static int cdb_clear(kr_cdb_pt db, struct kr_cdb_stats *stats)
                int ret = txn_get(env, &txn, false);
                if (ret == kr_ok()) {
                        ret = lmdb_error(mdb_drop(txn, env->dbi, 0));
-                       if (ret == kr_ok()) {
-                               ret = cdb_commit(db, stats);
+                       if (ret == kr_ok() && env->is_cache) {
+                               ret = cdb_commit(db, stats, true);
                        }
                        if (ret == kr_ok()) {
                                return ret;
@@ -577,7 +585,7 @@ static int cdb_clear(kr_cdb_pt db, struct kr_cdb_stats *stats)
 
        /* We are about to switch to a different file, so end all txns, to be sure. */
        txn_free_ro(env);
-       (void) cdb_commit(db, stats);
+       (void) cdb_commit(db, stats, env->is_cache);
 
        const char *path = NULL;
        int ret = mdb_env_get_path(env->env, &path);

diff --git a/lib/rules/api.c b/lib/rules/api.c
index 4d003641b3f33ac083749932e631ddef9ff55460..e179b2ce81c9641f4424ea22886d4703f3b46c6f 100644 (file)
--- a/lib/rules/api.c
+++ b/lib/rules/api.c
@@ -167,7 +167,6 @@ int kr_rules_init(void)
        knot_db_val_t key = { .data = key_rs, .len = sizeof(key_rs) };
        knot_db_val_t rulesets = { .data = &RULESET_DEFAULT, .len = strlen(RULESET_DEFAULT) + 1 };
        ret = ruledb_op(write, &key, &rulesets, 1);
-       if (ret == 0) ret = ruledb_op(commit);
        if (ret == 0) return kr_ok();
 failure:
        free(the_rules);
@@ -183,6 +182,12 @@ void kr_rules_deinit(void)
        the_rules = NULL;
 }
 
+int kr_rules_commit(bool accept)
+{
+       if (!the_rules) return kr_error(EINVAL);
+       return ruledb_op(commit, accept);
+}
+
 static bool kr_rule_consume_tags(knot_db_val_t *val, const struct kr_request *req)
 {
        kr_rule_tags_t tags;
@@ -534,7 +539,11 @@ int local_data_ins(knot_db_val_t key, const knot_rrset_t *rrs,
                        + rdataset_dematerialize_size(sig_rds);
        knot_db_val_t val = { .data = NULL, .len = to_alloc };
        int ret = ruledb_op(write, &key, &val, 1);
-       CHECK_RET(ret);
+       if (ret) {
+               // ENOSPC seems to be the only expectable error.
+               kr_assert(ret == kr_error(ENOSPC));
+               return kr_error(ret);
+       }
 
        // Write all the data.
        memcpy(val.data, &tags, sizeof(tags));
@@ -544,19 +553,14 @@ int local_data_ins(knot_db_val_t key, const knot_rrset_t *rrs,
        rdataset_dematerialize(&rrs->rrs, val.data);
        val.data += rr_ssize;
        rdataset_dematerialize(sig_rds, val.data);
-
-       return ruledb_op(commit);
+       return kr_ok();
 }
 int kr_rule_local_data_del(const knot_rrset_t *rrs, kr_rule_tags_t tags)
 {
        kr_require(the_rules);
        uint8_t key_data[KEY_MAXLEN];
        knot_db_val_t key = local_data_key(rrs, key_data, RULESET_DEFAULT);
-       int ret = ruledb_op(remove, &key, 1);
-       if (ret != 1)
-               return ret;
-       ret = ruledb_op(commit);
-       return ret == 0 ? 1 : ret;
+       return ruledb_op(remove, &key, 1);
 }
 
 /** Empty or NXDOMAIN or NODATA.  Returning kr_error(EAGAIN) means the rule didn't match. */
@@ -720,7 +724,11 @@ int insert_trivial_zone(val_zla_type_t ztype, uint32_t ttl,
        if (has_ttl)
                val.len += sizeof(ttl);
        int ret = ruledb_op(write, &key, &val, 1);
-       CHECK_RET(ret);
+       if (ret) {
+               // ENOSPC seems to be the only expectable error.
+               kr_assert(ret == kr_error(ENOSPC));
+               return kr_error(ret);
+       }
        memcpy(val.data, &tags, sizeof(tags));
        val.data += sizeof(tags);
        memcpy(val.data, &ztype, sizeof(ztype));
@@ -729,8 +737,7 @@ int insert_trivial_zone(val_zla_type_t ztype, uint32_t ttl,
                memcpy(val.data, &ttl, sizeof(ttl));
                val.data += sizeof(ttl);
        }
-
-       return ruledb_op(commit);
+       return kr_ok();
 }
 
 int kr_rule_local_data_emptyzone(const knot_dname_t *apex, kr_rule_tags_t tags)
@@ -840,8 +847,7 @@ int kr_view_insert_action(const char *subnet, const char *action)
                .data = (void *)/*const-cast*/action,
                .len = strlen(action),
        };
-       int ret = ruledb_op(write, &key, &val, 1);
-       return ret < 0 ? ret : ruledb_op(commit);
+       return ruledb_op(write, &key, &val, 1);
 }
 
 int kr_view_select_action(const struct kr_request *req, knot_db_val_t *selected)

diff --git a/lib/rules/api.h b/lib/rules/api.h
index 8af5e92463b1465ab94041cbf8ad23506e95bec4..90ea4a9e0fb0ce936ec41a49e500e026fccf21ba 100644 (file)
--- a/lib/rules/api.h
+++ b/lib/rules/api.h
@@ -21,6 +21,10 @@ int kr_rules_init(void);
 KR_EXPORT
 void kr_rules_deinit(void);
 
+/** Commit or abort changes done to the rule DB so far. */
+KR_EXPORT
+int kr_rules_commit(bool accept);
+
 /** Try answering the query from local data; WIP: otherwise determine data source overrides.
  *
  * \return kr_error() on errors, >0 if answered, 0 otherwise (also when forwarding)
@@ -47,7 +51,6 @@ int kr_view_select_action(const struct kr_request *req, knot_db_val_t *selected)
 /* APIs to modify the rule DB.
  *
  * FIXME:
- *  - what about transactions in this API?
  *  - a way to read/modify a rule?
  */
 

diff --git a/lib/rules/forward.c b/lib/rules/forward.c
index 289d13aa7a6e52ba3a12e6bc25a9c85ee9a73937..1f4248d01ba6c42d5400be1ec0b00afca5f0528b 100644 (file)
--- a/lib/rules/forward.c
+++ b/lib/rules/forward.c
@@ -163,6 +163,5 @@ int kr_rule_forward(const knot_dname_t *apex, kr_rule_fwd_flags_t flags,
                memcpy(val.data, &a, sizeof(a));
                val.data += sizeof(a);
        }
-
-       return ruledb_op(commit);
+       return kr_ok();
 }

diff --git a/lib/selection.c b/lib/selection.c
index dc3a601c65045ae08e2a6ef77e74702337f2c6f8..ea3a85ae3f69bf1efdf012050bb7d9d57322f467 100644 (file)
--- a/lib/selection.c
+++ b/lib/selection.c
@@ -173,7 +173,7 @@ int put_rtt_state(const uint8_t *ip, size_t len, struct rtt_state state,
                                .data = &state };
 
        int ret = cache->api->write(db, stats, &key, &value, 1);
-       cache->api->commit(db, stats);
+       cache->api->commit(db, stats, true);
 
        free(key.data);
        return ret;