[PATCH] Extending pyldb

Kamen Mazdrashki kamenim at samba.org
Thu Nov 18 05:06:23 MST 2010

Hi Jelmer,

On Thu, Nov 18, 2010 at 12:36, Jelmer Vernooij <jelmer at samba.org> wrote:
> 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 :-)
Actually I was hoping for this commit to sneak in, so that people will need to
have user 'kamenim' in order to be able to use my super-duper patch :)
Now on the serious side -> it is just my marker commit to say
"all my miserable attempts to write something goes after this one" :)
I will do my best to avoid sending this one for review anymore 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).
Oups, my bad. Thanks for noticing!

> 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.
I prefer to just pre-initialize msg = NULL.
The idea was to raise "unknown type" error in case one of the conversion
functions didn't set a more specific error.
Of course PyLdbMessage_AsMessage() can't fail currently (except for segfault),
but if we happen to change it in the future, then such an error
handling will still
work as it is.
Basically I assume, that if msg == NULL and no fine-grained error has been set,
then we have an unexpected type passed - I think this is reasonable.

> +               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().
Ah, yes. I just copy&paste previous implementation of Dict-to-Message
and I intend to refactor it a little bit more later.
Thanks for clarifying this one - I was wondering why this check is there, as it
seems as "not possible to happen" situations (assuming that PyObject_AsDn()
works of course) :)

> +       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;
> +       }
> relevant here. We should just rely on LDB_FLAG_MOD_*.
Hm. Yes and no. I've implemented it the way you suggest at first. But then took
a look at what the whole pyldb interface looks like.
More specifically -> what we are going to use this Message.from_dict() method
in the future.
And it seems to me, that we are going to use it to get rid of *_ldif()
type of calls.
So, when a *_ldif() code is to be refactored, we will end up mapping an LDIF
to a Dictionary right?
Now, in LDIFs we have 'changetype' and I thought it will more natural to
whoever is to refactor those calls to simply map LDIF's changetype to
one of ldb.CHANGETYPE_* types.
This is how I think about this. But I won't be the one to refactor LDIF
calls. So if you think using FLAG_MOD_* values will be more convenient,
I will be happy to change it back to using MessageElement flags directly.
It is all about convenience :)

> +       { "from_dict", (PyCFunction)py_ldb_msg_from_dict, METH_CLASS |
> +               "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
> +               "change_type defaults to CHANGETYPE_MODIFY"},
> Consistent with our other documentation, please avoid including the
> "ldb." bit.
Will fix it right away, thanks!
(btw, should I at least use "Message.from_dict(..." or just "S.from_dict(.."?)

> +       /* 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 idea! Thanks

> 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)
Great :)
I don't like the global var neither, just wanted to avoid dictionary searches
But you are totally right, they are fast.

>> 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?
Unfortunately not :(
I am trying this on a clean master.
On clean master, "ldb.pythong" test passes, but if you peek into st/subunit
you will find this "AttributeError: request" exception. And it is not
related to
any of tests in the suite. Very strange!


More information about the samba-technical mailing list