[RFC] Advice on SMB client python bindings?

Stefan Metzmacher metze at samba.org
Tue Dec 4 16:44:05 UTC 2018


Hi Tim,

> Thanks for the patches Metze. Unfortunately that doesn't help me with
> supporting SMBv2 at all. Attached is an example test-case
> (smbv2-problem.txt) that tries to run the .unlink() API against a DC
> with SMBv1 disabled.

Yes, sorry! I looked briefly at cli_unlink() and thought _send/_recv
would already do the correct thing and only others like cli_mkdir()
had the problem. But it seems I was just dreaming...

> Taking a step back, the problem I'm trying to solve is this:
>
> Finally, I don't really understand why using the async APIs in the
> Python bindings is so important. For example, smbclient uses the
> synchronous APIs. If someone could explain the rationale behind all
> this, it might help. Or to put it another way, why is using the async
> APIs more important than delivering software that fixes a real problem
> for real Samba users?

Yes, removing the dependency to SMB1 should be our first priority.

The async stuff is mainly for testing the async protocol behavior,
in order to simulate multi threaded applications.

Would you be able to work from the attached patchset?

Thanks!
metze
-------------- next part --------------
From 3b0ca635d380e5dbe80f15acd1c2f862edd71331 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Mon, 3 Dec 2018 15:44:43 +0100
Subject: [PATCH 01/18] python/tests: also initialize the s3 loadparm subsystem

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 python/samba/tests/__init__.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/python/samba/tests/__init__.py b/python/samba/tests/__init__.py
index 9b30d2efa1ed..6ae8fbd9c80e 100644
--- a/python/samba/tests/__init__.py
+++ b/python/samba/tests/__init__.py
@@ -24,6 +24,7 @@ import warnings
 import ldb
 import samba
 from samba import param
+from samba.samba3 import param as s3param
 from samba import credentials
 from samba.credentials import Credentials
 from samba import gensec
@@ -302,8 +303,10 @@ class TestCaseInTempDir(TestCase):
 
 def env_loadparm():
     lp = param.LoadParm()
+    lp3 = s3param.get_context()
     try:
         lp.load(os.environ["SMB_CONF_PATH"])
+        lp3.load(lp.configfile)
     except KeyError:
         raise KeyError("SMB_CONF_PATH not set")
     return lp
-- 
2.17.1


