[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
conversion,
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;
> + }
> This is wrong. LDB_CHANGETYPE_MODIFY and LDB_CHANGETYPE_DELETE aren't
> 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 |
> 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.
>
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.
Thanks!
>> 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!
--
CU,
Kamen
More information about the samba-technical
mailing list