[SCM] Samba Shared Repository - branch master updated

[Jelmer Vernooij]

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
-------------- 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/20101212/fad96aa3/attachment.pgp>