From 402ecbc4724a1d818a3be0ebb9bf523f6865985d Mon Sep 17 00:00:00 2001 From: Carsten Bock Date: Fri, 22 May 2015 10:45:51 +0200 Subject: [PATCH] ims_*_scscf: Encapsulated Locking on IMS-Subscriptions, easier to track locking bugs, little cleanup. --- .../ims_registrar_scscf/registrar_notify.c | 21 +++++++-------- modules/ims_registrar_scscf/save.c | 26 +++++++++--------- modules/ims_registrar_scscf/userdata_parser.c | 13 +++++---- modules/ims_usrloc_scscf/bin_utils.c | 27 +++++++++++++++---- modules/ims_usrloc_scscf/bin_utils.h | 3 +++ modules/ims_usrloc_scscf/impurecord.c | 26 +++++++++++------- modules/ims_usrloc_scscf/udomain.c | 5 ++-- modules/ims_usrloc_scscf/ul_rpc.c | 13 ++++----- modules/ims_usrloc_scscf/usrloc.c | 4 +++ modules/ims_usrloc_scscf/usrloc.h | 10 ++++++- 10 files changed, 95 insertions(+), 53 deletions(-) diff --git a/modules/ims_registrar_scscf/registrar_notify.c b/modules/ims_registrar_scscf/registrar_notify.c index 2b33fd7dc27..ae2d95b6558 100644 --- a/modules/ims_registrar_scscf/registrar_notify.c +++ b/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 @@ int process_contact(impurecord_t* presentity_impurecord, udomain_t * _d, int exp } } - 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); diff --git a/modules/ims_registrar_scscf/save.c b/modules/ims_registrar_scscf/save.c index 2638d3c0e8d..7100b2c8135 100644 --- a/modules/ims_registrar_scscf/save.c +++ b/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) { diff --git a/modules/ims_registrar_scscf/userdata_parser.c b/modules/ims_registrar_scscf/userdata_parser.c index 8fc93b0a18a..eeeb9013314 100644 --- a/modules/ims_registrar_scscf/userdata_parser.c +++ b/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; } diff --git a/modules/ims_usrloc_scscf/bin_utils.c b/modules/ims_usrloc_scscf/bin_utils.c index 05883805c4c..ec87241d4ec 100644 --- a/modules/ims_usrloc_scscf/bin_utils.c +++ b/modules/ims_usrloc_scscf/bin_utils.c @@ -897,16 +897,19 @@ ims_subscription *bin_decode_ims_subscription(bin_data *x) for(i=0;iservice_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 @@ ims_subscription *bin_decode_ims_subscription(bin_data *x) 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); +} + diff --git a/modules/ims_usrloc_scscf/bin_utils.h b/modules/ims_usrloc_scscf/bin_utils.h index 07a94f62821..0e3d0aa1301 100644 --- a/modules/ims_usrloc_scscf/bin_utils.h +++ b/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 */ diff --git a/modules/ims_usrloc_scscf/impurecord.c b/modules/ims_usrloc_scscf/impurecord.c index 9c5214b375f..3599b9e12a3 100644 --- a/modules/ims_usrloc_scscf/impurecord.c +++ b/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); diff --git a/modules/ims_usrloc_scscf/udomain.c b/modules/ims_usrloc_scscf/udomain.c index 15bf1c9fdd2..259ed40f1c4 100644 --- a/modules/ims_usrloc_scscf/udomain.c +++ b/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; } diff --git a/modules/ims_usrloc_scscf/ul_rpc.c b/modules/ims_usrloc_scscf/ul_rpc.c index b3932541bfc..bdc0d5d0175 100644 --- a/modules/ims_usrloc_scscf/ul_rpc.c +++ b/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) { diff --git a/modules/ims_usrloc_scscf/usrloc.c b/modules/ims_usrloc_scscf/usrloc.c index ae7518fae20..4e3c970d08d 100644 --- a/modules/ims_usrloc_scscf/usrloc.c +++ b/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; diff --git a/modules/ims_usrloc_scscf/usrloc.h b/modules/ims_usrloc_scscf/usrloc.h index 104fd1c1cfc..007801645f6 100644 --- a/modules/ims_usrloc_scscf/usrloc.h +++ b/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;