On Wed, 30 Apr 2008, Howard Chu wrote:
> rein(a)OpenLDAP.org wrote:
>> syncprov_findbase() must search the backend saved with the syncrepl
>> operation,
>> not the one from the operation passed as argument. The backend in the op
>> argument can be a subordinate database, in which case the search for the
>> base in
>> the superior database will fail, and syncrepl consumers will be force to do
>> a an
>> unneccessary full refresh of the database.
>
> OK.
>
>> The patch at the end should fix
>> this. Note that both fop.o_bd and fop.o_bd->bd_info can be changed by the
>> overlay_op_walk() call, which is the reason for the long pointer traversal
>> to
>> find the correct bd_info to save and restore.
> But the overlay_op_walk call is only appropriate when the DB to be searched
> is the current database, and the current DB is an overlay DB structure.
Ah, the changing of the BackendDB->bd_info that takes place when
overlays are called feels like an open pit I manage to fall into every
time I get close to it... I wish it could be replaced in a future
version.
> Your patch causes fc->fss->s_op->o_bd's bd_info pointer to change, which is
> not allowed. That's in the original backendDB, which must be treated as
> read-only since multiple threads may be accessing it. The correct approach
> here is to use a new local backendDB variable, copy the s_op->o_bd into it,
> and then just do a regular be_search invocation instead of using
> overlay_op_walk.
>
> But, this patch must not take effect on the first call to syncprov_findbase
> (which occurred in syncprov_op_search) - in that case, the current code is
> correct. So, you need to tweak things based on whether (s_flags &
> PS_IS_REFRESHING) is true or not - if true, this is the first search, and it
> should use the original code. Else, it must use be_search.
A new patch that I hope should fix this is at the end. It always use
be_search, after putting back the original bd_info if needed. I feel
that using the generic be_search is better than interfering directly
with the overlay code as overlay_op_walk does. I also tested for
SLAP_ISOVERLAY rather than PS_IS_REFRESHING, as that appeared more
generic to me. But again, I may be totally wrong here. Does this patch
look better?
Rein
Index: OpenLDAP/servers/slapd/overlays/syncprov.c
===================================================================
RCS file: /f/CVSROOT/drift/OpenLDAP/servers/slapd/overlays/syncprov.c,v
retrieving revision 1.1.1.18
diff -u -u -r1.1.1.18 syncprov.c
--- OpenLDAP/servers/slapd/overlays/syncprov.c 30 Apr 2008 11:17:58 -0000 1.1.1.18
+++ OpenLDAP/servers/slapd/overlays/syncprov.c 2 May 2008 11:19:46 -0000
@@ -404,7 +404,7 @@
slap_callback cb = {0};
Operation fop;
SlapReply frs = { REP_RESULT };
- BackendInfo *bi;
+ BackendDB be;
int rc;
fc->fss->s_flags ^= PS_FIND_BASE;
@@ -413,10 +413,15 @@
fop = *fc->fss->s_op;
fop.o_hdr = op->o_hdr;
- fop.o_bd = op->o_bd;
fop.o_time = op->o_time;
fop.o_tincr = op->o_tincr;
- bi = op->o_bd->bd_info;
+
+ if ( SLAP_ISOVERLAY( fop.o_bd )) {
+ slap_overinst *on = (slap_overinst *)fop.o_bd->bd_info;
+ be = *fop.o_bd;
+ be.bd_info = (BackendInfo *)on->on_info;
+ fop.o_bd = &be;
+ }
cb.sc_response = findbase_cb;
cb.sc_private = fc;
@@ -434,8 +439,7 @@
fop.ors_filter = &generic_filter;
fop.ors_filterstr = generic_filterstr;
- rc = overlay_op_walk( &fop, &frs, op_search, on->on_info, on );
- op->o_bd->bd_info = bi;
+ rc = fop.o_bd->be_search( &fop, &frs );
} else {
ldap_pvt_thread_mutex_unlock( &fc->fss->s_mutex );
fc->fbase = 1;