[PATCH] Extending pyldb

[Jelmer Vernooij]

Hi Kamen,

On Thu, 2010-11-18 at 01:48 +0200, Kamen Mazdrashki wrote:
> Could you please take a look at this branch:
> http://git.samba.org/?p=kamenim/samba.git;a=shortlog;h=refs/heads/py-review-build
Are you proposing the eclipse/pydev file for inclusion as well? That
patch contains references to /home/kamenin, which probably won't work
for other people :-)

In general, having extra data like this in the tree shouldn't be an
issue but if it's outdated it can be more harmful than if it wasn't
present. 

> I think I've covered all notes of yours (and a test) :)
> Except for throwing exception in ldb.modify() in case we have MessageElements()
> with no flags (flags = 0). I've avoided this as we have code in many places that
> looks like:
> 	m = ldb.Message('some dn')
> 	m['foo'] = 'bar'
> 	ldb_ctx.modify(m)
> This code will hit such an exception right away.
I think that's fine. we can use 
m['foo'] = ('bar', ldb.MOD_REPLACE)

which is clearer too. We can leave that for another time to fix though.

in 118d41e0eaba7ebf66224841b6234f4d06a9a13c the error checking is wrong.
If py_msg is neither a dict nor a ldb.Message then msg stays
uninitialised (rather than NULL). 

Perhaps just add an else clause that raises that exception if converting
from a dict or a message failed, rather than relying on msg being NULL ?
This would also mean we don't have to rely on PyErr_Occurred().

If you do this, you'll still need to return immediately if msg is NULL
of course.

+               if (msg->dn == NULL) {
+                       PyErr_SetString(PyExc_TypeError, "dn set but not
found");
+                       return NULL;
+               }
This shouldn't raise an exception but rather be an assert(). This should
never happen and would be a grave bug in PyObject_AsDn().

+       if (change_type == LDB_CHANGETYPE_MODIFY) {
+               flags = LDB_FLAG_MOD_REPLACE;
+       } else if (change_type == LDB_CHANGETYPE_DELETE) {
+               flags = LDB_FLAG_MOD_DELETE;
+       } else {
+               PyErr_SetString(PyExc_ValueError,
+                               "CHANGETYPE_MODIFY or CHANGETYPE_DELETE
"
+                               "expected as change_type");
+               return NULL;
+       }
This is wrong. LDB_CHANGETYPE_MODIFY and LDB_CHANGETYPE_DELETE aren't
relevant here. We should just rely on LDB_FLAG_MOD_*.

+       { "from_dict", (PyCFunction)py_ldb_msg_from_dict, METH_CLASS |
METH_VARARGS,
+               "ldb.Message.from_dict(ldb, dict, change_type) ->
ldb.Message\n"
+               "Class method to create ldb.Message object from
Dictionary.\n"
+               "change_type is one of CHANGETYPE_MODIFY or
CHANGETYPE_DELETE.\n"
+               "change_type defaults to CHANGETYPE_MODIFY"},
Consistent with our other documentation, please avoid including the
"ldb." bit. 

+       /* set default flags */
+       for (i = 0; i < msg->num_elements; i++) {
+               msg->elements[i].flags = flags;
+       }
It seems more sensible to pass the default flag to PyDict_AsMessage().
That leaves the option of e.g. allowing people to pass in tuples with
the change type. 

Nice work on the test.

There shouldn't be a need for a global when obtaining the temporary
directory. Getting something out of the environment is not that
expensive, and certainly not worth the extra complexity in a function
that only gets run a couple dozen times during a full test run.

Also, EAFP. Perhaps something along these lines:
def filename():
  try:
      tmp_prefix = os.path.join(os.environ["SELFTEST_PREFIX"], "tmp")
  except KeyError:
      tmp_prefix = None
  return os.tempname(tmp_prefix)

> Btw, ldb.python API test suite behaves a little bit strange.
> I am running it like this: make test TESTS='ldb.python'
> 
> It passes everything OK, but I have following in my st/subunit file:
> 	test: ldb.python.api.ModuleTests.test_register_module
> 	successful: ldb.python.api.ModuleTests.test_register_module
> 	test: ldb.python.api.ModuleTests.test_use_module
> 	successful: ldb.python.api.ModuleTests.test_use_module
> 	Traceback (most recent call last):
> 	  File "/home/kamenim/work/samba-drs/source4/scripting/bin/subunitrun",
> line 44, in <module>
> 		program = TestProgram(module=None, argv=[sys.argv[0]] + args,
> testRunner=runner)
> 	  File "/usr/lib/python2.6/unittest.py", line 817, in __init__
> 		self.runTests()
> 	  File "/usr/lib/python2.6/unittest.py", line 864, in runTests
> 		result = testRunner.run(self.test)
> 	  File "bin/python/samba/../../../../lib/subunit/python/subunit/run.py",
> line 42, in run
> 		test(result)
> 	  File "/usr/lib/python2.6/unittest.py", line 464, in __call__
> 		return self.run(*args, **kwds)
> 	  File "/usr/lib/python2.6/unittest.py", line 460, in run
> 		test(result)
> 	  File "/usr/lib/python2.6/unittest.py", line 464, in __call__
> 		return self.run(*args, **kwds)
> 	  File "/usr/lib/python2.6/unittest.py", line 460, in run
> 		test(result)
> 	  File "/usr/lib/python2.6/unittest.py", line 464, in __call__
> 		return self.run(*args, **kwds)
> 	  File "/usr/lib/python2.6/unittest.py", line 457, in run
> 		for test in self._tests:
> 	AttributeError: request
> 
> If I delete "class ModuleTests" test case though, I am getting:
> 	test: ldb.python.api.SimpleLdb.test_modify_delete
> 	error: ldb.python.api.SimpleLdb.test_modify_delete [
> 	_StringException: _StringException: Traceback (most recent call last):
> 	  File "/home/kamenim/work/samba-drs/source4/lib/ldb/tests/python/api.py",
> line 227, in test_modify_delete
> 		rm = l.search(m.dn, attrs=["bla"])[0]
> 	IndexError: list index out of range
> 
> 
> 	]
> But not the error I am getting in a normal case (with class ModuleTests).
> Any ideas?
I'm not sure about this, perhaps it's related to the issues I pointed
out above?

Cheers,

Jelmer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20101118/24d775e0/attachment.pgp>