[PATCH] Domain backup and restore samba-tool commands

Tim Beale timbeale at catalyst.net.nz
Wed Jun 27 22:20:49 UTC 2018


Hi Douglas,

I've:
- Fixed whitespace in #3
- Added return value checks in #2
- Also initialized pointers to NULL in #2
- Also tweaked the following logic in #2 (it didn't look quite right to me)
+    if (ret != LDB_SUCCESS || res == NULL || res->count == 0) {
+        talloc_free(db_context);
+        return ret;
+    }

The last CI run was good:
https://gitlab.com/catalyst-samba/samba/pipelines/24648347

Kicked off another one, just to be sure:
https://gitlab.com/catalyst-samba/samba/pipelines/24717130

Cheers,
Tim

On 27/06/18 17:59, Douglas Bagnall wrote:
> Hi Tim,
>
> On 27/06/18 16:52, Tim Beale via samba-technical wrote:
>> Hi,
>>
>> Attached are the first few 'pre-work' patches for the backup tool (the
>> backup tool is dependent on these changes, but this patch-set doesn't
>> include the backup tool itself). These are ready for review and delivery
>> (pending a clean CI run).
>> https://gitlab.com/catalyst-samba/samba/pipelines/24648347
>>
>> They're basically just a sub-set of the patches I sent out this morning,
>> with the following changes:
>> - new patch: tests: Add basic test for non-global LoadParm behaviour
>> - new patch: tests: Add test that Samba cannot be started with a  backup DB
>> - updated patch: samba: read backup date field on init and fail if
>> present (the new test case highlighted that the code didn't actually work).
>>
>> Review appreciated.
>>
>> Thanks,
>> Tim
> #1 looks good!
>
> #2:
>
>> +	ldb_ctx = samdb_connect(db_context,
>> +				event_ctx,
>> +				cmdline_lp_ctx,
>> +				system_session(cmdline_lp_ctx),
>> +				NULL,
>> +				0);
> There should be a NULL check here...
>
>> +	privilege_connect(db_context, cmdline_lp_ctx);
> And I suppose this one too. Obviously you're not dereferencing it,
> but presumably it does something useful and it would be better to hear
> about the error sooner rather than later.
>
> Otherwise it looks good.
>
> I will look at the rest in the morning, though I did notice `git am`
> had this complaint:
>
>   Applying: param: Add non-global smb.cfg option (support 2 different smb.confs)
>   .git/rebase-apply/patch:79: trailing whitespace.
>   		
>   warning: 1 line adds whitespace errors.
>   Applying: tests: Add basic test for non-global LoadParm behaviour
>
> cheers,
> Douglas

-------------- next part --------------
From c2df84db944c020490aac00a0f04eead80d8d53a Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 27 Jun 2018 14:06:54 +1200
Subject: [PATCH 1/6] tests: Add test that Samba cannot be started with a
 backup DB

We don't want users to take a backup file, and then simply untar it and
run Samba (Several modifications to the DB need to be made as part of
the restore process, so users should always run the 'backup restore'
command).

To enforce this, prime_ldb_databases() now refuses to start Samba if the
backupDate marker is present in the DB. This patch adds a test-case that
proves this basic behaviour works.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 selftest/knownfail.d/start_backup            |  1 +
 source4/selftest/tests.py                    |  5 ++
 source4/setup/tests/blackbox_start_backup.sh | 79 ++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+)
 create mode 100644 selftest/knownfail.d/start_backup
 create mode 100755 source4/setup/tests/blackbox_start_backup.sh

diff --git a/selftest/knownfail.d/start_backup b/selftest/knownfail.d/start_backup
new file mode 100644
index 0000000..223a743
--- /dev/null
+++ b/selftest/knownfail.d/start_backup
@@ -0,0 +1 @@
+samba4.blackbox.start_backup.start-samba-backup\(none\)
diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
index 8b1fb7b..46451e5 100755
--- a/source4/selftest/tests.py
+++ b/source4/selftest/tests.py
@@ -861,6 +861,11 @@ plantestsuite("samba4.blackbox.supported_features", "none",
                os.path.join(samba4srcdir,
                             "setup/tests/blackbox_supported_features.sh"),
                '$PREFIX/provision'])
