[Asterisk-code-review] res_pjsip_mwi: use an ao2_global object for mwi containers (...asterisk[16])
Kevin Harwell
asteriskteam at digium.com
Wed Oct 2 11:39:00 CDT 2019
Kevin Harwell has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/13003
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(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/03/13003/1
diff --git a/res/res_pjsip_mwi.c b/res/res_pjsip_mwi.c
index 6961455..298e219 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;
@@ -587,13 +588,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) {
@@ -606,18 +609,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);
}
@@ -683,6 +693,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) {
@@ -712,11 +726,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;
@@ -763,11 +779,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;
@@ -833,28 +852,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;
}
@@ -980,6 +1012,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)) {
@@ -1000,7 +1033,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;
@@ -1147,11 +1185,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;
@@ -1186,14 +1226,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;
}
@@ -1233,13 +1275,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)
@@ -1253,9 +1298,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",
@@ -1263,9 +1315,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
@@ -1274,10 +1329,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 */
@@ -1304,6 +1361,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);
@@ -1318,10 +1377,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);
@@ -1347,6 +1416,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);
@@ -1369,6 +1439,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);
@@ -1379,6 +1454,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 */
@@ -1391,7 +1467,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;
}
@@ -1439,8 +1520,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;
}
@@ -1451,22 +1562,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);
@@ -1489,35 +1601,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/+/13003
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I8f812286dc19a34916acacd71ce2ec26e1042047
Gerrit-Change-Number: 13003
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191002/9bc2ee37/attachment-0001.html>
More information about the asterisk-code-review
mailing list