[PATCH] lib/ldb minor fixes

[Kamen Mazdrashki]

On Thu, Nov 13, 2014 at 5:43 AM, Andrew Bartlett <abartlet at samba.org> wrote:

> On Wed, 2014-11-12 at 01:28 +0100, Kamen Mazdrashki wrote:
> > Hi Andrew,
> >
> > I have updated the patches according our discussion. See inline.
> > Also, there is one more commit that allows for adding MessageElements
> > from one Message object to another - at the moment such attempts lead
> > to crashes due to "Bad talloc magic value" error
> >
> > As usual, attached is the whole patchset.
> > A pull request for convenient review is here:
> >   https://github.com/kamenim/samba/pull/4
> >
> > On Tue, Nov 11, 2014 at 10:29 AM, Kamen Mazdrashki <kamenim at samba.org>
> > wrote:
> >
> > > Hi Andrew,
> > >
> > > On Tue, Nov 11, 2014 at 9:14 AM, Andrew Bartlett <abartlet at samba.org>
> > > wrote:
> > >
> > >> On Tue, 2014-11-11 at 05:29 +0100, Kamen Mazdrashki wrote:
> > >> > I have update the last patch to this:
> > >> >
> > >>
> https://github.com/kamenim/samba/commit/be1808de8bc5232dd9b827f93ab0d3151653ff88
> > >> > Also attached the whole set to this mail.
> > >>
> > >>
> > >> I still want it to instead check for
> > >>
> > >> if (msg->elements[i].name == NULL)
> > >>
> > >
> > > I have  considered this also.
> > > I have opted out for this check to be done
> > > in ldb_schema_attribute_by_name_internal()
> > > though, as every call for finding attribute by name end up being
> handled
> > > by this function.
> > > Hence the sanity check handles much more code paths.
> > > I see the benefit of such check here though, so we can get a much
> better
> > > error message
> > >
> > >
> > Please take a look at updated patch here:
> >
> >
> https://github.com/kamenim/samba/commit/2966a44cd900f597aed232c963693ab85bd33f4b
> > Let me know if this is what you had in mind too.
>
> 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.

Cheers,
Kamen



>
> The check for name == NULL seems reasonable, particularly if it can get
> that way from the python.
>
> Thanks,
>
> Andrew Bartlett
>
> --
> Andrew Bartlett
> http://samba.org/~abartlet/
> Authentication Developer, Samba Team  http://samba.org
> Samba Developer, Catalyst IT
> http://catalyst.net.nz/services/samba
>
>
>
>
>