Re: (ITS#5340) REP_ENTRY_MODIFIABLE bug in dynlist

[h.b.furuseth@usit.uio.no]

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

-- 
Hallvard