1 files changed, 136 insertions, 49 deletions

diff --git a/res/res_pjsip_outbound_publish.c b/res/res_pjsip_outbound_publish.c
index 073a2d1ed..ab8a6c9bd 100644
--- a/res/res_pjsip_outbound_publish.c
+++ b/res/res_pjsip_outbound_publish.c
@@ -406,22 +406,30 @@ static void sip_outbound_publish_synchronize(struct ast_sip_event_publisher_hand
 	ao2_ref(states, -1);
 }
 
-struct ast_sip_outbound_publish_client *ast_sip_publish_client_get(const char *name)
+static struct ast_sip_outbound_publish_state *sip_publish_state_get(const char *id)
 {
-	RAII_VAR(struct ao2_container *, states,
-		 ao2_global_obj_ref(current_states), ao2_cleanup);
-	RAII_VAR(struct ast_sip_outbound_publish_state *, state, NULL, ao2_cleanup);
+	struct ao2_container *states = ao2_global_obj_ref(current_states);
+	struct ast_sip_outbound_publish_state *res;
 
 	if (!states) {
 		return NULL;
 	}
 
-	state = ao2_find(states, name, OBJ_SEARCH_KEY);
+	res = ao2_find(states, id, OBJ_SEARCH_KEY);
+	ao2_ref(states, -1);
+	return res;
+}
+
+struct ast_sip_outbound_publish_client *ast_sip_publish_client_get(const char *name)
+{
+	struct ast_sip_outbound_publish_state *state = sip_publish_state_get(name);
+
 	if (!state) {
 		return NULL;
 	}
 
 	ao2_ref(state->client, +1);
+	ao2_ref(state, -1);
 	return state->client;
 }
 
@@ -687,8 +695,15 @@ static int explicit_publish_destroy(void *data)
 {
 	struct ast_sip_outbound_publish_client *client = data;
 
-	pjsip_publishc_destroy(client->client);
-	ao2_ref(client, -1);
+	/*
+	 * If there is no pjsip publishing client then we obviously don't need
+	 * to destroy it. Also, the ref for the Asterisk publishing client that
+	 * pjsip had would not exist or should already be gone as well.
+	 */
+	if (client->client) {
+		pjsip_publishc_destroy(client->client);
+		ao2_ref(client, -1);
+	}
 
 	return 0;
 }
@@ -868,6 +883,11 @@ static int sip_outbound_publish_client_alloc(void *data)
 /*! \brief Callback function for publish client responses */
 static void sip_outbound_publish_callback(struct pjsip_publishc_cbparam *param)
 {
+#define DESTROY_CLIENT() do {			   \
+	pjsip_publishc_destroy(client->client); \
+	client->client = NULL; \
+	ao2_ref(client, -1); } while (0)
+
 	RAII_VAR(struct ast_sip_outbound_publish_client *, client, ao2_bump(param->token), ao2_cleanup);
 	RAII_VAR(struct ast_sip_outbound_publish *, publish, ao2_bump(client->publish), ao2_cleanup);
 	SCOPED_AO2LOCK(lock, client);
@@ -885,8 +905,7 @@ static void sip_outbound_publish_callback(struct pjsip_publishc_cbparam *param)
 			ao2_ref(client, -1);
 		}
 		/* Once the destroy is called this callback will not get called any longer, so drop the client ref */
-		pjsip_publishc_destroy(client->client);
-		ao2_ref(client, -1);
+		DESTROY_CLIENT();
 		return;
 	}
 
