ntdb: make database read-only during ntdb_parse() callback.
authorRusty Russell <rusty@rustcorp.com.au>
Fri, 22 Jun 2012 00:14:41 +0000 (09:44 +0930)
committerRusty Russell <rusty@rustcorp.com.au>
Fri, 22 Jun 2012 05:35:17 +0000 (07:35 +0200)
Since we have a readlock, any write will grab a write lock: if it happens
to be on the same bucket, we'll fail.

For that reason, enforce read-only so every write operation fails
(even for NTDB_NOLOCK or NTDB_INTERNAL dbs), and document it!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
lib/ntdb/doc/TDB_porting.txt
lib/ntdb/lock.c
lib/ntdb/ntdb.c
lib/ntdb/ntdb.h
lib/ntdb/test/api-95-read-only-during-parse.c [new file with mode: 0644]
lib/ntdb/wscript

index 8b0ca2fec81fdf0df456b17b3f6a87cfd92f4751..5daf94b74b9234caa6faa0586047338b6837ce1e 100644 (file)
@@ -170,23 +170,6 @@ Interface differences between TDB and NTDB.
                                   O_CREAT|O_RDWR, 0600, &hashsize);
        }
 
-- ntdb does locking on read-only databases (ie. O_RDONLY passed to ntdb_open).
-  tdb did not: use the NTDB_NOLOCK flag if you want to suppress locking.
-
-  Example:
-       #include <tdb.h>
-       #include <ntdb.h>
-
-       struct tdb_context *tdb_example(void)
-       {
-               return tdb_open("example.tdb", 0, TDB_DEFAULT, O_RDONLY, 0);
-       }
-
-       struct ntdb_context *ntdb_example(void)
-       {
-               return ntdb_open("example.ntdb", NTDB_NOLOCK, O_RDONLY, NULL);
-       }
-
 - ntdb's log function is simpler than tdb's log function.  The string
   is already formatted, is not terminated by a '\n', and it takes an
   enum ntdb_log_level not a tdb_debug_level, and which has only three
@@ -304,6 +287,80 @@ Interface differences between TDB and NTDB.
                }
        }
 
+- ntdb's ntdb_parse_record() takes a type-checked callback data
+  pointer, not a void * (though a void * pointer still works).  The
+  callback function is allowed to do read operations on the database,
+  or write operations if you first call ntdb_lockall().  TDB's
+  tdb_parse_record() did not allow any database access within the
+  callback, could crash if you tried.
+
+  Example:
+       #include <tdb.h>
+       #include <ntdb.h>
+
+       static int tdb_parser(TDB_DATA key, TDB_DATA data, void *private_data)
+       {
+               TDB_DATA *expect = private_data;
+
+               return data.dsize == expect->dsize
+                       && !memcmp(data.dptr, expect->dptr, data.dsize);
+       }
+
+       void tdb_example(struct tdb_context *tdb, TDB_DATA key, NTDB_DATA d)
+       {
+               switch (tdb_parse_record(tdb, key, tdb_parser, &d)) {
+               case -1:
+                       printf("parse failed: %s\n", tdb_errorstr(tdb));
+                       break;
+               case 0:
+                       printf("data was different!\n");
+                       break;
+               case 1:
+                       printf("data was same!\n");
+                       break;
+               }
+       }
+
+       static int ntdb_parser(TDB_DATA key, TDB_DATA data, TDB_DATA *expect)
+       {
+               return ntdb_deq(data, *expect);
+       }
+
+       void ntdb_example(struct ntdb_context *ntdb, NTDB_DATA key, NTDB_DATA d)
+       {
+               enum NTDB_ERROR e;
+
+               e = tdb_parse_record(tdb, key, tdb_parser, &d);
+               switch (e) {
+               case 0:
+                       printf("data was different!\n");
+                       break;
+               case 1:
+                       printf("data was same!\n");
+                       break;
+               default:
+                       printf("parse failed: %s\n", ntdb_errorstr(e));
+                       break;
+               }
+       }
+
+- ntdb does locking on read-only databases (ie. O_RDONLY passed to ntdb_open).
+  tdb did not: use the NTDB_NOLOCK flag if you want to suppress locking.
+
+  Example:
+       #include <tdb.h>
+       #include <ntdb.h>
+
+       struct tdb_context *tdb_example(void)
+       {
+               return tdb_open("example.tdb", 0, TDB_DEFAULT, O_RDONLY, 0);
+       }
+
+       struct ntdb_context *ntdb_example(void)
+       {
+               return ntdb_open("example.ntdb", NTDB_NOLOCK, O_RDONLY, NULL);
+       }
+
 - Failure inside a transaction (such as a lock function failing) does
   not implicitly cancel the transaction; you still need to call
   ntdb_transaction_cancel().
