[LDB][PATCH] Make LDB fail on invalid baseDN

[simo]

On Mon, 2007-11-12 at 08:40 +1100, Andrew Bartlett wrote:
> On Sun, 2007-11-11 at 10:07 -0500, simo wrote:

> I clarified the cases in the check below.  The if statement kept blowing
> my mind every time I tried to parse it. 

Wow, if you really found that difficult to parse you could have written
a comment.

> > If this is just about saving cycles then consider that ltdb_serach_dn1()
> > is really simple and I'd rather see a special ltdb_check_base_dn()
> > function coded instead, so all other callers keep going with the
> > assumptions they had.
> 
> I really don't see the value in doing that.

1. In general you don't risk bugs with a change of assumptions on the
existing code
2. A search keeps being just a search and a check is a check, keeping
functions separate makes it clear what is what.


> > > +
> > > +               /* These must have a valid DN */
> > > +       } else if (req->op.search.scope == LDB_SCOPE_BASE ||
> > > req->op.search.scope == LDB_SCOPE_ONELEVEL) {
> > > +               ltdb_unlock_read(module);
> > > +               return LDB_ERR_INVALID_DN_SYNTAX;
> > > +       } else {
> > > +               ret = LDB_SUCCESS;
> > > +       }
> > > +
> > 
> > I'd like you to leave the checks on top, and simply do the base DN as a
> > separate thing.
> > It keeps the code cleaner and simpler to read.
> 
> That code was the most difficult, hairy bit of logic I've had the
> displeasure of trying to reverse-engineer.  I would rather not, thanks. 

Except that by moving the logic down, you do a number of unnecessary
operations when you already know you are going to fail.


Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Senior Software Engineer at Red Hat Inc. <ssorce at redhat.com>