samba4_ldap_controls

[tridge at samba.org]

Simo,

Congratulations on the progress you've made on the ldap controls code!

I think the general approach is good, and we should aim to get this
into the main Samba4 branch as soon as possible.

Here are a few more specific comments:

1) It looks like you have a bug in paged_search(), when looking for a
matching cookie in the list of results_store structures. The variable
'current' is not updated in the loop, so it will spin forever if the
cookie doesn't match. I'd suggest something like:

 for (current=private_data->store; current; current=current->next) 

instead of the while loop

2) I think much of the code could be broken up into smaller, simpler
   functions. For example, in the paged search control, adding a
   ldb_request_has_control() function would get rid of that awkward
   loop, and as you suggested on irc, you could have functions to
   add/remove controls from the lists. Also, pulling the if
   (current==NULL) case out into a separate function would be good
   (the code that handles a new request).

3) the code is currently vulnerable to a DOS attack. I think we need a
   limit on the number of paged searches active per client, and maybe
   a timer to destoy inactive searches? I wonder how openldap handles
   this?

4) why was messageid changed from a uint32_t to an int?

5) in ldap_ildap.c you have:

	if (res->controls) {
		*control_res = talloc_steal(conn, res->controls);
	}

better to remove the if() and just have this:

	*control_res = talloc_steal(conn, res->controls);

as talloc_steal() knows how to deal with NULL. This also means the
result is guaranteed to be initialised to NULL on no controls (which
prevents confusion in callers).

6) the check for remaining critical extensions in each of the backends
   should be broken out into a common function

7) the ldb_dn_explode_or_special() function looks strange to me. It
   looks like it doesn't explode the main part of the DN at all? What
   is this function meant to handle? (a comment would be nice too!). 

   I would have expected you to extend ldb_dn_explode() to look for a
   leading '<' in any component, and mark the DN as having this
   extended form for that component. Then extend the linearise
   functions to do the opposite, if needed. The "<GUID" check looks
   very ad-hoc to me. Are you sure its right?

8) looking at the 2nd stage init, I see you've done it exactly how I
   suggested on IRC, but I think I must have had too much/little
   coffee at the time, and my suggestion wasn't very good! It looks
   rather strange having that return NULL. Maybe we should go back to
   the old init, then have a new LDB_OP_INITIALISE separate ldb
   operation, and guarantee that LDB_OP_INITIALISE will be the first
   ldb_request given. That seems more a more natural approach, and it
   allows modules that don't need the 2nd stage init to just ignore it
   in their operation switch statement

9) as discussed on irc, I think we need to extend
   lib/ldb/tools/cmdline.c to allow for controls to be specified to
   all the existing ldb tools, rather than having a new
   ldbtest_controls.c tool.

10) I get LDAP_UNAVAILABLE_CRITICAL_EXTENSION when trying
    ldbtest_controls with the sort control against a w2k3 server. Does
    that one work for you?

More comments as I come across them ....

Cheers, Tridge