Reworked again the caching in case of failure.

Now operations that set the status of an entry to CREATING (add.c, modrdn.c)
need to set it to COMMIT, by calling cache_entry_commit, before returning
the entry itself, otherwise the entry is removed from the cache
and its private data is freed.
Should fix crashes due to add failures as in ITS#1245

diff --git a/servers/slapd/back-ldbm/add.c b/servers/slapd/back-ldbm/add.c
index bd9d7969529b7b19648072ba1b36fb7e6bc36c5b..99dfe0fdb50c8ae4ba3c31d066f7ab254afa4562 100644 (file)
--- a/servers/slapd/back-ldbm/add.c
+++ b/servers/slapd/back-ldbm/add.c
@@ -354,7 +354,12 @@ ldbm_back_add(
 
        send_ldap_result( conn, op, LDAP_SUCCESS,
                NULL, NULL, NULL, NULL );
+
+       /* marks the entry as committed, so it is added to the cache;
+        * otherwise it is removed from the cache, but not destroyed;
+        * it will be destroyed by the caller */
        rc = 0;
+       cache_entry_commit( e );
 
 return_results:;
        if (p != NULL) {
@@ -368,10 +373,8 @@ return_results:;
        }
 
        if ( rc ) {
-               /* FIXME: remove from cache? */
-               cache_entry_private_destroy_mark( e );
-
-               /* free entry and writer lock */
+               /* in case of error, writer lock is freed 
+                * and entry's private data is destroyed */
                cache_return_entry_w( &li->li_cache, e );
        }
 

diff --git a/servers/slapd/back-ldbm/cache.c b/servers/slapd/back-ldbm/cache.c
index 65c1e4fec24934c9c9f72bb87b266f931837cf88..4227f3be0b731ebff074e6bb222ee62d29be795d 100644 (file)
--- a/servers/slapd/back-ldbm/cache.c
+++ b/servers/slapd/back-ldbm/cache.c
@@ -29,9 +29,9 @@ typedef struct ldbm_entry_info {
        int             lei_state;      /* for the cache */
 #define        CACHE_ENTRY_UNDEFINED   0
 #define CACHE_ENTRY_CREATING   1
-#define CACHE_ENTRY_READY              2
-#define CACHE_ENTRY_DELETED            3
-#define CACHE_ENTRY_DESTROY_PRIVATE    4
+#define CACHE_ENTRY_READY      2
+#define CACHE_ENTRY_DELETED    3
+#define CACHE_ENTRY_COMMITTED  4
        
        int             lei_refcnt;     /* # threads ref'ing this entry */
        Entry   *lei_lrunext;   /* for cache lru list */
@@ -136,17 +136,20 @@ cache_entry_private_init( Entry*e )
 }
 
 /*
- * assumes that the entry is write-locked;marks it i a manner that
- * makes e_private be destroyed at the following cache_return_entry_w,
+ * marks an entry in CREATING state as committed, so it is really returned
+ * to the cache. Otherwise an entry in CREATING state is removed.
+ * Makes e_private be destroyed at the following cache_return_entry_w,
  * but lets the entry untouched (owned by someone else)
  */
 void
-cache_entry_private_destroy_mark( Entry *e )
+cache_entry_commit( Entry *e )
 {
        assert( e );
        assert( e->e_private );
+       assert( LEI(e)->lei_state == CACHE_ENTRY_CREATING );
+       /* assert( LEI(e)->lei_refcnt == 1 ); */
 
-       LEI(e)->lei_state = CACHE_ENTRY_DESTROY_PRIVATE;
+       LEI(e)->lei_state = CACHE_ENTRY_COMMITTED;
 }
 
 static int
@@ -165,7 +168,7 @@ void
 cache_return_entry_rw( Cache *cache, Entry *e, int rw )
 {
        ID id;
-       int refcnt;
+       int refcnt, freeit = 1;
 
        /* set cache mutex */
        ldap_pvt_thread_mutex_lock( &cache->c_mutex );
@@ -177,7 +180,18 @@ cache_return_entry_rw( Cache *cache, Entry *e, int rw )
        id = e->e_id;
        refcnt = --LEI(e)->lei_refcnt;
 
-       if ( LEI(e)->lei_state == CACHE_ENTRY_CREATING ) {
+       /*
+        * if the entry is returned when in CREATING state, it is deleted
+        * but not freed because it may belong to someone else (do_add,
+        * for instance)
+        */
+       if (  LEI(e)->lei_state == CACHE_ENTRY_CREATING ) {
+               cache_delete_entry_internal( cache, e );
+               freeit = 0;
+               /* now the entry is in DELETED state */
+       }
+
+       if ( LEI(e)->lei_state == CACHE_ENTRY_COMMITTED ) {
                LEI(e)->lei_state = CACHE_ENTRY_READY;
 
                /* free cache mutex */
@@ -194,8 +208,7 @@ cache_return_entry_rw( Cache *cache, Entry *e, int rw )
 #endif
 
 
-       } else if ( LEI(e)->lei_state == CACHE_ENTRY_DELETED
-                || LEI(e)->lei_state == CACHE_ENTRY_DESTROY_PRIVATE ) {
+       } else if ( LEI(e)->lei_state == CACHE_ENTRY_DELETED ) {
                if( refcnt > 0 ) {
                        /* free cache mutex */
                        ldap_pvt_thread_mutex_unlock( &cache->c_mutex );
@@ -212,10 +225,8 @@ cache_return_entry_rw( Cache *cache, Entry *e, int rw )
 
 
                } else {
-                       int state = LEI(e)->lei_state;
-
                        cache_entry_private_destroy( e );
-                       if ( state == CACHE_ENTRY_DELETED ) {
+                       if ( freeit ) {
                                entry_free( e );
                        }
 

diff --git a/servers/slapd/back-ldbm/id2entry.c b/servers/slapd/back-ldbm/id2entry.c
index a0e11c8073764dac79a2cbbe01775dc6b2b26c8b..69a5ceb3dad5f429637fcefb5727bb6e11624b46 100644 (file)
--- a/servers/slapd/back-ldbm/id2entry.c
+++ b/servers/slapd/back-ldbm/id2entry.c
@@ -274,6 +274,9 @@ id2entry_rw( Backend *be, ID id, int rw )
                rw ? "w" : "r", id, (unsigned long) e );
 #endif
 
+       /* marks the entry as committed, so it will get added to the cache
+        * when the lock is released */
+       cache_entry_commit( e );
 
        return( e );
 }

diff --git a/servers/slapd/back-ldbm/modrdn.c b/servers/slapd/back-ldbm/modrdn.c
index f10a4b353deaa74bbd1ca7b68b0b3d9e8c83651b..78b2ef096250fec432e11c1d1d58cec798158289 100644 (file)
--- a/servers/slapd/back-ldbm/modrdn.c
+++ b/servers/slapd/back-ldbm/modrdn.c
@@ -847,6 +847,7 @@ ldbm_back_modrdn(
            
            goto return_results;
        }
+       rc = -1;
        
        (void) cache_update_entry( &li->li_cache, e );
 
@@ -856,7 +857,6 @@ ldbm_back_modrdn(
 
        /* id2entry index */
        if ( id2entry_add( be, e ) != 0 ) {
-               entry_free( e );
                send_ldap_result( conn, op, LDAP_OTHER,
                        NULL, "entry update failed", NULL, NULL );
                goto return_results;
@@ -865,6 +865,7 @@ ldbm_back_modrdn(
        send_ldap_result( conn, op, LDAP_SUCCESS,
                NULL, NULL, NULL, NULL );
        rc = 0;
+       cache_entry_commit( e );
 
 return_results:
        if( new_dn != NULL ) free( new_dn );
@@ -917,5 +918,10 @@ return_results:
 
        /* free entry and writer lock */
        cache_return_entry_w( &li->li_cache, e );
+       if ( rc ) {
+               /* if rc != 0 the entry is uncached and its private data 
+                * is destroyed; the entry must be freed */
+               entry_free( e );
+       }
        return( rc );
 }

diff --git a/servers/slapd/back-ldbm/proto-back-ldbm.h b/servers/slapd/back-ldbm/proto-back-ldbm.h
index 7d798fe55aacaf55bf9901c61f8e6758146ed58a..1f9426f2a5c7d611ca98108b3edab3e625f5b825 100644 (file)
--- a/servers/slapd/back-ldbm/proto-back-ldbm.h
+++ b/servers/slapd/back-ldbm/proto-back-ldbm.h
@@ -51,7 +51,7 @@ int cache_update_entry LDAP_P(( Cache *cache, Entry *e ));
 void cache_return_entry_rw LDAP_P(( Cache *cache, Entry *e, int rw ));
 #define cache_return_entry_r(c, e) cache_return_entry_rw((c), (e), 0)
 #define cache_return_entry_w(c, e) cache_return_entry_rw((c), (e), 1)
-void cache_entry_private_destroy_mark LDAP_P(( Entry *e ));
+void cache_entry_commit LDAP_P(( Entry *e ));
 
 ID cache_find_entry_dn2id LDAP_P(( Backend *be, Cache *cache, const char *dn ));
 ID cache_find_entry_ndn2id LDAP_P(( Backend *be, Cache *cache, const char *ndn ));