[PATCH] lib/ldb minor fixes

[Jelmer Vernooij]

On Sat, Nov 15, 2014 at 03:44:49AM +0100, Kamen Mazdrashki wrote:
> On Fri, Nov 14, 2014 at 10:56 PM, Kamen Mazdrashki <kamenim at samba.org>
> wrote:
> 
> > Hi Andrew,
> >
> > On Fri, Nov 14, 2014 at 8:33 PM, Andrew Bartlett <abartlet at samba.org>
> > wrote:
> >
> >> On Fri, 2014-11-14 at 04:16 +0100, Kamen Mazdrashki wrote:
> >> > Hi Andrew,
> >> >
> >> > On Fri, Nov 14, 2014 at 2:35 AM, Kamen Mazdrashki <kamenim at samba.org>
> >> wrote:
> >> >
> >> > >
> >> > >
> >> > > On Thu, Nov 13, 2014 at 5:43 AM, Andrew Bartlett <abartlet at samba.org>
> >> > > wrote:
> >> > >
> >> > >>
> >> > >> I still don't like the check for a == NULL at:
> >> > >>
> >> > >>
> >> https://github.com/kamenim/samba/commit/2966a44cd900f597aed232c963693ab85bd33f4b#diff-d7e2445ee96f1098ad238e62cf47cb92R323
> >> > >>
> >> > >> The reason I don't like it is that this function is designed not to
> >> > >> fail, and if we start checking here, for a condition that can't
> >> happen,
> >> > >> then we will start checking everywhere, as folks work to make it
> >> > >> consistent.
> >> > >>
> >> > >
> >> > >
> >> > > Well, I have changed it to fall explicitely in case 'name == null'
> >> :). See:
> >> > >
> >> > >
> >> https://github.com/kamenim/samba/commit/194658167d65f0f96e7150b1f5e5652175830999#diff-a410d07b1af338be3b0b3d1c3d62622aR131
> >> > >
> >> > > I didn't know "no fail" is the intended design as half of the calls to
> >> > > this function
> >> > > actually check the return value.
> >> > > I will change then
> >> > >
> >> > >
> >> https://github.com/kamenim/samba/commit/194658167d65f0f96e7150b1f5e5652175830999#diff-a410d07b1af338be3b0b3d1c3d62622aR131
> >> > > to always succeed and remove the check that bothers you. It is not
> >> needed
> >> > > indeed.
> >> > >
> >> >
> >> > I have updated the branch (PR): https://github.com/kamenim/samba/pull/4
> >> >
> >> > Reworked commits are:
> >> >
> >> https://github.com/kamenim/samba/commit/dd677c73377eae6d3f317c97e7adccfcf85cbaa8
> >> >
> >> https://github.com/kamenim/samba/commit/888a764d3045bca86a8143fe3f0d2135628fb149
> >>
> >> Thanks.  I've been trying to figure out why I didn't quite feel right
> >> about this patch.  Belts and braces are an OK strategy, and can avoid
> >> crashes after an unchecked allocation failure elsewhere, but shouldn't
> >> we be checking for name == NULL in the python LDB bindings?
> >>
> >> Yes, we should do that also. My plan was to fix the root implementation -
> > more or less
> > ment as a "one fix to rule them all" and make sure this won't left hidden
> > anymore.
> > At the same time, my primary goal wasn't to fix param checks everywhere
> > (i.e. fix the root and continue with what I was doing already)
> >
> > I can take a look for other uses of course too
> >
> > Look taken.
> I can't find out how we could avoid name == NULL in python bindings.
> PyLdbMessageElement by design allows optional parameters in constructor
> (name = NULL)
> Honestly said, I don't see what an empty PyLdbMessageElement could be used
> for
> (object interface is close to readonly as far as I can see)
> 
> The only optoin I could see here is to change PyLdbMessageElement
> constructor to require
> name and elements to be non-optional.
> This may affect lots of code though, because making 'elements' and 'name'
> non-optional
> and leaving 'flags' optional will require change in interface (and tests
> and so on)
> 
> <<including Jelmer as author for this code>>
This is not one of the classes I am most proud of. :-) The order of the
arguments as non-keyword arguments is kindof backwards. The main reason
'name' is last is so that it can be optional when you do dictionary-style assignments, e.g.:

msg["foo"] = MessageElement([1,2,3], ldb.MOD_MODIFY)

For a while, I've been wanting to get rid of PyLdbMessageElement completely,
and just allowing the user to use tuples, e.g.:

msg["foo"] = ([1,2,3], ldb.MOD_MODIFY)
msg.add("foo", [1,2,3], ldb.MOD_MODIFY)

If you'd be interested in working on that, I'd happily review code.

For the example mentioned above, I think it would make sense to allow name=NULL
in a very select number of cases. By the time we call one of the ldb APIs and 
pass in a ldb_message, the name should be filled in though. We could add a
check to see if there are any message elements that have name set to NULL before we
pass ldb messages on to the C LDB API.

Cheers,

Jelmer