lua cache bindings: error out if cache isn't open yet

The catch is that during configuration file processing,
no cache is open (yet), as kresd can't know if the config
does open it in some later part (with non-default path or size).
Now we just throw an error.  Exceptions:
 - cache.open() and cache.backends(), of course :-)
 - cache.ns_tout() - not required, it's not really inside cache
 - cache.close() - it sounds reasonable to allow "closing a closed cache"

This immediately caught a typo in cache metatable.

diff --git a/NEWS b/NEWS
index 751bd72ae1eca6b64014e5ef209e0290985ac433..2bc662684dfe249dc4e06f348e4bfa5806519b86 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,9 @@
+Incompatible changes
+--------------------
+- cache: fail lua cache operations if it isn't open (!639)
+  Note that by default cache is open *after* reading the configuration,
+  and older versions were silently doing nothing.
+
 Knot Resolver 2.4.1 (2018-08-02)
 ================================
 

diff --git a/daemon/bindings.c b/daemon/bindings.c
index 18035e2b11fd8ed0af7e417df5368b7755265884..ee5ba62a2a08fb079704036b81876ee5c97e4099 100644 (file)
--- a/daemon/bindings.c
+++ b/daemon/bindings.c
@@ -848,6 +848,21 @@ int lib_net(lua_State *L)
        return 1;
 }
 
