[SCM] Samba Shared Repository - branch master updated

[Jelmer Vernooij]

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. 

> >> 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.

> >> 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.

> >> 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.

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