[SCM] Samba Shared Repository - branch master updated

[Matthias Dieter Wallnöfer]

Hi Jelmer,

Jelmer Vernooij wrote:
> On Sun, 2010-12-12 at 20:51 +0100, Matthias Dieter Wallnöfer wrote:
>    
>> @@ -163,6 +165,8 @@ static PyObject *py_samdb_set_ntds_settings_dn(PyLdbObject *self, PyObject *args
>>   	}
>>
>>   	if (!PyObject_AsDn(tmp_ctx, py_ntds_settings_dn, ldb,&ntds_settings_dn)) {
>> +		PyErr_NoMemory();
>> +		talloc_free(tmp_ctx);
>>   		return NULL;
>>   	}
>>      
> This is incorrect, PyObject_AsDn will already set an exception itself.
> The fact that it fails is not necessarily an indication of an out of
> memory error.
>    
ah - sorry.
>> @@ -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.
>> 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.
>> diff --git a/source4/lib/ldb/pyldb.c b/source4/lib/ldb/pyldb.c
>> index 3bee9ab..44a006f 100644
>> --- a/source4/lib/ldb/pyldb.c
>> +++ b/source4/lib/ldb/pyldb.c
>> @@ -26,9 +26,6 @@
>>      License along with this library; if not, see<http://www.gnu.org/licenses/>.
>>   */
>>
>> -#include<Python.h>
>> -#include "replace.h"
>> -#include "ldb_private.h"
>>   #include "pyldb.h"
>>
>>   /* There's no Py_ssize_t in 2.4, apparently */
>>      
> Same here.
>
>    
>> diff --git a/source4/lib/ldb/pyldb.h b/source4/lib/ldb/pyldb.h
>> index 1f4bdf7..afc8c51 100644
>> --- a/source4/lib/ldb/pyldb.h
>> +++ b/source4/lib/ldb/pyldb.h
>> @@ -28,6 +28,7 @@
>>
>>   #include<Python.h>
>>   #include<talloc.h>
>> +#include "ldb_private.h"
>>
>>   typedef struct {
>>   	PyObject_HEAD
>>      
> ^^^ We can't include ldb_private.h here, it's not installed so this will
> break system installs of pyldb.
>    
Oh sorry - that wasn't clear. I will change this.
>> 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.
>    
>> 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.

Cheers,
Matthias