+plantestsuite("samba4.blackbox.start_backup", "none",
+              ["PYTHON=%s" % python,
+               os.path.join(samba4srcdir,
+                            "setup/tests/blackbox_start_backup.sh"),
+               '$PREFIX/provision'])
 plantestsuite("samba4.blackbox.upgradeprovision.current", "none", ["PYTHON=%s" % python, os.path.join(samba4srcdir, "setup/tests/blackbox_upgradeprovision.sh"), '$PREFIX/provision'])
 plantestsuite("samba4.blackbox.setpassword.py", "none", ["PYTHON=%s" % python, os.path.join(samba4srcdir, "setup/tests/blackbox_setpassword.sh"), '$PREFIX/provision'])
 plantestsuite("samba4.blackbox.newuser.py", "none", ["PYTHON=%s" % python, os.path.join(samba4srcdir, "setup/tests/blackbox_newuser.sh"), '$PREFIX/provision'])
diff --git a/source4/setup/tests/blackbox_start_backup.sh b/source4/setup/tests/blackbox_start_backup.sh
new file mode 100755
index 0000000..f1cfd53
--- /dev/null
+++ b/source4/setup/tests/blackbox_start_backup.sh
@@ -0,0 +1,79 @@
+#!/bin/sh
+
+# Simple test that a DB from a backup file cannot be untarred and started
+# manually (you have to run the samba-tool 'backup restore' command instead).
+
+if [ $# -lt 1 ]; then
+cat <<EOF
+Usage: $0 PREFIX
+EOF
+exit 1;
+fi
+
+PREFIX="$1"
+shift 1
+
+DBPATH=$PREFIX/start-backup
+mkdir -p $DBPATH
+
+. `dirname $0`/../../../testprogs/blackbox/subunit.sh
+
+do_provision()
+{
+    $PYTHON $BINDIR/samba-tool domain provision \
+           --domain=FOO --realm=foo.example.com --use-ntvfs \
+           --targetdir=$DBPATH --option="pid directory = $DBPATH"
+}
+
+add_backup_marker()
+{
+# manually add the backup marker that the backup cmd usually adds
+    $BINDIR/ldbmodify \
+       -H tdb://$DBPATH/private/sam.ldb <<EOF
+dn: @SAMBA_DSDB
+changetype: modify
+add: backupDate
+backupDate: who-knows-when
+-
+
+EOF
+}
+
+start_backup()
+{
+    # start samba in interactive mode (if we don't, samba daemonizes and so the
+    # command's exit status is always zero (success), regardless of whether
+    # samba actually starts up or not). However, this means if this assertion
+    # were ever to fail (i.e. samba DOES startup from a backup file), then the
+    # test case would just hang. So we use a max-run-time of 5 secs so that
+    # samba will self-destruct in the bad case (max_runtime_handler() returns
+    # zero/success in this case, which allows us to tell the good case from the
+    # bad case).
+    OPTS="--maximum-runtime=5 -i"
+
+    # redirect logs to stderr (which we'll then redirect to stdout so we can
+    # capture it in a bash variable)
+    OPTS="$OPTS --debug-stderr"
+
+    # start samba and capture the debug output
+    OUTPUT=$($BINDIR/samba -s $DBPATH/etc/smb.conf $OPTS 2>&1)
+    if [ $? -eq 0 ] ; then
+        echo "ERROR: Samba should not have started successfully"
+        return 1
+    fi
+
+    # check the reason we're failing is because prime_ldb_databases() is
+    # detecting that this is a backup DB (and not some other reason)
+    echo "$OUTPUT" | grep "failed to start: Database is a backup"
+}
+
+# setup a DB and manually mark it as being a "backup"
+testit "provision" do_provision
+testit "add-backup-marker" add_backup_marker
+
+# check that Samba won't start using this DB (because it's a backup)
+testit "start-samba-backup" start_backup
+
+rm -rf $DBPATH
+
+exit $failed
-- 
2.7.4


From 4e39c056372fb0fcdaf9d5ce429b6ebb232a5028 Mon Sep 17 00:00:00 2001
From: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Date: Tue, 1 May 2018 15:48:38 +1200
Subject: [PATCH 2/6] samba: read backup date field on init and fail if present

This prevents a backup tar file, created with the new official
backup tools, from being extracted and replicated.

This is done here to ensure that samba-tool and ldbsearch can
still operate on the backup (eg for forensics) but starting
Samba as an AD DC will fail.

Signed-off-by: Aaron Haslett <aaronhaslett at catalyst.net.nz>
---
 selftest/knownfail.d/start_backup |  1 -
 source4/smbd/server.c             | 83 ++++++++++++++++++++++++++++++++-------
 2 files changed, 68 insertions(+), 16 deletions(-)
 delete mode 100644 selftest/knownfail.d/start_backup

diff --git a/selftest/knownfail.d/start_backup b/selftest/knownfail.d/start_backup
deleted file mode 100644
index 223a743..0000000
--- a/selftest/knownfail.d/start_backup
+++ /dev/null
@@ -1 +0,0 @@
-samba4.blackbox.start_backup.start-samba-backup\(none\)
diff --git a/source4/smbd/server.c b/source4/smbd/server.c
index ed81c01..5eca0b9 100644
--- a/source4/smbd/server.c
+++ b/source4/smbd/server.c
@@ -44,6 +44,7 @@
 #include "nsswitch/winbind_client.h"
 #include "libds/common/roles.h"
 #include "lib/util/tfork.h"
+#include "dsdb/samdb/ldb_modules/util.h"
 
 #ifdef HAVE_PTHREAD
 #include <pthread.h>
@@ -232,23 +233,63 @@ _NORETURN_ static void max_runtime_handler(struct tevent_context *ev,
   pre-open the key databases. This saves a lot of time in child
   processes
  */
-static void prime_ldb_databases(struct tevent_context *event_ctx)
+static int prime_ldb_databases(struct tevent_context *event_ctx, bool *am_backup)
 {
-	TALLOC_CTX *db_context;
-	db_context = talloc_new(event_ctx);
-
-	samdb_connect(db_context,
-		      event_ctx,
-		      cmdline_lp_ctx,
-		      system_session(cmdline_lp_ctx),
-		      NULL,
-		      0);
-	privilege_connect(db_context, cmdline_lp_ctx);
-
-	/* we deliberately leave these open, which allows them to be
+	struct ldb_result *res = NULL;
+	struct ldb_dn *samba_dsdb_dn = NULL;
+	struct ldb_context *ldb_ctx = NULL;
+	struct ldb_context *pdb = NULL;
+	static const char *attrs[] = { "backupDate", NULL };
+	const char *msg = NULL;
+	int ret;
+	TALLOC_CTX *db_context = talloc_new(event_ctx);
+	if (db_context == NULL) {
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+
+	*am_backup = false;
+
+	/* note we deliberately leave these open, which allows them to be
 	 * re-used in ldb_wrap_connect() */
-}
+	ldb_ctx = samdb_connect(db_context,
+				event_ctx,
+				cmdline_lp_ctx,
+				system_session(cmdline_lp_ctx),
+				NULL,
+				0);
+	if (ldb_ctx == NULL) {
+		talloc_free(db_context);
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+	pdb = privilege_connect(db_context, cmdline_lp_ctx);
+	if (pdb == NULL) {
+		talloc_free(db_context);
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
 
+	/* check the root DB object to see if it's marked as a backup */
+	samba_dsdb_dn = ldb_dn_new(db_context, ldb_ctx, "@SAMBA_DSDB");
+	if (!samba_dsdb_dn) {
+		talloc_free(db_context);
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+
+	ret = dsdb_search_dn(ldb_ctx, db_context, &res, samba_dsdb_dn, attrs,
+			     DSDB_FLAG_AS_SYSTEM);
+	if (ret != LDB_SUCCESS) {
+		talloc_free(db_context);
+		return ret;
+	}
+
+	if (res->count > 0) {
+		msg = ldb_msg_find_attr_as_string(res->msgs[0], "backupDate",
+						  NULL);
+		if (msg != NULL) {
+			*am_backup = true;
+		}
+	}
+	return LDB_SUCCESS;
+}
 
 /*
   called when a fatal condition occurs in a child task
@@ -366,7 +407,9 @@ static int binary_smbd_main(const char *binary_name,
 	bool opt_fork = true;
 	bool opt_interactive = false;
 	bool opt_no_process_group = false;
+	bool db_is_backup = false;
 	int opt;
+	int ret;
 	poptContext pc;
 #define _MODULE_PROTO(init) extern NTSTATUS init(TALLOC_CTX *);
 	STATIC_service_MODULES_PROTO;
@@ -631,7 +674,17 @@ static int binary_smbd_main(const char *binary_name,
 			"and exited. Check logs for details", EINVAL);
 	};
 
-	prime_ldb_databases(state->event_ctx);
+	ret = prime_ldb_databases(state->event_ctx, &db_is_backup);
+	if (ret != LDB_SUCCESS) {
+		TALLOC_FREE(state);
+		exit_daemon("Samba failed to prime database", EINVAL);
+	}
+
+	if (db_is_backup) {
+		TALLOC_FREE(state);
+		exit_daemon("Database is a backup. Please run samba-tool domain"
+			    " backup restore", EINVAL);
+	}
 
 	status = setup_parent_messaging(state, cmdline_lp_ctx);
 	if (!NT_STATUS_IS_OK(status)) {
-- 
2.7.4


From b6dbebe9697a43a9260dcea2d491b31c78b82a7e Mon Sep 17 00:00:00 2001
From: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Date: Tue, 1 May 2018 11:10:36 +1200
Subject: [PATCH 3/6] param: Add non-global smb.cfg option (support 2 different
 smb.confs)

The default behaviour is that there is only a single global underlying
LoadParm object. E.g. if you create 2 different LoadParm objects in
python, they both modify the same underlying object.

This patch adds a mechanism to override this and create a separate
non-global LoadParm object. The use-case is the backup tool, where we
want to manipulate 2 different smb.conf files (the one used to create
the backup, and the smb.conf in the backup itself).

Signed-off-by: Aaron Haslett <aaronhaslett at catalyst.net.nz>
---
 lib/param/loadparm.c    | 15 +++++++++++++--
 source4/param/pyparam.c | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c
index 3b7f805..9684a52 100644
--- a/lib/param/loadparm.c
+++ b/lib/param/loadparm.c
@@ -3148,7 +3148,8 @@ bool lpcfg_load_default(struct loadparm_context *lp_ctx)
  *
  * Return True on success, False on failure.
  */
-bool lpcfg_load(struct loadparm_context *lp_ctx, const char *filename)
+static bool lpcfg_load_internal(struct loadparm_context *lp_ctx,
+				const char *filename, bool set_global)
 {
 	char *n2;
 	bool bRetval;
@@ -3183,7 +3184,7 @@ bool lpcfg_load(struct loadparm_context *lp_ctx, const char *filename)
 	   for a missing smb.conf */
 	reload_charcnv(lp_ctx);
 
-	if (bRetval == true) {
+	if (bRetval == true && set_global) {
 		/* set this up so that any child python tasks will
 		   find the right smb.conf */
 		setenv("SMB_CONF_PATH", filename, 1);
@@ -3197,6 +3198,16 @@ bool lpcfg_load(struct loadparm_context *lp_ctx, const char *filename)
 	return bRetval;
 }
 
+bool lpcfg_load_no_global(struct loadparm_context *lp_ctx, const char *filename)
+{
+    return lpcfg_load_internal(lp_ctx, filename, false);
+}
+
+bool lpcfg_load(struct loadparm_context *lp_ctx, const char *filename)
+{
+    return lpcfg_load_internal(lp_ctx, filename, true);
+}
+
 /**
  * Return the max number of services.
  */
diff --git a/source4/param/pyparam.c b/source4/param/pyparam.c
index e7e908f..4596e52 100644
--- a/source4/param/pyparam.c
+++ b/source4/param/pyparam.c
@@ -445,7 +445,42 @@ static PyGetSetDef py_lp_ctx_getset[] = {
 
 static PyObject *py_lp_ctx_new(PyTypeObject *type, PyObject *args, PyObject *kwargs)
 {
-	return pytalloc_reference(type, loadparm_init_global(false));
+	const char *kwnames[] = {"filename_for_non_global_lp", NULL};
+	PyObject *lp_ctx;
+	const char *non_global_conf = NULL;
+	struct loadparm_context *ctx;
+
+	if (!PyArg_ParseTupleAndKeywords(args,
+					 kwargs,
+					 "|s",
+					 discard_const_p(char *,
+							 kwnames),
+					 &non_global_conf)) {
+		return NULL;
+	}
+
+	/*
+	 * by default, any LoadParm python objects map to a single global
+	 * underlying object. The filename_for_non_global_lp arg overrides this
+	 * default behaviour and creates a separate underlying LoadParm object.
+	 */
+	if (non_global_conf != NULL) {
+		ctx = loadparm_init(NULL);
+		if (ctx == NULL) {
+			return NULL;
+		}
+
+		lp_ctx = pytalloc_reference(type, ctx);
+		if (lp_ctx == NULL) {
+			return NULL;
+		}
+
+		lpcfg_load_no_global(PyLoadparmContext_AsLoadparmContext(lp_ctx),
+				     non_global_conf);
+		return lp_ctx;
+	} else{
+		return pytalloc_reference(type, loadparm_init_global(false));
+	}
 }
 
 static Py_ssize_t py_lp_ctx_len(PyObject *self)
-- 
2.7.4


From 260cea5a29e8901e2d663b7e8878a231245316a8 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 27 Jun 2018 10:39:23 +1200
Subject: [PATCH 4/6] tests: Add basic test for non-global LoadParm behaviour

Add a simple test to show that the new non-global LoadParm behaviour
works, i.e.
- by default all LoadParm objects are linked to the same underlying
  object
- using a non-global LoadParm creates a separate underlying object.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 python/samba/tests/loadparm.py | 68 ++++++++++++++++++++++++++++++++++++++++++
 source4/selftest/tests.py      |  2 ++
 2 files changed, 70 insertions(+)
 create mode 100644 python/samba/tests/loadparm.py

diff --git a/python/samba/tests/loadparm.py b/python/samba/tests/loadparm.py
new file mode 100644
index 0000000..8054103
--- /dev/null
+++ b/python/samba/tests/loadparm.py
@@ -0,0 +1,68 @@
+# Unix SMB/CIFS implementation.
+# Copyright (C) Andrew Bartlett <abartlet at samba.org>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+from samba.tests import TestCaseInTempDir
+from samba import param
+import os
+
+# the python bindings for LoadParm objects map (by default) to a single global
+# object in the underlying C code. E.g. if we create 2 different LoadParm
+# objects in python, really they're just the same object underneath.
+class LoadParmTest(TestCaseInTempDir):
+
+    def test_global_loadparm(self):
+        # create 2 different Loadparm objects (which are really the same
+        # object underneath)
+        lp1 = param.LoadParm()
+        lp2 = param.LoadParm()
+
+        # we can prove this by setting a value on lp1 and assert that the
+        # change is also reflected on lp2
+        lp1_realm = "JUST.A.TEST"
+        self.assertNotEqual(lp2.get('realm'), lp1_realm)
+        lp1.set('realm', lp1_realm)
+        self.assertEqual(lp1.get('realm'), lp1_realm)
+        self.assertEqual(lp2.get('realm'), lp1_realm)
+
+    def touch_temp_file(self, filename):
+        filepath = os.path.join(self.tempdir, filename)
+        open(filepath, 'a').close()
+        # delete the file once the test completes
+        self.addCleanup(os.remove, filepath)
+        return filepath
+
+    def test_non_global_loadparm(self):
+        # create a empty smb.conf file
+        smb_conf = self.touch_temp_file("smb.conf")
+
+        # we can create a non-global Loadparm that overrides the default
+        # behaviour and creates a separate underlying object
+        lp1 = param.LoadParm()
+        lp2 = param.LoadParm(filename_for_non_global_lp = smb_conf)
+
+        # setting a value for the global LP does not affect the non-global LP
+        lp1_realm = "JUST.A.TEST"
+        self.assertNotEqual(lp2.get('realm'), lp1_realm)
+        lp1.set('realm', lp1_realm)
+        self.assertEqual(lp1.get('realm'), lp1_realm)
+        self.assertNotEqual(lp2.get('realm'), lp1_realm)
+
+        # and vice versa
+        lp2_realm = "TEST.REALM.LP2"
+        lp2.set('realm', lp2_realm)
+        self.assertEqual(lp2.get('realm'), lp2_realm)
+        self.assertEqual(lp1.get('realm'), lp1_realm)
diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
index 46451e5..1b592c3 100755
--- a/source4/selftest/tests.py
+++ b/source4/selftest/tests.py
@@ -771,6 +771,8 @@ planoldpythontestsuite("ad_dc_ntvfs",
 planoldpythontestsuite("ad_dc_ntvfs",
                        "samba.tests.blackbox.traffic_summary",
                        extra_args=['-U"$USERNAME%$PASSWORD"'])
+planoldpythontestsuite("none", "samba.tests.loadparm")
+
 #
 # Want a selection of environments across the process models
 #
-- 
2.7.4


From 2554365ba8b84bf0632efcba3d06100e61552ba8 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 29 May 2018 15:22:07 +1200
Subject: [PATCH 5/6] selftest: Update MAX_WRAPPED_INTERFACES comment to match
 code

Commit 19606e4dc657b0baf3ea84d updated the MAX_WRAPPED_INTERFACES define
in the C code from 40 to 64.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 selftest/target/Samba.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/selftest/target/Samba.pm b/selftest/target/Samba.pm
index b0482d3..81d3d21 100644
--- a/selftest/target/Samba.pm
+++ b/selftest/target/Samba.pm
@@ -409,8 +409,8 @@ sub get_interface($)
     $interfaces{"vampire2000dc"} = 39;
 
     # update lib/socket_wrapper/socket_wrapper.c
-    #  #define MAX_WRAPPED_INTERFACES 40
-    # if you wish to have more than 40 interfaces
+    #  #define MAX_WRAPPED_INTERFACES 64
+    # if you wish to have more than 64 interfaces
 
     if (not defined($interfaces{$netbiosname})) {
 	die();
-- 
2.7.4


From dc8eeaeea38c69ea555883b7dca20775b8b13db4 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 25 Jun 2018 14:00:59 +1200
Subject: [PATCH 6/6] provision: set 'binddns dir' when making new smb.conf

When creating a new smb.conf from scratch during a join/clone/etc, the
'binddns dir' setting still uses the source smb.conf/default setting,
instead of the targetdir sub-directory.

I noticed this problem when trying to create a new testenv - the
provision() was trying to create /usr/local/samba/bind-dns directory,
which would fail if samba hadn't already been installed on the host
machine.

Now that this is fixed, we also need to fix tests that were explicitly
asserting that no unexpected directories were left behind after the test
completes.

This change also breaks the upgradeprovision script. The upgrade-
provision calls newprovision() to create a reference provision in a
temporary directory. However, previously this temporary provision was
creating the bind-dns directory in the actual upgrade directory as a
side-effect, e.g. it did a provision() with
targetdir=alpha13_upgrade_full/private/referenceprovisionLBKBh2 and this
ended up creating alpha13_upgrade_full/bind-dns as a side-effect.
The provision() now creates bind-dns in the specified targetdir, but
this means check_for_DNS() fails (it tries to create bind-dns sub-
directories, but the upgrade's bind-dns doesn't exist). I've avoided
this problem by making sure bind-dns exists as part of the
check_for_DNS() processing.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 python/samba/provision/__init__.py           | 2 ++
 python/samba/tests/join.py                   | 1 +
 python/samba/tests/samdb.py                  | 2 +-
 source4/scripting/bin/samba_upgradeprovision | 3 +++
 source4/torture/drs/python/samba_tool_drs.py | 1 +
 5 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/python/samba/provision/__init__.py b/python/samba/provision/__init__.py
index 36f50f2..e571894 100644
--- a/python/samba/provision/__init__.py
+++ b/python/samba/provision/__init__.py
@@ -744,10 +744,12 @@ def make_smbconf(smbconf, hostname, domain, realm, targetdir,
         global_settings["lock dir"] = os.path.abspath(targetdir)
         global_settings["state directory"] = os.path.abspath(os.path.join(targetdir, "state"))
         global_settings["cache directory"] = os.path.abspath(os.path.join(targetdir, "cache"))
+        global_settings["binddns dir"] = os.path.abspath(os.path.join(targetdir, "bind-dns"))
 
         lp.set("lock dir", os.path.abspath(targetdir))
         lp.set("state directory",  global_settings["state directory"])
         lp.set("cache directory", global_settings["cache directory"])
+        lp.set("binddns dir", global_settings["binddns dir"])
 
     if eadb:
         if use_ntvfs and not lp.get("posix:eadb"):
diff --git a/python/samba/tests/join.py b/python/samba/tests/join.py
index 1f9fab1..17de3ab 100644
--- a/python/samba/tests/join.py
+++ b/python/samba/tests/join.py
@@ -73,6 +73,7 @@ class JoinTestCase(DNSTKeyTest):
             shutil.rmtree(os.path.join(self.tempdir, "etc"))
             shutil.rmtree(os.path.join(self.tempdir, "msg.lock"))
             os.unlink(os.path.join(self.tempdir, "names.tdb"))
+            shutil.rmtree(os.path.join(self.tempdir, "bind-dns"))
 
         self.join_ctx.cleanup_old_join(force=True)
 
diff --git a/python/samba/tests/samdb.py b/python/samba/tests/samdb.py
index d4279b4..8c477b2 100644
--- a/python/samba/tests/samdb.py
+++ b/python/samba/tests/samdb.py
@@ -59,7 +59,7 @@ class SamDBTestCase(TestCaseInTempDir):
         for f in ['names.tdb']:
             os.remove(os.path.join(self.tempdir, f))
 
-        for d in ['etc', 'msg.lock', 'private', 'state']:
+        for d in ['etc', 'msg.lock', 'private', 'state', 'bind-dns']:
             shutil.rmtree(os.path.join(self.tempdir, d))
 
         super(SamDBTestCase, self).tearDown()
diff --git a/source4/scripting/bin/samba_upgradeprovision b/source4/scripting/bin/samba_upgradeprovision
index 9d3e736..5d040d2 100755
--- a/source4/scripting/bin/samba_upgradeprovision
+++ b/source4/scripting/bin/samba_upgradeprovision
@@ -226,6 +226,9 @@ def check_for_DNS(refprivate, private, refbinddns_dir, binddns_dir, dns_backend)
     if not os.path.exists(dnsfile):
         shutil.copy("%s/dns_update_list" % refprivate, "%s" % dnsfile)
 
+    if not os.path.exists(binddns_dir):
+        os.mkdir(binddns_dir)
+
     if dns_backend not in ['BIND9_DLZ', 'BIND9_FLATFILE']:
        return
 
diff --git a/source4/torture/drs/python/samba_tool_drs.py b/source4/torture/drs/python/samba_tool_drs.py
index 502c809..29c6016 100644
--- a/source4/torture/drs/python/samba_tool_drs.py
+++ b/source4/torture/drs/python/samba_tool_drs.py
@@ -47,6 +47,7 @@ class SambaToolDrsTests(drs_base.DrsBaseTestCase):
             shutil.rmtree(os.path.join(self.tempdir, "msg.lock"))
             os.remove(os.path.join(self.tempdir, "names.tdb"))
             shutil.rmtree(os.path.join(self.tempdir, "state"))
+            shutil.rmtree(os.path.join(self.tempdir, "bind-dns"))
         except Exception:
             pass
 
-- 
2.7.4



More information about the samba-technical mailing list