ims_*_scscf: Encapsulated Locking on IMS-Subscriptions, easier to track locking bugs, little cleanup.

modules/ims_registrar_scscf/registrar_notify.c

@@ -221,7 +221,7 @@ int can_publish_reg(struct sip_msg *msg, char *_t, char *str2) {
     }
 
     //check if asserted identity is in service profile
-    lock_get(r->s->lock);
+    ul.lock_subscription(r->s);
     if (r->s) {
 	for (i = 0; i < r->s->service_profiles_cnt; i++)
 	    for (j = 0; j < r->s->service_profiles[i].public_identities_cnt; j++) {
@@ -233,12 +233,12 @@ int can_publish_reg(struct sip_msg *msg, char *_t, char *str2) {
 			    i, j);
 		    ret = CSCF_RETURN_TRUE;
 		    ul.unlock_udomain((udomain_t*) _t, &presentity_uri);
-		    lock_release(r->s->lock);
+		    ul.unlock_subscription(r->s);
 		    goto done;
 		}
 	    }
     }
-    lock_release(r->s->lock);
+    ul.unlock_subscription(r->s);
     LM_DBG("Did not find p-asserted-identity <%.*s> in SP\n", asserted_id.len, asserted_id.s);
 
     //check if asserted is present in any of the path headers
@@ -379,7 +379,7 @@ int can_subscribe_to_reg(struct sip_msg *msg, char *_t, char *str2) {
     }
 
     //check if asserted identity is in service profile
-    lock_get(r->s->lock);
+    ul.lock_subscription(r->s);
     if (r->s) {
         for (i = 0; i < r->s->service_profiles_cnt; i++)
             for (j = 0; j < r->s->service_profiles[i].public_identities_cnt; j++) {
@@ -391,12 +391,12 @@ int can_subscribe_to_reg(struct sip_msg *msg, char *_t, char *str2) {
                             i, j);
                     ret = CSCF_RETURN_TRUE;
                     ul.unlock_udomain((udomain_t*) _t, &presentity_uri);
-                    lock_release(r->s->lock);
+	            ul.unlock_subscription(r->s);
                     goto done;
                 }
             }
     }
-    lock_release(r->s->lock);
+    ul.unlock_subscription(r->s);
     LM_DBG("Did not find p-asserted-identity <%.*s> in SP\n", asserted_id.len, asserted_id.s);
 
     //check if asserted is present in any of the path headers
@@ -571,10 +571,10 @@ int process_contact(impurecord_t* presentity_impurecord, udomain_t * _d, int exp
 	    goto done;
 	} 
 
-	lock_get(subscription->lock);
+        ul.lock_subscription(subscription);
 	subscription->ref_count++;
 	LM_DBG("subscription ref count after add is now %d\n", subscription->ref_count);
-	lock_release(subscription->lock);
+        ul.unlock_subscription(subscription);
 
 	//now update the implicit set
 	for (i = 0; i < subscription->service_profiles_cnt; i++) {
@@ -617,11 +617,10 @@ next_implicit_impu:
 	    }
 	}
 
-	lock_get(subscription->lock);
+        ul.lock_subscription(subscription);
 	subscription->ref_count--;
 	LM_DBG("subscription ref count after sub is now %d\n", subscription->ref_count);
-	lock_release(subscription->lock);
-
+        ul.unlock_subscription(subscription);
 	
 	ul.lock_udomain(_d, &presentity_impurecord->public_identity);
 	

modules/ims_registrar_scscf/save.c

@@ -472,9 +472,12 @@ void free_ims_subscription_data(ims_subscription *s) {
         shm_free(s->service_profiles);
     if (s->private_identity.s)
         shm_free(s->private_identity.s);
-    lock_release(s->lock);
-    lock_destroy(s->lock);
-    lock_dealloc(s->lock);
+    ul.unlock_subscription(s);
+#ifdef EXTRA_DEBUG
+    LM_DBG("SUBSCRIPTION LOCK %p destroyed\n", s->slock);
+#endif
+    lock_destroy(s->slock);
+    lock_dealloc(s->slock);
     shm_free(s);
 
 }
@@ -768,10 +771,10 @@ int update_contacts_new(struct sip_msg* msg, udomain_t* _d,
                 break;
             }
 
-            lock_get(subscription->lock);
+            ul.lock_subscription(subscription);
             subscription->ref_count++;
             LM_DBG("ref count after add is now %d\n", subscription->ref_count);
-            lock_release(subscription->lock);
+            ul.unlock_subscription(subscription);
             ul.unlock_udomain(_d, public_identity);
 
             //now update the implicit set
@@ -810,10 +813,10 @@ int update_contacts_new(struct sip_msg* msg, udomain_t* _d,
                     ul.unlock_udomain(_d, &pi->public_identity);
                 }
             }
