Jose,

few preliminary comments:

- you coded your contribution for OpenLDAP 2.3; however, since it configures as a new feature, it should be coded for OpenLDAP 2.4 (better for HEAD), as 2.3 is feature-frozen ever since. This comment may appear trivial, but right now your contribution does not compile with HEAD/re24 because of some internal API changes.

- the coding style you used does not conform to that of the OpenLDAP project. Although this is usually only loosely enforced, you used things we're a bit picky about, like C++-style comments, and late declarations; if you compile with enough warnings (e.g., with gcc, with -Wall -pedantic) you'll see that in many cases functions are not returning when a return value is expected or vice-versa; incorrect formats are used for (long) integers, pointers and so. Also, I note that besides being a totally new contribution, many of the comments are already no longer aligned with the code.

- according to agreed development lines, new contributions should always provide back-config support (this can be added later, if and when the rest works as expected)

- with respect to the possible improvements you indicate, I think: 1) permission checking should occur along the lines of, e.g., slapo-dynlist(5), honoring dgIdentity and dgAuthz. 2) avoiding unnecessary expansion would probably be mandatory for production use 3) use back-config rather than naive in-database configuration using an ordinary database

- The build instructions you provide are incorrect: you suggest to place the source in servers/slapd/overlays/, and to modify statover.c; this is a generated file, so it should never be modified. You should rather suggest to place the sources in an arbitrary directory (it could be contrib/slapd-modules/nestedAggregateAttr as soon as your contribution enters the official distribution) and build it as a run-time module.

- you should rather eliminate duplicate values from result sets (much like slapo-dynlist(5) does) as they are not allowed by the definition of the "member" attribute, which has a distinguishedNameMatch EQUALITY rule.

- instead of copying compare_entry(), you should rather ask to make the one in servers/slapd/compare.c non-static

- since the "OR" operator in filters allows more than 2 operands, I think your filter combination strategy could be optimized by simply defining a combine_filters() function that counts the number of filter strings in the array and creates a filter like "(|(f1)(f2)(f3)...)"

- in check_filter(), you compare strings; you should rather compare pointers to AttributeDescription structures.

- in nestedAggregateAttrURL_search() you don't need to run str2filter() on op->ors_filterstr, since an exploded filter is already available in op->ors_filter

- strings[] containing "(objectClass=<ocname>)" could be prepared once for all during config; it would save a lot of mallocs (BTW: use the slab for operation-spanning allocation).

- I would recommend you do some consistent checking of return values before dereferencing pointers.

- I would recommend that you either remove or turn into useful standard debugging the plethora of non-standard messages you added. Use LDAP_DEBUG_ARGS to mark entering/exiting functions (and logging useful args); use LDAP_DEBUG_TRACE to trace operations; use slap_loglevel_get() to register friendly names for specific log subsystems (you'll get back the number to compare with ldap_debug).

- with respect to the OID for the test schema items, you'll get an OID arc in the experimental OID namespace of the OpenLDAP project as soon as your contribution is incorporated.

p.

Ing. Pierangelo Masarati OpenLDAP Core Team

SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it --------------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Email: pierangelo.masarati@sys-net.it ---------------------------------------