+
+
+/** @internal return cache, or throw lua error if not open */
+struct kr_cache * cache_assert_open(lua_State *L)
+{
+       struct engine *engine = engine_luaget(L);
+       struct kr_cache *cache = &engine->resolver.cache;
+       assert(cache);
+       if (!cache || !kr_cache_is_open(cache)) {
+               format_error(L, "no cache is open yet, use cache.open() or cache.size, etc.");
+               lua_error(L);
+       }
+       return cache;
+}
+
 /** Return available cached backends. */
 static int cache_backends(lua_State *L)
 {
@@ -865,12 +880,10 @@ static int cache_backends(lua_State *L)
 /** Return number of cached records. */
 static int cache_count(lua_State *L)
 {
-       struct engine *engine = engine_luaget(L);
-       const struct kr_cdb_api *api = engine->resolver.cache.api;
+       struct kr_cache *cache = cache_assert_open(L);
 
-       struct kr_cache *cache = &engine->resolver.cache;
-       int count = api->count(cache->db);
-       if (kr_cache_is_open(cache) && count >= 0) {
+       int count = cache->api->count(cache->db);
+       if (count >= 0) {
                /* First key is a version counter, omit it if nonempty. */
                lua_pushinteger(L, count ? count - 1 : 0);
                return 1;
@@ -881,8 +894,7 @@ static int cache_count(lua_State *L)
 /** Return time of last checkpoint, or re-set it if passed `true`. */
 static int cache_checkpoint(lua_State *L)
 {
-       struct engine *engine = engine_luaget(L);
-       struct kr_cache *cache = &engine->resolver.cache;
+       struct kr_cache *cache = cache_assert_open(L);
 
        if (lua_gettop(L) == 0) { /* Return the current value. */
                lua_newtable(L);
@@ -908,8 +920,7 @@ static int cache_checkpoint(lua_State *L)
 /** Return cache statistics. */
 static int cache_stats(lua_State *L)
 {
-       struct engine *engine = engine_luaget(L);
-       struct kr_cache *cache = &engine->resolver.cache;
+       struct kr_cache *cache = cache_assert_open(L);
        lua_newtable(L);
        lua_pushnumber(L, cache->stats.hit);
        lua_setfield(L, -2, "hit");
@@ -943,8 +954,7 @@ static const struct kr_cdb_api *cache_select(struct engine *engine, const char *
 
 static int cache_max_ttl(lua_State *L)
 {
-       struct engine *engine = engine_luaget(L);
-       struct kr_cache *cache = &engine->resolver.cache;
+       struct kr_cache *cache = cache_assert_open(L);
 
        int n = lua_gettop(L);
        if (n > 0) {
@@ -967,8 +977,7 @@ static int cache_max_ttl(lua_State *L)
 
 static int cache_min_ttl(lua_State *L)
 {
-       struct engine *engine = engine_luaget(L);
-       struct kr_cache *cache = &engine->resolver.cache;
+       struct kr_cache *cache = cache_assert_open(L);
 
        int n = lua_gettop(L);
        if (n > 0) {
@@ -1125,12 +1134,7 @@ static int cache_remove_prefix(struct kr_cache *cache, const char *args)
 /** Prune expired/invalid records. */
 static int cache_prune(lua_State *L)
 {
-       struct engine *engine = engine_luaget(L);
-       struct kr_cache *cache = &engine->resolver.cache;
-       if (!kr_cache_is_open(cache)) {
-               return 0;
-       }
-
+       struct kr_cache *cache = cache_assert_open(L);
        /* Check parameters */
        int prune_max = UINT16_MAX;
        int n = lua_gettop(L);
@@ -1155,11 +1159,7 @@ static int cache_prune(lua_State *L)
 /** Clear all records. */
 static int cache_clear(lua_State *L)
 {
-       struct engine *engine = engine_luaget(L);
-       struct kr_cache *cache = &engine->resolver.cache;
-       if (!kr_cache_is_open(cache)) {
-               return 0;
-       }
+       struct kr_cache *cache = cache_assert_open(L);
 
        /* Check parameters */
        const char *args = NULL;
@@ -1187,6 +1187,7 @@ static int cache_clear(lua_State *L)
        }
 
        /* Clear reputation tables */
+       struct engine *engine = engine_luaget(L);
        lru_reset(engine->resolver.cache_rtt);
        lru_reset(engine->resolver.cache_rep);
        lru_reset(engine->resolver.cache_cookie);
@@ -1235,11 +1236,7 @@ static void cache_dump_key(lua_State *L, knot_db_val_t *key)
 /** Query cached records. */
 static int cache_get(lua_State *L)
 {
-       struct engine *engine = engine_luaget(L);
-       struct kr_cache *cache = &engine->resolver.cache;
-       if (!kr_cache_is_open(cache)) {
-               return 0;
-       }
+       //struct kr_cache *cache = cache_assert_open(L); // to be fixed soon
 
        /* Check parameters */
        int n = lua_gettop(L);
@@ -1325,16 +1322,7 @@ static int cache_zone_import(lua_State *L)
                goto finish;
        }
 
-       struct engine *engine = engine_luaget(L);
-       if (!engine) {
-               strncpy(msg, "internal error, empty engine pointer", sizeof(msg));
-               goto finish;
-       }
-       struct kr_cache *cache = &engine->resolver.cache;
-       if (!kr_cache_is_open(cache)) {
-               strncpy(msg, "cache isn't open", sizeof(msg));
-               goto finish;
-       }
+       (void)cache_assert_open(L); /* just check it in advance */
 
        /* Check parameters */
        int n = lua_gettop(L);

diff --git a/daemon/engine.c b/daemon/engine.c
index 0e73747b4fad2361225fb992bc0acd9a75a8def6..2a2afa313c3b1d107b268953a0778abe8d1f33fc 100644 (file)
--- a/daemon/engine.c
+++ b/daemon/engine.c
@@ -1020,6 +1020,7 @@ struct engine *engine_luaget(lua_State *L)
 {
        lua_getglobal(L, "__engine");
        struct engine *engine = lua_touserdata(L, -1);
+       if (!engine) luaL_error(L, "internal error, empty engine pointer");
        lua_pop(L, 1);
        return engine;
 }

diff --git a/daemon/lua/sandbox.lua b/daemon/lua/sandbox.lua
index 4d8d308161d623e2ed546b3b45a403ea90cae754..4891846375f73726d50f7231224f38642cafb1b7 100644 (file)
--- a/daemon/lua/sandbox.lua
+++ b/daemon/lua/sandbox.lua
@@ -163,7 +163,7 @@ setmetatable(modules, {
 setmetatable(cache, {
        __index = function (t, k)
                local res = rawget(t, k)
-               if res and not rawget(t, 'current_size') then return res end
+               if not res and not rawget(t, 'current_size') then return res end
                -- Beware: t.get returns empty table on failure to find.
                -- That would be confusing here (breaking kresc), so return nil instead.
                res = t.get(k)

diff --git a/tests/config/cache.test.lua b/tests/config/cache.test.lua
index 39548a87d3e1a513c715c4910d52088addd70765..646eed7d80f55b1bdca22cf49ad75ca2e960c859 100644 (file)
--- a/tests/config/cache.test.lua
+++ b/tests/config/cache.test.lua
@@ -13,10 +13,15 @@ local function test_properties()
        is(cache.min_ttl(), 1, 'stored minimum TTL')
 end
 
--- test if the stats work with reopening the cache
+-- test if the stats work with reopening the cache and operations fail with closed cache
 local function test_stats()
        ok(cache.close(), 'cache can be closed')
        boom(cache.open, {100 * MB, 'invalid://'}, 'cache cannot be opened with invalid backend')
+
+       boom(cache.clear, {}, '.clear() does not work on closed cache')
+       boom(cache.count, {}, '.count() does not work on closed cache')
+       boom(cache.get, { 'key' }, '.get(...) does not work on closed cache')
+
        ok(cache.open(100 * MB), 'cache can be reopened')
        local s = cache.stats()
        is(type(s), 'table', 'stats returns a table')