-            lock_get(subscription->lock);
+            ul.lock_subscription(subscription);
             subscription->ref_count--;
             LM_DBG("ref count after sub is now %d\n", subscription->ref_count);
-            lock_release(subscription->lock);
+            ul.unlock_subscription(subscription);
 
             //finally we update the explicit IMPU record with the new data
             ul.lock_udomain(_d, public_identity);
@@ -870,11 +873,10 @@ int update_contacts_new(struct sip_msg* msg, udomain_t* _d,
 
             if (!subscription) {
                 LM_WARN("subscription is null..... continuing without de-registering implicit set\n");
-                unlock(subscription->lock);
             } else {
-                lock(subscription->lock);
+                ul.lock_subscription(subscription);
                 subscription->ref_count++; //this is so we can de-reg the implicit set just now without holding the lock on the current IMPU
-                unlock(subscription->lock);
+                ul.unlock_subscription(subscription);
 
                 ul.unlock_udomain(_d, public_identity);
 
@@ -937,9 +939,9 @@ int update_contacts_new(struct sip_msg* msg, udomain_t* _d,
                         ul.unlock_udomain(_d, &pi->public_identity);
                     }
                 }
-                lock(subscription->lock);
+                ul.lock_subscription(subscription);
                 subscription->ref_count--;
-                unlock(subscription->lock);
+                ul.unlock_subscription(subscription);
             }
 
 	    if (ret == 2) {

modules/ims_registrar_scscf/userdata_parser.c

@@ -859,19 +859,22 @@ static ims_subscription* parse_ims_subscription(xmlDocPtr doc, xmlNodePtr root)
 				if (rc)
 					s->service_profiles_cnt++;
 			}				
