[Asterisk-code-review] res parking: Misc fixes. (asterisk[13])

Anonymous Coward asteriskteam at digium.com
Tue Mar 29 08:53:47 CDT 2016


Anonymous Coward #1000019 has submitted this change and it was merged.

Change subject: res_parking: Misc fixes.
......................................................................


res_parking: Misc fixes.

res/parking/parking_applications.c:

* Add malloc fail checks in setup_park_common_datastore().

* Fix playing parking failed announcement to only happen on non-blind
transfers in park_app_exec().  It could never go out before because a test
was provedly always false.

res/parking/parking_bridge.c:

* Fix NULL tolerance in generate_parked_user() because
bridge_parking_push() can theoretically pass a NULL parker channel if the
parker channel went away for some reason.

* Clarify some weird code dealing with blind_transfer in
bridge_parking_push().

res/parking/parking_bridge_features.c:

* Made park_local_transfer() set BLINDTRANSFER on the Local;1 channel
which will be bulk copied to the Local;2 channel on the subsequent
ast_call().  The additional advantage is if the parker channel has the
BLINDTRANSFER and ATTENDEDTRANSFER variables set they are now guaranteed
to be overridden.

res/parking/parking_manager.c:

* Fix AMI Park action input range checking of the Timeout header in
manager_park().

* Reduced locking scope to where needed in manager_park().

res/res_parking.c:

* Fix some off nominal missing unlocks by eliminating the returns.

Change-Id: Ib64945bc285acb05a306dc12e6f16854898915ca
---
M res/parking/parking_applications.c
M res/parking/parking_bridge.c
M res/parking/parking_bridge_features.c
M res/parking/parking_manager.c
M res/res_parking.c
5 files changed, 38 insertions(+), 52 deletions(-)

Approvals:
  Anonymous Coward #1000019: Verified
  Matt Jordan: Looks good to me, approved
  Joshua Colp: Looks good to me, but someone else must approve



diff --git a/res/parking/parking_applications.c b/res/parking/parking_applications.c
index d8cda6f..1bdb50f 100644
--- a/res/parking/parking_applications.c
+++ b/res/parking/parking_applications.c
@@ -339,16 +339,17 @@
 		ast_datastore_free(datastore);
 		return -1;
 	}
+	datastore->data = park_datastore;
 
-	if (parker_uuid) {
-		park_datastore->parker_uuid = ast_strdup(parker_uuid);
+	park_datastore->parker_uuid = ast_strdup(parker_uuid);
+	if (!park_datastore->parker_uuid) {
+		ast_datastore_free(datastore);
+		return -1;
 	}
 
 	ast_channel_lock(parkee);
-
 	attended_transfer = pbx_builtin_getvar_helper(parkee, "ATTENDEDTRANSFER");
 	blind_transfer = pbx_builtin_getvar_helper(parkee, "BLINDTRANSFER");
-
 	if (!ast_strlen_zero(attended_transfer)) {
 		parker_dial_string = ast_strdupa(attended_transfer);
 	} else if (!ast_strlen_zero(blind_transfer)) {
@@ -356,7 +357,6 @@
 		/* Ensure that attended_transfer is NULL and not an empty string. */
 		attended_transfer = NULL;
 	}
-
 	ast_channel_unlock(parkee);
 
 	if (!ast_strlen_zero(parker_dial_string)) {
@@ -365,6 +365,10 @@
 			parker_dial_string,
 			attended_transfer ? "ATTENDEDTRANSFER" : "BLINDTRANSFER");
 		park_datastore->parker_dial_string = ast_strdup(parker_dial_string);
+		if (!park_datastore->parker_dial_string) {
+			ast_datastore_free(datastore);
+			return -1;
+		}
 	}
 
 	park_datastore->randomize = randomize;
@@ -373,10 +377,13 @@
 
 	if (comeback_override) {
 		park_datastore->comeback_override = ast_strdup(comeback_override);
+		if (!park_datastore->comeback_override) {
+			ast_datastore_free(datastore);
+			return -1;
+		}
 	}
 
 
-	datastore->data = park_datastore;
 	ast_channel_lock(parkee);
 	ast_channel_datastore_add(parkee, datastore);
 	ast_channel_unlock(parkee);
@@ -391,23 +398,23 @@
 	struct park_common_datastore *data_copy;
 
 	SCOPED_CHANNELLOCK(lock, parkee);
+
 	if (!(datastore = ast_channel_datastore_find(parkee, &park_common_info, NULL))) {
 		return NULL;
 	}
 
 	data = datastore->data;
 
