Hi Matthias, As discussed earlier, can you please send this kind of patch to the list for review first?
I have my doubts about the usefulness of these changes in general
compared to the benefit. Eliminating talloc contexts doesn't really gain
us much. The overhead of creating an extra talloc context compared to
the overhead of using Python in the first place is negligible.
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.
> @@ -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.
> 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?
> 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.
> 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).
> 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.
Cheers,
Jelmer
signature.asc
Description: This is a digitally signed message part
