Ticket 47599 - Reduce lock scope in retro changelog plug-in

Description:  Change the "changenumber" type to PRUint64. All changes to the static
              changenumbers are now atomic by use of slapi_counters.

              We still need to do the locking in retrocl_po.c as we need to
              serialize the actual updates.

https://fedorahosted.org/389/ticket/47599

Reviewed by: richm(Thanks!!)

ldap/servers/plugins/retrocl/retrocl.c

@@ -482,6 +482,8 @@ retrocl_plugin_init(Slapi_PBlock *pb)
 	    rc= slapi_register_plugin_ext("internalpostoperation", 1 /* Enabled */, "retrocl_internalpostop_init", retrocl_internalpostop_init, "Retrocl internal postoperation plugin", NULL, identity, precedence);
 	  }
 
+	  retrocl_internal_cn = slapi_counter_new();
+	  retrocl_first_cn = slapi_counter_new();
 	  retrocl_internal_lock = PR_NewLock();
 	  if (retrocl_internal_lock == NULL) return -1;
 	}

ldap/servers/plugins/retrocl/retrocl.h

@@ -57,7 +57,7 @@
 
 /* max len of a long (2^64), represented as a string, including null byte */
 #define	CNUMSTR_LEN		21
-typedef unsigned long changeNumber;
+typedef PRUint64 changeNumber;
 
 typedef struct _cnum_result_t {
     int		crt_nentries;	/* number of entries returned from search */
@@ -131,6 +131,8 @@ extern const char *attr_nsuniqueid;
 extern const char *attr_isreplicated;
 
 extern PRLock *retrocl_internal_lock;
+extern Slapi_Counter *retrocl_internal_cn;
+extern Slapi_Counter *retrocl_first_cn;
 
 /* Functions */
 

ldap/servers/plugins/retrocl/retrocl_cn.c

@@ -43,8 +43,8 @@
 
 #include "retrocl.h"
 
-static changeNumber retrocl_internal_cn = 0;
-static changeNumber retrocl_first_cn = 0;
+Slapi_Counter *retrocl_internal_cn;
+Slapi_Counter *retrocl_first_cn;
 
 /*
  * Function: a2changeNumber
@@ -86,36 +86,31 @@ handle_cnum_entry( Slapi_Entry *e, void *callback_data )
     cr->cr_time = NULL;
 
     if ( NULL != e ) {
-	Slapi_Attr *chattr = NULL;
-	sval = NULL;
-	value = NULL;
-	if ( slapi_entry_attr_find( e, attr_changenumber, &chattr ) == 0 ) {
-	    slapi_attr_first_value( chattr,&sval );
-	    if ( NULL != sval ) {
-		value = slapi_value_get_berval ( sval );
-		if( NULL != value && NULL != value->bv_val &&
-		    '\0' != value->bv_val[0]) {
-		cr->cr_cnum = a2changeNumber( value->bv_val );
-		}
-	    }
-	}
-	chattr = NULL;
-	sval = NULL;
-	value = NULL;
-
-	chattr = NULL;
-	sval = NULL;
-	value = NULL;
-	if ( slapi_entry_attr_find( e, attr_changetime, &chattr ) == 0 ) {
-	    slapi_attr_first_value( chattr,&sval );
-	    if ( NULL != sval) {
-		value = slapi_value_get_berval ( sval );
-		if (NULL != value && NULL != value->bv_val &&
-		    '\0' != value->bv_val[0]) {
-		cr->cr_time = slapi_ch_strdup( value->bv_val );
-		}
-	    }
-	}
+        Slapi_Attr *chattr = NULL;
+        sval = NULL;
+        value = NULL;
+        if ( slapi_entry_attr_find( e, attr_changenumber, &chattr ) == 0 ) {
+            slapi_attr_first_value( chattr,&sval );
+            if ( NULL != sval ) {
+                value = slapi_value_get_berval ( sval );
+                if( NULL != value && NULL != value->bv_val && '\0' != value->bv_val[0]) {
+                    cr->cr_cnum = a2changeNumber( value->bv_val );
+                }
+            }
+        }
+        chattr = NULL;
+        sval = NULL;
+        value = NULL;
+
+        if ( slapi_entry_attr_find( e, attr_changetime, &chattr ) == 0 ) {
+            slapi_attr_first_value( chattr,&sval );
+            if ( NULL != sval) {
+                value = slapi_value_get_berval ( sval );
+                if (NULL != value && NULL != value->bv_val && '\0' != value->bv_val[0]) {
+                    cr->cr_time = slapi_ch_strdup( value->bv_val );
+                }
+            }
+        }
     }
     return 0;
 }
@@ -163,7 +158,7 @@ int retrocl_get_changenumbers(void)
 		       NULL,NULL,0,&cr,NULL,handle_cnum_result,
 		       handle_cnum_entry, NULL);
 
-    retrocl_first_cn = cr.cr_cnum;
+    slapi_counter_set_value(retrocl_first_cn, cr.cr_cnum);
 
     slapi_ch_free(( void **) &cr.cr_time );
 
@@ -172,11 +167,11 @@ int retrocl_get_changenumbers(void)
 		       NULL,NULL,0,&cr,NULL,handle_cnum_result,
 		       handle_cnum_entry, NULL);
 
-    retrocl_internal_cn = cr.cr_cnum;
+    slapi_counter_set_value(retrocl_internal_cn,cr.cr_cnum);
     
     slapi_log_error(SLAPI_LOG_PLUGIN,"retrocl","Got changenumbers %lu and %lu\n",
-		    retrocl_first_cn,
-		    retrocl_internal_cn);
+            slapi_counter_get_value(retrocl_first_cn),
+            slapi_counter_get_value(retrocl_internal_cn));
 
     slapi_ch_free(( void **) &cr.cr_time );
 
@@ -200,10 +195,10 @@ time_t retrocl_getchangetime( int type, int *err )
     time_t ret;
 
     if ( type != SLAPI_SEQ_FIRST && type != SLAPI_SEQ_LAST ) {
-	if ( err != NULL ) {
-	    *err = -1;
-	}
-	return NO_TIME;
+        if ( err != NULL ) {
+            *err = -1;
+        }
+        return NO_TIME;
     }
     memset( &cr, '\0', sizeof( cnumRet ));
     slapi_seq_callback( RETROCL_CHANGELOG_DN, type, 
@@ -213,13 +208,13 @@ time_t retrocl_getchangetime( int type, int *err )
 			handle_cnum_result, handle_cnum_entry, NULL ); 
     
     if ( err != NULL ) {
-	*err = cr.cr_lderr;
+        *err = cr.cr_lderr;
     }
     
     if ( NULL == cr.cr_time ) {
-	ret = NO_TIME;
+        ret = NO_TIME;
     } else {
-	ret = parse_localTime( cr.cr_time );
+        ret = parse_localTime( cr.cr_time );
     }
     slapi_ch_free(( void **) &cr.cr_time );
     return ret;
@@ -238,10 +233,8 @@ time_t retrocl_getchangetime( int type, int *err )
 
 void retrocl_forget_changenumbers(void) 
 { 
-    PR_Lock(retrocl_internal_lock);
-    retrocl_first_cn = 0;
-    retrocl_internal_cn = 0;
-    PR_Unlock(retrocl_internal_lock);
+    slapi_counter_set_value(retrocl_first_cn, 0);
+    slapi_counter_set_value(retrocl_internal_cn, 0);
 }
 
 /*
@@ -257,11 +250,7 @@ void retrocl_forget_changenumbers(void)
 
 changeNumber retrocl_get_first_changenumber(void) 
 { 
-    changeNumber cn;
-    PR_Lock(retrocl_internal_lock);
-    cn = retrocl_first_cn;
-    PR_Unlock(retrocl_internal_lock);
-    return cn;
+    return (changeNumber)slapi_counter_get_value(retrocl_first_cn);
 }
 
 /*
@@ -277,9 +266,7 @@ changeNumber retrocl_get_first_changenumber(void)
 
 void retrocl_set_first_changenumber(changeNumber cn) 
 { 
-    PR_Lock(retrocl_internal_lock);
-    retrocl_first_cn = cn;
-    PR_Unlock(retrocl_internal_lock);
+    slapi_counter_set_value(retrocl_first_cn, cn);
 }
 
 
@@ -295,12 +282,8 @@ void retrocl_set_first_changenumber(changeNumber cn)
  */
 
 changeNumber retrocl_get_last_changenumber(void) 
-{ 
-    changeNumber cn;
-    PR_Lock(retrocl_internal_lock);
-    cn = retrocl_internal_cn;
-    PR_Unlock(retrocl_internal_lock);
-    return cn;
+{
+    return (changeNumber)slapi_counter_get_value(retrocl_internal_cn);
 }
 
 /*
@@ -308,7 +291,7 @@ changeNumber retrocl_get_last_changenumber(void)
  *
  * Returns: none
  * 
- * Arguments: none, lock must be held
+ * Arguments: none
  *
  * Description: NOTE! MUST BE PRECEEDED BY retrocl_assign_changenumber
  *
@@ -316,8 +299,8 @@ changeNumber retrocl_get_last_changenumber(void)
 
 void retrocl_commit_changenumber(void) 
 { 
-    if ( retrocl_first_cn == 0) {
-        retrocl_first_cn = retrocl_internal_cn;
+    if ( slapi_counter_get_value(retrocl_first_cn) == 0) {
+        slapi_counter_set_value(retrocl_first_cn, slapi_counter_get_value(retrocl_internal_cn));
     }
 }
 
@@ -326,7 +309,7 @@ void retrocl_commit_changenumber(void)
  *
  * Returns: none
  * 
- * Arguments: none, lock must be held
+ * Arguments: none
  *
  * Description: NOTE! MUST BE PRECEEDED BY retrocl_assign_changenumber
  *
@@ -334,7 +317,7 @@ void retrocl_commit_changenumber(void)
 
 void retrocl_release_changenumber(void) 
 { 
-    retrocl_internal_cn--;
+    slapi_counter_decrement(retrocl_internal_cn);
 }
 
 /*
@@ -362,10 +345,8 @@ int retrocl_update_lastchangenumber(void)
                NULL,NULL,0,&cr,NULL,handle_cnum_result,
                handle_cnum_entry, NULL);
 
-
-    retrocl_internal_cn = cr.cr_cnum;
-    slapi_log_error(SLAPI_LOG_PLUGIN,"retrocl","Refetched last changenumber =  %lu \n",
-            retrocl_internal_cn);
+    slapi_counter_set_value(retrocl_internal_cn, cr.cr_cnum);
+    slapi_log_error(SLAPI_LOG_PLUGIN,"retrocl","Refetched last changenumber =  %lu \n", cr.cr_cnum);
 
     slapi_ch_free(( void **) &cr.cr_time );
 
@@ -394,7 +375,7 @@ changeNumber retrocl_assign_changenumber(void)
      * validity of the internal assignment of retrocl_internal_cn 
      * we had from the startup */  
 
-    if(retrocl_internal_cn <= retrocl_first_cn){ 
+    if(slapi_counter_get_value(retrocl_internal_cn) <= slapi_counter_get_value(retrocl_first_cn)){
         /* the numbers have become out of sync - retrocl_get_changenumbers
          * gets called only once during startup and it may have had a problem 
          * getting the last changenumber.         
@@ -405,7 +386,8 @@ changeNumber retrocl_assign_changenumber(void)
         retrocl_update_lastchangenumber();                                   
     }                             
 
-    retrocl_internal_cn++;
-    cn = retrocl_internal_cn;
+    slapi_counter_increment(retrocl_internal_cn);
+    cn = slapi_counter_get_value(retrocl_internal_cn);
+
     return cn;
 }