-	if (!data) {
-		/* This data should always be populated if this datastore was appended to the channel */
-		ast_assert(0);
-	}
+	/* This data should always be populated if this datastore was appended to the channel */
+	ast_assert(data != NULL);
 
 	data_copy = ast_calloc(1, sizeof(*data_copy));
 	if (!data_copy) {
 		return NULL;
 	}
 
-	if (!(data_copy->parker_uuid = ast_strdup(data->parker_uuid))) {
+	data_copy->parker_uuid = ast_strdup(data->parker_uuid);
+	if (!data_copy->parker_uuid) {
 		park_common_datastore_free(data_copy);
 		return NULL;
 	}
@@ -457,7 +464,6 @@
 	if (!lot) {
 		lot = parking_create_dynamic_lot(lot_name, parkee);
 	}
-
 	if (!lot) {
 		ast_log(LOG_ERROR, "Could not find parking lot: '%s'\n", lot_name);
 		return NULL;
@@ -504,7 +510,7 @@
 	struct ast_bridge_features chan_features;
 	int res;
 	int silence_announcements = 0;
-	const char *transferer;
+	int blind_transfer;
 
 	/* Answer the channel if needed */
 	if (ast_channel_state(chan) != AST_STATE_UP) {
@@ -512,15 +518,12 @@
 	}
 
 	ast_channel_lock(chan);
-	if (!(transferer = pbx_builtin_getvar_helper(chan, "ATTENDEDTRANSFER"))) {
-		transferer = pbx_builtin_getvar_helper(chan, "BLINDTRANSFER");
-	}
-	transferer = ast_strdupa(S_OR(transferer, ""));
+	blind_transfer = !ast_strlen_zero(pbx_builtin_getvar_helper(chan, "BLINDTRANSFER"));
 	ast_channel_unlock(chan);
 
 	/* Handle the common parking setup stuff */
 	if (!(parking_bridge = park_application_setup(chan, NULL, data, &silence_announcements))) {
-		if (!silence_announcements && !transferer) {
+		if (!silence_announcements && !blind_transfer) {
 			ast_stream_and_wait(chan, "pbx-parkingfailed", "");
 		}
 		publish_parked_call_failure(chan);
@@ -593,7 +596,6 @@
 	}
 
 	lot = parking_lot_find_by_name(lot_name);
-
 	if (!lot) {
 		ast_log(LOG_ERROR, "Could not find the requested parking lot\n");
 		ast_stream_and_wait(chan, "pbx-invalidpark", "");
diff --git a/res/parking/parking_bridge.c b/res/parking/parking_bridge.c
index 4f7198b..e61486e 100644
--- a/res/parking/parking_bridge.c
+++ b/res/parking/parking_bridge.c
@@ -167,7 +167,7 @@
 	if (parker_dial_string) {
 		new_parked_user->parker_dial_string = ast_strdup(parker_dial_string);
 	} else {
-		if (parked_user_set_parker_dial_string(new_parked_user, parker)) {
+		if (!parker || parked_user_set_parker_dial_string(new_parked_user, parker)) {
 			ao2_ref(new_parked_user, -1);
 			ao2_unlock(lot);
 			return NULL;
@@ -269,14 +269,11 @@
 	 * the park application. It's possible that the channel that transferred it is still alive (particularly
 	 * when a multichannel bridge is parked), so try to get the real parker if possible. */
 	ast_channel_lock(bridge_channel->chan);
-	blind_transfer = S_OR(pbx_builtin_getvar_helper(bridge_channel->chan, "BLINDTRANSFER"),
-		ast_channel_name(bridge_channel->chan));
-	if (blind_transfer) {
-		blind_transfer = ast_strdupa(blind_transfer);
-	}
+	blind_transfer = pbx_builtin_getvar_helper(bridge_channel->chan, "BLINDTRANSFER");
+	blind_transfer = ast_strdupa(S_OR(blind_transfer, ""));
 	ast_channel_unlock(bridge_channel->chan);
-
-	if (parker == bridge_channel->chan) {
+	if ((!parker || parker == bridge_channel->chan)
+		&& !ast_strlen_zero(blind_transfer)) {
 		struct ast_channel *real_parker = ast_channel_get_by_name(blind_transfer);
 
 		if (real_parker) {
@@ -300,8 +297,8 @@
 	/* Generate ParkedCall Stasis Message */
 	publish_parked_call(pu, PARKED_CALL);
 
-	/* If the parkee and the parker are the same and silence_announce isn't set, play the announcement to the parkee */
-	if (!strcmp(blind_transfer, ast_channel_name(bridge_channel->chan)) && !park_datastore->silence_announce) {
+	/* If not a blind transfer and silence_announce isn't set, play the announcement to the parkee */
+	if (ast_strlen_zero(blind_transfer) && !park_datastore->silence_announce) {
 		char saynum_buf[16];
 
 		snprintf(saynum_buf, sizeof(saynum_buf), "%d %d", 0, pu->parking_space);
diff --git a/res/parking/parking_bridge_features.c b/res/parking/parking_bridge_features.c
index a21be90..ab0ea6f 100644
--- a/res/parking/parking_bridge_features.c
+++ b/res/parking/parking_bridge_features.c
@@ -238,6 +238,7 @@
 	ast_channel_req_accountcodes(parkee, parker, AST_CHANNEL_REQUESTOR_REPLACEMENT);
 	ast_connected_line_copy_from_caller(ast_channel_connected(parkee), ast_channel_caller(parker));
 	ast_channel_inherit_variables(parker, parkee);
+	ast_bridge_set_transfer_variables(parkee, ast_channel_name(parker), 0);
 	ast_channel_datastore_inherit(parker, parkee);
 	ast_channel_unlock(parker);
 
@@ -251,8 +252,6 @@
 		ast_hangup(parkee);
 		return NULL;
 	}
-
-	ast_bridge_set_transfer_variables(parkee_side_2, ast_channel_name(parker), 0);
 
 	ast_channel_unref(parkee_side_2);
 
diff --git a/res/parking/parking_manager.c b/res/parking/parking_manager.c
index 435200b..ed28164 100644
--- a/res/parking/parking_manager.c
+++ b/res/parking/parking_manager.c
@@ -537,12 +537,12 @@
 	}
 
 	if (!ast_strlen_zero(timeout)) {
-		if (sscanf(timeout, "%30d", &timeout_override) != 1 || timeout < 0) {
+		if (sscanf(timeout, "%30d", &timeout_override) != 1 || timeout_override < 0) {
 			astman_send_error(s, m, "Invalid Timeout value.");
 			return 0;
 		}
 
-		if (timeout_override > 0) {
+		if (timeout_override) {
 			/* If greater than zero, convert to seconds for internal use. Must be >= 1 second. */
 			timeout_override = MAX(1, timeout_override / 1000);
 		}
@@ -554,11 +554,11 @@
 		return 0;
 	}
 
-	ast_channel_lock(chan);
 	if (!ast_strlen_zero(timeout_channel)) {
+		ast_channel_lock(chan);
 		ast_bridge_set_transfer_variables(chan, timeout_channel, 0);
+		ast_channel_unlock(chan);
 	}
-	ast_channel_unlock(chan);
 
 	parker_chan = ast_channel_bridge_peer(chan);
 	if (!parker_chan || strcmp(ast_channel_name(parker_chan), timeout_channel)) {
diff --git a/res/res_parking.c b/res/res_parking.c
index 3edbd46..6696980 100644
--- a/res/res_parking.c
+++ b/res/res_parking.c
@@ -738,31 +738,21 @@
 	}
 
 	/* We need the contexts list locked to safely be able to both read and lock the specific context within */
-	if (ast_wrlock_contexts()) {
-		ast_log(LOG_ERROR, "Failed to lock the contexts list.\n");
-		return -1;
-	}
+	ast_wrlock_contexts();
 
 	if (!(lot_context = ast_context_find_or_create(NULL, NULL, lot_cfg->parking_con, parkext_registrar_pointer))) {
 		ast_log(LOG_ERROR, "Parking lot '%s' -- Needs a context '%s' which does not exist and Asterisk was unable to create\n",
 			lot_cfg->name, lot_cfg->parking_con);
-		if (ast_unlock_contexts()) {
-			ast_assert(0);
-		}
+		ast_unlock_contexts();
 		return -1;
 	}
 
 	/* Once we know what context we will be modifying, we need to write lock it because we will be reading extensions
 	 * and we don't want something else to destroy them while we are looking at them.
 	 */
-	if (ast_wrlock_context(lot_context)) {
-		ast_log(LOG_ERROR, "failed to obtain write lock on context\n");
-		return -1;
-	}
+	ast_wrlock_context(lot_context);
 
-	if (ast_unlock_contexts()) {
-		ast_assert(0);
-	}
+	ast_unlock_contexts();
 
 	/* Handle generation/confirmation for the Park extension */
 	if ((existing_exten = pbx_find_extension(NULL, NULL, &find_info, lot_cfg->parking_con, lot_cfg->parkext, 1, NULL, NULL, E_MATCH))) {
@@ -830,9 +820,7 @@
 		}
 	}
 
-	if (ast_unlock_context(lot_context)) {
-		ast_assert(0);
-	}
+	ast_unlock_context(lot_context);
 
 	return 0;
 }

-- 
To view, visit https://gerrit.asterisk.org/2473
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib64945bc285acb05a306dc12e6f16854898915ca
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>



More information about the asterisk-code-review mailing list