[PATCH] Messaging improvements and fixes needed for auth logging
Andrew Bartlett
abartlet at samba.org
Mon Mar 20 22:59:45 UTC 2017
On Mon, 2017-03-20 at 15:08 -0700, Jeremy Allison wrote:
> 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.
I'll amend the commit messages, but the patches in the series I posted
here are (essentially) the unit tests for the bug in the server_id
database, because it was adding the tests for the bindings that found
the bug.
> 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 !
Thanks.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12705
> Cheers,
>
> Jeremy.
>
> > --
> > Andrew Bartlett http://samba.org/~abartlet/
> > Authentication Developer, Samba Team http://samba.org
> > Samba Developer, Catalyst IT http://catalyst.net.nz/servic
> > es/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_a
> > dd_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_a
> > dd_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/licen
> > ses/>.
> > +*/
> > +
> > +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_wr
> > apper, &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