-	s->lock = lock_alloc();
-	if (s->lock==0) {
+	s->slock = lock_alloc();
+	if (s->slock==0) {
 		LM_ERR("Failed to allocate Lock for IMS Subscription\n");
 		shm_free(s);
 		return 0;
 	}
-	if (lock_init(s->lock)==0){
+	if (lock_init(s->slock)==0){
 		LM_ERR("Failed to initialize Lock for IMS Subscription\n");
-		lock_dealloc(s->lock);
-		s->lock=0;
+		lock_dealloc(s->slock);
+		s->slock=0;
 		shm_free(s);
 		return 0;
 	}
+#ifdef EXTRA_DEBUG
+    	LM_DBG("LOCK CREATED FOR SUBSCRIPTION [%.*s]: %p\n", s->private_identity.len, s->private_identity.s, s->slock);
+#endif
 	return s;
 }
 

modules/ims_usrloc_scscf/bin_utils.c

@@ -897,16 +897,19 @@ ims_subscription *bin_decode_ims_subscription(bin_data *x)
 	for(i=0;i<imss->service_profiles_cnt;i++)
 		if (!bin_decode_service_profile(x,imss->service_profiles+i)) goto error;
 
-	imss->lock = lock_alloc();
-	if (imss->lock==0){
+	imss->slock = lock_alloc();
+	if (imss->slock==0){
 		goto error;
 	}
-	if (lock_init(imss->lock)==0){
-		lock_dealloc(imss->lock);
-		imss->lock=0;
+	if (lock_init(imss->slock)==0){
+		lock_dealloc(imss->slock);
+		imss->slock=0;
 		goto error;
 	}
 	imss->ref_count = 1;
+#ifdef EXTRA_DEBUG
+    	LM_DBG("LOCK CREATED FOR SUBSCRIPTION [%.*s]: %p\n", imss->private_identity.len, imss->private_identity.s, imss->slock);
+#endif
 
 	return imss;
 error:
@@ -919,3 +922,17 @@ error:
 	return 0;
 }
 
+void lock_ims_subscription(ims_subscription * s) {
+#ifdef EXTRA_DEBUG
+    LM_DBG("LOCKING SUBSCRIPTION [%.*s]: %p (Refcount: %d)\n", s->private_identity.len, s->private_identity.s, s->slock, s->ref_count);
+#endif
+    lock_get(s->slock);
+}
+
+void unlock_ims_subscription(ims_subscription * s) {
+#ifdef EXTRA_DEBUG
+    LM_DBG("UN-LOCKING SUBSCRIPTION [%.*s]: %p (Refcount: %d)\n", s->private_identity.len, s->private_identity.s, s->slock, s->ref_count);
+#endif
+    lock_release(s->slock);
+}
+

modules/ims_usrloc_scscf/bin_utils.h

@@ -95,5 +95,8 @@ inline int bin_decode_str(bin_data *x,str *s);
 int bin_encode_ims_subscription(bin_data *x, ims_subscription *s);
 ims_subscription *bin_decode_ims_subscription(bin_data *x);
 
+void lock_ims_subscription(ims_subscription *);
+void unlock_ims_subscription(ims_subscription *);
+
 #endif	/* BIN_UTILS_H */
 

modules/ims_usrloc_scscf/impurecord.c

@@ -129,9 +129,9 @@ int new_impurecord(str* _dom, str* public_identity, int reg_state, int barring,
     /*assign ims subscription profile*/
     if (*s) {
         (*_r)->s = *s;
-        lock_get((*_r)->s->lock);
+	lock_ims_subscription((*_r)->s);
         (*_r)->s->ref_count++;
-        lock_release((*_r)->s->lock);
+	unlock_ims_subscription((*_r)->s);
     }
 
     return 0;
@@ -164,14 +164,14 @@ void free_impurecord(impurecord_t* _r) {
         shm_free(_r->ecf2.s);
     if (_r->s) {
         LM_DBG("ref count on this IMS data is %d\n", _r->s->ref_count);
-        lock_get(_r->s->lock);
+	lock_ims_subscription(_r->s);
         if (_r->s->ref_count == 1) {
             LM_DBG("freeing IMS subscription data\n");
             free_ims_subscription_data(_r->s);
         } else {
             LM_DBG("decrementing IMS subscription data ref count\n");
             _r->s->ref_count--;
-            lock_release(_r->s->lock);
+            unlock_ims_subscription(_r->s);
         }
     }
 
@@ -771,8 +771,14 @@ void free_ims_subscription_data(ims_subscription *s) {
     }
     if (s->service_profiles) shm_free(s->service_profiles);
     if (s->private_identity.s) shm_free(s->private_identity.s);
-    lock_destroy(s->lock);
-    lock_dealloc(s->lock);
+#pragma message __FILE__ " add unlocking here????"
+    // ul.unlock_subscription(s);
+#ifdef EXTRA_DEBUG
+    LM_DBG("SUBSCRIPTION LOCK %p destroyed\n", s->slock);
+#endif
+    lock_destroy(s->slock);
+    lock_dealloc(s->slock);
+
     shm_free(s);
 
 }
@@ -846,7 +852,7 @@ int update_impurecord(struct udomain* _d, str* public_identity, int reg_state, i
     if (s) {
         LM_DBG("we have a new ims_subscription\n");
         if ((*_r)->s) {
-            lock_get((*_r)->s->lock);
+	    lock_ims_subscription((*_r)->s);
             if ((*_r)->s->ref_count == 1) {
                 LM_DBG("freeing user data as no longer referenced\n");
                 free_ims_subscription_data((*_r)->s); //no need to release lock after this. its gone ;)
@@ -854,13 +860,13 @@ int update_impurecord(struct udomain* _d, str* public_identity, int reg_state, i
             } else {
                 (*_r)->s->ref_count--;
                 LM_DBG("new ref count for ims sub is %d\n", (*_r)->s->ref_count);
-                lock_release((*_r)->s->lock);
+	        unlock_ims_subscription((*_r)->s);
             }
         }
         (*_r)->s = *s;
-        lock_get((*_r)->s->lock);
+	lock_ims_subscription((*_r)->s);
         (*_r)->s->ref_count++;
-        lock_release((*_r)->s->lock);
+	unlock_ims_subscription((*_r)->s);
     }
 
     run_ul_callbacks((*_r)->cbs, UL_IMPU_UPDATE, *_r, NULL);

modules/ims_usrloc_scscf/udomain.c

@@ -655,8 +655,7 @@ int get_impus_from_subscription_as_string(udomain_t* _d, impurecord_t* impu_rec,
 	LM_DBG("no subscription associated with impu\n");
 	return 0;
     }
-
-    lock_get(impu_rec->s->lock);
+    lock_ims_subscription(impu_rec->s);
     for (i = 0; i < impu_rec->s->service_profiles_cnt; i++) {
 	for (j = 0; j < impu_rec->s->service_profiles[i].public_identities_cnt; j++) {
 	    impi = &(impu_rec->s->service_profiles[i].public_identities[j]);
@@ -713,7 +712,7 @@ int get_impus_from_subscription_as_string(udomain_t* _d, impurecord_t* impu_rec,
 	return 1;
     }
 
-    lock_release(impu_rec->s->lock);
+    unlock_ims_subscription(impu_rec->s);
 
     return 0;
 }

modules/ims_usrloc_scscf/ul_rpc.c

@@ -25,6 +25,7 @@
 #include "dlist.h"
 #include "ucontact.h"
 #include "udomain.h"
+#include "bin_utils.h"
 
 static const char* ul_rpc_dump_doc[2] = 	{"Dump SCSCF user location tables", 0 };
 static const char* ul_rpc_showimpu_doc[2] = {"Dump SCSCF IMPU information", 0 };
@@ -90,11 +91,11 @@ static void ul_rpc_show_impu(rpc_t* rpc, void* ctx) {
     }
 
     ims_subscription* subscription = impu_rec->s;
-    lock_get(subscription->lock);
+    lock_ims_subscription(subscription);
     //add subscription data
     if (rpc->struct_add(sh, "S{", "impi", &subscription->private_identity, "service profiles", &sph) < 0) {
 	rpc->fault(ctx, 500, "Internal error adding impu subscription data");
-	lock_release(subscription->lock);
+        unlock_ims_subscription(subscription);
 	unlock_udomain(domain, &impu);
 	return;
     }
@@ -104,13 +105,13 @@ static void ul_rpc_show_impu(rpc_t* rpc, void* ctx) {
 	sprintf(numstr, "%d", i + 1);
 	if (rpc->struct_add(sph, "{", numstr, &sdh) < 0) {
 	    rpc->fault(ctx, 500, "Internal error adding impu subscription detail data");
-	    lock_release(subscription->lock);
+	    unlock_ims_subscription(subscription);
 	    unlock_udomain(domain, &impu);
 	    return;
 	}
 	if (rpc->struct_add(sdh, "{", "impus", &spi) < 0) {
 	    rpc->fault(ctx, 500, "Internal error adding impu subscription data");
-	    lock_release(subscription->lock);
+	    unlock_ims_subscription(subscription);
 	    unlock_udomain(domain, &impu);
 	    return;
 	}
@@ -119,14 +120,14 @@ static void ul_rpc_show_impu(rpc_t* rpc, void* ctx) {
 	    sprintf(numstr, "%d", j + 1);
 	    if (rpc->struct_add(spi, "S", numstr, &subscription->service_profiles[i].public_identities[j].public_identity) < 0) {
 		rpc->fault(ctx, 500, "Internal error adding impu subscription detail data");
-		lock_release(subscription->lock);
+		unlock_ims_subscription(subscription);
 		unlock_udomain(domain, &impu);
 		return;
 	    }
 	}
     }
 
-    lock_release(subscription->lock);
+    unlock_ims_subscription(subscription);
 
     //add contact data
     if (rpc->struct_add(ah, "{", "contacts", &ch) < 0) {

modules/ims_usrloc_scscf/usrloc.c

@@ -51,6 +51,7 @@
 #include "subscribe.h"
 #include "../../sr_module.h"
 #include "ul_mod.h"
+#include "bin_utils.h"
 
 /*! nat branch flag */
 extern unsigned int nat_bflag;
@@ -84,6 +85,9 @@ int bind_usrloc(usrloc_api_t* api) {
 	api->lock_udomain = lock_udomain;
 	api->unlock_udomain = unlock_udomain;
 
+	api->lock_subscription = lock_ims_subscription;
+	api->unlock_subscription = unlock_ims_subscription;
+
 	api->lock_contact_slot = lock_contact_slot;
 	api->unlock_contact_slot = unlock_contact_slot;
 	api->lock_contact_slot_i = lock_contact_slot_i;

modules/ims_usrloc_scscf/usrloc.h

@@ -243,7 +243,7 @@ typedef struct ims_subscription_s {
     unsigned short service_profiles_cnt; /**< size of the array above		*/
 
     int ref_count; /**< referenced count 				*/
-    gen_lock_t *lock; /**< lock for operations on it 		*/
+    gen_lock_t * slock; /**< lock for operations on it 		*/
     struct ims_subscription_s* next;
     struct ims_subscription_s* prev;
 } ims_subscription;
@@ -428,6 +428,10 @@ typedef void (*lock_contact_slot_i_t)(int sl);
 
 typedef void (*unlock_contact_slot_i_t)(int sl);
 
+typedef void (*lock_subscription_t)(ims_subscription *);
+
+typedef void (*unlock_subscription_t)(ims_subscription *);
+
 typedef int (*update_ucontact_t)(struct impurecord* _r, struct ucontact* _c, struct ucontact_info* _ci);
 
 typedef int (*expire_ucontact_t)(struct impurecord* _r, struct ucontact* _c);
@@ -490,6 +494,10 @@ typedef struct usrloc_api {
 
     lock_contact_slot_t lock_contact_slot;
     unlock_contact_slot_t unlock_contact_slot;
+
+    lock_subscription_t lock_subscription;
+    unlock_subscription_t unlock_subscription;
+
     lock_contact_slot_i_t lock_contact_slot_i;
     unlock_contact_slot_i_t unlock_contact_slot_i;
     insert_ucontact_t insert_ucontact;