[Asterisk-code-review] CDR: Fix deadlock setting some CDR values. (asterisk[15])
Jenkins2
asteriskteam at digium.com
Mon Dec 11 08:51:38 CST 2017
Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/7456 )
Change subject: CDR: Fix deadlock setting some CDR values.
......................................................................
CDR: Fix deadlock setting some CDR values.
Setting channel variables with the AMI Originate action caused a deadlock
when you set CDR(amaflags) or CDR(accountcode). This path has the channel
locked when the CDR function is called. The CDR function then
synchronously passes the job to a stasis thread. The stasis handling
function then attempts to lock the channel. Deadlock results.
* Avoid deadlock by making the CDR function handle setting amaflags and
accountcode directly on the channel rather than passing it off to the CDR
processing code under a stasis thread to do it.
* Made the CHANNEL function and the CDR function process amaflags the same
way.
* Fixed referencing the wrong message type in cdr_prop_write().
ASTERISK-27460
Change-Id: I5eacb47586bc0b8f8ff76a19bd92d1dc38b75e8f
---
M funcs/func_cdr.c
M funcs/func_channel.c
2 files changed, 90 insertions(+), 58 deletions(-)
Approvals:
Joshua Colp: Looks good to me, but someone else must approve
George Joseph: Looks good to me, approved
Jenkins2: Approved for Submit
diff --git a/funcs/func_cdr.c b/funcs/func_cdr.c
index 5734312..2dd9f15 100644
--- a/funcs/func_cdr.c
+++ b/funcs/func_cdr.c
@@ -356,7 +356,7 @@
static void cdr_write_callback(void *data, struct stasis_subscription *sub, struct stasis_message *message)
{
- struct cdr_func_payload *payload = stasis_message_data(message);
+ struct cdr_func_payload *payload;
struct ast_flags flags = { 0 };
AST_DECLARE_APP_ARGS(args,
AST_APP_ARG(variable);
@@ -367,21 +367,17 @@
if (cdr_write_message_type() != stasis_message_type(message)) {
return;
}
-
+ payload = stasis_message_data(message);
if (!payload) {
return;
}
+ if (ast_strlen_zero(payload->arguments)
+ || !payload->value) {
+ /* Sanity check. cdr_write() could never send these bad messages */
+ ast_assert(0);
+ return;
+ }
- if (ast_strlen_zero(payload->arguments)) {
- ast_log(AST_LOG_WARNING, "%s requires a variable (%s(variable)=value)\n)",
- payload->cmd, payload->cmd);
- return;
- }
- if (!payload->value) {
- ast_log(AST_LOG_WARNING, "%s requires a value (%s(variable)=value)\n)",
- payload->cmd, payload->cmd);
- return;
- }
parse = ast_strdupa(payload->arguments);
AST_STANDARD_APP_ARGS(args, parse);
@@ -389,32 +385,16 @@
ast_app_parse_options(cdr_func_options, &flags, NULL, args.options);
}
- if (!strcasecmp(args.variable, "accountcode")) {
- ast_log(AST_LOG_WARNING, "Using the CDR function to set 'accountcode' is deprecated. Please use the CHANNEL function instead.\n");
- ast_channel_lock(payload->chan);
- ast_channel_accountcode_set(payload->chan, payload->value);
- ast_channel_unlock(payload->chan);
- } else if (!strcasecmp(args.variable, "peeraccount")) {
- ast_log(AST_LOG_WARNING, "The 'peeraccount' setting is not supported. Please set the 'accountcode' on the appropriate channel using the CHANNEL function.\n");
- } else if (!strcasecmp(args.variable, "userfield")) {
+ /* These are already handled by cdr_write() */
+ ast_assert(strcasecmp(args.variable, "accountcode")
+ && strcasecmp(args.variable, "peeraccount")
+ && strcasecmp(args.variable, "amaflags"));
+
+ if (!strcasecmp(args.variable, "userfield")) {
ast_cdr_setuserfield(ast_channel_name(payload->chan), payload->value);
- } else if (!strcasecmp(args.variable, "amaflags")) {
- ast_log(AST_LOG_WARNING, "Using the CDR function to set 'amaflags' is deprecated. Please use the CHANNEL function instead.\n");
- if (isdigit(*payload->value)) {
- int amaflags;
- sscanf(payload->value, "%30d", &amaflags);
- ast_channel_lock(payload->chan);
- ast_channel_amaflags_set(payload->chan, amaflags);
- ast_channel_unlock(payload->chan);
- } else {
- ast_channel_lock(payload->chan);
- ast_channel_amaflags_set(payload->chan, ast_channel_string2amaflag(payload->value));
- ast_channel_unlock(payload->chan);
- }
} else {
ast_cdr_setvar(ast_channel_name(payload->chan), args.variable, payload->value);
}
- return;
}
static void cdr_prop_write_callback(void *data, struct stasis_subscription *sub, struct stasis_message *message)
@@ -523,27 +503,70 @@
return 0;
}
-static int cdr_write(struct ast_channel *chan, const char *cmd, char *parse,
- const char *value)
+static int cdr_write(struct ast_channel *chan, const char *cmd, char *arguments,
+ const char *value)
{
- RAII_VAR(struct stasis_message *, message, NULL, ao2_cleanup);
- RAII_VAR(struct cdr_func_payload *, payload, NULL, ao2_cleanup);
- RAII_VAR(struct stasis_message_router *, router,
- ast_cdr_message_router(), ao2_cleanup);
+ struct stasis_message *message;
+ struct cdr_func_payload *payload;
+ struct stasis_message_router *router;
+ AST_DECLARE_APP_ARGS(args,
+ AST_APP_ARG(variable);
+ AST_APP_ARG(options);
+ );
+ char *parse;
if (!chan) {
ast_log(LOG_WARNING, "No channel was provided to %s function.\n", cmd);
return -1;
}
-
- if (!router) {
- ast_log(AST_LOG_WARNING, "Failed to manipulate CDR for channel %s: no message router\n",
- ast_channel_name(chan));
+ if (ast_strlen_zero(arguments)) {
+ ast_log(LOG_WARNING, "%s requires a variable (%s(variable)=value)\n)",
+ cmd, cmd);
+ return -1;
+ }
+ if (!value) {
+ ast_log(LOG_WARNING, "%s requires a value (%s(variable)=value)\n)",
+ cmd, cmd);
return -1;
}
+ parse = ast_strdupa(arguments);
+ AST_STANDARD_APP_ARGS(args, parse);
+
+ /* These CDR variables are no longer supported or set directly on the channel */
+ if (!strcasecmp(args.variable, "accountcode")) {
+ ast_log(LOG_WARNING, "Using the %s function to set 'accountcode' is deprecated. Please use the CHANNEL function instead.\n",
+ cmd);
+ ast_channel_lock(chan);
+ ast_channel_accountcode_set(chan, value);
+ ast_channel_unlock(chan);
+ return 0;
+ }
+ if (!strcasecmp(args.variable, "amaflags")) {
+ int amaflags;
+
+ ast_log(LOG_WARNING, "Using the %s function to set 'amaflags' is deprecated. Please use the CHANNEL function instead.\n",
+ cmd);
+ if (isdigit(*value)) {
+ if (sscanf(value, "%30d", &amaflags) != 1) {
+ amaflags = AST_AMA_NONE;
+ }
+ } else {
+ amaflags = ast_channel_string2amaflag(value);
+ }
+ ast_channel_lock(chan);
+ ast_channel_amaflags_set(chan, amaflags);
+ ast_channel_unlock(chan);
+ return 0;
+ }
+ if (!strcasecmp(args.variable, "peeraccount")) {
+ ast_log(LOG_WARNING, "The 'peeraccount' setting is not supported. Please set the 'accountcode' on the appropriate channel using the CHANNEL function.\n");
+ return 0;
+ }
+
+ /* The remaining CDR variables are handled by CDR processing code */
if (!cdr_write_message_type()) {
- ast_log(AST_LOG_WARNING, "Failed to manipulate CDR for channel %s: message type not available\n",
+ ast_log(LOG_WARNING, "Failed to manipulate CDR for channel %s: message type not available\n",
ast_channel_name(chan));
return -1;
}
@@ -554,16 +577,26 @@
}
payload->chan = chan;
payload->cmd = cmd;
- payload->arguments = parse;
+ payload->arguments = arguments;
payload->value = value;
message = stasis_message_create(cdr_write_message_type(), payload);
+ ao2_ref(payload, -1);
if (!message) {
- ast_log(AST_LOG_WARNING, "Failed to manipulate CDR for channel %s: unable to create message\n",
+ ast_log(LOG_WARNING, "Failed to manipulate CDR for channel %s: unable to create message\n",
ast_channel_name(chan));
return -1;
}
+ router = ast_cdr_message_router();
+ if (!router) {
+ ast_log(LOG_WARNING, "Failed to manipulate CDR for channel %s: no message router\n",
+ ast_channel_name(chan));
+ ao2_ref(message, -1);
+ return -1;
+ }
stasis_message_router_publish_sync(router, message);
+ ao2_ref(router, -1);
+ ao2_ref(message, -1);
return 0;
}
@@ -586,7 +619,7 @@
return -1;
}
- if (!cdr_write_message_type()) {
+ if (!cdr_prop_write_message_type()) {
ast_log(AST_LOG_WARNING, "Failed to manipulate CDR for channel %s: message type not available\n",
ast_channel_name(chan));
return -1;
diff --git a/funcs/func_channel.c b/funcs/func_channel.c
index eb3cedd..b72cb14 100644
--- a/funcs/func_channel.c
+++ b/funcs/func_channel.c
@@ -495,18 +495,17 @@
ast_bridge_set_after_go_on(chan, ast_channel_context(chan), ast_channel_exten(chan), ast_channel_priority(chan), value);
}
} else if (!strcasecmp(data, "amaflags")) {
- ast_channel_lock(chan);
+ int amaflags;
+
if (isdigit(*value)) {
- int amaflags;
- sscanf(value, "%30d", &amaflags);
- ast_channel_amaflags_set(chan, amaflags);
- } else if (!strcasecmp(value,"OMIT")){
- ast_channel_amaflags_set(chan, 1);
- } else if (!strcasecmp(value,"BILLING")){
- ast_channel_amaflags_set(chan, 2);
- } else if (!strcasecmp(value,"DOCUMENTATION")){
- ast_channel_amaflags_set(chan, 3);
+ if (sscanf(value, "%30d", &amaflags) != 1) {
+ amaflags = AST_AMA_NONE;
+ }
+ } else {
+ amaflags = ast_channel_string2amaflag(value);
}
+ ast_channel_lock(chan);
+ ast_channel_amaflags_set(chan, amaflags);
ast_channel_unlock(chan);
} else if (!strcasecmp(data, "peeraccount"))
locked_string_field_set(chan, peeraccount, value);
--
To view, visit https://gerrit.asterisk.org/7456
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: merged
Gerrit-Change-Id: I5eacb47586bc0b8f8ff76a19bd92d1dc38b75e8f
Gerrit-Change-Number: 7456
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171211/9371c3bc/attachment-0001.html>
More information about the asterisk-code-review
mailing list