[PATCH 0/5] tdb trivia

[Kirill Smelkov]

While studying TDB (thanks for it, btw!) I've found some trivial bits to fix.
Please apply.

Kirill Smelkov (5):
  tdb: kill last bits from swig
  tdb: fix typo in python's Tdb.get() docstring
  tdb: reset tdb->fd to -1 in tdb_close()
  tdb: add tests for double .close() in pytdb
  tdb: update README a bit

 lib/tdb/common/open.c          |    4 +++-
 lib/tdb/docs/README            |    9 +--------
 lib/tdb/pytdb.c                |    4 ++--
 lib/tdb/python/tests/simple.py |    9 +++++++++
 lib/tdb/rules.mk               |    5 -----
 5 files changed, 15 insertions(+), 16 deletions(-)

[Kirill Smelkov]

We no longer use swig for pytdb, so there is no need for swig make
rules. Also pytdb.c header should be updated.

Signed-off-by: Kirill Smelkov <kirr@...>
---
 lib/tdb/pytdb.c  |    2 +-
 lib/tdb/rules.mk |    5 -----
 2 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/lib/tdb/pytdb.c b/lib/tdb/pytdb.c
index 159bc4d..0d63391 100644
--- a/lib/tdb/pytdb.c
+++ b/lib/tdb/pytdb.c
@@ -1,7 +1,7 @@
 /*
    Unix SMB/CIFS implementation.
 
-   Swig interface to tdb.
+   Python interface to tdb.
 
    Copyright (C) 2004-2006 Tim Potter <tpot@...>
    Copyright (C) 2007-2008 Jelmer Vernooij <jelmer@...>
diff --git a/lib/tdb/rules.mk b/lib/tdb/rules.mk
index 73ab771..023e0ce 100644
--- a/lib/tdb/rules.mk
+++ b/lib/tdb/rules.mk
@@ -1,8 +1,3 @@
-.SUFFIXES: .i _wrap.c
-
-.i_wrap.c:
- $(SWIG) -O -Wall -python -keyword $<
-
 showflags::
  @echo 'tdb will be compiled with flags:'
  @echo '  CFLAGS = $(CFLAGS)'
--
1.6.5.1.63.ga9d7

[Kirill Smelkov]

It's Tdb.get(), not Tdb.fetch().

Signed-off-by: Kirill Smelkov <kirr@...>
---
 lib/tdb/pytdb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/tdb/pytdb.c b/lib/tdb/pytdb.c
index 0d63391..202dca1 100644
--- a/lib/tdb/pytdb.c
+++ b/lib/tdb/pytdb.c
@@ -337,7 +337,7 @@ static PyMethodDef tdb_object_methods[] = {
  { "read_lock_all", (PyCFunction)obj_lockall_read, METH_NOARGS, NULL },
  { "read_unlock_all", (PyCFunction)obj_unlockall_read, METH_NOARGS, NULL },
  { "close", (PyCFunction)obj_close, METH_NOARGS, NULL },
- { "get", (PyCFunction)obj_get, METH_VARARGS, "S.fetch(key) -> value\n"
+ { "get", (PyCFunction)obj_get, METH_VARARGS, "S.get(key) -> value\n"
  "Fetch a value." },
  { "append", (PyCFunction)obj_append, METH_VARARGS, "S.append(key, value) -> None\n"
  "Append data to an existing key." },
--
1.6.5.1.63.ga9d7

[Kirill Smelkov]

So that erroneous double tdb_close() calls do not try to close() same
fd again. This is like SAFE_FREE() but for fd.

Signed-off-by: Kirill Smelkov <kirr@...>
---
 lib/tdb/common/open.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lib/tdb/common/open.c b/lib/tdb/common/open.c
index 1ba2e7b..64efafe 100644
--- a/lib/tdb/common/open.c
+++ b/lib/tdb/common/open.c
@@ -403,8 +403,10 @@ int tdb_close(struct tdb_context *tdb)
  tdb_munmap(tdb);
  }
  SAFE_FREE(tdb->name);
- if (tdb->fd != -1)
+ if (tdb->fd != -1) {
  ret = close(tdb->fd);
+ tdb->fd = -1;
+ }
  SAFE_FREE(tdb->lockrecs);
 
  /* Remove from contexts list */
--
1.6.5.1.63.ga9d7

[Kirill Smelkov]

The reason I do it is that when using older python-tdb as shipped in
Debian Lenny, python interpreter crashes on this test:

    (gdb) bt
    #0  0xb7f8c424 in __kernel_vsyscall ()
    #1  0xb7df5640 in raise () from /lib/i686/cmov/libc.so.6
    #2  0xb7df7018 in abort () from /lib/i686/cmov/libc.so.6
    #3  0xb7e3234d in __libc_message () from /lib/i686/cmov/libc.so.6
    #4  0xb7e38624 in malloc_printerr () from /lib/i686/cmov/libc.so.6
    #5  0xb7e3a826 in free () from /lib/i686/cmov/libc.so.6
    #6  0xb7b39c84 in tdb_close () from /usr/lib/libtdb.so.1
    #7  0xb7b43e14 in ?? () from /var/lib/python-support/python2.5/_tdb.so
    #8  0x0a038d08 in ?? ()
    #9  0x00000000 in ?? ()

master's pytdb does not (we have a check for self->closed in obj_close()),
but still...

Signed-off-by: Kirill Smelkov <kirr@...>
---
 lib/tdb/python/tests/simple.py |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/lib/tdb/python/tests/simple.py b/lib/tdb/python/tests/simple.py
