[PATCH] Extending pyldb
Jelmer Vernooij
jelmer at samba.org
Thu Nov 18 05:24:41 MST 2010
Hi Kamen,
On Thu, 2010-11-18 at 14:06 +0200, Kamen Mazdrashki wrote:
> On Thu, Nov 18, 2010 at 12:36, Jelmer Vernooij <jelmer at samba.org> wrote:
> > 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 :)
:-)
> > 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.
I think the conversion functions should always set an error. If they
don't then that's a bug.
> 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.
I'm not too sure. PyErr_Occurred has caused issues for me in the past,
when some part of the code (which called my function) was also setting
an exception but not returning. This can cause really hard to track down
bugs (and might be related to the weird traceback you were seeing
earlier).
Either way, as long as you can avoid PyErr_Occurred() and not access any
uninitialized memory I'm happy. :-)
> > + 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.
Ah, ok. So that one was actually my fault. :-) Yeah, this can definitely
go. It would only happen in the case of programmer error.
> > + 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 :)
Let's stick to LDB_FLAG_MOD for the moment. It would be really odd if we
were inconsistent with the rest of LDB which uses those flags for
modifies.
> > + { "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(.."?)
I think Message.from_dict() is appropriate here since it is a class
method and not an instance.
> >> 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!
I wonder if this is caused by an error that doesn't get cleared properly
and so shows up because you use PyErr_Occurred. Other than that, I have
no idea what could be the cause here.
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/db74589e/attachment.pgp>
More information about the samba-technical
mailing list