Re: New overlay, your opinion?

[Hallvard B Furuseth <h.b.furuseth@usit.uio.no>]

Johan Jakus writes:
> I've been developing an overlay and I got to a well working solution.
> But,I would really appreciate your opinions about it before sending it to
> the contribs.
> You can see my source on my site:
> http://www.dataworld.be/johan/openldap/parsearch/parsearch.htm

Some notes after a quick scan:

- Try to stay below 80-char line length.  OpenLDAP mostly uses
  tab-width 4 and indentation 4, though it's gotten intermixed quite
  a bit of code written with tab-width 8 code over the years.

- Also OpenLDAP code triest to stay C90-compatible, so no '//' comments
  or declarations after executable statements.

- LDAP_DEBUG_ANY is for log messages which should always be output, so
  parsearch would be very chatty in the log.  Use LDAP_DEBUG_TRACE
  for most of these messages.  Sometimes LDAP_DEBUG_ARGS/ACL.
  See include/ldap_log.h.

- parsearch_response() should inspect rs->sr_type see what kind of
  kind of response this is - search entry, intermediate response etc.
  See e.g. valsort_response.  It can look at op->o_tag to see what
  kind of request is being processed.
  Note that 'rs->sr_un.sru_search.r_entry != NULL' will be true for an
  intermediate response, because sr_un is a union and
  sru_extended.r_rspoid has been set.

  Unfortunately the SlapReply flags are not always reliable.  We've
  cleaned up a lot lately.

  There are a bunch of macros like sr_entry = sr_un.sru_search.r_entry
  in slapd.conf, so these expresions can be written more briefly.

- This is wrong:
      Entry* dupEntry = entry_dup(rs->sr_un.sru_search.r_entry);
      rs->sr_un.sru_search.r_entry = dupEntry;
      rs->sr_flags = REP_ENTRY_MUSTFLUSH;
  You are not obeying the flags and flushing the old entry.
  You are resetting non-entry-related flags.  And MUSTFLUSH
  = MUSTBEFREED | MUSTRELEASE, you want MUSTFREE since you have no
  release function.

  Just use
	slap_overinst *on = (slap_overinst *) op->o_bd->bd_info;
	Entry *e;
	rs_entry2modifiable( op, rs, on );
  	e = rs->sr_entry;  /* this is now modifiable */

-- 
Hallvard