Make all transactions nested in ldb. The current samba4 code expects this

behavior anyway, and given we can only have one transaction active per
ldb context this is the only sane model we can support.

Fix ldb_tdb transactions, we could return back with an error with neither
committing nor canceling the actual tdb transaction in some error paths
within the ltdb commit and cancel transaction paths.

Added also some debugging to trace what was going on.

2 files changed, 75 insertions, 59 deletions

diff --git a/source4/lib/ldb/common/ldb.c b/source4/lib/ldb/common/ldb.c
index 2fb5a8f9be..f1b28b6819 100644
--- a/source4/lib/ldb/common/ldb.c
+++ b/source4/lib/ldb/common/ldb.c
@@ -34,6 +34,19 @@
 
 #include "ldb_private.h"
 
+static int ldb_context_destructor(void *ptr)
+{
+	struct ldb_context *ldb = talloc_get_type(ptr, struct ldb_context);
+
+	if (ldb->transaction_active) {
+		ldb_debug(ldb, LDB_DEBUG_FATAL,
+			  "A transaction is still active in ldb context [%p]",
+			  ldb);
+	}
+
+	return 0;
+}
+
 /*
    initialise a ldb context
    The mem_ctx is required
@@ -65,6 +78,8 @@ struct ldb_context *ldb_init(TALLOC_CTX *mem_ctx, struct tevent_context *ev_ctx)
 	/* TODO: get timeout from options if available there */
 	ldb->default_timeout = 300; /* set default to 5 minutes */
 
+	talloc_set_destructor((TALLOC_CTX *)ldb, ldb_context_destructor);
+
 	return ldb;
 }
 
@@ -242,10 +257,24 @@ void ldb_reset_err_string(struct ldb_context *ldb)
 /*
   start a transaction
 */
