[RFC] Advice on SMB client python bindings?
Tim Beale
timbeale at catalyst.net.nz
Mon Dec 3 03:03:09 UTC 2018
Hi Metze,
OK, just to re-cap, would you be happy with the following?
- I add an optional async=True|False parameter to
libsmb_samba_internal.Conn(). Default to async=False. This changes the
py_cli_state_init() behaviour (mostly around self->oplock_waiter).
- Where possible/simple, we use the async libsmb APIs so they work in
either case.
- Where that's not so simple, we just use the synchronous API, i.e. more
work would be needed in future if we ever wanted to support that API in
the async=True case.
A bunch of the libsmb APIs (cli_unlink(), cli_rmdir(), cli_mkdir(),
cli_chkpath(), cli_query_security_descriptor()) seem to do the
SMBv1/SMBv2 check in the synchronous API rather than the asynchronous
API. And it looks to me like trying to add SMBv2 support to the async
API would not be trivial.
The attached patch-set (#3 & #4) is an example of what I mean.
Thanks,
Tim
On 27/11/18 11:17 AM, Stefan Metzmacher wrote:
> Hi Tim,
>
>> Finally, I noticed there's some existing source3 python bindings
>> (pylibsmb.c, aka libsmb_samba_internal), however, they don't really
>> appear to be used. Should I consolidate this code with what I'm
>> changing, so we have a single set of SMB client python bindings? If so,
>> does anything actually rely on libsmb_samba_internal? (Removing the
>> .get_oplock_break API so we can use the synchronous SMB client APIs
>> would make my life a lot easier).
> I think it would be good to have just one set of python bindings.
>
> I recently had the need for changes as well. See
> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-dcerpc-auth
> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=cb40716a117e512fa35897f8e19cf2e5029af4d5
> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=b1ebc6f68e067dba3288e000e723738bd1414a98
> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=0da84a6006f814c0df9760ea4aa4d44122d2a0c6
>
> I'd also keep the async stuff, it doesn't look too hard at the python
> layer.
>
> I think we could disable the poll_mt stuff by default and let the caller
> select between the two py_cli_state_setup_ev() implementations in
> py_cli_state_init().
>
> metze
>
>
-------------- next part --------------
From fcd1b69ad695a61815d3c6c96ef40f6dbc58f733 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 1/4] 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 e0e60e3..de2ecfc 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.7.4
From e46a846d83a66caf2cdcf885937a1d821b57ae94 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 2/4] 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 de2ecfc..6ee5eaa 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.7.4
From 59ad0d52d8ea4cdd13e96cb62e7ba4b558ea8abe Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 27 Nov 2018 18:13:51 +1300
Subject: [PATCH 3/4] s3/libsmb: Add 'async' connection option to SMB python
bindings
Update the source3 SMB python bindings so that we can use synchronous
client library APIs.
Currently we can't use the synchronous APIs due to the (unused)
.get_oplock_break() API. Because there's always an asynchronous
oplock_break_waiter call in progress, none of the synchronous APIs can
be called.
This patch adds an 'async' option when the python binding SMB connection
is created. We only initialize the oplock_break_waiter call in async
mode, which means the API can't be used if async=False (the new
default).
The reason we want to do this is the synchronous client APIs are simpler
to use, and we have a working example for comparison (in the form of
smbclient). More importantly, the synchronous APIs have SMBv2 support,
whereas not all the async APIS do (e.g. cli_unlink(), cli_rmdir(),
cli_chkpath(), etc).
The tests for libsmb_samba_internal have been updated to show both the
async and synchronous connections work.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
python/samba/tests/libsmb_samba_internal.py | 11 ++++++--
source3/libsmb/pylibsmb.c | 39 +++++++++++++++++++++--------
2 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/python/samba/tests/libsmb_samba_internal.py b/python/samba/tests/libsmb_samba_internal.py
index c88095c..e48d368 100644
--- a/python/samba/tests/libsmb_samba_internal.py
+++ b/python/samba/tests/libsmb_samba_internal.py
@@ -49,7 +49,7 @@ class LibsmbTestCase(samba.tests.TestCase):
except Exception:
self.exc = sys.exc_info()
- def test_OpenClose(self):
+ def _test_OpenClose(self, async):
lp = s3param.get_context()
lp.load(os.getenv("SMB_CONF_PATH"))
@@ -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,
+ async=async)
mythreads = []
@@ -75,6 +76,12 @@ class LibsmbTestCase(samba.tests.TestCase):
if t.exc:
raise t.exc[0](t.exc[1])
+ def test_OpenClose_async(self):
+ self._test_OpenClose(async=True)
+
+ def test_OpenClose(self):
+ self._test_OpenClose(async=False)
+
if __name__ == "__main__":
import unittest
diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
index 99e5587..cfe27a3 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -86,6 +86,7 @@ struct py_cli_state {
struct tevent_req *oplock_waiter;
struct py_cli_oplock_break *oplock_breaks;
struct py_tevent_cond *oplock_cond;
+ bool use_async_calls;
};
#ifdef HAVE_PTHREAD
@@ -398,6 +399,7 @@ static PyObject *py_cli_state_new(PyTypeObject *type, PyObject *args,
self->oplock_waiter = NULL;
self->oplock_cond = NULL;
self->oplock_breaks = NULL;
+ self->use_async_calls = false;
return (PyObject *)self;
}
@@ -419,9 +421,10 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
* into the sync wrapper functions currently.
*/
int flags = CLI_FULL_CONNECTION_FORCE_SMB1;
+ PyObject *async = Py_False;
static const char *kwlist[] = {
- "host", "share", "credentials", NULL
+ "host", "share", "credentials", "async", NULL
};
PyTypeObject *py_type_Credentials = get_pytype(
@@ -431,8 +434,9 @@ 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,
+ &async);
Py_DECREF(py_type_Credentials);
@@ -440,6 +444,8 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
return -1;
}
+ self->use_async_calls = PyObject_IsTrue(async);
+
if (!py_cli_state_setup_ev(self)) {
return -1;
}
@@ -464,14 +470,21 @@ static int py_cli_state_init(struct py_cli_state *self, PyObject *args,
return -1;
}
- self->oplock_waiter = cli_smb_oplock_break_waiter_send(
- self->ev, self->ev, self->cli);
- if (self->oplock_waiter == NULL) {
- PyErr_NoMemory();
- return -1;
+ /*
+ * only initialize the oplock_waiter if we're using the async client
+ * APIs. Otherwise, we can't call any synchronous APIs, because we
+ * always have an async call in flight
+ */
+ if (self->use_async_calls) {
+ self->oplock_waiter = cli_smb_oplock_break_waiter_send(
+ self->ev, self->ev, self->cli);
+ if (self->oplock_waiter == NULL) {
+ PyErr_NoMemory();
+ return -1;
+ }
+ tevent_req_set_callback(self->oplock_waiter, py_cli_got_oplock_break,
+ self);
}
- tevent_req_set_callback(self->oplock_waiter, py_cli_got_oplock_break,
- self);
return 0;
}
@@ -519,6 +532,12 @@ static PyObject *py_cli_get_oplock_break(struct py_cli_state *self,
{
size_t num_oplock_breaks;
+ if (!self->use_async_calls) {
+ DBG_ERR("SMB connection not setup as async");
+ PyErr_SetNTSTATUS(NT_STATUS_INVALID_PARAMETER);
+ return NULL;
+ }
+
if (!PyArg_ParseTuple(args, "")) {
return NULL;
}
--
2.7.4
From 0a09219f86c671abb92d2ad6373ec48007776fcb 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 4/4] 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. Note that
the s3 loadparm will be cleaned up more in a subsequent patch.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
python/samba/tests/smb.py | 10 +++++++++-
source3/libsmb/pylibsmb.c | 23 +++++++++++++++++++++++
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/python/samba/tests/smb.py b/python/samba/tests/smb.py
index 6ee5eaa..83ad37c 100644
--- a/python/samba/tests/smb.py
+++ b/python/samba/tests/smb.py
@@ -22,6 +22,8 @@ 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
+from samba.samba3 import param as s3param
PY3 = sys.version_info[0] == 3
addom = 'addom.samba.example.com/'
@@ -43,6 +45,12 @@ class SMBTests(samba.tests.TestCase):
"sysvol",
lp=self.get_loadparm(),
creds=creds)
+
+ # temporarily create a 2nd SMB connection for migrating the py-bindings
+ lp = s3param.get_context()
+ lp.load(os.getenv("SMB_CONF_PATH"))
+ self.smb_conn = libsmb_samba_internal.Conn(self.server, "sysvol", creds)
+
self.conn.mkdir(test_dir)
def tearDown(self):
@@ -81,7 +89,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 cfe27a3..aaef9a7 100644
--- a/source3/libsmb/pylibsmb.c
+++ b/source3/libsmb/pylibsmb.c
@@ -871,6 +871,22 @@ 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;
+
+ if (!PyArg_ParseTuple(args, "s:unlink", &filename)) {
+ return NULL;
+ }
+
+ status = cli_unlink(self->cli, filename,
+ FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
+ 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" },
@@ -891,6 +907,13 @@ 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" },
+
+ /*
+ * Synchronous APIs (cannot be used with async=True connections)
+ */
+ { "unlink", (PyCFunction)py_smb_unlink, METH_VARARGS,
+ "unlink(path) -> None\n\n \
+ Delete a file." },
{ NULL, NULL, 0, NULL }
};
--
2.7.4
More information about the samba-technical
mailing list