Hi Matthias,
On Sun, 2010-12-12 at 22:22 +0100, Matthias Dieter Wallnöfer wrote:
> Jelmer Vernooij wrote:
> > On Sun, 2010-12-12 at 21:29 +0100, Matthias Dieter Wallnöfer wrote:
> >
> >> Jelmer Vernooij wrote:
> >>
> >>> On Sun, 2010-12-12 at 20:51 +0100, Matthias Dieter Wallnöfer wrote:
> >>>
> >>>> @@ -248,27 +253,19 @@ static PyObject
> >>>> *py_dsdb_get_oid_from_attid(PyObject *self, PyObject *args)
> >>>>
> >>>> PyErr_LDB_OR_RAISE(py_ldb, ldb);
> >>>>
> >>>> - mem_ctx = talloc_new(NULL);
> >>>> - if (mem_ctx == NULL) {
> >>>> - PyErr_NoMemory();
> >>>> - return NULL;
> >>>> - }
> >>>> -
> >>>> schema = dsdb_get_schema(ldb, NULL);
> >>>>
> >>>> if (!schema) {
> >>>> PyErr_SetString(PyExc_RuntimeError, "Failed to find a
> >>>> schema from ldb \n");
> >>>> - talloc_free(mem_ctx);
> >>>> return NULL;
> >>>> }
> >>>>
> >>>> status = dsdb_schema_pfm_oid_from_attid(schema->prefixmap,
> >>>> attid,
> >>>> - mem_ctx,&oid);
> >>>> + NULL,&oid);
> >>>> PyErr_WERROR_IS_ERR_RAISE(status);
> >>>>
> >>>> ret = PyString_FromString(oid);
> >>>> -
> >>>> - talloc_free(mem_ctx);
> >>>> + talloc_free(discard_const_p(char, oid));
> >>>>
> >>>>
> >>> ^^ Is this really necessary? I'd rather have the extra memory context
> >>> than add an extra discard_const_p.
> >>>
> >>>
> >> I've really thought hard about this change - but it seems more correct
> >> to me.
> >> The problem is that the memory context isn't freed when
> >> PyERR_WERROR_IS_ERR_RAISE raises an exception.
> >>
> > I suspect you mean PyErr_LDB_OR_RAISE?
> >
> > That macro never does a return at the moment, it's just a stub for "ldb
> > = PyLdb_AsLdbContext". Even if it did return, I think we should just
> > avoid using it, and manually check whether py_ldb is a ldb handle and
> > talloc_free and return if it isn't.
> >
> No Jelmer, I mean the macro below "dsdb_schema_pfm_oid_from_attid". When
> that one raises an exception, what does succeed to "tmp_ctx"? I imagine
> that it never will be freed - therefore I've provided this fix.
The same goes for that macro - if it doesn't deal with proper free'ing,
then why not avoid it rather than rewrite the rest of the function that
uses it?discard_const_p is bad, and we should avoid it unless we really can. > >>>> diff --git a/source4/lib/ldb/pyldb_util.c b/source4/lib/ldb/pyldb_util.c > >>>> index 3e015d0..35071f3 100644 > >>>> --- a/source4/lib/ldb/pyldb_util.c > >>>> +++ b/source4/lib/ldb/pyldb_util.c > >>>> @@ -23,10 +23,7 @@ > >>>> License along with this library; if not, > >>>> see<http://www.gnu.org/licenses/>. > >>>> */ > >>>> > >>>> -#include<Python.h> > >>>> -#include "replace.h" > >>>> #include "pyldb.h" > >>>> -#include<ldb.h> > >>>> > >>>> static PyObject *ldb_module = NULL; > >>>> > >>>> > >>> See above. Python.h is included for a reason. Also, replace.h might not > >>> be necessary on your system but necessary on others (as some > >>> functionality is not provided by the OS). > >>> > >> But let me think - we include system headers only by libreplace. So at > >> the end it doesn't matter if replace or a system header defines it. > >> Obviously there we have no need for a system call - otherwise the build > >> would have broken. > >> > > replace.h isn't the only way in which we get system headers, e.g. > > Python.h also includes a bunch - at least stdlib.h, unistd.h, stddef.h, > > string.h, stdio.h, limits.h and assert.h. > > > In this special case we don't need the "replace.h" anymore since I do > now include "ldb_private.h" (which itself includes it). Is there any particular reason why pyldb_util.c requires ldb_private.h ? We should avoid including it if we can (and thus avoid tying pyldb_util to a specific version of ldb). > PS: Could you please start the implementation of the other message > attribute-mapping function in pyldb? Sorry, that's on my todo list as are several other things. I'd be happy to review a patch that adds it though. Cheers, Jelmer
signature.asc
Description: This is a digitally signed message part