-static int ldb_transaction_start_internal(struct ldb_context *ldb)
+int ldb_transaction_start(struct ldb_context *ldb)
 {
 	struct ldb_module *module;
 	int status;
+
+	ldb_debug(ldb, LDB_DEBUG_TRACE,
+		  "start ldb transaction (nesting: %d)",
+		  ldb->transaction_active);
+
+	/* explicit transaction active, count nested requests */
+	if (ldb->transaction_active) {
+		ldb->transaction_active++;
+		return LDB_SUCCESS;
+	}
+
+	/* start a new transaction */
+	ldb->transaction_active++;
+
 	FIRST_OP(ldb, start_transaction);
 
 	ldb_reset_err_string(ldb);
@@ -266,10 +295,29 @@ static int ldb_transaction_start_internal(struct ldb_context *ldb)
 /*
   commit a transaction
 */
-static int ldb_transaction_commit_internal(struct ldb_context *ldb)
+int ldb_transaction_commit(struct ldb_context *ldb)
 {
 	struct ldb_module *module;
 	int status;
+
+	ldb->transaction_active--;
+
+	ldb_debug(ldb, LDB_DEBUG_TRACE,
+		  "commit ldb transaction (nesting: %d)",
+		  ldb->transaction_active);
+
+	/* commit only when all nested transactions are complete */
+	if (ldb->transaction_active > 0) {
+		return LDB_SUCCESS;
+	}
+
+	if (ldb->transaction_active < 0) {
+		ldb_debug(ldb, LDB_DEBUG_FATAL,
+			  "commit called but no ldb transactions are active!");
+		ldb->transaction_active = 0;
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+
 	FIRST_OP(ldb, end_transaction);
 
 	ldb_reset_err_string(ldb);
@@ -290,10 +338,29 @@ static int ldb_transaction_commit_internal(struct ldb_context *ldb)
 /*
   cancel a transaction
 */
-static int ldb_transaction_cancel_internal(struct ldb_context *ldb)
+int ldb_transaction_cancel(struct ldb_context *ldb)
 {
 	struct ldb_module *module;
 	int status;
+
+	ldb->transaction_active--;
+
+	ldb_debug(ldb, LDB_DEBUG_TRACE,
+		  "cancel ldb transaction (nesting: %d)",
+		  ldb->transaction_active);
+
+	/* really cancel only if all nested transactions are complete */
+	if (ldb->transaction_active > 0) {
+		return LDB_SUCCESS;
+	}
+
+	if (ldb->transaction_active < 0) {
+		ldb_debug(ldb, LDB_DEBUG_FATAL,
+			  "commit called but no ldb transactions are active!");
+		ldb->transaction_active = 0;
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+
 	FIRST_OP(ldb, del_transaction);
 
 	status = module->ops->del_transaction(module);
@@ -309,66 +376,13 @@ static int ldb_transaction_cancel_internal(struct ldb_context *ldb)
 	return status;
 }
 
-int ldb_transaction_start(struct ldb_context *ldb)
-{
-	/* disable autotransactions */
-	ldb->transaction_active++;
-
-	return ldb_transaction_start_internal(ldb);
-}
-
-int ldb_transaction_commit(struct ldb_context *ldb)
-{
-	/* renable autotransactions (when we reach 0) */
-	if (ldb->transaction_active > 0)
-		ldb->transaction_active--;
-
-	return ldb_transaction_commit_internal(ldb);
-}
-
-int ldb_transaction_cancel(struct ldb_context *ldb)
-{
-	/* renable autotransactions (when we reach 0) */
-	if (ldb->transaction_active > 0)
-		ldb->transaction_active--;
-
-	return ldb_transaction_cancel_internal(ldb);
-}
-
-static int ldb_autotransaction_start(struct ldb_context *ldb)
-{
-	/* explicit transaction active, ignore autotransaction request */
-	if (ldb->transaction_active)
-		return LDB_SUCCESS;
-
-	return ldb_transaction_start_internal(ldb);
-}
-
-static int ldb_autotransaction_commit(struct ldb_context *ldb)
-{
-	/* explicit transaction active, ignore autotransaction request */
-	if (ldb->transaction_active)
-		return LDB_SUCCESS;
-
-	return ldb_transaction_commit_internal(ldb);
-}
-
-static int ldb_autotransaction_cancel(struct ldb_context *ldb)
-{
-	/* explicit transaction active, ignore autotransaction request */
-	if (ldb->transaction_active)
-		return LDB_SUCCESS;
-
-	return ldb_transaction_cancel_internal(ldb);
-}
-
 /* autostarts a transacion if none active */
 static int ldb_autotransaction_request(struct ldb_context *ldb,
 				       struct ldb_request *req)
 {
 	int ret;
 
-	ret = ldb_autotransaction_start(ldb);
+	ret = ldb_transaction_start(ldb);
 	if (ret != LDB_SUCCESS) {
 		return ret;
 	}
@@ -379,9 +393,9 @@ static int ldb_autotransaction_request(struct ldb_context *ldb,
 	}
 
 	if (ret == LDB_SUCCESS) {
-		return ldb_autotransaction_commit(ldb);
+		return ldb_transaction_commit(ldb);
 	}
-	ldb_autotransaction_cancel(ldb);
+	ldb_transaction_cancel(ldb);
 
 	if (ldb->err_string == NULL) {
 		/* no error string was setup by the backend */
diff --git a/source4/lib/ldb/ldb_tdb/ldb_tdb.c b/source4/lib/ldb/ldb_tdb/ldb_tdb.c
index d6276c4b86..24ec06ea32 100644
--- a/source4/lib/ldb/ldb_tdb/ldb_tdb.c
+++ b/source4/lib/ldb/ldb_tdb/ldb_tdb.c
@@ -865,6 +865,7 @@ static int ltdb_end_trans(struct ldb_module *module)
 	ltdb->in_transaction--;
 
 	if (ltdb_index_transaction_commit(module) != 0) {
+		tdb_transaction_cancel(ltdb->tdb);
 		return ltdb_err_map(tdb_error(ltdb->tdb));
 	}
 
@@ -883,6 +884,7 @@ static int ltdb_del_trans(struct ldb_module *module)
 	ltdb->in_transaction--;
 
 	if (ltdb_index_transaction_cancel(module) != 0) {
+		tdb_transaction_cancel(ltdb->tdb);
 		return ltdb_err_map(tdb_error(ltdb->tdb));
 	}