@@ -904,9 +923,7 @@ static void sip_outbound_publish_callback(struct pjsip_publishc_cbparam *param)
 		client->auth_attempts++;
 
 		if (client->auth_attempts == publish->max_auth_attempts) {
-			pjsip_publishc_destroy(client->client);
-			client->client = NULL;
-
+			DESTROY_CLIENT();
 			ast_log(LOG_ERROR, "Reached maximum number of PUBLISH authentication attempts on outbound publish '%s'\n",
 				ast_sorcery_object_get_id(publish));
 
@@ -918,9 +935,7 @@ static void sip_outbound_publish_callback(struct pjsip_publishc_cbparam *param)
 	client->auth_attempts = 0;
 
 	if (param->code == 412) {
-		pjsip_publishc_destroy(client->client);
-		client->client = NULL;
-
+		DESTROY_CLIENT();
 		if (sip_outbound_publish_client_alloc(client)) {
 			ast_log(LOG_ERROR, "Failed to create a new outbound publish client for '%s' on 412 response\n",
 				ast_sorcery_object_get_id(publish));
@@ -935,10 +950,9 @@ static void sip_outbound_publish_callback(struct pjsip_publishc_cbparam *param)
 
 		expires = pjsip_msg_find_hdr(param->rdata->msg_info.msg, PJSIP_H_MIN_EXPIRES, NULL);
 		if (!expires || !expires->ivalue) {
+			DESTROY_CLIENT();
 			ast_log(LOG_ERROR, "Received 423 response on outbound publish '%s' without a Min-Expires header\n",
 				ast_sorcery_object_get_id(publish));
-			pjsip_publishc_destroy(client->client);
-			client->client = NULL;
 			goto end;
 		}
 
@@ -1065,25 +1079,89 @@ static struct ast_sip_outbound_publish_state *sip_outbound_publish_state_alloc(
 	return state;
 }
 
-/*! \brief Apply function which finds or allocates a state structure */
-static int sip_outbound_publish_apply(const struct ast_sorcery *sorcery, void *obj)
+static int initialize_publish_client(struct ast_sip_outbound_publish *publish,
+				     struct ast_sip_outbound_publish_state *state)
 {
-	RAII_VAR(struct ao2_container *, states, ao2_global_obj_ref(current_states), ao2_cleanup);
-	RAII_VAR(struct ast_sip_outbound_publish_state *, state, NULL, ao2_cleanup);
-	struct ast_sip_outbound_publish *applied = obj;
+	if (ast_sip_push_task_synchronous(NULL, sip_outbound_publish_client_alloc, state->client)) {
+		ast_log(LOG_ERROR, "Unable to create client for outbound publish '%s'\n",
+			ast_sorcery_object_get_id(publish));
+		return -1;
+	}
 
-	if (ast_strlen_zero(applied->server_uri)) {
+	return 0;
+}
+
+static int validate_publish_config(struct ast_sip_outbound_publish *publish)
+{
+	if (ast_strlen_zero(publish->server_uri)) {
 		ast_log(LOG_ERROR, "No server URI specified on outbound publish '%s'\n",
-			ast_sorcery_object_get_id(applied));
+			ast_sorcery_object_get_id(publish));
 		return -1;
-	} else if (ast_strlen_zero(applied->event)) {
+	} else if (ast_strlen_zero(publish->event)) {
 		ast_log(LOG_ERROR, "No event type specified for outbound publish '%s'\n",
-			ast_sorcery_object_get_id(applied));
+			ast_sorcery_object_get_id(publish));
 		return -1;
 	}
+	return 0;
+}
+
+static int current_state_reusable(struct ast_sip_outbound_publish *publish,
+				  struct ast_sip_outbound_publish_state *current_state)
+{
+	struct ast_sip_outbound_publish *old_publish;
 
+	if (!can_reuse_publish(current_state->client->publish, publish)) {
+		/*
+		 * Something significant has changed in the configuration, so we are
+		 * unable to use the old state object. The current state needs to go
+		 * away and a new one needs to be created.
+		 */
+		return 0;
+	}
+
+	/*
+	 * We can reuse the current state object so keep it, but swap out the
+	 * underlying publish object with the new one.
+	 */
+	old_publish = current_state->client->publish;
+	current_state->client->publish = publish;
+	if (initialize_publish_client(publish, current_state)) {
+		/*
+		 * If the state object fails to re-initialize then swap
+		 * the old publish info back in.
+		 */
+		current_state->client->publish = publish;
+		return -1;
+	}
+
+	/*
+	 * Since we swapped out the publish object the new one needs a ref
+	 * while the old one needs to go away.
+	 */
+	ao2_ref(current_state->client->publish, +1);
+	ao2_cleanup(old_publish);
+
+	/* Tell the caller that the current state object should be used */
+	return 1;
+}
+
+/*! \brief Apply function which finds or allocates a state structure */
+static int sip_outbound_publish_apply(const struct ast_sorcery *sorcery, void *obj)
+{
+#define ADD_TO_NEW_STATES(__obj) \
+	do { if (__obj) { \
+	     ao2_link(new_states, __obj); \
+	     ao2_ref(__obj, -1); } } while (0)
+
+	struct ast_sip_outbound_publish *applied = obj;
+	struct ast_sip_outbound_publish_state *current_state, *new_state;
+	int res;
+
+	/*
+	 * New states are being loaded or reloaded. We'll need to add the new
+	 * object if created/updated, or keep the old object if an error occurs.
+	 */
 	if (!new_states) {
-		/* make sure new_states has been allocated as we will be adding to it */
 		new_states = ao2_container_alloc_options(
 			AO2_ALLOC_OPT_LOCK_NOLOCK, DEFAULT_STATE_BUCKETS,
 			outbound_publish_state_hash, outbound_publish_state_cmp);
@@ -1094,35 +1172,44 @@ static int sip_outbound_publish_apply(const struct ast_sorcery *sorcery, void *o
 		}
 	}
 
-	if (states) {
-		state = ao2_find(states, ast_sorcery_object_get_id(obj), OBJ_SEARCH_KEY);
-		if (state) {
-			if (can_reuse_publish(state->client->publish, applied)) {
-				ao2_replace(state->client->publish, applied);
-			} else {
-				ao2_ref(state, -1);
-				state = NULL;
-			}
-		}
+	/* If there is current state we'll want to maintain it if any errors occur */
+	current_state = sip_publish_state_get(ast_sorcery_object_get_id(applied));
+
+	if ((res = validate_publish_config(applied))) {
+		ADD_TO_NEW_STATES(current_state);
+		return res;
 	}
 
-	if (!state) {
-		state = sip_outbound_publish_state_alloc(applied);
-		if (!state) {
-			ast_log(LOG_ERROR, "Unable to create state for outbound publish '%s'\n",
-				ast_sorcery_object_get_id(applied));
-			return -1;
-		};
+	if (current_state && (res = current_state_reusable(applied, current_state))) {
+		/*
+		 * The current state object was able to be reused, or an error
+		 * occurred. Either way we keep the current state and be done.
+		 */
+		ADD_TO_NEW_STATES(current_state);
+		return res == 1 ? 0 : -1;
 	}
 
-	if (ast_sip_push_task_synchronous(NULL, sip_outbound_publish_client_alloc, state->client)) {
-		ast_log(LOG_ERROR, "Unable to create client for outbound publish '%s'\n",
+	/*
+	 * No current state was found or it was unable to be reused. Either way
+	 * we'll need to create a new state object.
+	 */
+	new_state = sip_outbound_publish_state_alloc(applied);
+	if (!new_state) {
+		ast_log(LOG_ERROR, "Unable to create state for outbound publish '%s'\n",
 			ast_sorcery_object_get_id(applied));
+		ADD_TO_NEW_STATES(current_state);
+		return -1;
+	};
+
+	if (initialize_publish_client(applied, new_state)) {
+		ADD_TO_NEW_STATES(current_state);
+		ao2_ref(new_state, -1);
 		return -1;
 	}
 
-	ao2_link(new_states, state);
-	return 0;
+	ADD_TO_NEW_STATES(new_state);
+	ao2_cleanup(current_state);
+	return res;
 }
 
 static int outbound_auth_handler(const struct aco_option *opt, struct ast_variable *var, void *obj)
@@ -1201,7 +1288,7 @@ static int unload_module(void)
 
 	/* wait for items to unpublish */
 	ast_verb(5, "Waiting to complete unpublishing task(s)\n");
-	while (unloading.count) {
+	while (unloading.count && !res) {
 		res = ast_cond_timedwait(&unloading.cond, &unloading.lock, &end);
 	}
 	ast_mutex_unlock(&unloading.lock);