[SCM] Samba Shared Repository - branch master updated

[Matthias Dieter Wallnöfer]

Hi Jelmer,

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.
>    
>>>> diff --git a/source4/lib/ldb-samba/pyldb.c b/source4/lib/ldb-samba/pyldb.c
>>>> index e8cdb90..f198d74 100644
>>>> --- a/source4/lib/ldb-samba/pyldb.c
>>>> +++ b/source4/lib/ldb-samba/pyldb.c
>>>> @@ -19,10 +19,8 @@
>>>>       License along with this library; if not, see<http://www.gnu.org/licenses/>.
>>>>    */
>>>>
>>>> -#include<Python.h>
>>>> -#include "includes.h"
>>>> -#include<ldb.h>
>>>>    #include "lib/ldb/pyldb.h"
>>>> +#include "includes.h"
>>>>    #include "param/pyparam.h"
>>>>    #include "auth/credentials/pycredentials.h"
>>>>    #include "ldb_wrap.h"
>>>>
>>>>          
>>> Can you please stop reordering include files? There's a good reason
>>> Python.h is included first, it prevents warnings on some systems.
>>>
>>> What is the benefit of this sort of reordering?
>>>
>>>        
>> "pyldb.h" includes"<Python.h>" - therefore I had to do the reordering.
>>      
> But why did you have to change these include lines at all? I can
> understand removing #include lines that are not necessary but there's no
> need to use the smallest subset of include lines. If an include file was
> already processed earlier then the overhead of including it again is
> minimal.
>    
Sorry, this was a mistake - the previous behaviour was better. I will 
push a revert fix.
>    
>>>> 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).
>    
>>>> diff --git a/source4/libnet/py_net.c b/source4/libnet/py_net.c
>>>> index 9775e24..28dee59 100644
>>>> --- a/source4/libnet/py_net.c
>>>> +++ b/source4/libnet/py_net.c
>>>> @@ -18,19 +18,15 @@
>>>>       along with this program.  If not, see<http://www.gnu.org/licenses/>.
>>>>    */
>>>>
>>>> -#include<Python.h>
>>>> +#include "lib/ldb/pyldb.h"
>>>>
>>>>          
>>> ^^ We shouldn't include lib/ldb/pyldb.h directly, but always<pyldb.h>
>>> in case the system pyldb is being used.
>>>        
>> Could you point out a rule when to use "lib/ldb/pyldb.h" or<pyldb.h>?
>> This isn't clear.
>>      
> Always use<pyldb.h>, except in lib/ldb/ itself.
>    
Okay, I will fix this.

PS: Could you please start the implementation of the other message 
attribute-mapping function in pyldb?

Cheers,
Matthias