Below I'll summarize SlapReply.sr_flags as I now understand it after un-confusing myself a little, since this ITS is getting rather long. Mostly about how code which receives a filled SlapReply should use it, which I presume is primarily overlays, slap_callbacks, maybe back-relay.
About discussion so far:
The sr_flags are inconsistent about when which ones may or must be obeyed. E.g. REP_ENTRY_MUST<RELEASE/BEFREED> does not mean the entry will be released/freed, but REP_<MATCHED/REF> does in some functions. This needs to be normalized.
We can state that overlays with a bi_entry_release_rw function must keep track of which module should release which entry, like glue does. At least for now. Never mind my worries about which overlay/backend should release an REP_ENTRY_MUSTRELEASE'd sr_entry. That doesn't matter with current OpenLDAP code. As far as I can see glue is the only overlay with a release function, and glue can dispatch on the entry DN.
rs.sr_ref bugs, fixing now: - slap_send_ldap_result() forgets to free rs->sr_ref if it does not call send_ldap_response(). - send_ldap_response() can return with sr_ref set to a freed value. I'll make it return with sr_ref to NULL if REP_REF_MUSTBEFREED.
ando@sys-net.it writes:
Similarly, the existence of REP_ENTRY_MUSTBEFREED is not totally clear:
Looked a bit at cvs logs... Since you are the one who introduced MUSTBEFREED and MODIFIABLE (slap.h 1.503 for rwm) I think we can take it the intent of REP_ENTRY_MUSTBEFREED is indeed unclear/forgotten:-)
Anyway, I think I've gotten the flags straight now:
* REP_<ENTRY, MATCHED, REF>_MUST<RELEASE, BEFREED>:
Overlays/callbacks may modify the rs->sr_<entry, text, ref> pointers. If they do, they must release/free the old value if and only if the corresponding MUST<RELEASE/BEFREED> flag is set. They should (re)set the relevant flags to describe the new value. Many overlays don't, or not thoroughly enough.
- REP_<MATCHED, REF>_MUSTBEFREED:
Callers setting these can depend on send_ldap_response() to free sr_<matched, ref>. That is, "send response/disconnect" calls.
These flags do apparently not apply and should not be set when sending an entry or search continuation reference. Dunno why not REP_REF_MUSTBEFREED when sending continuation references.
- REP_ENTRY_MUSTRELEASE and REP_ENTRY_MUSTBEFREED:
Callers that set these flags when sending an entry, *allow* the callee to release/free rs->sr_entry. Callers cannot depend on it being release/freed, but some do anyway.
It should simplify things to make the send functions always obey this flag, but I'm not sure how much cleanup that requires. There is partial code for that in slap_send_search_entry(), along with a FIXME note.
Free MUSTBEFREED-entries with entry_free(), and MUSTRELEASE-entries with be_entry_release_r(). There is no "use be_entry_release_w()" flag, but no OpenLDAP code sends write-locked entries anyway.
+ REP_ENTRY_MUSTRELEASE:
Created for ITS#3671 to ensure that network hangs don't delay cache entry unlocks:
Modules should release their cache entry locks before sending anything. They can't do that with cached entries they send in rs->sr_entry, so instead they should set REP_ENTRY_MUSTRELEASE. Conversely, anything which does network I/O (and other potentially slooow ops I guess) should first obey this flag.
Several overlays don't obey, nor some other code for sending an entry when it decides to send an error response instead.
* REP_ENTRY_MODIFIABLE:
Overlays and callbacks may modify the entry which rs->sr_entry points at if this flag is set.
If they want to modify an incoming sr_entry when REP_ENTRY_MODIFIABLE is not set, they can use entry_dup(), release or free the old entry if sr_flags says so, put the new entry in sr_entry, and maybe set REP_ENTRY_MODIFIABLE | REP_ENTRY_MUSTBEFREED.
Thus code which creates a temporary Entry struct and sends it as rs->sr_entry, should set REP_ENTRY_MODIFIABLE if the entry is of no further interest to it (and it has set no pointers into it, e.g. rs->sr_matched). That allows overlays that wish to modify the entry being sent, to use this entry instead of copying it.
* REP_NO_<OPERATIONALS, ENTRYDN, SUBSCHEMA>:
Set by the chain overlay to "tell the frontend not to add generated operational attributes". Only backend.c reacts to these flags.
Dunno if overlays should reset these flags when replacing sr_entry with a "private" entry (with another DN) rather than a dup of the current one, or something. Some overlays set sr_flags to fresh values while others only update some bits in sr_flags, so the effect is likely somewhat ad-hoc. Leaving that worry to someone else.
I've been fiddling with some functions to generalize this, but they seem to need a lot of flags or variants for different modes... Should maybe just commit some draft variants with warnings that they may change, and clean up later. After my changes don't cause slapd to crash, anyway:-)