Re: (ITS#3899) locks in bdb_search()

[hyc@symas.com]

h.b.furuseth@usit.uio.no wrote:
>  Full_Name: Hallvard B Furuseth Version: HEAD OS: URL:
>  http://folk.uio.no/hbf/OpenLDAP/bdb0727.diff Submission from: (NULL)
>  (129.240.186.42) Submitted by: hallvard
>
>
>  back-bdb/search.c:bdb_search() gets a LOCK_ID() if !(opinfo &&
>  opinfo->boi_txn), but frees it if !opinfo. A fix is to free it if
>  !ltid instead; that's set iff (opinfo && opinfo->boi_txn).
>
>  Also this is not done before all return statements.
>
>  I'll fix (patch attached) if someone confirms; bdb is a bit too
>  magical for me to meddle with locks like that without asking:-)

That patch looks fine.

>  I would note that I generally find lock/unlock-code easier to read
>  with just one unlock per lock, i.e. using goto <unlock; return>
>  instead of multiple {unlock; return}.

I agree. The original code mostly does that; these two cases appear to 
have been added more recently and did not follow the established pattern.

>  Might have avoided the bug with not unlocking before each return too.
>  That's a bigger change to 6 files, so I'm leaving that alone unless
>  someone says yes.

I'll note that the LOCK_ID_FREE() invocations here are actually no-ops, 
so their omission won't actually cause any malfunction in a default 
compile. It's cruft left over from when we didn't use long-lived 
per-thread locker IDs. Since it appears we've overcome all the issues 
with long-lived lockers and don't need to consider the possibility of 
running with #undef BDB_REUSE_LOCKERS it may be a better idea to just 
unifdef this stuff and remove it.

-- 
  -- Howard Chu
  Chief Architect, Symas Corp.  http://www.symas.com
  Director, Highland Sun        http://highlandsun.com/hyc
  OpenLDAP Core Team            http://www.openldap.org/project/