index f6a811a3fa1fe2d7cbf3cab0f0850a1dd9ec1f6a..4517e2556857865c605dad7c50ef3b27c06385b6 100644 (file)
@@ -188,15 +188,15 @@ static enum NTDB_ERROR ntdb_brlock(struct ntdb_context *ntdb,
 {
        int ret;
 
-       if (ntdb->flags & NTDB_NOLOCK) {
-               return NTDB_SUCCESS;
-       }
-
        if (rw_type == F_WRLCK && (ntdb->flags & NTDB_RDONLY)) {
                return ntdb_logerr(ntdb, NTDB_ERR_RDONLY, NTDB_LOG_USE_ERROR,
                                  "Write lock attempted on read-only database");
        }
 
+       if (ntdb->flags & NTDB_NOLOCK) {
+               return NTDB_SUCCESS;
+       }
+
        /* A 32 bit system cannot open a 64-bit file, but it could have
         * expanded since then: check here. */
        if ((size_t)(offset + len) != offset + len) {
@@ -533,8 +533,9 @@ enum NTDB_ERROR ntdb_allrecord_lock(struct ntdb_context *ntdb, int ltype,
        enum NTDB_ERROR ecode;
        ntdb_bool_err berr;
 
-       if (ntdb->flags & NTDB_NOLOCK)
+       if (ntdb->flags & NTDB_NOLOCK) {
                return NTDB_SUCCESS;
+       }
 
        if (!check_lock_pid(ntdb, "ntdb_allrecord_lock", true)) {
                return NTDB_ERR_LOCK;
index 6c75915899bfeb881e5837567f5ae0f8bf914b04..5d56b33b5a123a5da087741b5e1cc03e5de6a511 100644 (file)
@@ -500,10 +500,23 @@ _PUBLIC_ enum NTDB_ERROR ntdb_parse_record_(struct ntdb_context *ntdb,
        if (!off) {
                ecode = NTDB_ERR_NOEXIST;
        } else {
+               unsigned int old_flags;
                NTDB_DATA d = ntdb_mkdata(keyp + key.dsize,
                                          rec_data_length(&rec));
 
+               /*
+                * Make sure they don't try to write db, since they
+                * have read lock!  They can if they've done
+                * ntdb_lockall(): if it was ntdb_lockall_read, that'll
+                * stop them doing a write operation anyway.
+                */
+               old_flags = ntdb->flags;
+               if (!ntdb->file->allrecord_lock.count &&
+                   !(ntdb->flags & NTDB_NOLOCK)) {
+                       ntdb->flags |= NTDB_RDONLY;
+               }
                ecode = parse(key, d, data);
+               ntdb->flags = old_flags;
                ntdb_access_release(ntdb, keyp);
        }
 
index 8e8458e4d17d811ae9d89eed19ed540a875a6a71..df3a9ddc4eaee3a6e6f05d81247f6af8bdddbd70 100644 (file)
@@ -368,9 +368,14 @@ int64_t ntdb_traverse_(struct ntdb_context *ntdb,
  *
  * This avoids a copy for many cases, by handing you a pointer into
  * the memory-mapped database.  It also locks the record to prevent
- * other accesses at the same time.
+ * other accesses at the same time, so it won't change.
  *
- * Do not alter the data handed to parse()!
+ * Within the @parse callback you can perform read operations on the
+ * database, but no write operations: no ntdb_store() or
+ * ntdb_delete(), for example.  The exception is if you call
+ * ntdb_lockall() before ntdb_parse_record().
+ *
+ * Never alter the data handed to parse()!
  */
 #define ntdb_parse_record(ntdb, key, parse, data)                      \
        ntdb_parse_record_((ntdb), (key),                               \
diff --git a/lib/ntdb/test/api-95-read-only-during-parse.c b/lib/ntdb/test/api-95-read-only-during-parse.c
new file mode 100644 (file)
index 0000000..8252b81
--- /dev/null
@@ -0,0 +1,94 @@
+/* Make sure write operations fail during ntdb_parse(). */
+#include "config.h"
+#include "ntdb.h"
+#include "tap-interface.h"
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include "logging.h"
+
+static struct ntdb_context *ntdb;
+
+/* We could get either of these. */
+static bool xfail(enum NTDB_ERROR ecode)
+{
+       return ecode == NTDB_ERR_RDONLY || ecode == NTDB_ERR_LOCK;
+}
+
+static enum NTDB_ERROR parse(NTDB_DATA key, NTDB_DATA data,
+                            NTDB_DATA *expected)
+{
+       NTDB_DATA add = ntdb_mkdata("another", strlen("another"));
+
+       if (!ntdb_deq(data, *expected)) {
+               return NTDB_ERR_EINVAL;
+       }
+
+       /* These should all fail.*/
+       if (!xfail(ntdb_store(ntdb, add, add, NTDB_INSERT))) {
+               return NTDB_ERR_EINVAL;
+       }
+       tap_log_messages--;
+
+       if (!xfail(ntdb_append(ntdb, key, add))) {
+               return NTDB_ERR_EINVAL;
+       }
+       tap_log_messages--;
+
+       if (!xfail(ntdb_delete(ntdb, key))) {
+               return NTDB_ERR_EINVAL;
+       }
+       tap_log_messages--;
+
+       if (!xfail(ntdb_transaction_start(ntdb))) {
+               return NTDB_ERR_EINVAL;
+       }
+       tap_log_messages--;
+
+       if (!xfail(ntdb_chainlock(ntdb, key))) {
+               return NTDB_ERR_EINVAL;
+       }
+       tap_log_messages--;
+
+       if (!xfail(ntdb_lockall(ntdb))) {
+               return NTDB_ERR_EINVAL;
+       }
+       tap_log_messages--;
+
+       if (!xfail(ntdb_wipe_all(ntdb))) {
+               return NTDB_ERR_EINVAL;
+       }
+       tap_log_messages--;
+
+       if (!xfail(ntdb_repack(ntdb))) {
+               return NTDB_ERR_EINVAL;
+       }
+       tap_log_messages--;
+
+       /* Access the record one more time. */
+       if (!ntdb_deq(data, *expected)) {
+               return NTDB_ERR_EINVAL;
+       }
+
+       return NTDB_SUCCESS;
+}
+
+int main(int argc, char *argv[])
+{
+       unsigned int i;
+       int flags[] = { NTDB_DEFAULT, NTDB_NOMMAP, NTDB_CONVERT };
+       NTDB_DATA key = ntdb_mkdata("hello", 5), data = ntdb_mkdata("world", 5);
+
+       plan_tests(sizeof(flags) / sizeof(flags[0]) * 2 + 1);
+       for (i = 0; i < sizeof(flags) / sizeof(flags[0]); i++) {
+               ntdb = ntdb_open("api-95-read-only-during-parse.ntdb",
+                                flags[i]|MAYBE_NOSYNC,
+                                O_RDWR|O_CREAT|O_TRUNC, 0600, &tap_log_attr);
+               ok1(ntdb_store(ntdb, key, data, NTDB_INSERT) == NTDB_SUCCESS);
+               ok1(ntdb_parse_record(ntdb, key, parse, &data) == NTDB_SUCCESS);
+               ntdb_close(ntdb);
+       }
+
+       ok1(tap_log_messages == 0);
+       return exit_status();
+}
index f034631058da8b6bdde8790b380083176e549361..e211a9fc0c7c827a388c949428b9d9269c5122f7 100644 (file)
@@ -79,6 +79,7 @@ def configure(conf):
                                 'test/api-91-get-stats.c',
                                 'test/api-92-get-set-readonly.c',
                                 'test/api-93-repack.c',
+                                'test/api-95-read-only-during-parse.c',
                                 'test/api-add-remove-flags.c',
                                 'test/api-check-callback.c',
                                 'test/api-firstkey-nextkey.c',