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