index d242e66..c7443c0 100644
--- a/lib/tdb/python/tests/simple.py
+++ b/lib/tdb/python/tests/simple.py
@@ -15,6 +15,15 @@ class OpenTdbTests(TestCase):
     def test_nonexistant_read(self):
         self.assertRaises(IOError, tdb.Tdb, "/some/nonexistant/file", 0, tdb.DEFAULT, os.O_RDWR)
 
+class CloseTdbTests(TestCase):
+    def test_double_close(self):
+        self.tdb = tdb.Tdb(tempfile.mkstemp()[1], 0, tdb.DEFAULT, os.O_CREAT|os.O_RDWR)
+        self.assertNotEqual(None, self.tdb)
+
+        # ensure that double close does not crash python
+        self.tdb.close()
+        self.tdb.close()
+
 
 class SimpleTdbTests(TestCase):
     def setUp(self):
--
1.6.5.1.63.ga9d7

[Kirill Smelkov]

While studying tdb, I've noticed a couple of mismatches between readme
and actual code:

- tdb_open_ex changed it's log_fn argument to log_ctx
- there is now no tdb_update(), which it seems was transformed into
  non-exported tdb_update_hash()

There were other mismatches, but I don't remember them now, sorry.

Signed-off-by: Kirill Smelkov <kirr@...>
---
 lib/tdb/docs/README |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/lib/tdb/docs/README b/lib/tdb/docs/README
index 4eb809f..7bf4854 100644
--- a/lib/tdb/docs/README
+++ b/lib/tdb/docs/README
@@ -73,7 +73,7 @@ TDB_CONTEXT *tdb_open(char *name, int hash_size, int tdb_flags,
 ----------------------------------------------------------------------
 TDB_CONTEXT *tdb_open_ex(char *name, int hash_size, int tdb_flags,
          int open_flags, mode_t mode,
- tdb_log_func log_fn,
+ const struct tdb_logging_context *log_ctx,
  tdb_hash_func hash_fn)
 
 This is like tdb_open(), but allows you to pass an initial logging and
@@ -93,13 +93,6 @@ int tdb_close(TDB_CONTEXT *tdb);
    close a database
 
 ----------------------------------------------------------------------
-int tdb_update(TDB_CONTEXT *tdb, TDB_DATA key, TDB_DATA dbuf);
-
-   update an entry in place - this only works if the new data size
-   is <= the old data size and the key exists.
-   on failure return -1
-
-----------------------------------------------------------------------
 TDB_DATA tdb_fetch(TDB_CONTEXT *tdb, TDB_DATA key);
 
    fetch an entry in the database given a key
--
1.6.5.1.63.ga9d7

[Rusty Russell]

On Thu, 22 Oct 2009 03:48:56 am Kirill Smelkov wrote:
> So that erroneous double tdb_close() calls do not try to close() same
> fd again. This is like SAFE_FREE() but for fd.

Hi Kirill,

   These patches look good.  This one of course doesn't allow multiple
tdb_close(), since it frees the tdb context anyway.  But the extra paranoia
can't hurt.

Thanks,
Rusty.

[Kirill Smelkov]

On Mon, Oct 26, 2009 at 01:41:20PM +1030, Rusty Russell wrote:
> On Thu, 22 Oct 2009 03:48:56 am Kirill Smelkov wrote:
> > So that erroneous double tdb_close() calls do not try to close() same
> > fd again. This is like SAFE_FREE() but for fd.
>
> Hi Kirill,
>
>    These patches look good.  This one of course doesn't allow multiple
> tdb_close(), since it frees the tdb context anyway.  But the extra paranoia
> can't hurt.

Rusty, thanks for your reply.

I see these patches are still not merged in, so how does one then
actually get patches applied to samba.git?

Maybe I need to somehow rework them?


Thanks,
Kirill

[Rusty Russell]

On Thu, 29 Oct 2009 01:06:06 am Kirill Smelkov wrote:

> On Mon, Oct 26, 2009 at 01:41:20PM +1030, Rusty Russell wrote:
> > On Thu, 22 Oct 2009 03:48:56 am Kirill Smelkov wrote:
> > > So that erroneous double tdb_close() calls do not try to close() same
> > > fd again. This is like SAFE_FREE() but for fd.
> >
> > Hi Kirill,
> >
> >    These patches look good.  This one of course doesn't allow multiple
> > tdb_close(), since it frees the tdb context anyway.  But the extra paranoia
> > can't hurt.
>
> Rusty, thanks for your reply.
>
> I see these patches are still not merged in, so how does one then
> actually get patches applied to samba.git?
>
> Maybe I need to somehow rework them?

No, your patches were fine!  I just needed to commit them, check it was all OK
and push.  Done now.

Thanks for the excellent series!
Rusty.

[Kirill Smelkov]

On Thu, Oct 29, 2009 at 10:15:49AM +1030, Rusty Russell wrote:

> On Thu, 29 Oct 2009 01:06:06 am Kirill Smelkov wrote:
> > On Mon, Oct 26, 2009 at 01:41:20PM +1030, Rusty Russell wrote:
> > > On Thu, 22 Oct 2009 03:48:56 am Kirill Smelkov wrote:
> > > > So that erroneous double tdb_close() calls do not try to close() same
> > > > fd again. This is like SAFE_FREE() but for fd.
> > >
> > > Hi Kirill,
> > >
> > >    These patches look good.  This one of course doesn't allow multiple
> > > tdb_close(), since it frees the tdb context anyway.  But the extra paranoia
> > > can't hurt.
> >
> > Rusty, thanks for your reply.
> >
> > I see these patches are still not merged in, so how does one then
> > actually get patches applied to samba.git?
> >
> > Maybe I need to somehow rework them?
>
> No, your patches were fine!  I just needed to commit them, check it was all OK
> and push.  Done now.

Ah, I see, thanks.

> Thanks for the excellent series!

Come on, Rusty, it's just trivia :)

Thanks again,
Kirill