[PATCH] Messaging improvements and fixes needed for auth logging
Jeremy Allison
jra at samba.org
Mon Mar 20 22:08:28 UTC 2017
On Mon, Mar 20, 2017 at 08:21:13PM +1300, Andrew Bartlett via samba-technical wrote:
> To test the auth and authz logging, we decided to use the messaging
> layer (details in our auth-logging branch in Catalyst's repo), as we
> have bindings for that in python.
>
> It turned out that there were a number of issues in these layers, both
> the incomplete python bindings (they never previously worked: the tests
> were not actually tests) and an issue in the serverid database due to
> the way tdb_fetch_talloc() behaved with zero-length records.
>
> The attached patches addresses both issues and allows us to build a
> robust testsuite for authentication and authorization logging!
>
> Please review!
Can you split this into separate patchsets for the serverid database
problem and the incomplete python problem ? It's a little confusing
having them mixed together like this.
In addition I'm assuming we'll need a bug logged for the serverid database
bug, and the commit messages for that fix should be tagged with the
BUG: https://.... url.
Thanks for finding and fixing this !
Cheers,
Jeremy.
> --
> Andrew Bartlett http://samba.org/~abartlet/
> Authentication Developer, Samba Team http://samba.org
> Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
> From 48ec03952ab0663257a846838272ba213683ee6a Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Wed, 8 Mar 2017 14:53:26 +1300
> Subject: [PATCH 1/7] pymessaging: Add support for irpc_add_name
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> Pair-Programmed-by: Gary Lockyer <gary at catalyst.net.nz>
> Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
> ---
> python/samba/tests/messaging.py | 4 ++++
> source4/lib/messaging/pymessaging.c | 23 ++++++++++++++++++++++-
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/python/samba/tests/messaging.py b/python/samba/tests/messaging.py
> index 5d32d60..1c5dfe5 100644
> --- a/python/samba/tests/messaging.py
> +++ b/python/samba/tests/messaging.py
> @@ -49,6 +49,10 @@ class MessagingTests(TestCase):
> x = self.get_context()
> self.assertTrue(isinstance(x.server_id, server_id))
>
> + def test_add_name(self):
> + x = self.get_context()
> + x.irpc_add_name("samba.messaging test")
> +
> def test_ping_speed(self):
> server_ctx = self.get_context((0, 1))
> def ping_callback(src, data):
> diff --git a/source4/lib/messaging/pymessaging.c b/source4/lib/messaging/pymessaging.c
> index f62354b..9d0997f 100644
> --- a/source4/lib/messaging/pymessaging.c
> +++ b/source4/lib/messaging/pymessaging.c
> @@ -241,6 +241,25 @@ static PyObject *py_imessaging_deregister(PyObject *self, PyObject *args, PyObje
> Py_RETURN_NONE;
> }
>
> +static PyObject *py_irpc_add_name(PyObject *self, PyObject *args, PyObject *kwargs)
> +{
> + imessaging_Object *iface = (imessaging_Object *)self;
> + char *server_name;
> + NTSTATUS status;
> +
> + if (!PyArg_ParseTuple(args, "s", &server_name)) {
> + return NULL;
> + }
> +
> + status = irpc_add_name(iface->msg_ctx, server_name);
> + if (!NT_STATUS_IS_OK(status)) {
> + PyErr_SetNTSTATUS(status);
> + return NULL;
> + }
> +
> + Py_RETURN_NONE;
> +}
> +
> static PyObject *py_irpc_servers_byname(PyObject *self, PyObject *args, PyObject *kwargs)
> {
> imessaging_Object *iface = (imessaging_Object *)self;
> @@ -341,10 +360,12 @@ static PyMethodDef py_imessaging_methods[] = {
> "S.register(callback, msg_type=None) -> msg_type\nRegister a message handler" },
> { "deregister", (PyCFunction)py_imessaging_deregister, METH_VARARGS|METH_KEYWORDS,
> "S.deregister(callback, msg_type) -> None\nDeregister a message handler" },
> + { "irpc_add_name", (PyCFunction)py_irpc_add_name, METH_VARARGS,
> + "S.irpc_add_name(name) -> None\nAdd this context to the list of server_id values that are registered for a particular name" },
> { "irpc_servers_byname", (PyCFunction)py_irpc_servers_byname, METH_VARARGS,
> "S.irpc_servers_byname(name) -> list\nGet list of server_id values that are registered for a particular name" },
> { "irpc_all_servers", (PyCFunction)py_irpc_all_servers, METH_NOARGS,
> - "S.irpc_servers_byname() -> list\nGet list of all registered names and the associated server_id values" },
> + "S.irpc_all_servers() -> list\nGet list of all registered names and the associated server_id values" },
> { NULL, NULL, 0, NULL }
> };
>
> --
> 2.9.3
>
>
> From 461fc025fba11d3ff6b723b8aa421c7d94083ce7 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 14 Mar 2017 13:39:00 +1300
> Subject: [PATCH 2/7] pymessaging: Add irpc_remove_name
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
> python/samba/tests/messaging.py | 8 ++++++++
> source4/lib/messaging/pymessaging.c | 16 ++++++++++++++++
> 2 files changed, 24 insertions(+)
>
> diff --git a/python/samba/tests/messaging.py b/python/samba/tests/messaging.py
> index 1c5dfe5..3eeab52 100644
> --- a/python/samba/tests/messaging.py
> +++ b/python/samba/tests/messaging.py
> @@ -22,6 +22,7 @@ import samba
> from samba.messaging import Messaging
> from samba.tests import TestCase
> from samba.dcerpc.server_id import server_id
> +from samba.ndr import ndr_print
>
> class MessagingTests(TestCase):
>
> @@ -52,6 +53,13 @@ class MessagingTests(TestCase):
> def test_add_name(self):
> x = self.get_context()
> x.irpc_add_name("samba.messaging test")
> + name_list = x.irpc_servers_byname("samba.messaging test")
> + self.assertEqual(len(name_list), 1)
> + self.assertEqual(ndr_print(x.server_id),
> + ndr_print(name_list[0]))
> + x.irpc_remove_name("samba.messaging test")
> + self.assertEqual([],
> + x.irpc_servers_byname("samba.messaging test"))
>
> def test_ping_speed(self):
> server_ctx = self.get_context((0, 1))
> diff --git a/source4/lib/messaging/pymessaging.c b/source4/lib/messaging/pymessaging.c
> index 9d0997f..b317955 100644
> --- a/source4/lib/messaging/pymessaging.c
> +++ b/source4/lib/messaging/pymessaging.c
> @@ -260,6 +260,20 @@ static PyObject *py_irpc_add_name(PyObject *self, PyObject *args, PyObject *kwar
> Py_RETURN_NONE;
> }
>
> +static PyObject *py_irpc_remove_name(PyObject *self, PyObject *args, PyObject *kwargs)
> +{
> + imessaging_Object *iface = (imessaging_Object *)self;
> + char *server_name;
> +
> + if (!PyArg_ParseTuple(args, "s", &server_name)) {
> + return NULL;
> + }
> +
> + irpc_remove_name(iface->msg_ctx, server_name);
> +
> + Py_RETURN_NONE;
> +}
> +
> static PyObject *py_irpc_servers_byname(PyObject *self, PyObject *args, PyObject *kwargs)
> {
> imessaging_Object *iface = (imessaging_Object *)self;
> @@ -362,6 +376,8 @@ static PyMethodDef py_imessaging_methods[] = {
> "S.deregister(callback, msg_type) -> None\nDeregister a message handler" },
> { "irpc_add_name", (PyCFunction)py_irpc_add_name, METH_VARARGS,
> "S.irpc_add_name(name) -> None\nAdd this context to the list of server_id values that are registered for a particular name" },
> + { "irpc_remove_name", (PyCFunction)py_irpc_remove_name, METH_VARARGS,
> + "S.irpc_remove_name(name) -> None\nAdd this context to the list of server_id values that are registered for a particular name" },
> { "irpc_servers_byname", (PyCFunction)py_irpc_servers_byname, METH_VARARGS,
> "S.irpc_servers_byname(name) -> list\nGet list of server_id values that are registered for a particular name" },
> { "irpc_all_servers", (PyCFunction)py_irpc_all_servers, METH_NOARGS,
> --
> 2.9.3
>
>
> From c540d344b987f688c8fbdcbef87c85b0c8cd525f Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 14 Mar 2017 16:07:46 +1300
> Subject: [PATCH 3/7] selftest: Test server_id database add and removal
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
> python/samba/tests/messaging.py | 19 +++++++++++++------
> selftest/knownfail | 1 +
> 2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/python/samba/tests/messaging.py b/python/samba/tests/messaging.py
> index 3eeab52..a70be96 100644
> --- a/python/samba/tests/messaging.py
> +++ b/python/samba/tests/messaging.py
> @@ -23,6 +23,7 @@ from samba.messaging import Messaging
> from samba.tests import TestCase
> from samba.dcerpc.server_id import server_id
> from samba.ndr import ndr_print
> +import random
>
> class MessagingTests(TestCase):
>
> @@ -46,20 +47,26 @@ class MessagingTests(TestCase):
> for name in x.irpc_all_servers():
> self.assertTrue(isinstance(x.irpc_servers_byname(name.name), list))
>
> + def test_unknown_name(self):
> + x = self.get_context()
> + self.assertRaises(KeyError,
> + x.irpc_servers_byname, "samba.messaging test NONEXISTING")
> +
> def test_assign_server_id(self):
> x = self.get_context()
> self.assertTrue(isinstance(x.server_id, server_id))
>
> - def test_add_name(self):
> + def test_add_remove_name(self):
> x = self.get_context()
> - x.irpc_add_name("samba.messaging test")
> - name_list = x.irpc_servers_byname("samba.messaging test")
> + name = "samba.messaging test-%d" % random.randint(1, 1000000)
> + x.irpc_add_name(name)
> + name_list = x.irpc_servers_byname(name)
> self.assertEqual(len(name_list), 1)
> self.assertEqual(ndr_print(x.server_id),
> ndr_print(name_list[0]))
> - x.irpc_remove_name("samba.messaging test")
> - self.assertEqual([],
> - x.irpc_servers_byname("samba.messaging test"))
> + x.irpc_remove_name(name)
> + self.assertRaises(KeyError,
> + x.irpc_servers_byname, name)
>
> def test_ping_speed(self):
> server_ctx = self.get_context((0, 1))
> diff --git a/selftest/knownfail b/selftest/knownfail
> index cfd4b35..2f3b22b 100644
> --- a/selftest/knownfail
> +++ b/selftest/knownfail
> @@ -317,3 +317,4 @@
> ^samba3.smb2.credits.skipped_mid.*
> ^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dangling_multi_valued_dbcheck
> ^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dangling_multi_valued_check_missing
> +^samba.tests.messaging.samba.tests.messaging.MessagingTests.test_add_remove_name
> \ No newline at end of file
> --
> 2.9.3
>
>
> From eef04949817bab65aa3c3268cf8690e7753e174e Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 14 Mar 2017 15:22:01 +1300
> Subject: [PATCH 4/7] lib/util: Do not return an unterminated pointer in
> tdb_fetch_talloc()
>
> Otherwise, if a TDB entry is truncated to 0 length, this will return
> uninitialised memory!
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
> lib/util/server_id_db.c | 42 +++++++++++++++++++++++++++++++++---------
> lib/util/util_tdb.c | 9 +++++----
> lib/util/util_tdb.h | 2 +-
> selftest/knownfail | 1 -
> 4 files changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/lib/util/server_id_db.c b/lib/util/server_id_db.c
> index e0b8476..937de89 100644
> --- a/lib/util/server_id_db.c
> +++ b/lib/util/server_id_db.c
> @@ -137,7 +137,8 @@ int server_id_db_prune_name(struct server_id_db *db, const char *name,
> size_t idbuf_len = server_id_str_buf_unique(server, NULL, 0);
> char idbuf[idbuf_len];
> TDB_DATA key;
> - uint8_t *data;
> + TDB_DATA data;
> + TDB_DATA data_store;
> char *ids, *id;
> int ret;
>
> @@ -156,18 +157,32 @@ int server_id_db_prune_name(struct server_id_db *db, const char *name,
> return ret;
> }
>
> - ids = (char *)data;
> + if (data.dsize == 0) {
> + tdb_chainunlock(tdb, key);
> + TALLOC_FREE(data.dptr);
> + return ENOENT;
> + }
> +
> + /* We assert that the DB contains a NULL-terminated string */
> + ids = (char *)data.dptr;
>
> id = strv_find(ids, idbuf);
> if (id == NULL) {
> tdb_chainunlock(tdb, key);
> - TALLOC_FREE(data);
> + TALLOC_FREE(data.dptr);
> return ENOENT;
> }
>
> strv_delete(&ids, id);
> - ret = tdb_store(tdb, key, talloc_tdb_data(ids), TDB_MODIFY);
> - TALLOC_FREE(data);
> +
> + data_store = talloc_tdb_data(ids);
> +
> + if (data_store.dsize == 0) {
> + ret = tdb_delete(tdb, key);
> + } else {
> + ret = tdb_store(tdb, key, data_store, TDB_MODIFY);
> + }
> + TALLOC_FREE(data.dptr);
>
> tdb_chainunlock(tdb, key);
>
> @@ -199,7 +214,7 @@ int server_id_db_lookup(struct server_id_db *db, const char *name,
> {
> struct tdb_context *tdb = db->tdb->tdb;
> TDB_DATA key;
> - uint8_t *data;
> + TDB_DATA data;
> char *ids, *id;
> unsigned num_servers;
> struct server_id *servers;
> @@ -212,12 +227,17 @@ int server_id_db_lookup(struct server_id_db *db, const char *name,
> return ret;
> }
>
> - ids = (char *)data;
> + if (data.dsize == 0) {
> + return ENOENT;
> + }
> +
> + /* We assert that the DB contains a NULL-terminated string */
> + ids = (char *)data.dptr;
> num_servers = strv_count(ids);
>
> servers = talloc_array(mem_ctx, struct server_id, num_servers);
> if (servers == NULL) {
> - TALLOC_FREE(data);
> + TALLOC_FREE(data.dptr);
> return ENOMEM;
> }
>
> @@ -227,7 +247,7 @@ int server_id_db_lookup(struct server_id_db *db, const char *name,
> servers[i++] = server_id_from_string(NONCLUSTER_VNN, id);
> }
>
> - TALLOC_FREE(data);
> + TALLOC_FREE(data.dptr);
>
> *pnum_servers = num_servers;
> *pservers = servers;
> @@ -283,6 +303,10 @@ static int server_id_db_traverse_fn(struct tdb_context *tdb,
> }
> name = (const char *)key.dptr;
>
> + if (data.dsize == 0) {
> + return 0;
> + }
> +
> ids = (char *)talloc_memdup(state->mem_ctx, data.dptr, data.dsize);
> if (ids == NULL) {
> return 0;
> diff --git a/lib/util/util_tdb.c b/lib/util/util_tdb.c
> index 9bf18dc..8ff89ff 100644
> --- a/lib/util/util_tdb.c
> +++ b/lib/util/util_tdb.c
> @@ -486,19 +486,20 @@ int map_unix_error_from_tdb(enum TDB_ERROR err)
>
> struct tdb_fetch_talloc_state {
> TALLOC_CTX *mem_ctx;
> - uint8_t *buf;
> + TDB_DATA buf;
> };
>
> static int tdb_fetch_talloc_parser(TDB_DATA key, TDB_DATA data,
> void *private_data)
> {
> struct tdb_fetch_talloc_state *state = private_data;
> - state->buf = talloc_memdup(state->mem_ctx, data.dptr, data.dsize);
> + state->buf.dptr = talloc_memdup(state->mem_ctx, data.dptr, data.dsize);
> + state->buf.dsize = data.dsize;
> return 0;
> }
>
> int tdb_fetch_talloc(struct tdb_context *tdb, TDB_DATA key,
> - TALLOC_CTX *mem_ctx, uint8_t **buf)
> + TALLOC_CTX *mem_ctx, TDB_DATA *buf)
> {
> struct tdb_fetch_talloc_state state = { .mem_ctx = mem_ctx };
> int ret;
> @@ -509,7 +510,7 @@ int tdb_fetch_talloc(struct tdb_context *tdb, TDB_DATA key,
> return map_unix_error_from_tdb(err);
> }
>
> - if (state.buf == NULL) {
> + if (state.buf.dptr == NULL && state.buf.dsize != 0) {
> return ENOMEM;
> }
>
> diff --git a/lib/util/util_tdb.h b/lib/util/util_tdb.h
> index 3b50789..1f56341 100644
> --- a/lib/util/util_tdb.h
> +++ b/lib/util/util_tdb.h
> @@ -146,6 +146,6 @@ NTSTATUS map_nt_error_from_tdb(enum TDB_ERROR err);
> int map_unix_error_from_tdb(enum TDB_ERROR err);
>
> int tdb_fetch_talloc(struct tdb_context *tdb, TDB_DATA key,
> - TALLOC_CTX *mem_ctx, uint8_t **buf);
> + TALLOC_CTX *mem_ctx, TDB_DATA *buf);
>
> #endif /* _____LIB_UTIL_UTIL_TDB_H__ */
> diff --git a/selftest/knownfail b/selftest/knownfail
> index 2f3b22b..cfd4b35 100644
> --- a/selftest/knownfail
> +++ b/selftest/knownfail
> @@ -317,4 +317,3 @@
> ^samba3.smb2.credits.skipped_mid.*
> ^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dangling_multi_valued_dbcheck
> ^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dangling_multi_valued_check_missing
> -^samba.tests.messaging.samba.tests.messaging.MessagingTests.test_add_remove_name
> \ No newline at end of file
> --
> 2.9.3
>
>
> From fd22a3fdbd6e3561f76dbad05730c78f7500a135 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Mon, 20 Mar 2017 14:57:41 +1300
> Subject: [PATCH 5/7] server_id: Add runtime check for null termination
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
> lib/util/server_id_db.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/lib/util/server_id_db.c b/lib/util/server_id_db.c
> index 937de89..8aeda98 100644
> --- a/lib/util/server_id_db.c
> +++ b/lib/util/server_id_db.c
> @@ -164,6 +164,10 @@ int server_id_db_prune_name(struct server_id_db *db, const char *name,
> }
>
> /* We assert that the DB contains a NULL-terminated string */
> + if (data.dptr[data.dsize - 1] != '\0') {
> + return EINVAL;
> + }
> +
> ids = (char *)data.dptr;
>
> id = strv_find(ids, idbuf);
> @@ -232,6 +236,10 @@ int server_id_db_lookup(struct server_id_db *db, const char *name,
> }
>
> /* We assert that the DB contains a NULL-terminated string */
> + if (data.dptr[data.dsize - 1] != '\0') {
> + return EINVAL;
> + }
> +
> ids = (char *)data.dptr;
> num_servers = strv_count(ids);
>
> --
> 2.9.3
>
>
> From a78ec79a012c4b0e8face1efa4b043180b26e001 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 14 Mar 2017 12:39:13 +1300
> Subject: [PATCH 6/7] pymessaging: Add a hook to run the event loop, make
> callbacks practical
>
> These change allow us to write a messaging server in python.
>
> The previous ping_speed test did not actually test anything, so
> we use .loop_once() to make it actually work. To enable practial use
> a context is supplied in the tuple with the callback, and the server_id
> for the reply is not placed inside an additional tuple.
>
> In order to get at the internal event context on which to loop, we
> expose imessaging_context in messaging_internal.h and allow the python
> bindings to use that header.
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
> python/samba/tests/messaging.py | 52 +++++++++++++------
> source4/lib/messaging/messaging.c | 17 +------
> source4/lib/messaging/messaging_internal.h | 36 ++++++++++++++
> source4/lib/messaging/pymessaging.c | 80 +++++++++++++++++++++++++++---
> 4 files changed, 147 insertions(+), 38 deletions(-)
> create mode 100644 source4/lib/messaging/messaging_internal.h
>
> diff --git a/python/samba/tests/messaging.py b/python/samba/tests/messaging.py
> index a70be96..6ee18e7 100644
> --- a/python/samba/tests/messaging.py
> +++ b/python/samba/tests/messaging.py
> @@ -21,8 +21,9 @@
> import samba
> from samba.messaging import Messaging
> from samba.tests import TestCase
> -from samba.dcerpc.server_id import server_id
> +import time
> from samba.ndr import ndr_print
> +from samba.dcerpc import server_id
> import random
>
> class MessagingTests(TestCase):
> @@ -35,7 +36,8 @@ class MessagingTests(TestCase):
> x = self.get_context()
> def callback():
> pass
> - msg_type = x.register(callback)
> + msg_type = x.register((callback, None))
> + self.assertTrue(isinstance(msg_type, long))
> x.deregister(callback, msg_type)
>
> def test_all_servers(self):
> @@ -54,7 +56,7 @@ class MessagingTests(TestCase):
>
> def test_assign_server_id(self):
> x = self.get_context()
> - self.assertTrue(isinstance(x.server_id, server_id))
> + self.assertTrue(isinstance(x.server_id, server_id.server_id))
>
> def test_add_remove_name(self):
> x = self.get_context()
> @@ -69,19 +71,41 @@ class MessagingTests(TestCase):
> x.irpc_servers_byname, name)
>
> def test_ping_speed(self):
> + got_ping = {"count": 0}
> + got_pong = {"count": 0}
> + timeout = False
> +
> + msg_pong = 0
> + msg_ping = 0
> +
> server_ctx = self.get_context((0, 1))
> - def ping_callback(src, data):
> - server_ctx.send(src, data)
> - def exit_callback():
> - print "received exit"
> - msg_ping = server_ctx.register(ping_callback)
> - msg_exit = server_ctx.register(exit_callback)
> -
> - def pong_callback():
> - print "received pong"
> + def ping_callback(got_ping, msg_type, src, data):
> + got_ping["count"] += 1
> + server_ctx.send(src, msg_pong, data)
> +
> + msg_ping = server_ctx.register((ping_callback, got_ping))
> +
> + def pong_callback(got_pong, msg_type, src, data):
> + got_pong["count"] += 1
> +
> client_ctx = self.get_context((0, 2))
> - msg_pong = client_ctx.register(pong_callback)
> + msg_pong = client_ctx.register((pong_callback, got_pong))
>
> + # Try both server_id forms (structure and tuple)
> client_ctx.send((0, 1), msg_ping, "testing")
> - client_ctx.send((0, 1), msg_ping, "")
>
> + client_ctx.send((0, 1), msg_ping, "testing2")
> +
> + start_time = time.time()
> +
> + # NOTE WELL: If debugging this with GDB, then the timeout will
> + # fire while you are trying to understand it.
> +
> + while (got_ping["count"] < 2 or got_pong["count"] < 2) and not timeout:
> + client_ctx.loop_once(0.1)
> + server_ctx.loop_once(0.1)
> + if time.time() - start_time > 1:
> + timeout = True
> +
> + self.assertEqual(got_ping["count"], 2)
> + self.assertEqual(got_pong["count"], 2)
> diff --git a/source4/lib/messaging/messaging.c b/source4/lib/messaging/messaging.c
> index 84df934..4d75f09 100644
> --- a/source4/lib/messaging/messaging.c
> +++ b/source4/lib/messaging/messaging.c
> @@ -24,6 +24,7 @@
> #include "lib/util/server_id.h"
> #include "system/filesys.h"
> #include "messaging/messaging.h"
> +#include "messaging/messaging_internal.h"
> #include "../lib/util/dlinklist.h"
> #include "lib/socket/socket.h"
> #include "librpc/gen_ndr/ndr_irpc.h"
> @@ -55,22 +56,6 @@ struct irpc_request {
> } incoming;
> };
>
> -struct imessaging_context {
> - struct imessaging_context *prev, *next;
> - struct tevent_context *ev;
> - struct server_id server_id;
> - const char *sock_dir;
> - const char *lock_dir;
> - struct dispatch_fn **dispatch;
> - uint32_t num_types;
> - struct idr_context *dispatch_tree;
> - struct irpc_list *irpc;
> - struct idr_context *idr;
> - struct server_id_db *names;
> - struct timeval start_time;
> - void *msg_dgm_ref;
> -};
> -
> /* we have a linked list of dispatch handlers for each msg_type that
> this messaging server can deal with */
> struct dispatch_fn {
> diff --git a/source4/lib/messaging/messaging_internal.h b/source4/lib/messaging/messaging_internal.h
> new file mode 100644
> index 0000000..93c5c4b
> --- /dev/null
> +++ b/source4/lib/messaging/messaging_internal.h
> @@ -0,0 +1,36 @@
> +/*
> + Unix SMB/CIFS implementation.
> +
> + Samba internal messaging functions
> +
> + Copyright (C) Andrew Tridgell 2004
> +
> + 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/>.
> +*/
> +
> +struct imessaging_context {
> + struct imessaging_context *prev, *next;
> + struct tevent_context *ev;
> + struct server_id server_id;
> + const char *sock_dir;
> + const char *lock_dir;
> + struct dispatch_fn **dispatch;
> + uint32_t num_types;
> + struct idr_context *dispatch_tree;
> + struct irpc_list *irpc;
> + struct idr_context *idr;
> + struct server_id_db *names;
> + struct timeval start_time;
> + void *msg_dgm_ref;
> +};
> diff --git a/source4/lib/messaging/pymessaging.c b/source4/lib/messaging/pymessaging.c
> index b317955..84ceff5 100644
> --- a/source4/lib/messaging/pymessaging.c
> +++ b/source4/lib/messaging/pymessaging.c
> @@ -34,6 +34,7 @@
> #include "librpc/rpc/dcerpc.h"
> #include "librpc/gen_ndr/server_id.h"
> #include <pytalloc.h>
> +#include "messaging_internal.h"
>
> void initmessaging(void);
>
> @@ -173,7 +174,8 @@ static void py_msg_callback_wrapper(struct imessaging_context *msg, void *privat
> uint32_t msg_type,
> struct server_id server_id, DATA_BLOB *data)
> {
> - PyObject *py_server_id, *callback = (PyObject *)private_data;
> + PyObject *py_server_id, *callback_and_tuple = (PyObject *)private_data;
> + PyObject *callback, *py_private;
>
> struct server_id *p_server_id = talloc(NULL, struct server_id);
> if (!p_server_id) {
> @@ -182,10 +184,18 @@ static void py_msg_callback_wrapper(struct imessaging_context *msg, void *privat
> }
> *p_server_id = server_id;
>
> + if (!PyArg_ParseTuple(callback_and_tuple, "OO",
> + &callback,
> + &py_private)) {
> + return;
> + }
> +
> py_server_id = py_return_ndr_struct("samba.dcerpc.server_id", "server_id", p_server_id, p_server_id);
> talloc_unlink(NULL, p_server_id);
>
> - PyObject_CallFunction(callback, discard_const_p(char, "i(O)s#"), msg_type,
> + PyObject_CallFunction(callback, discard_const_p(char, "OiOs#"),
> + py_private,
> + msg_type,
> py_server_id,
> data->data, data->length);
> }
> @@ -194,24 +204,30 @@ static PyObject *py_imessaging_register(PyObject *self, PyObject *args, PyObject
> {
> imessaging_Object *iface = (imessaging_Object *)self;
> int msg_type = -1;
> - PyObject *callback;
> + PyObject *callback_and_context;
> NTSTATUS status;
> - const char *kwnames[] = { "callback", "msg_type", NULL };
> + const char *kwnames[] = { "callback_and_context", "msg_type", NULL };
>
> if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|i:register",
> - discard_const_p(char *, kwnames), &callback, &msg_type)) {
> + discard_const_p(char *, kwnames),
> + &callback_and_context, &msg_type)) {
> + return NULL;
> + }
> + if (!PyTuple_Check(callback_and_context)
> + || PyTuple_Size(callback_and_context) != 2) {
> + PyErr_SetString(PyExc_ValueError, "Expected of size 2 for callback_and_context");
> return NULL;
> }
>
> - Py_INCREF(callback);
> + Py_INCREF(callback_and_context);
>
> if (msg_type == -1) {
> uint32_t msg_type32 = msg_type;
> - status = imessaging_register_tmp(iface->msg_ctx, callback,
> + status = imessaging_register_tmp(iface->msg_ctx, callback_and_context,
> py_msg_callback_wrapper, &msg_type32);
> msg_type = msg_type32;
> } else {
> - status = imessaging_register(iface->msg_ctx, callback,
> + status = imessaging_register(iface->msg_ctx, callback_and_context,
> msg_type, py_msg_callback_wrapper);
> }
> if (NT_STATUS_IS_ERR(status)) {
> @@ -241,6 +257,52 @@ static PyObject *py_imessaging_deregister(PyObject *self, PyObject *args, PyObje
> Py_RETURN_NONE;
> }
>
> +static void simple_timer_handler(struct tevent_context *ev,
> + struct tevent_timer *te,
> + struct timeval current_time,
> + void *private_data)
> +{
> + return;
> +}
> +
> +static PyObject *py_imessaging_loop_once(PyObject *self, PyObject *args, PyObject *kwargs)
> +{
> + imessaging_Object *iface = (imessaging_Object *)self;
> + double offset;
> + int seconds;
> + struct timeval next_event;
> + struct tevent_timer *timer = NULL;
> + const char *kwnames[] = { "timeout", NULL };
> +
> + TALLOC_CTX *frame = talloc_stackframe();
> +
> + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "d",
> + discard_const_p(char *, kwnames), &offset)) {
> + TALLOC_FREE(frame);
> + return NULL;
> + }
> +
> + if (offset != 0.0) {
> + seconds = offset;
> + offset -= seconds;
> + next_event = tevent_timeval_current_ofs(seconds, (int)(offset*1000000));
> +
> + timer = tevent_add_timer(iface->msg_ctx->ev, frame, next_event, simple_timer_handler,
> + NULL);
> + if (timer == NULL) {
> + PyErr_NoMemory();
> + TALLOC_FREE(frame);
> + return NULL;
> + }
> + }
> +
> + tevent_loop_once(iface->msg_ctx->ev);
> +
> + TALLOC_FREE(frame);
> +
> + Py_RETURN_NONE;
> +}
> +
> static PyObject *py_irpc_add_name(PyObject *self, PyObject *args, PyObject *kwargs)
> {
> imessaging_Object *iface = (imessaging_Object *)self;
> @@ -374,6 +436,8 @@ static PyMethodDef py_imessaging_methods[] = {
> "S.register(callback, msg_type=None) -> msg_type\nRegister a message handler" },
> { "deregister", (PyCFunction)py_imessaging_deregister, METH_VARARGS|METH_KEYWORDS,
> "S.deregister(callback, msg_type) -> None\nDeregister a message handler" },
> + { "loop_once", (PyCFunction)py_imessaging_loop_once, METH_VARARGS|METH_KEYWORDS,
> + "S.loop_once(timeout) -> None\nLoop on the internal event context until we get an event (which might be a message calling the callback), timeout after timeout seconds (if not 0)" },
> { "irpc_add_name", (PyCFunction)py_irpc_add_name, METH_VARARGS,
> "S.irpc_add_name(name) -> None\nAdd this context to the list of server_id values that are registered for a particular name" },
> { "irpc_remove_name", (PyCFunction)py_irpc_remove_name, METH_VARARGS,
> --
> 2.9.3
>
>
> From 02d35eda70ea0ad8bf097f4dec32470bf55a0e66 Mon Sep 17 00:00:00 2001
> From: Gary Lockyer <gary at catalyst.net.nz>
> Date: Thu, 16 Mar 2017 16:26:01 +1300
> Subject: [PATCH 7/7] pymessaging: add single element tupple form of the
> server_id
>
> This avoids the python code needing to call getpid() internally,
> while declaring a stable task_id.
>
> Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
> ---
> python/samba/tests/messaging.py | 42 +++++++++++++++++++++++++++++++++++++
> source4/lib/messaging/pymessaging.c | 9 +++++++-
> 2 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/python/samba/tests/messaging.py b/python/samba/tests/messaging.py
> index 6ee18e7..e9e6bf1 100644
> --- a/python/samba/tests/messaging.py
> +++ b/python/samba/tests/messaging.py
> @@ -25,6 +25,7 @@ import time
> from samba.ndr import ndr_print
> from samba.dcerpc import server_id
> import random
> +import os
>
> class MessagingTests(TestCase):
>
> @@ -109,3 +110,44 @@ class MessagingTests(TestCase):
>
> self.assertEqual(got_ping["count"], 2)
> self.assertEqual(got_pong["count"], 2)
> +
> + def test_pid_defaulting(self):
> + got_ping = {"count": 0}
> + got_pong = {"count": 0}
> + timeout = False
> +
> + msg_pong = 0
> + msg_ping = 0
> +
> + pid = os.getpid()
> + server_ctx = self.get_context((pid, 1))
> + def ping_callback(got_ping, msg_type, src, data):
> + got_ping["count"] += 1
> + server_ctx.send(src, msg_pong, data)
> +
> + msg_ping = server_ctx.register((ping_callback, got_ping))
> +
> + def pong_callback(got_pong, msg_type, src, data):
> + got_pong["count"] += 1
> +
> + client_ctx = self.get_context((2,))
> + msg_pong = client_ctx.register((pong_callback, got_pong))
> +
> + # Try 1 an two element tuple forms
> + client_ctx.send((pid, 1), msg_ping, "testing")
> +
> + client_ctx.send((1,), msg_ping, "testing2")
> +
> + start_time = time.time()
> +
> + # NOTE WELL: If debugging this with GDB, then the timeout will
> + # fire while you are trying to understand it.
> +
> + while (got_ping["count"] < 2 or got_pong["count"] < 2) and not timeout:
> + client_ctx.loop_once(0.1)
> + server_ctx.loop_once(0.1)
> + if time.time() - start_time > 1:
> + timeout = True
> +
> + self.assertEqual(got_ping["count"], 2)
> + self.assertEqual(got_pong["count"], 2)
> diff --git a/source4/lib/messaging/pymessaging.c b/source4/lib/messaging/pymessaging.c
> index 84ceff5..54920dd 100644
> --- a/source4/lib/messaging/pymessaging.c
> +++ b/source4/lib/messaging/pymessaging.c
> @@ -62,13 +62,20 @@ static bool server_id_from_py(PyObject *object, struct server_id *server_id)
> server_id->task_id = task_id;
> server_id->vnn = vnn;
> return true;
> - } else {
> + } else if (PyTuple_Size(object) == 2) {
> unsigned long long pid;
> int task_id;
> if (!PyArg_ParseTuple(object, "KI", &pid, &task_id))
> return false;
> *server_id = cluster_id(pid, task_id);
> return true;
> + } else {
> + unsigned long long pid = getpid();
> + int task_id;
> + if (!PyArg_ParseTuple(object, "I", &task_id))
> + return false;
> + *server_id = cluster_id(pid, task_id);
> + return true;
> }
> }
>
> --
> 2.9.3
>
More information about the samba-technical
mailing list