From ac1ccf45d7dd84a0cab1c349dedca9e3b5d4f3f8 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Mon, 3 Dec 2018 14:37:05 +0100
Subject: [PATCH 02/18] s3:pylibsmb: pass self to py_tevent_req_wait_exc()

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 source3/libsmb/pylibsmb.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index 311820c7d667..f36c6ea9da83 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -364,7 +364,7 @@ static int py_tevent_req_wait(struct tevent_context *ev,
 
 #endif
 
-static bool py_tevent_req_wait_exc(struct tevent_context *ev,
+static bool py_tevent_req_wait_exc(struct py_cli_state *self,
 				   struct tevent_req *req)
 {
 	int ret;
@@ -373,7 +373,7 @@ static bool py_tevent_req_wait_exc(struct tevent_context *ev,
 		PyErr_NoMemory();
 		return false;
 	}
-	ret = py_tevent_req_wait(ev, req);
+	ret = py_tevent_req_wait(self->ev, req);
 	if (ret != 0) {
 		TALLOC_FREE(req);
 		errno = ret;
@@ -453,7 +453,7 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 	req = cli_full_connection_creds_send(
 		NULL, self->ev, "myname", host, NULL, 0, share, "?????",
 		cli_creds, flags, SMB_SIGNING_DEFAULT);
-	if (!py_tevent_req_wait_exc(self->ev, req)) {
+	if (!py_tevent_req_wait_exc(self, req)) {
 		return -1;
 	}
 	status = cli_full_connection_creds_recv(req, &self->cli);
@@ -612,7 +612,7 @@ static PyObject *py_cli_create(struct py_cli_state *self, PyObject *args,
 				DesiredAccess, FileAttributes, ShareAccess,
 				CreateDisposition, CreateOptions,
 				SecurityFlags);
-	if (!py_tevent_req_wait_exc(self->ev, req)) {
+	if (!py_tevent_req_wait_exc(self, req)) {
 		return NULL;
 	}
 	status = cli_ntcreate_recv(req, &fnum, NULL);
@@ -636,7 +636,7 @@ static PyObject *py_cli_close(struct py_cli_state *self, PyObject *args)
 	}
 
 	req = cli_close_send(NULL, self->ev, self->cli, fnum);
-	if (!py_tevent_req_wait_exc(self->ev, req)) {
+	if (!py_tevent_req_wait_exc(self, req)) {
 		return NULL;
 	}
 	status = cli_close_recv(req);
@@ -672,7 +672,7 @@ static PyObject *py_cli_write(struct py_cli_state *self, PyObject *args,
 
 	req = cli_write_andx_send(NULL, self->ev, self->cli, fnum, mode,
 				  (uint8_t *)buf, offset, buflen);
-	if (!py_tevent_req_wait_exc(self->ev, req)) {
+	if (!py_tevent_req_wait_exc(self, req)) {
 		return NULL;
 	}
 	status = cli_write_andx_recv(req, &written);
@@ -708,7 +708,7 @@ static PyObject *py_cli_read(struct py_cli_state *self, PyObject *args,
 
 	req = cli_read_andx_send(NULL, self->ev, self->cli, fnum,
 				 offset, size);
-	if (!py_tevent_req_wait_exc(self->ev, req)) {
+	if (!py_tevent_req_wait_exc(self, req)) {
 		return NULL;
 	}
 	status = cli_read_andx_recv(req, &buflen, &buf);
@@ -740,7 +740,7 @@ static PyObject *py_cli_ftruncate(struct py_cli_state *self, PyObject *args,
 	}
 
 	req = cli_ftruncate_send(NULL, self->ev, self->cli, fnum, size);
-	if (!py_tevent_req_wait_exc(self->ev, req)) {
+	if (!py_tevent_req_wait_exc(self, req)) {
 		return NULL;
 	}
 	status = cli_ftruncate_recv(req);
@@ -771,7 +771,7 @@ static PyObject *py_cli_delete_on_close(struct py_cli_state *self,
 
 	req = cli_nt_delete_on_close_send(NULL, self->ev, self->cli, fnum,
 					  flag);
-	if (!py_tevent_req_wait_exc(self->ev, req)) {
+	if (!py_tevent_req_wait_exc(self, req)) {
 		return NULL;
 	}
 	status = cli_nt_delete_on_close_recv(req);
@@ -812,7 +812,7 @@ static PyObject *py_cli_list(struct py_cli_state *self,
 
 	req = cli_list_send(NULL, self->ev, self->cli, mask, attribute,
 			    info_level);
-	if (!py_tevent_req_wait_exc(self->ev, req)) {
+	if (!py_tevent_req_wait_exc(self, req)) {
 		return NULL;
 	}
 	status = cli_list_recv(req, NULL, &finfos, &num_finfos);
-- 
2.17.1


From 4f3c46861df608343df65684355d6ad8c6d8cf03 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Mon, 3 Dec 2018 15:02:06 +0100
Subject: [PATCH 03/18] s3:pylibsmb: only use poll_mt backend if
 multi_threaded=True is specified

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 python/samba/tests/libsmb_samba_internal.py |  3 +-
 source3/libsmb/pylibsmb.c                   | 52 ++++++++++++++++-----
 2 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/python/samba/tests/libsmb_samba_internal.py b/python/samba/tests/libsmb_samba_internal.py
index c88095c8bc2f..db99c0bfb2bc 100644
--- a/python/samba/tests/libsmb_samba_internal.py
+++ b/python/samba/tests/libsmb_samba_internal.py
@@ -59,7 +59,8 @@ class LibsmbTestCase(samba.tests.TestCase):
         creds.set_username(os.getenv("USERNAME"))
         creds.set_password(os.getenv("PASSWORD"))
 
-        c = libsmb_samba_internal.Conn(os.getenv("SERVER_IP"), "tmp", creds)
+        c = libsmb_samba_internal.Conn(os.getenv("SERVER_IP"), "tmp",
+                                       creds, multi_threaded=True)
 
         mythreads = []
 
diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index f36c6ea9da83..b07952c98d1b 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -81,6 +81,8 @@ struct py_cli_state {
 	PyObject_HEAD
 	struct cli_state *cli;
 	struct tevent_context *ev;
+	int (*req_wait_fn)(struct tevent_context *ev,
+			   struct tevent_req *req);
 	struct py_cli_thread *thread_state;
 
 	struct tevent_req *oplock_waiter;
@@ -194,7 +196,10 @@ static int py_cli_thread_destructor(struct py_cli_thread *t)
 	return 0;
 }
 
-static bool py_cli_state_setup_ev(struct py_cli_state *self)
+static int py_tevent_cond_req_wait(struct tevent_context *ev,
+				   struct tevent_req *req);
+
+static bool py_cli_state_setup_mt_ev(struct py_cli_state *self)
 {
 	struct py_cli_thread *t = NULL;
 	int ret;
@@ -206,6 +211,8 @@ static bool py_cli_state_setup_ev(struct py_cli_state *self)
 	samba_tevent_set_debug(self->ev, "pylibsmb_tevent_mt");
 	tevent_set_trace_callback(self->ev, py_cli_state_trace_callback, self);
 
+	self->req_wait_fn = py_tevent_cond_req_wait;
+
 	self->thread_state = talloc_zero(NULL, struct py_cli_thread);
 	if (self->thread_state == NULL) {
 		goto fail;
@@ -303,8 +310,8 @@ fail:
 	return result;
 }
 
-static int py_tevent_req_wait(struct tevent_context *ev,
-			      struct tevent_req *req)
+static int py_tevent_cond_req_wait(struct tevent_context *ev,
+				   struct tevent_req *req)
 {
 	struct py_tevent_cond cond;
 	tevent_req_set_callback(req, py_tevent_signalme, &cond);
@@ -334,7 +341,10 @@ static void py_tevent_signalme(struct tevent_req *req)
 	py_tevent_cond_signal(cond);
 }
 
-#else
+#endif
+
+static int py_tevent_req_wait(struct tevent_context *ev,
+			      struct tevent_req *req);
 
 static bool py_cli_state_setup_ev(struct py_cli_state *self)
 {
@@ -345,6 +355,8 @@ static bool py_cli_state_setup_ev(struct py_cli_state *self)
 
 	samba_tevent_set_debug(self->ev, "pylibsmb_tevent");
 
+	self->req_wait_fn = py_tevent_req_wait;
+
 	return true;
 }
 
@@ -362,8 +374,6 @@ static int py_tevent_req_wait(struct tevent_context *ev,
 	return 0;
 }
 
-#endif
-
 static bool py_tevent_req_wait_exc(struct py_cli_state *self,
 				   struct tevent_req *req)
 {
@@ -373,7 +383,7 @@ static bool py_tevent_req_wait_exc(struct py_cli_state *self,
 		PyErr_NoMemory();
 		return false;
 	}
-	ret = py_tevent_req_wait(self->ev, req);
+	ret = self->req_wait_fn(self->ev, req);
 	if (ret != 0) {
 		TALLOC_FREE(req);
 		errno = ret;
@@ -410,6 +420,8 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 	char *host, *share;
 	PyObject *creds = NULL;
 	struct cli_credentials *cli_creds;
+	PyObject *py_multi_threaded = Py_False;
+	bool multi_threaded = false;
 	struct tevent_req *req;
 	bool ret;
 	/*
@@ -421,7 +433,7 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 	int flags = CLI_FULL_CONNECTION_FORCE_SMB1;
 
 	static const char *kwlist[] = {
-		"host", "share", "credentials", NULL
+		"host", "share", "credentials", "multi_threaded", NULL
 	};
 
 	PyTypeObject *py_type_Credentials = get_pytype(
@@ -431,8 +443,10 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 	}
 
 	ret = ParseTupleAndKeywords(
-		args, kwds, "ss|O!", kwlist,
-		&host, &share, py_type_Credentials, &creds);
+		args, kwds, "ss|O!O", kwlist,
+		&host, &share,
+		py_type_Credentials, &creds,
+		&py_multi_threaded);
 
 	Py_DECREF(py_type_Credentials);
 
@@ -440,8 +454,24 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 		return -1;
 	}
 
-	if (!py_cli_state_setup_ev(self)) {
+	multi_threaded = PyObject_IsTrue(py_multi_threaded);
+
+	if (multi_threaded) {
+#ifdef HAVE_PTHREAD
+		ret = py_cli_state_setup_mt_ev(self);
+		if (!ret) {
+			return -1;
+		}
+#else
+		PyErr_SetString(PyExc_RuntimeError,
+				"No PTHREAD support available");
 		return -1;
+#endif
+	} else {
+		ret = py_cli_state_setup_ev(self);
+		if (!ret) {
+			return -1;
+		}
 	}
 
 	if (creds == NULL) {
-- 
2.17.1


From 6d4b66b850ffbba56ea426e6b5cb178cc3be85ea Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Mon, 3 Dec 2018 15:42:50 +0100
Subject: [PATCH 04/18] s3:pylibsmb: add sign=True to require signing

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 source3/libsmb/pylibsmb.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index b07952c98d1b..6c98c7066dad 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -422,6 +422,9 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 	struct cli_credentials *cli_creds;
 	PyObject *py_multi_threaded = Py_False;
 	bool multi_threaded = false;
+	PyObject *py_sign = Py_False;
+	bool sign = false;
+	int signing_state = SMB_SIGNING_DEFAULT;
 	struct tevent_req *req;
 	bool ret;
 	/*
@@ -433,7 +436,8 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 	int flags = CLI_FULL_CONNECTION_FORCE_SMB1;
 
 	static const char *kwlist[] = {
-		"host", "share", "credentials", "multi_threaded", NULL
+		"host", "share", "credentials",
+		"multi_threaded", "sign", NULL
 	};
 
 	PyTypeObject *py_type_Credentials = get_pytype(
@@ -443,10 +447,11 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 	}
 
 	ret = ParseTupleAndKeywords(
-		args, kwds, "ss|O!O", kwlist,
+		args, kwds, "ss|O!OO", kwlist,
 		&host, &share,
 		py_type_Credentials, &creds,
-		&py_multi_threaded);
+		&py_multi_threaded,
+		&py_sign);
 
 	Py_DECREF(py_type_Credentials);
 
@@ -455,6 +460,11 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 	}
 
 	multi_threaded = PyObject_IsTrue(py_multi_threaded);
+	sign = PyObject_IsTrue(py_sign);
+
+	if (sign) {
+		signing_state = SMB_SIGNING_REQUIRED;
+	}
 
 	if (multi_threaded) {
 #ifdef HAVE_PTHREAD
@@ -482,7 +492,7 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 
 	req = cli_full_connection_creds_send(
 		NULL, self->ev, "myname", host, NULL, 0, share, "?????",
-		cli_creds, flags, SMB_SIGNING_DEFAULT);
+		cli_creds, flags, signing_state);
 	if (!py_tevent_req_wait_exc(self, req)) {
 		return -1;
 	}
-- 
2.17.1


From 4d6809eeb5ef88bd8db4b8c4b110768bd76f5aae Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 4 Dec 2018 10:40:18 +0100
Subject: [PATCH 05/18] s3:pylibsmb: add force_smb1=True in order to control
 forcing of SMB1

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 python/samba/tests/libsmb_samba_internal.py |  3 +-
 source3/libsmb/pylibsmb.c                   | 35 +++++++++++++++------
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/python/samba/tests/libsmb_samba_internal.py b/python/samba/tests/libsmb_samba_internal.py
index db99c0bfb2bc..8918d848ea85 100644
--- a/python/samba/tests/libsmb_samba_internal.py
+++ b/python/samba/tests/libsmb_samba_internal.py
@@ -60,7 +60,8 @@ class LibsmbTestCase(samba.tests.TestCase):
         creds.set_password(os.getenv("PASSWORD"))
 
         c = libsmb_samba_internal.Conn(os.getenv("SERVER_IP"), "tmp",
-                                       creds, multi_threaded=True)
+                                       creds, multi_threaded=True,
+                                       force_smb1=True)
 
         mythreads = []
 
diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index 6c98c7066dad..cfca9ac6c516 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -425,19 +425,16 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 	PyObject *py_sign = Py_False;
 	bool sign = false;
 	int signing_state = SMB_SIGNING_DEFAULT;
+	PyObject *py_force_smb1 = Py_False;
+	bool force_smb1 = false;
 	struct tevent_req *req;
 	bool ret;
-	/*
-	 * For now we only support SMB1,
-	 * as most of the cli_*_send() function
-	 * don't support SMB2, it's only plugged
-	 * into the sync wrapper functions currently.
-	 */
-	int flags = CLI_FULL_CONNECTION_FORCE_SMB1;
+	int flags = 0;
 
 	static const char *kwlist[] = {
 		"host", "share", "credentials",
-		"multi_threaded", "sign", NULL
+		"multi_threaded", "sign", "force_smb1",
+		NULL
 	};
 
 	PyTypeObject *py_type_Credentials = get_pytype(
@@ -447,11 +444,12 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 	}
 
 	ret = ParseTupleAndKeywords(
-		args, kwds, "ss|O!OO", kwlist,
+		args, kwds, "ss|O!OOO", kwlist,
 		&host, &share,
 		py_type_Credentials, &creds,
 		&py_multi_threaded,
-		&py_sign);
+		&py_sign,
+		&py_force_smb1);
 
 	Py_DECREF(py_type_Credentials);
 
@@ -461,11 +459,22 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 
 	multi_threaded = PyObject_IsTrue(py_multi_threaded);
 	sign = PyObject_IsTrue(py_sign);
+	force_smb1 = PyObject_IsTrue(py_force_smb1);
 
 	if (sign) {
 		signing_state = SMB_SIGNING_REQUIRED;
 	}
 
+	if (force_smb1) {
+		/*
+		 * As most of the cli_*_send() function
+		 * don't support SMB2 (it's only plugged
+		 * into the sync wrapper functions currently)
+		 * we have a way to force SMB1.
+		 */
+		flags = CLI_FULL_CONNECTION_FORCE_SMB1;
+	}
+
 	if (multi_threaded) {
 #ifdef HAVE_PTHREAD
 		ret = py_cli_state_setup_mt_ev(self);
@@ -477,6 +486,12 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 				"No PTHREAD support available");
 		return -1;
 #endif
+		if (!force_smb1) {
+			PyErr_SetString(PyExc_RuntimeError,
+					"multi_threaded is only possible on "
+					"SMB1 connections");
+			return -1;
+		}
 	} else {
 		ret = py_cli_state_setup_ev(self);
 		if (!ret) {
-- 
2.17.1


From 04d474128fcc536546f4768fc63f87d583af7bab Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 4 Dec 2018 10:42:55 +0100
Subject: [PATCH 06/18] s3:pylibsmb: remember that a connection uses SMB1

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 source3/libsmb/pylibsmb.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index cfca9ac6c516..fa3315fc6e3a 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -21,6 +21,7 @@
 #include <Python.h>
 #include "includes.h"
 #include "python/py3compat.h"
+#include "libcli/smb/smbXcli_base.h"
 #include "libsmb/libsmb.h"
 #include "libcli/security/security.h"
 #include "system/select.h"
@@ -80,6 +81,7 @@ struct py_cli_oplock_break {
 struct py_cli_state {
 	PyObject_HEAD
 	struct cli_state *cli;
+	bool is_smb1;
 	struct tevent_context *ev;
 	int (*req_wait_fn)(struct tevent_context *ev,
 			   struct tevent_req *req);
@@ -403,6 +405,7 @@ static PyObject *py_cli_state_new(PyTypeObject *type, PyObject *args,
 		return NULL;
 	}
 	self->cli = NULL;
+	self->is_smb1 = false;
 	self->ev = NULL;
 	self->thread_state = NULL;
 	self->oplock_waiter = NULL;
@@ -519,6 +522,10 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 		return -1;
 	}
 
+	if (smbXcli_conn_protocol(self->cli->conn) < PROTOCOL_SMB2_02) {
+		self->is_smb1 = true;
+	}
+
 	self->oplock_waiter = cli_smb_oplock_break_waiter_send(
 		self->ev, self->ev, self->cli);
 	if (self->oplock_waiter == NULL) {
-- 
2.17.1


From 795c3df34f64493df56817c6fedf2066883a3af8 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 4 Dec 2018 12:32:58 +1300
Subject: [PATCH 07/18] s3:pylibsmb: .get_oplock_break API is dependent on
 multi_threaded=True

The .get_oplock_break is dependent on the pthread code, which is only
used when creating a SMB connection with multi_threaded=True.

Add an explicit error to the .get_oplock_break() if someone tries to use
it in non-multithreaded mode.

Initializing self->oplock_waiter in non-multithreaded mode is similarly
redundant if the API can never be used.

Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 source3/libsmb/pylibsmb.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index fa3315fc6e3a..547d449c9604 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -526,6 +526,13 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
 		self->is_smb1 = true;
 	}
 
+	/*
+	 * Oplocks require a multi threaded connection
+	 */
+	if (self->thread_state == NULL) {
+		return 0;
+	}
+
 	self->oplock_waiter = cli_smb_oplock_break_waiter_send(
 		self->ev, self->ev, self->cli);
 	if (self->oplock_waiter == NULL) {
@@ -585,6 +592,13 @@ static PyObject *py_cli_get_oplock_break(struct py_cli_state *self,
 		return NULL;
 	}
 
+	if (self->thread_state == NULL) {
+		PyErr_SetString(PyExc_RuntimeError,
+				"get_oplock_break() only possible on "
+				"a multi_threaded connection");
+		return NULL;
+	}
+
 	if (self->oplock_cond != NULL) {
 		errno = EBUSY;
 		PyErr_SetFromErrno(PyExc_RuntimeError);
-- 
2.17.1


From 2a633bd796cbfaf7038fa47ff3ffdac73a968ddb Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 3 Dec 2018 11:01:14 +1300
Subject: [PATCH 08/18] tests: Add SMB chkpath test case

chkpath was only tested incidentally (and that assertion was wrong). Add
a proper test to prove it works correctly. We can then clean-up the
incorrect assertion in the next patch.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 python/samba/tests/smb.py | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/python/samba/tests/smb.py b/python/samba/tests/smb.py
index e0e60e371027..de2ecfc18fe8 100644
--- a/python/samba/tests/smb.py
+++ b/python/samba/tests/smb.py
@@ -65,6 +65,26 @@ class SMBTests(samba.tests.TestCase):
         self.conn.unlink(test_file)
         self.assertFalse(self.conn.chkpath(test_file))
 
+    def test_chkpath(self):
+        """Tests .chkpath determines whether or not a directory exists"""
+
+        self.assertTrue(self.conn.chkpath(test_dir))
+
+        # should return False for a non-existent directory
+        bad_dir = os.path.join(test_dir, 'dont_exist').replace('/', '\\')
+        self.assertFalse(self.conn.chkpath(bad_dir))
+
+        # should return False for files (because they're not directories)
+        self.conn.savefile(test_file, binary_contents)
+        self.assertFalse(self.conn.chkpath(test_file))
+
+        # check correct result after creating and then deleting a new dir
+        new_dir = os.path.join(test_dir, 'test-new').replace('/', '\\')
+        self.conn.mkdir(new_dir)
+        self.assertTrue(self.conn.chkpath(new_dir))
+        self.conn.rmdir(new_dir)
+        self.assertFalse(self.conn.chkpath(new_dir))
+
     def test_save_load_text(self):
 
         self.conn.savefile(test_file, test_contents.encode('utf8'))
-- 
2.17.1


From b15da988c393477e0dfafe6a3bd5b5a82213930d Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 3 Dec 2018 11:15:14 +1300
Subject: [PATCH 09/18] tests: Fix SMB unlink test case assertion

The current assertion would never detect if the unlink API is broken.
The chkpath() API is only useful for checking if directories exist, so
it will always return False for a regular file (regardless of whether
the file actually exists or not).

Rework the test case so we assert that the file exists by trying to read
its contents (which will throw an error if the file doesn't exist).

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 python/samba/tests/smb.py | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/python/samba/tests/smb.py b/python/samba/tests/smb.py
index de2ecfc18fe8..6ee5eaabb880 100644
--- a/python/samba/tests/smb.py
+++ b/python/samba/tests/smb.py
@@ -20,6 +20,8 @@ import os
 import random
 import sys
 from samba import smb
+from samba import NTSTATUSError
+from samba.ntstatus import NT_STATUS_OBJECT_NAME_NOT_FOUND
 
 PY3 = sys.version_info[0] == 3
 addom = 'addom.samba.example.com/'
@@ -57,13 +59,30 @@ class SMBTests(samba.tests.TestCase):
         self.assertIn('Policies', ls,
                       msg='"Policies" directory not found in sysvol')
 
+    def file_exists(self, filepath):
+        """Returns whether a regular file exists (by trying to open it)"""
+        try:
+            self.conn.loadfile(filepath)
+            exists = True;
+        except NTSTATUSError as err:
+            if err.args[0] == NT_STATUS_OBJECT_NAME_NOT_FOUND:
+                exists = False
+            else:
+                raise err
+        return exists
+
     def test_unlink(self):
         """
         The smb.unlink API should delete file
         """
+        # create the test file
+        self.assertFalse(self.file_exists(test_file))
         self.conn.savefile(test_file, binary_contents)
+        self.assertTrue(self.file_exists(test_file))
+
+        # delete it and check that it's gone
         self.conn.unlink(test_file)
-        self.assertFalse(self.conn.chkpath(test_file))
+        self.assertFalse(self.file_exists(test_file))
 
     def test_chkpath(self):
         """Tests .chkpath determines whether or not a directory exists"""
-- 
2.17.1


From 77bb498b625ed3093046df6d6ae3fff381c7b16b Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 3 Dec 2018 10:50:19 +1300
Subject: [PATCH 10/18] s3:pylibsmb: Add .unlink API

Add a basic .unlink() API to the source3 bindings. This is based on the
source4 python bindings, but uses the source3 client library APIs.

Update the source4 test to use the source3 API. We will gradually
convert it over, and then delete the source4 python bindings.

Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 python/samba/tests/smb.py |  7 ++++++-
 source3/libsmb/pylibsmb.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/python/samba/tests/smb.py b/python/samba/tests/smb.py
index 6ee5eaabb880..91b487dfad13 100644
--- a/python/samba/tests/smb.py
+++ b/python/samba/tests/smb.py
@@ -22,6 +22,7 @@ import sys
 from samba import smb
 from samba import NTSTATUSError
 from samba.ntstatus import NT_STATUS_OBJECT_NAME_NOT_FOUND
+from samba.samba3 import libsmb_samba_internal
 
 PY3 = sys.version_info[0] == 3
 addom = 'addom.samba.example.com/'
@@ -43,6 +44,10 @@ class SMBTests(samba.tests.TestCase):
                             "sysvol",
                             lp=self.get_loadparm(),
                             creds=creds)
+
+        # temporarily create a 2nd SMB connection for migrating the py-bindings
+        self.smb_conn = libsmb_samba_internal.Conn(self.server, "sysvol", creds)
+
         self.conn.mkdir(test_dir)
 
     def tearDown(self):
@@ -81,7 +86,7 @@ class SMBTests(samba.tests.TestCase):
         self.assertTrue(self.file_exists(test_file))
 
         # delete it and check that it's gone
-        self.conn.unlink(test_file)
+        self.smb_conn.unlink(test_file)
         self.assertFalse(self.file_exists(test_file))
 
     def test_chkpath(self):
diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index 547d449c9604..a6d4aa9708ed 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -928,6 +928,35 @@ static PyObject *py_cli_list(struct py_cli_state *self,
 	return result;
 }
 
+static PyObject *py_smb_unlink(struct py_cli_state *self, PyObject *args)
+{
+	NTSTATUS status;
+	const char *filename = NULL;
+	uint16_t attrs = (FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
+
+	if (!PyArg_ParseTuple(args, "s:unlink", &filename)) {
+		return NULL;
+	}
+
+	if (self->is_smb1) {
+		struct tevent_req *req = NULL;
+
+		req = cli_unlink_send(NULL, self->ev, self->cli, filename,
+				      attrs);
+		if (!py_tevent_req_wait_exc(self, req)) {
+			return NULL;
+		}
+		status = cli_unlink_recv(req);
+		TALLOC_FREE(req);
+	} else {
+		status = cli_unlink(self->cli, filename, attrs);
+	}
+
+	PyErr_NTSTATUS_NOT_OK_RAISE(status);
+
+	Py_RETURN_NONE;
+}
+
 static PyMethodDef py_cli_state_methods[] = {
 	{ "create", (PyCFunction)py_cli_create, METH_VARARGS|METH_KEYWORDS,
 	  "Open a file" },
@@ -948,6 +977,9 @@ static PyMethodDef py_cli_state_methods[] = {
 	  "List a directory" },
 	{ "get_oplock_break", (PyCFunction)py_cli_get_oplock_break,
 	  METH_VARARGS, "Wait for an oplock break" },
+	{ "unlink", (PyCFunction)py_smb_unlink,
+	  METH_VARARGS,
+	  "unlink(path) -> None\n\n \t\tDelete a file." },
 	{ NULL, NULL, 0, NULL }
 };
 
-- 
2.17.1


From dd87ddce62a6ee958c534d8078b3b12f527443db Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 3 Dec 2018 16:27:07 +1300
Subject: [PATCH 11/18] s3:pylibsmb: Add .mkdir(), .rmdir() APIS to SMB py
 bindings

I kept these separate from the chkpath API because it's a nice way to
use the old s4 API in the tests to verify the new s3 API works
correctly.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 python/samba/tests/smb.py |  6 ++--
 source3/libsmb/pylibsmb.c | 62 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/python/samba/tests/smb.py b/python/samba/tests/smb.py
index 91b487dfad13..378ed334312a 100644
--- a/python/samba/tests/smb.py
+++ b/python/samba/tests/smb.py
@@ -48,7 +48,7 @@ class SMBTests(samba.tests.TestCase):
         # temporarily create a 2nd SMB connection for migrating the py-bindings
         self.smb_conn = libsmb_samba_internal.Conn(self.server, "sysvol", creds)
 
-        self.conn.mkdir(test_dir)
+        self.smb_conn.mkdir(test_dir)
 
     def tearDown(self):
         super(SMBTests, self).tearDown()
@@ -104,9 +104,9 @@ class SMBTests(samba.tests.TestCase):
 
         # check correct result after creating and then deleting a new dir
         new_dir = os.path.join(test_dir, 'test-new').replace('/', '\\')
-        self.conn.mkdir(new_dir)
+        self.smb_conn.mkdir(new_dir)
         self.assertTrue(self.conn.chkpath(new_dir))
-        self.conn.rmdir(new_dir)
+        self.smb_conn.rmdir(new_dir)
         self.assertFalse(self.conn.chkpath(new_dir))
 
     def test_save_load_text(self):
diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index a6d4aa9708ed..1faa018e6e27 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -957,6 +957,64 @@ static PyObject *py_smb_unlink(struct py_cli_state *self, PyObject *args)
 	Py_RETURN_NONE;
 }
 
+/*
+ * Delete an empty directory
+ */
+static PyObject *py_smb_rmdir(struct py_cli_state *self, PyObject *args)
+{
+	NTSTATUS status;
+	const char *dirname;
+
+	if (!PyArg_ParseTuple(args, "s:rmdir", &dirname)) {
+		return NULL;
+	}
+
+	if (self->is_smb1) {
+		struct tevent_req *req = NULL;
+
+		req = cli_rmdir_send(NULL, self->ev, self->cli, dirname);
+		if (!py_tevent_req_wait_exc(self, req)) {
+			return NULL;
+		}
+		status = cli_rmdir_recv(req);
+		TALLOC_FREE(req);
+	} else {
+		status = cli_rmdir(self->cli, dirname);
+	}
+	PyErr_NTSTATUS_IS_ERR_RAISE(status);
+
+	Py_RETURN_NONE;
+}
+
+/*
+ * Create a directory
+ */
+static PyObject *py_smb_mkdir(struct py_cli_state *self, PyObject *args)
+{
+	NTSTATUS status;
+	const char *dirname;
+
+	if (!PyArg_ParseTuple(args, "s:mkdir", &dirname)) {
+		return NULL;
+	}
+
+	if (self->is_smb1) {
+		struct tevent_req *req = NULL;
+
+		req = cli_mkdir_send(NULL, self->ev, self->cli, dirname);
+		if (!py_tevent_req_wait_exc(self, req)) {
+			return NULL;
+		}
+		status = cli_mkdir_recv(req);
+		TALLOC_FREE(req);
+	} else {
+		status = cli_mkdir(self->cli, dirname);
+	}
+	PyErr_NTSTATUS_IS_ERR_RAISE(status);
+
+	Py_RETURN_NONE;
+}
+
 static PyMethodDef py_cli_state_methods[] = {
 	{ "create", (PyCFunction)py_cli_create, METH_VARARGS|METH_KEYWORDS,
 	  "Open a file" },
@@ -980,6 +1038,10 @@ static PyMethodDef py_cli_state_methods[] = {
 	{ "unlink", (PyCFunction)py_smb_unlink,
 	  METH_VARARGS,
 	  "unlink(path) -> None\n\n \t\tDelete a file." },
+	{ "mkdir", (PyCFunction)py_smb_mkdir, METH_VARARGS,
+	  "mkdir(path) -> None\n\n \t\tCreate a directory." },
+	{ "rmdir", (PyCFunction)py_smb_rmdir, METH_VARARGS,
+	  "rmdir(path) -> None\n\n \t\tDelete a directory." },
 	{ NULL, NULL, 0, NULL }
 };
 
-- 
2.17.1


From 5826f888d343a3f0afc94bf1546973716fad484a Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 3 Dec 2018 16:33:19 +1300
Subject: [PATCH 12/18] s3:pylibsmb: Add .chkpath() API to SMB py bindings

Note that this is checking the existence of *directories*, not *files*.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 python/samba/tests/smb.py | 10 +++++-----
 source3/libsmb/pylibsmb.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/python/samba/tests/smb.py b/python/samba/tests/smb.py
index 378ed334312a..188ff121500b 100644
--- a/python/samba/tests/smb.py
+++ b/python/samba/tests/smb.py
@@ -92,22 +92,22 @@ class SMBTests(samba.tests.TestCase):
     def test_chkpath(self):
         """Tests .chkpath determines whether or not a directory exists"""
 
-        self.assertTrue(self.conn.chkpath(test_dir))
+        self.assertTrue(self.smb_conn.chkpath(test_dir))
 
         # should return False for a non-existent directory
         bad_dir = os.path.join(test_dir, 'dont_exist').replace('/', '\\')
-        self.assertFalse(self.conn.chkpath(bad_dir))
+        self.assertFalse(self.smb_conn.chkpath(bad_dir))
 
         # should return False for files (because they're not directories)
         self.conn.savefile(test_file, binary_contents)
-        self.assertFalse(self.conn.chkpath(test_file))
+        self.assertFalse(self.smb_conn.chkpath(test_file))
 
         # check correct result after creating and then deleting a new dir
         new_dir = os.path.join(test_dir, 'test-new').replace('/', '\\')
         self.smb_conn.mkdir(new_dir)
-        self.assertTrue(self.conn.chkpath(new_dir))
+        self.assertTrue(self.smb_conn.chkpath(new_dir))
         self.smb_conn.rmdir(new_dir)
-        self.assertFalse(self.conn.chkpath(new_dir))
+        self.assertFalse(self.smb_conn.chkpath(new_dir))
 
     def test_save_load_text(self):
 
diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index 1faa018e6e27..5bdee74d15a0 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -1015,6 +1015,38 @@ static PyObject *py_smb_mkdir(struct py_cli_state *self, PyObject *args)
 	Py_RETURN_NONE;
 }
 
+/*
+ * Checks existence of a directory
+ */
+static PyObject *py_smb_chkpath(struct py_cli_state *self, PyObject *args)
+{
+	NTSTATUS status;
+	const char *path;
+
+	if (!PyArg_ParseTuple(args, "s:chkpath", &path)) {
+		return NULL;
+	}
+
+	if (self->is_smb1) {
+		struct tevent_req *req = NULL;
+
+		req = cli_chkpath_send(NULL, self->ev, self->cli, path);
+		if (!py_tevent_req_wait_exc(self, req)) {
+			return NULL;
+		}
+		status = cli_chkpath_recv(req);
+		TALLOC_FREE(req);
+	} else {
+		status = cli_chkpath(self->cli, path);
+	}
+
+	if (NT_STATUS_IS_OK(status)) {
+		Py_RETURN_TRUE;
+	}
+
+	Py_RETURN_FALSE;
+}
+
 static PyMethodDef py_cli_state_methods[] = {
 	{ "create", (PyCFunction)py_cli_create, METH_VARARGS|METH_KEYWORDS,
 	  "Open a file" },
@@ -1042,6 +1074,9 @@ static PyMethodDef py_cli_state_methods[] = {
 	  "mkdir(path) -> None\n\n \t\tCreate a directory." },
 	{ "rmdir", (PyCFunction)py_smb_rmdir, METH_VARARGS,
 	  "rmdir(path) -> None\n\n \t\tDelete a directory." },
+	{ "chkpath", (PyCFunction)py_smb_chkpath, METH_VARARGS,
+		"chkpath(dir_path) -> True or False\n\n \
+		Return true if directory exists, false otherwise." },
 	{ NULL, NULL, 0, NULL }
 };
 
-- 
2.17.1


From 1f9ebeb97bf839cf19789c45d39576fc01ebb570 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 3 Dec 2018 17:22:43 +1300
Subject: [PATCH 13/18] tests: Extend SMB py binding .list() test-case

Extend the tests to better reflect some of the .list() functionality we
expect.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 python/samba/tests/smb.py | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/python/samba/tests/smb.py b/python/samba/tests/smb.py
index 188ff121500b..c095ad5a6330 100644
--- a/python/samba/tests/smb.py
+++ b/python/samba/tests/smb.py
@@ -58,11 +58,33 @@ class SMBTests(samba.tests.TestCase):
             pass
 
     def test_list(self):
+        # check a basic listing returns the items we expect
         ls = [f['name'] for f in self.conn.list(addom)]
         self.assertIn('scripts', ls,
                       msg='"scripts" directory not found in sysvol')
         self.assertIn('Policies', ls,
                       msg='"Policies" directory not found in sysvol')
+        self.assertNotIn('..', ls,
+                         msg='Parent (..) found in directory listing')
+        self.assertNotIn('.', ls,
+                         msg='Current dir (.) found in directory listing')
+
+        # using a '*' mask should be the same as using no mask
+        ls_wildcard = [f['name'] for f in self.conn.list(addom, "*")]
+        self.assertEqual(ls, ls_wildcard)
+
+        # applying a mask should only return items that match that mask
+        ls_pol = [f['name'] for f in self.conn.list(addom, "Pol*")]
+        expected = ["Policies"]
+        self.assertEqual(ls_pol, expected)
+
+        # each item in the listing is a has with expected keys
+        expected_keys = ['attrib', 'mtime', 'name', 'short_name', 'size']
+        for item in self.conn.list(addom):
+            for key in expected_keys:
+                self.assertIn(key, item,
+                              msg="Key '%s' not in listing '%s'" % (key, item))
+
 
     def file_exists(self, filepath):
         """Returns whether a regular file exists (by trying to open it)"""
-- 
2.17.1


From 7871db6f88911674da6fce61707b77ba5dfa3774 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 4 Dec 2018 09:15:30 +1300
Subject: [PATCH 14/18] s3:pylibsmb: Consolidate .readdir()/.list() API

Rename the .readdir API to .list and make the parameters match up with
the s4 python bindings, i.e. 'mask' is optional and separate from the
directory parameter.

Note that nothing uses the .readdir source3 API currently, so it's safe
to rename.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source3/libsmb/pylibsmb.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index 5bdee74d15a0..bc8bcb3134ec 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -865,6 +865,8 @@ static PyObject *py_cli_list(struct py_cli_state *self,
 			     PyObject *kwds)
 {
 	char *mask;
+	char *base_dir;
+	char *user_mask = NULL;
 	unsigned attribute =
 		FILE_ATTRIBUTE_DIRECTORY |
 		FILE_ATTRIBUTE_SYSTEM |
@@ -876,16 +878,24 @@ static PyObject *py_cli_list(struct py_cli_state *self,
 	size_t i, num_finfos;
 	PyObject *result;
 
-	const char *kwlist[] = {
-		"mask", "attribute", "info_level", NULL
-	};
+	const char *kwlist[] = { "directory", "mask", "attribs", NULL };
 
-	if (!ParseTupleAndKeywords(
-		    args, kwds, "s|II", kwlist,
-		    &mask, &attribute, &info_level)) {
+	if (!ParseTupleAndKeywords(args, kwds, "z|sH:list", kwlist,
+				   &base_dir, &user_mask, &attribute)) {
 		return NULL;
 	}
 
+	if (user_mask == NULL) {
+		mask = talloc_asprintf(NULL, "%s\\*", base_dir);
+	} else {
+		mask = talloc_asprintf(NULL, "%s\\%s", base_dir, user_mask);
+	}
+	if (mask == NULL) {
+		PyErr_NoMemory();
+		return NULL;
+	}
+	dos_format(mask);
+
 	req = cli_list_send(NULL, self->ev, self->cli, mask, attribute,
 			    info_level);
 	if (!py_tevent_req_wait_exc(self, req)) {
@@ -893,6 +903,7 @@ static PyObject *py_cli_list(struct py_cli_state *self,
 	}
 	status = cli_list_recv(req, NULL, &finfos, &num_finfos);
 	TALLOC_FREE(req);
+	TALLOC_FREE(mask);
 
 	if (!NT_STATUS_IS_OK(status)) {
 		PyErr_SetNTSTATUS(status);
@@ -1062,9 +1073,12 @@ static PyMethodDef py_cli_state_methods[] = {
 	{ "delete_on_close", (PyCFunction)py_cli_delete_on_close,
 	  METH_VARARGS|METH_KEYWORDS,
 	  "Set/Reset the delete on close flag" },
-	{ "readdir", (PyCFunction)py_cli_list,
-	  METH_VARARGS|METH_KEYWORDS,
-	  "List a directory" },
+	{ "list", (PyCFunction)py_cli_list, METH_VARARGS|METH_KEYWORDS,
+	  "list(directory, mask='*', attribs=DEFAULT_ATTRS) -> "
+	  "directory contents as a dictionary\n"
+	  "\t\tDEFAULT_ATTRS: FILE_ATTRIBUTE_SYSTEM | "
+	  "FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_ARCHIVE\n\n"
+	  "\t\tList contents of a directory." },
 	{ "get_oplock_break", (PyCFunction)py_cli_get_oplock_break,
 	  METH_VARARGS, "Wait for an oplock break" },
 	{ "unlink", (PyCFunction)py_smb_unlink,
-- 
2.17.1


From a3f55702a9a55a06ca4862e0efd72edebef61dc5 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 4 Dec 2018 15:43:53 +1300
Subject: [PATCH 15/18] s3:pylibsmb: Split out code into list_helper()

Refactor out the code so we can re-use it for the synchronous API as
well.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source3/libsmb/pylibsmb.c | 42 +++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index bc8bcb3134ec..99220ac3dcac 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -860,6 +860,31 @@ static PyObject *py_cli_delete_on_close(struct py_cli_state *self,
 	Py_RETURN_NONE;
 }
 
+/*
+ * Helper to add directory listing entries to an overall Python list
+ */
+static NTSTATUS list_helper(const char *mntpoint, struct file_info *finfo,
+			    const char *mask, void *state)
+{
+	PyObject *result = (PyObject *)state;
+	PyObject *file = NULL;
+	int ret;
+
+	file = Py_BuildValue("{s:s,s:i}",
+			     "name", finfo->name,
+			     "mode", (int)finfo->mode);
+	if (file == NULL) {
+		return NT_STATUS_NO_MEMORY;
+	}
+
+	ret = PyList_Append(result, file);
+	if (ret == -1) {
+		return NT_STATUS_INTERNAL_ERROR;
+	}
+
+	return NT_STATUS_OK;
+}
+
 static PyObject *py_cli_list(struct py_cli_state *self,
 			     PyObject *args,
 			     PyObject *kwds)
@@ -916,21 +941,8 @@ static PyObject *py_cli_list(struct py_cli_state *self,
 	}
 
 	for (i=0; i<num_finfos; i++) {
-		struct file_info *finfo = &finfos[i];
-		PyObject *file;
-		int ret;
-
-		file = Py_BuildValue(
-			"{s:s,s:i}",
-			"name", finfo->name,
-			"mode", (int)finfo->mode);
-		if (file == NULL) {
-			Py_XDECREF(result);
-			return NULL;
-		}
-
-		ret = PyList_Append(result, file);
-		if (ret == -1) {
+		status = list_helper(base_dir, &finfos[i], user_mask, result);
+		if (!NT_STATUS_IS_OK(status)) {
 			Py_XDECREF(result);
 			return NULL;
 		}
-- 
2.17.1


From 08991e4fff313000c85a6f3182812f2b0a32a34f Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 4 Dec 2018 15:56:50 +1300
Subject: [PATCH 16/18] s3:pylibsmb: Make .list() work for SMBv2

In order for .list() to work on an SMBv2 connection we currently need to
use the synchronous API. We only do this if the connection is not using
multi-threading.

It the multi-threading case, it still uses the asynchronous API that it
currently does. Note however that this won't work currently with SMBv2.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source3/libsmb/pylibsmb.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index 99220ac3dcac..df0cc74b6fb7 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -897,7 +897,6 @@ static PyObject *py_cli_list(struct py_cli_state *self,
 		FILE_ATTRIBUTE_SYSTEM |
 		FILE_ATTRIBUTE_HIDDEN;
 	unsigned info_level = SMB_FIND_FILE_BOTH_DIRECTORY_INFO;
-	struct tevent_req *req;
 	NTSTATUS status;
 	struct file_info *finfos;
 	size_t i, num_finfos;
@@ -921,22 +920,36 @@ static PyObject *py_cli_list(struct py_cli_state *self,
 	}
 	dos_format(mask);
 
-	req = cli_list_send(NULL, self->ev, self->cli, mask, attribute,
-			    info_level);
-	if (!py_tevent_req_wait_exc(self, req)) {
+	result = Py_BuildValue("[]");
+	if (result == NULL) {
+		TALLOC_FREE(mask);
 		return NULL;
 	}
-	status = cli_list_recv(req, NULL, &finfos, &num_finfos);
-	TALLOC_FREE(req);
-	TALLOC_FREE(mask);
 
-	if (!NT_STATUS_IS_OK(status)) {
-		PyErr_SetNTSTATUS(status);
-		return NULL;
+	if (self->is_smb1) {
+		struct tevent_req *req = NULL;
+
+		req = cli_list_send(NULL, self->ev, self->cli, mask, attribute,
+				    info_level);
+		TALLOC_FREE(mask);
+
+		if (!py_tevent_req_wait_exc(self, req)) {
+			return NULL;
+		}
+		status = cli_list_recv(req, NULL, &finfos, &num_finfos);
+		TALLOC_FREE(req);
+	} else {
+		status = cli_list(self->cli, mask, attribute, list_helper,
+				  result);
+		TALLOC_FREE(mask);
+
+		if (NT_STATUS_IS_OK(status)) {
+			return result;
+		}
 	}
 
-	result = Py_BuildValue("[]");
-	if (result == NULL) {
+	if (!NT_STATUS_IS_OK(status)) {
+		PyErr_SetNTSTATUS(status);
 		return NULL;
 	}
 
-- 
2.17.1


From 8b2b39f69e668cacee3929743537ad3e57e232d4 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 4 Dec 2018 16:03:35 +1300
Subject: [PATCH 17/18] s3:pylibsmb: Don't return '.'/'..' in .list()

The source4 .list() API wasn't doing this. This makes source3 and source4
have *almost* equivalent functionality, so we can start usign the
source3 API in the tests.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 python/samba/tests/smb.py | 6 +++---
 source3/libsmb/pylibsmb.c | 5 +++++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/python/samba/tests/smb.py b/python/samba/tests/smb.py
index c095ad5a6330..f0bc6b6ef5be 100644
--- a/python/samba/tests/smb.py
+++ b/python/samba/tests/smb.py
@@ -59,7 +59,7 @@ class SMBTests(samba.tests.TestCase):
 
     def test_list(self):
         # check a basic listing returns the items we expect
-        ls = [f['name'] for f in self.conn.list(addom)]
+        ls = [f['name'] for f in self.smb_conn.list(addom)]
         self.assertIn('scripts', ls,
                       msg='"scripts" directory not found in sysvol')
         self.assertIn('Policies', ls,
@@ -70,11 +70,11 @@ class SMBTests(samba.tests.TestCase):
                          msg='Current dir (.) found in directory listing')
 
         # using a '*' mask should be the same as using no mask
-        ls_wildcard = [f['name'] for f in self.conn.list(addom, "*")]
+        ls_wildcard = [f['name'] for f in self.smb_conn.list(addom, "*")]
         self.assertEqual(ls, ls_wildcard)
 
         # applying a mask should only return items that match that mask
-        ls_pol = [f['name'] for f in self.conn.list(addom, "Pol*")]
+        ls_pol = [f['name'] for f in self.smb_conn.list(addom, "Pol*")]
         expected = ["Policies"]
         self.assertEqual(ls_pol, expected)
 
diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index df0cc74b6fb7..bf6aebda5f12 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -870,6 +870,11 @@ static NTSTATUS list_helper(const char *mntpoint, struct file_info *finfo,
 	PyObject *file = NULL;
 	int ret;
 
+	/* suppress '.' and '..' in the results we return */
+	if (ISDOT(finfo->name) || ISDOTDOT(finfo->name)) {
+		return NT_STATUS_OK;
+	}
+
 	file = Py_BuildValue("{s:s,s:i}",
 			     "name", finfo->name,
 			     "mode", (int)finfo->mode);
-- 
2.17.1


From 63eef3fd99276e9d5ae2a068ffe2fd1daf0f31d5 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 4 Dec 2018 16:25:10 +1300
Subject: [PATCH 18/18] s3:pylibsmb: Make s3 and s4 listings return the same
 dict

Make the python dictionary generated by the s3 .list() use the same keys
as the current source4 dict. The reason for using the source4 dict is
that other python code depends on these keys (e.g. ntacls.py), whereas
the source3 API is currently unused.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 python/samba/tests/smb.py |  2 +-
 source3/libsmb/pylibsmb.c | 28 ++++++++++++++++++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/python/samba/tests/smb.py b/python/samba/tests/smb.py
index f0bc6b6ef5be..1049d106f5ab 100644
--- a/python/samba/tests/smb.py
+++ b/python/samba/tests/smb.py
@@ -80,7 +80,7 @@ class SMBTests(samba.tests.TestCase):
 
         # each item in the listing is a has with expected keys
         expected_keys = ['attrib', 'mtime', 'name', 'short_name', 'size']
-        for item in self.conn.list(addom):
+        for item in self.smb_conn.list(addom):
             for key in expected_keys:
                 self.assertIn(key, item,
                               msg="Key '%s' not in listing '%s'" % (key, item))
diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index bf6aebda5f12..504bd06c51e2 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -869,19 +869,34 @@ static NTSTATUS list_helper(const char *mntpoint, struct file_info *finfo,
 	PyObject *result = (PyObject *)state;
 	PyObject *file = NULL;
 	int ret;
+	time_t mtime;
 
 	/* suppress '.' and '..' in the results we return */
 	if (ISDOT(finfo->name) || ISDOTDOT(finfo->name)) {
 		return NT_STATUS_OK;
 	}
 
-	file = Py_BuildValue("{s:s,s:i}",
-			     "name", finfo->name,
-			     "mode", (int)finfo->mode);
+	file = PyDict_New();
 	if (file == NULL) {
 		return NT_STATUS_NO_MEMORY;
 	}
 
+	PyDict_SetItemString(file, "name", PyStr_FromString(finfo->name));
+
+	/* Windows does not always return short_name */
+	if (finfo->short_name) {
+		PyDict_SetItemString(file, "short_name",
+				     PyStr_FromString(finfo->short_name));
+	} else {
+		PyDict_SetItemString(file, "short_name", Py_None);
+	}
+
+	PyDict_SetItemString(file, "size",
+			     PyLong_FromUnsignedLongLong(finfo->size));
+	PyDict_SetItemString(file, "attrib", PyInt_FromLong(finfo->mode));
+	mtime = convert_timespec_to_time_t(finfo->mtime_ts);
+	PyDict_SetItemString(file, "mtime", PyInt_FromLong(mtime));
+
 	ret = PyList_Append(result, file);
 	if (ret == -1) {
 		return NT_STATUS_INTERNAL_ERROR;
@@ -1108,7 +1123,12 @@ static PyMethodDef py_cli_state_methods[] = {
 	  "directory contents as a dictionary\n"
 	  "\t\tDEFAULT_ATTRS: FILE_ATTRIBUTE_SYSTEM | "
 	  "FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_ARCHIVE\n\n"
-	  "\t\tList contents of a directory." },
+	  "\t\tList contents of a directory. The keys are, \n"
+	  "\t\t\tname: Long name of the directory item\n"
+	  "\t\t\tshort_name: Short name of the directory item\n"
+	  "\t\t\tsize: File size in bytes\n"
+	  "\t\t\tattrib: Attributes\n"
+	  "\t\t\tmtime: Modification time\n" },
 	{ "get_oplock_break", (PyCFunction)py_cli_get_oplock_break,
 	  METH_VARARGS, "Wait for an oplock break" },
 	{ "unlink", (PyCFunction)py_smb_unlink,
-- 
2.17.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20181204/69a643cb/signature.sig>


More information about the samba-technical mailing list