[Asterisk-code-review] res_pjsip_mwi: use an ao2_global object for mwi containers (...asterisk[17])
George Joseph
asteriskteam at digium.com
Thu Oct 10 09:13:51 CDT 2019
George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/c/asterisk/+/13006 )
Change subject: res_pjsip_mwi: use an ao2_global object for mwi containers
......................................................................
res_pjsip_mwi: use an ao2_global object for mwi containers
On shutdown it's possible for the unsolicited mwi container to be freed before
other dependent threads are done using it. This patch ensures this can no
longer happen by wrapping the container in an ao2_global object. The solicited
container was also changed too.
ASTERISK-28552
Change-Id: I8f812286dc19a34916acacd71ce2ec26e1042047
---
M res/res_pjsip_mwi.c
1 file changed, 142 insertions(+), 59 deletions(-)
Approvals:
Joshua Colp: Looks good to me, but someone else must approve
Benjamin Keith Ford: Looks good to me, but someone else must approve
George Joseph: Looks good to me, approved; Approved for Submit
diff --git a/res/res_pjsip_mwi.c b/res/res_pjsip_mwi.c
index 49b5dc5..d7749fa 100644
--- a/res/res_pjsip_mwi.c
+++ b/res/res_pjsip_mwi.c
@@ -42,8 +42,9 @@
#include "asterisk/mwi.h"
struct mwi_subscription;
-static struct ao2_container *unsolicited_mwi;
-static struct ao2_container *solicited_mwi;
+
+AO2_GLOBAL_OBJ_STATIC(mwi_unsolicited);
+AO2_GLOBAL_OBJ_STATIC(mwi_solicited);
static char *default_voicemail_extension;
@@ -581,13 +582,15 @@
}
static int create_unsolicited_mwi_subscriptions(struct ast_sip_endpoint *endpoint,
- int recreate, int send_now);
+ int recreate, int send_now, struct ao2_container *unsolicited_mwi, struct ao2_container *solicited_mwi);
static void mwi_subscription_shutdown(struct ast_sip_subscription *sub)
{
struct mwi_subscription *mwi_sub;
struct ast_datastore *mwi_datastore;
struct ast_sip_endpoint *endpoint = NULL;
+ struct ao2_container *unsolicited_mwi;
+ struct ao2_container *solicited_mwi;
mwi_datastore = ast_sip_subscription_get_datastore(sub, MWI_DATASTORE);
if (!mwi_datastore) {
@@ -601,18 +604,25 @@
endpoint = ast_sorcery_retrieve_by_id(ast_sip_get_sorcery(), "endpoint", mwi_sub->id);
ao2_ref(mwi_datastore, -1);
- ao2_unlink(solicited_mwi, mwi_sub);
+
+ solicited_mwi = ao2_global_obj_ref(mwi_solicited);
+ if (solicited_mwi) {
+ ao2_unlink(solicited_mwi, mwi_sub);
+ }
/*
* When a solicited subscription is removed it's possible an unsolicited one
* needs to be [re-]created. Attempt to establish unsolicited MWI.
*/
+ unsolicited_mwi = ao2_global_obj_ref(mwi_unsolicited);
if (unsolicited_mwi && endpoint) {
ao2_lock(unsolicited_mwi);
- create_unsolicited_mwi_subscriptions(endpoint, 1, 1);
+ create_unsolicited_mwi_subscriptions(endpoint, 1, 1, unsolicited_mwi, solicited_mwi);
ao2_unlock(unsolicited_mwi);
+ ao2_ref(unsolicited_mwi, -1);
}
+ ao2_cleanup(solicited_mwi);
ao2_cleanup(endpoint);
}
@@ -678,6 +688,10 @@
*mwi_sub = NULL;
*mwi_stasis = NULL;
+ if (!container) {
+ return 0;
+ }
+
mwi_subs = ao2_find(container, ast_sorcery_object_get_id(endpoint),
OBJ_SEARCH_KEY | OBJ_MULTIPLE | OBJ_NOLOCK);
if (!mwi_subs) {
@@ -707,11 +721,13 @@
*
* \param endpoint The endpoint
* \param mailbox The mailbox
+ * \param unsolicited_mwi A container of unsolicited mwi objects
*
* \retval 1 if a solicited subscription is allowed for the endpoint/mailbox
* 0 otherwise
*/
-static int allow_and_or_replace_unsolicited(struct ast_sip_endpoint *endpoint, const char *mailbox)
+static int allow_and_or_replace_unsolicited(struct ast_sip_endpoint *endpoint, const char *mailbox,
+ struct ao2_container *unsolicited_mwi)
{
struct mwi_subscription *mwi_sub;
struct mwi_stasis_subscription *mwi_stasis;
@@ -758,11 +774,14 @@
*
* \param endpoint The endpoint
* \param mailbox The mailbox
+ * \param unsolicited_mwi A container of unsolicited mwi objects
+ * \param solicited_mwi A container of solicited mwi objects
*
* \retval 1 if an unsolicited subscription is allowed for the endpoint/mailbox
* 0 otherwise
*/
-static int is_unsolicited_allowed(struct ast_sip_endpoint *endpoint, const char *mailbox)
+static int is_unsolicited_allowed(struct ast_sip_endpoint *endpoint, const char *mailbox,
+ struct ao2_container *unsolicited_mwi, struct ao2_container *solicited_mwi)
{
struct mwi_subscription *mwi_sub;
struct mwi_stasis_subscription *mwi_stasis;
@@ -828,28 +847,41 @@
struct ast_sip_endpoint *endpoint = arg;
char *mailboxes;
char *mailbox;
+ struct ao2_container *unsolicited_mwi;
if (ast_strlen_zero(aor->mailboxes)) {
return 0;
}
/* A reload could be taking place so lock while checking if allowed */
- ao2_lock(unsolicited_mwi);
+ unsolicited_mwi = ao2_global_obj_ref(mwi_unsolicited);
+ if (unsolicited_mwi) {
+ ao2_lock(unsolicited_mwi);
+ }
+
mailboxes = ast_strdupa(aor->mailboxes);
while ((mailbox = ast_strip(strsep(&mailboxes, ",")))) {
if (ast_strlen_zero(mailbox)) {
continue;
}
- if (!allow_and_or_replace_unsolicited(endpoint, mailbox)) {
+ if (!allow_and_or_replace_unsolicited(endpoint, mailbox, unsolicited_mwi)) {
ast_debug(1, "Endpoint '%s' already configured for unsolicited MWI for mailbox '%s'. "
"Denying MWI subscription to %s\n", ast_sorcery_object_get_id(endpoint), mailbox,
ast_sorcery_object_get_id(aor));
- ao2_unlock(unsolicited_mwi);
+
+ if (unsolicited_mwi) {
+ ao2_unlock(unsolicited_mwi);
+ ao2_ref(unsolicited_mwi, -1);
+ }
return -1;
}
}
- ao2_unlock(unsolicited_mwi);
+
+ if (unsolicited_mwi) {
+ ao2_unlock(unsolicited_mwi);
+ ao2_ref(unsolicited_mwi, -1);
+ }
return 0;
}
@@ -975,6 +1007,7 @@
const char *resource = ast_sip_subscription_get_resource_name(sip_sub);
struct mwi_subscription *sub;
struct ast_sip_endpoint *endpoint = ast_sip_subscription_get_endpoint(sip_sub);
+ struct ao2_container *solicited_mwi;
/* no aor in uri? subscribe to all on endpoint */
if (ast_strlen_zero(resource)) {
@@ -995,7 +1028,12 @@
ast_sip_subscription_remove_datastore(sip_sub, MWI_DATASTORE);
}
- ao2_link(solicited_mwi, sub);
+ solicited_mwi = ao2_global_obj_ref(mwi_solicited);
+ if (solicited_mwi) {
+ ao2_link(solicited_mwi, sub);
+ ao2_ref(solicited_mwi, -1);
+ }
+
ao2_cleanup(sub);
ao2_cleanup(endpoint);
return 0;
@@ -1142,11 +1180,13 @@
* \param endpoint An endpoint object
* \param recreate Whether or not unsolicited subscriptions are potentially being recreated
* \param send_now Whether or not to send a notify once the subscription is created
+ * \param unsolicited_mwi A container of unsolicited mwi objects
+ * \param solicited_mwi A container of solicited mwi objects
*
* \retval 0
*/
static int create_unsolicited_mwi_subscriptions(struct ast_sip_endpoint *endpoint,
- int recreate, int send_now)
+ int recreate, int send_now, struct ao2_container *unsolicited_mwi, struct ao2_container *solicited_mwi)
{
RAII_VAR(struct mwi_subscription *, aggregate_sub, NULL, ao2_cleanup);
char *mailboxes;
@@ -1181,14 +1221,16 @@
}
/* Lock solicited so we don't potentially add to both containers */
- ao2_lock(solicited_mwi);
+ if (solicited_mwi) {
+ ao2_lock(solicited_mwi);
+ }
mailboxes = ast_strdupa(endpoint->subscription.mwi.mailboxes);
while ((mailbox = ast_strip(strsep(&mailboxes, ",")))) {
struct mwi_subscription *sub;
struct mwi_stasis_subscription *mwi_stasis_sub;
- if (!is_unsolicited_allowed(endpoint, mailbox)) {
+ if (!is_unsolicited_allowed(endpoint, mailbox, unsolicited_mwi, solicited_mwi)) {
continue;
}
@@ -1228,13 +1270,16 @@
}
}
- ao2_unlock(solicited_mwi);
+ if (solicited_mwi) {
+ ao2_unlock(solicited_mwi);
+ }
+
return 0;
}
-static int create_mwi_subscriptions_for_endpoint(void *obj, void *arg, int flags)
+static int create_mwi_subscriptions_for_endpoint(void *obj, void *arg, void *data, int flags)
{
- return create_unsolicited_mwi_subscriptions(obj, 0, 0);
+ return create_unsolicited_mwi_subscriptions(obj, 0, 0, arg, data);
}
static int unsubscribe(void *obj, void *arg, int flags)
@@ -1248,9 +1293,16 @@
static void create_mwi_subscriptions(void)
{
+ struct ao2_container *unsolicited_mwi;
+ struct ao2_container *solicited_mwi;
struct ao2_container *endpoints;
struct ast_variable *var;
+ unsolicited_mwi = ao2_global_obj_ref(mwi_unsolicited);
+ if (!unsolicited_mwi) {
+ return;
+ }
+
var = ast_variable_new("mailboxes !=", "", "");
endpoints = ast_sorcery_retrieve_by_fields(ast_sip_get_sorcery(), "endpoint",
@@ -1258,9 +1310,12 @@
ast_variables_destroy(var);
if (!endpoints) {
+ ao2_ref(unsolicited_mwi, -1);
return;
}
+ solicited_mwi = ao2_global_obj_ref(mwi_solicited);
+
/* We remove all the old stasis subscriptions first before applying the new configuration. This
* prevents a situation where there might be multiple overlapping stasis subscriptions for an
* endpoint for mailboxes. Though there may be mailbox changes during the gap between unsubscribing
@@ -1269,10 +1324,12 @@
*/
ao2_lock(unsolicited_mwi);
ao2_callback(unsolicited_mwi, OBJ_NOLOCK | OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE, unsubscribe, NULL);
- ao2_callback(endpoints, OBJ_NODATA, create_mwi_subscriptions_for_endpoint, NULL);
+ ao2_callback_data(endpoints, OBJ_NODATA, create_mwi_subscriptions_for_endpoint, unsolicited_mwi, solicited_mwi);
ao2_unlock(unsolicited_mwi);
ao2_ref(endpoints, -1);
+ ao2_cleanup(solicited_mwi);
+ ao2_ref(unsolicited_mwi, -1);
}
/*! \brief Function called to send MWI NOTIFY on any unsolicited mailboxes relating to this AOR */
@@ -1299,6 +1356,8 @@
char *id = ast_strdupa(ast_sorcery_object_get_id(contact));
char *aor = NULL;
struct ast_sip_endpoint *endpoint = NULL;
+ struct ao2_container *unsolicited_mwi;
+ struct ao2_container *solicited_mwi;
if (contact->endpoint) {
endpoint = ao2_bump(contact->endpoint);
@@ -1313,10 +1372,20 @@
return;
}
+ unsolicited_mwi = ao2_global_obj_ref(mwi_unsolicited);
+ if (!unsolicited_mwi) {
+ ao2_cleanup(endpoint);
+ return;
+ }
+
+ solicited_mwi = ao2_global_obj_ref(mwi_solicited);
+
ao2_lock(unsolicited_mwi);
- create_mwi_subscriptions_for_endpoint(endpoint, NULL, 0);
+ create_unsolicited_mwi_subscriptions(endpoint, 0, 0, unsolicited_mwi, solicited_mwi);
ao2_unlock(unsolicited_mwi);
ao2_cleanup(endpoint);
+ ao2_cleanup(solicited_mwi);
+ ao2_ref(unsolicited_mwi, -1);
aor = strsep(&id, ";@");
ao2_callback(unsolicited_mwi, OBJ_NODATA, send_contact_notify, aor);
@@ -1342,6 +1411,7 @@
struct mwi_subscription *mwi_sub;
struct ast_sip_endpoint *endpoint = NULL;
struct ast_sip_contact *found_contact;
+ struct ao2_container *unsolicited_mwi;
if (contact->endpoint) {
endpoint = ao2_bump(contact->endpoint);
@@ -1364,6 +1434,11 @@
return;
}
+ unsolicited_mwi = ao2_global_obj_ref(mwi_unsolicited);
+ if (!unsolicited_mwi) {
+ return;
+ }
+
ao2_lock(unsolicited_mwi);
mwi_subs = ao2_find(unsolicited_mwi, contact->endpoint_name,
OBJ_SEARCH_KEY | OBJ_MULTIPLE | OBJ_NOLOCK | OBJ_UNLINK);
@@ -1374,6 +1449,7 @@
ao2_iterator_destroy(mwi_subs);
}
ao2_unlock(unsolicited_mwi);
+ ao2_ref(unsolicited_mwi, -1);
}
/*! \brief Observer for contacts so unsolicited MWI is sent when a contact changes */
@@ -1386,7 +1462,12 @@
/*! \brief Task invoked to send initial MWI NOTIFY for unsolicited */
static int send_initial_notify_all(void *obj)
{
- ao2_callback(unsolicited_mwi, OBJ_NODATA, send_notify, NULL);
+ struct ao2_container *unsolicited_mwi = ao2_global_obj_ref(mwi_unsolicited);
+
+ if (unsolicited_mwi) {
+ ao2_callback(unsolicited_mwi, OBJ_NODATA, send_notify, NULL);
+ ao2_ref(unsolicited_mwi, -1);
+ }
return 0;
}
@@ -1434,8 +1515,38 @@
return 0;
}
+static int unload_module(void)
+{
+ struct ao2_container *unsolicited_mwi;
+
+ ast_sorcery_observer_remove(ast_sip_get_sorcery(), "global", &global_observer);
+ ast_sorcery_observer_remove(ast_sip_get_sorcery(), "contact", &mwi_contact_observer);
+
+ unsolicited_mwi = ao2_global_obj_replace(mwi_unsolicited, NULL);
+ if (unsolicited_mwi) {
+ ao2_callback(unsolicited_mwi, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE, unsubscribe, NULL);
+ ao2_ref(unsolicited_mwi, -1);
+ }
+
+ ao2_global_obj_release(mwi_solicited);
+
+ if (ast_serializer_pool_destroy(mwi_serializer_pool)) {
+ ast_log(LOG_WARNING, "Unload incomplete. Try again later\n");
+ return -1;
+ }
+ mwi_serializer_pool = NULL;
+
+ ast_sip_unregister_subscription_handler(&mwi_handler);
+
+ ast_free(default_voicemail_extension);
+ default_voicemail_extension = NULL;
+ return 0;
+}
+
static int load_module(void)
{
+ struct ao2_container *mwi_container;
+
if (ast_sip_register_subscription_handler(&mwi_handler)) {
return AST_MODULE_LOAD_DECLINE;
}
@@ -1446,22 +1557,23 @@
ast_log(AST_LOG_WARNING, "Failed to create MWI serializer pool. The default SIP pool will be used for MWI\n");
}
- solicited_mwi = ao2_container_alloc_hash(AO2_ALLOC_OPT_LOCK_MUTEX, 0, MWI_BUCKETS,
+ mwi_container = ao2_container_alloc_hash(AO2_ALLOC_OPT_LOCK_MUTEX, 0, MWI_BUCKETS,
mwi_sub_hash, NULL, mwi_sub_cmp);
- if (!solicited_mwi) {
- ast_serializer_pool_destroy(mwi_serializer_pool);
- ast_sip_unregister_subscription_handler(&mwi_handler);
+ if (!mwi_container) {
+ unload_module();
return AST_MODULE_LOAD_DECLINE;
}
+ ao2_global_obj_replace_unref(mwi_solicited, mwi_container);
+ ao2_ref(mwi_container, -1);
- unsolicited_mwi = ao2_container_alloc_hash(AO2_ALLOC_OPT_LOCK_MUTEX, 0, MWI_BUCKETS,
+ mwi_container = ao2_container_alloc_hash(AO2_ALLOC_OPT_LOCK_MUTEX, 0, MWI_BUCKETS,
mwi_sub_hash, NULL, mwi_sub_cmp);
- if (!unsolicited_mwi) {
- ast_serializer_pool_destroy(mwi_serializer_pool);
- ast_sip_unregister_subscription_handler(&mwi_handler);
- ao2_ref(solicited_mwi, -1);
+ if (!mwi_container) {
+ unload_module();
return AST_MODULE_LOAD_DECLINE;
}
+ ao2_global_obj_replace_unref(mwi_unsolicited, mwi_container);
+ ao2_ref(mwi_container, -1);
ast_sorcery_observer_add(ast_sip_get_sorcery(), "contact", &mwi_contact_observer);
ast_sorcery_observer_add(ast_sip_get_sorcery(), "global", &global_observer);
@@ -1493,35 +1605,6 @@
return AST_MODULE_LOAD_SUCCESS;
}
-static int unload_module(void)
-{
- ast_sorcery_observer_remove(ast_sip_get_sorcery(), "global", &global_observer);
- ast_sorcery_observer_remove(ast_sip_get_sorcery(), "contact", &mwi_contact_observer);
-
- if (unsolicited_mwi) {
- ao2_callback(unsolicited_mwi, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE, unsubscribe, NULL);
- ao2_ref(unsolicited_mwi, -1);
- unsolicited_mwi = NULL;
- }
-
- if (solicited_mwi) {
- ao2_ref(solicited_mwi, -1);
- solicited_mwi = NULL;
- }
-
- if (ast_serializer_pool_destroy(mwi_serializer_pool)) {
- ast_log(LOG_WARNING, "Unload incomplete. Try again later\n");
- return -1;
- }
- mwi_serializer_pool = NULL;
-
- ast_sip_unregister_subscription_handler(&mwi_handler);
-
- ast_free(default_voicemail_extension);
- default_voicemail_extension = NULL;
- return 0;
-}
-
AST_MODULE_INFO(ASTERISK_GPL_KEY, AST_MODFLAG_LOAD_ORDER, "PJSIP MWI resource",
.support_level = AST_MODULE_SUPPORT_CORE,
.load = load_module,
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13006
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 17
Gerrit-Change-Id: I8f812286dc19a34916acacd71ce2ec26e1042047
Gerrit-Change-Number: 13006
Gerrit-PatchSet: 3
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191010/6c9b3e42/attachment-0001.html>
More information about the asterisk-code-review
mailing list