Re: (ITS#5305) Contribware: Two overlays for implementing NESTED dynamic groups

[ando@sys-net.it]

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
---------------------------------------