summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRusty Russell <rusty@rustcorp.com.au>2013-02-17 20:56:34 +1030
committerRusty Russell <rusty@rustcorp.com.au>2013-02-20 05:31:19 +0100
commit3d82f786ecdd6747e90fe480a15de8c3fcea5f7b (patch)
tree4197bb0051b0effbae636d7dc559a433647ee428
parent85b6329b91f402da474547410e82438ea2554775 (diff)
downloadsamba-3d82f786ecdd6747e90fe480a15de8c3fcea5f7b.tar.gz
samba-3d82f786ecdd6747e90fe480a15de8c3fcea5f7b.tar.bz2
samba-3d82f786ecdd6747e90fe480a15de8c3fcea5f7b.zip
ntdb: fix database corruption when transaction doesn't change anything.
ntdb's transaction code has an optimization which tdb's doesnt: it only writes the parts of blocks whose contents have changed. This means we can actually have a transaction which turns out to need no recovery region. This breaks the recovery setup logic, which sets the current recovery size to 0 if there's no recovery area, and assumes that we'll always create a new recovery area since the recovery will always need > 0 bytes. In fact, if we really haven't changed anything, we can skip the transaction commit altogether: since this happens at least once with Samba, it's worth doing. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Reviewed-by: Andrew Bartlett <abartlet@samba.org>
-rw-r--r--lib/ntdb/test/api-60-noop-transaction.c58
-rw-r--r--lib/ntdb/transaction.c47
-rw-r--r--lib/ntdb/wscript1
3 files changed, 91 insertions, 15 deletions
diff --git a/lib/ntdb/test/api-60-noop-transaction.c b/lib/ntdb/test/api-60-noop-transaction.c
new file mode 100644
index 0000000000..a429e67907
--- /dev/null
+++ b/lib/ntdb/test/api-60-noop-transaction.c
@@ -0,0 +1,58 @@
+#include "private.h" // struct ntdb_context
+#include "ntdb.h"
+#include "tap-interface.h"
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include "logging.h"
+
+int main(int argc, char *argv[])
+{
+ unsigned int i;
+ struct ntdb_context *ntdb;
+ int flags[] = { NTDB_DEFAULT, NTDB_NOMMAP,
+ NTDB_CONVERT, NTDB_NOMMAP|NTDB_CONVERT };
+ NTDB_DATA key = ntdb_mkdata("key", 3);
+ NTDB_DATA data = ntdb_mkdata("data", 4), d;
+
+ plan_tests(sizeof(flags) / sizeof(flags[0]) * 12 + 1);
+
+ for (i = 0; i < sizeof(flags) / sizeof(flags[0]); i++) {
+ ntdb = ntdb_open("api-60-transaction.ntdb",
+ flags[i]|MAYBE_NOSYNC,
+ O_RDWR|O_CREAT|O_TRUNC, 0600, &tap_log_attr);
+ ok1(ntdb);
+ if (!ntdb)
+ continue;
+
+ ok1(ntdb_store(ntdb, key, data, NTDB_INSERT) == 0);
+
+ ok1(ntdb_transaction_start(ntdb) == 0);
+ /* Do an identical replace. */
+ ok1(ntdb_store(ntdb, key, data, NTDB_REPLACE) == 0);
+ ok1(ntdb_transaction_commit(ntdb) == 0);
+
+ ok1(ntdb_check(ntdb, NULL, NULL) == 0);
+ ok1(ntdb_fetch(ntdb, key, &d) == NTDB_SUCCESS);
+ ok1(ntdb_deq(data, d));
+ free(d.dptr);
+ ntdb_close(ntdb);
+
+ /* Reopen, fetch. */
+ ntdb = ntdb_open("api-60-transaction.ntdb",
+ flags[i]|MAYBE_NOSYNC,
+ O_RDWR, 0600, &tap_log_attr);
+ ok1(ntdb);
+ if (!ntdb)
+ continue;
+ ok1(ntdb_check(ntdb, NULL, NULL) == 0);
+ ok1(ntdb_fetch(ntdb, key, &d) == NTDB_SUCCESS);
+ ok1(ntdb_deq(data, d));
+ free(d.dptr);
+ ntdb_close(ntdb);
+ }
+
+ ok1(tap_log_messages == 0);
+ return exit_status();
+}
diff --git a/lib/ntdb/transaction.c b/lib/ntdb/transaction.c
index 9608be43e8..f276216632 100644
--- a/lib/ntdb/transaction.c
+++ b/lib/ntdb/transaction.c
@@ -453,10 +453,23 @@ static enum NTDB_ERROR transaction_sync(struct ntdb_context *ntdb,
return NTDB_SUCCESS;
}
+static void free_transaction_blocks(struct ntdb_context *ntdb)
+{
+ int i;
+
+ /* free all the transaction blocks */
+ for (i=0;i<ntdb->transaction->num_blocks;i++) {
+ if (ntdb->transaction->blocks[i] != NULL) {
+ ntdb->free_fn(ntdb->transaction->blocks[i],
+ ntdb->alloc_data);
+ }
+ }
+ SAFE_FREE(ntdb, ntdb->transaction->blocks);
+ ntdb->transaction->num_blocks = 0;
+}
static void _ntdb_transaction_cancel(struct ntdb_context *ntdb)
{
- int i;
enum NTDB_ERROR ecode;
if (ntdb->transaction == NULL) {
@@ -473,14 +486,7 @@ static void _ntdb_transaction_cancel(struct ntdb_context *ntdb)
ntdb->file->map_size = ntdb->transaction->old_map_size;
- /* free all the transaction blocks */
- for (i=0;i<ntdb->transaction->num_blocks;i++) {
- if (ntdb->transaction->blocks[i] != NULL) {
- ntdb->free_fn(ntdb->transaction->blocks[i],
- ntdb->alloc_data);
- }
- }
- SAFE_FREE(ntdb, ntdb->transaction->blocks);
+ free_transaction_blocks(ntdb);
if (ntdb->transaction->magic_offset) {
const struct ntdb_methods *methods = ntdb->transaction->io_methods;
@@ -875,6 +881,17 @@ static enum NTDB_ERROR transaction_setup_recovery(struct ntdb_context *ntdb)
if (NTDB_PTR_IS_ERR(recovery))
return NTDB_PTR_ERR(recovery);
+ /* If we didn't actually change anything we overwrote? */
+ if (recovery_size == 0) {
+ /* In theory, we could have just appended data. */
+ if (ntdb->transaction->num_blocks * NTDB_PGSIZE
+ < ntdb->transaction->old_map_size) {
+ free_transaction_blocks(ntdb);
+ }
+ ntdb->free_fn(recovery, ntdb->alloc_data);
+ return NTDB_SUCCESS;
+ }
+
ecode = ntdb_recovery_area(ntdb, methods, &recovery_off, recovery);
if (ecode) {
ntdb->free_fn(recovery, ntdb->alloc_data);
@@ -1064,12 +1081,6 @@ _PUBLIC_ enum NTDB_ERROR ntdb_transaction_commit(struct ntdb_context *ntdb)
return NTDB_SUCCESS;
}
- /* check for a null transaction */
- if (ntdb->transaction->blocks == NULL) {
- _ntdb_transaction_cancel(ntdb);
- return NTDB_SUCCESS;
- }
-
if (!ntdb->transaction->prepared) {
ecode = _ntdb_transaction_prepare_commit(ntdb);
if (ecode != NTDB_SUCCESS) {
@@ -1078,6 +1089,12 @@ _PUBLIC_ enum NTDB_ERROR ntdb_transaction_commit(struct ntdb_context *ntdb)
}
}
+ /* check for a null transaction (prepare_commit may do this!) */
+ if (ntdb->transaction->blocks == NULL) {
+ _ntdb_transaction_cancel(ntdb);
+ return NTDB_SUCCESS;
+ }
+
methods = ntdb->transaction->io_methods;
/* perform all the writes */
diff --git a/lib/ntdb/wscript b/lib/ntdb/wscript
index 1a4b02bb25..63c8402d66 100644
--- a/lib/ntdb/wscript
+++ b/lib/ntdb/wscript
@@ -72,6 +72,7 @@ def configure(conf):
'test/api-20-alloc-attr.c',
'test/api-21-parse_record.c',
'test/api-55-transaction.c',
+ 'test/api-60-noop-transaction.c',
'test/api-80-tdb_fd.c',
'test/api-81-seqnum.c',
'test/api-82-lockattr.c',