From 4a8b1940b896f7e783a5429d0fffee8bed0e4afd Mon Sep 17 00:00:00 2001 From: Mark Michelson Date: Thu, 3 Nov 2016 16:36:13 -0500 Subject: [PATCH 1/4] Add API for channel frame deferral. There are several places in Asterisk that have duplicated logic for deferring important frames until later. This commit adds a couple of API calls to facilitate this automatically. ast_channel_start_defer_frames(): Future reads of deferrable frames on this channel will be deferred until later. ast_channel_stop_defer_frames(): Any frames that have been deferred get requeued onto the channel. ASTERISK-26343 Change-Id: I3e1b87bc6796f222442fa6f7d1b6a4706fb33641 --- include/asterisk/channel.h | 38 +++++++++++++++++++++++++++++++ main/channel.c | 45 +++++++++++++++++++++++++++++++++++++ main/channel_internal_api.c | 6 +++++ 3 files changed, 89 insertions(+) diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h index 78db878b3c..cf8c5447b8 100644 --- a/include/asterisk/channel.h +++ b/include/asterisk/channel.h @@ -967,6 +967,11 @@ enum { * The channel is executing a subroutine or macro */ AST_FLAG_SUBROUTINE_EXEC = (1 << 27), + /*! + * The channel is currently in an operation where + * frames should be deferred. + */ + AST_FLAG_DEFER_FRAMES = (1 << 28), }; /*! \brief ast_bridge_config flags */ @@ -4661,4 +4666,37 @@ enum ast_channel_error { */ enum ast_channel_error ast_channel_errno(void); +/*! + * \brief Retrieve the deferred read queue. + */ +struct ast_readq_list *ast_channel_deferred_readq(struct ast_channel *chan); + +/*! + * \brief Start deferring deferrable frames on this channel + * + * Sometimes, a channel gets entered into a mode where a "main" application + * is tasked with servicing frames on the channel, but that application does + * not need to act on those frames. However, it would be imprudent to simply + * drop important frames. This function can be called so that important frames + * will be deferred, rather than placed in the channel frame queue as normal. + * + * \pre chan MUST be locked before calling + * + * \param chan The channel on which frames should be deferred + */ +void ast_channel_start_defer_frames(struct ast_channel *chan); + +/*! + * \brief Stop deferring deferrable frames on this channel + * + * When it is time to stop deferring frames on the channel, all deferred frames + * will be queued onto the channel's read queue so that the next servicer of + * the channel can handle those frames as necessary. + * + * \pre chan MUST be locked before calling + * + * \param chan The channel on which to stop deferring frames. + */ +void ast_channel_stop_defer_frames(struct ast_channel *chan); + #endif /* _ASTERISK_CHANNEL_H */ diff --git a/main/channel.c b/main/channel.c index 6e24ee6f70..f00f222c98 100644 --- a/main/channel.c +++ b/main/channel.c @@ -1064,6 +1064,25 @@ struct ast_channel *__ast_dummy_channel_alloc(const char *file, int line, const return tmp; } +void ast_channel_start_defer_frames(struct ast_channel *chan) +{ + ast_set_flag(ast_channel_flags(chan), AST_FLAG_DEFER_FRAMES); +} + +void ast_channel_stop_defer_frames(struct ast_channel *chan) +{ + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_DEFER_FRAMES); + + /* Move the deferred frames onto the channel read queue, ahead of other queued frames */ + ast_queue_frame_head(chan, AST_LIST_FIRST(ast_channel_deferred_readq(chan))); + /* ast_frfree will mosey down the list and free them all */ + if (!AST_LIST_EMPTY(ast_channel_deferred_readq(chan))) { + ast_frfree(AST_LIST_FIRST(ast_channel_deferred_readq(chan))); + } + /* Reset the list to be empty */ + AST_LIST_HEAD_INIT_NOLOCK(ast_channel_deferred_readq(chan)); +} + static int __ast_queue_frame(struct ast_channel *chan, struct ast_frame *fin, int head, struct ast_frame *after) { struct ast_frame *f; @@ -3885,6 +3904,32 @@ static struct ast_frame *__ast_read(struct ast_channel *chan, int dropaudio) if (!AST_LIST_EMPTY(ast_channel_readq(chan))) { int skip_dtmf = should_skip_dtmf(chan); + if (ast_test_flag(ast_channel_flags(chan), AST_FLAG_DEFER_FRAMES)) { + AST_LIST_TRAVERSE_SAFE_BEGIN(ast_channel_readq(chan), f, frame_list) { + if (ast_is_deferrable_frame(f)) { + if (f->frametype == AST_FRAME_CONTROL && f->subclass.integer == AST_CONTROL_HANGUP) { + struct ast_frame *dup; + + /* Hangup is a special case. We want to defer the frame, but we also do not + * want to remove it from the frame queue. So rather than just moving the frame + * over, we duplicate it and move the copy to the deferred readq. + * + * The reason for this? This way, whoever calls ast_read() will get a NULL return + * immediately and can tell the channel has hung up and do what it needs to. Also, + * when frame deferral finishes, then whoever calls ast_read() next will also get + * the hangup. + */ + dup = ast_frdup(f); + AST_LIST_INSERT_TAIL(ast_channel_deferred_readq(chan), dup, frame_list); + } else { + AST_LIST_INSERT_TAIL(ast_channel_deferred_readq(chan), f, frame_list); + AST_LIST_REMOVE_CURRENT(frame_list); + } + } + } + AST_LIST_TRAVERSE_SAFE_END; + } + AST_LIST_TRAVERSE_SAFE_BEGIN(ast_channel_readq(chan), f, frame_list) { /* We have to be picky about which frame we pull off of the readq because * there are cases where we want to leave DTMF frames on the queue until diff --git a/main/channel_internal_api.c b/main/channel_internal_api.c index 3c156d4fa2..a30c605c06 100644 --- a/main/channel_internal_api.c +++ b/main/channel_internal_api.c @@ -223,6 +223,7 @@ struct ast_channel { struct stasis_cp_single *topics; /*!< Topic for all channel's events */ struct stasis_forward *endpoint_forward; /*!< Subscription for event forwarding to endpoint's topic */ struct stasis_forward *endpoint_cache_forward; /*!< Subscription for cache updates to endpoint's topic */ + struct ast_readq_list deferred_readq; }; /*! \brief The monotonically increasing integer counter for channel uniqueids */ @@ -1683,3 +1684,8 @@ enum ast_channel_error ast_channel_internal_errno(void) return *error_code; } + +struct ast_readq_list *ast_channel_deferred_readq(struct ast_channel *chan) +{ + return &chan->deferred_readq; +} From 8d8323b1427b1abe60b4b8777dccb3a2232315e8 Mon Sep 17 00:00:00 2001 From: Mark Michelson Date: Thu, 3 Nov 2016 16:42:40 -0500 Subject: [PATCH 2/4] AGI: Only defer frames when in an interception routine. AGI recently was modified to defer important frames. This was because when AGI was used in a connected line interception routine, the resulting connected line frame would end up getting discarded by the AGI. However, this caused bad behavior in other cases. Specifically, during a transfer, if someone attempted to manually set the Caller ID on a channel in an AGI, the deferred connected line frame would end up overwriting what had been manually set in the AGI. Since the initial issue was specific to interception routines, this change removes the manual frame deferral from AGI and instead uses the new frame deferral API in interception routines. ASTERISK-26343 #close Reported by Morton Tryfoss Change-Id: Iab7d39436d0ee99bfe32ad55ef91e9bd88db4208 --- main/channel.c | 24 ++++++++++++++++++++++++ res/res_agi.c | 38 +------------------------------------- 2 files changed, 25 insertions(+), 37 deletions(-) diff --git a/main/channel.c b/main/channel.c index f00f222c98..9ac8a640f0 100644 --- a/main/channel.c +++ b/main/channel.c @@ -10261,9 +10261,15 @@ int ast_channel_connected_line_macro(struct ast_channel *autoservice_chan, struc ast_party_connected_line_copy(ast_channel_connected(macro_chan), connected); } + ast_channel_start_defer_frames(macro_chan); ast_channel_unlock(macro_chan); retval = ast_app_run_macro(autoservice_chan, macro_chan, macro, macro_args); + + ast_channel_lock(macro_chan); + ast_channel_stop_defer_frames(macro_chan); + ast_channel_unlock(macro_chan); + if (!retval) { struct ast_party_connected_line saved_connected; @@ -10311,9 +10317,15 @@ int ast_channel_redirecting_macro(struct ast_channel *autoservice_chan, struct a ast_party_redirecting_copy(ast_channel_redirecting(macro_chan), redirecting); } + ast_channel_start_defer_frames(macro_chan); ast_channel_unlock(macro_chan); retval = ast_app_run_macro(autoservice_chan, macro_chan, macro, macro_args); + + ast_channel_lock(macro_chan); + ast_channel_stop_defer_frames(macro_chan); + ast_channel_unlock(macro_chan); + if (!retval) { struct ast_party_redirecting saved_redirecting; @@ -10354,9 +10366,15 @@ int ast_channel_connected_line_sub(struct ast_channel *autoservice_chan, struct ast_party_connected_line_copy(ast_channel_connected(sub_chan), connected); } + ast_channel_start_defer_frames(sub_chan); ast_channel_unlock(sub_chan); retval = ast_app_run_sub(autoservice_chan, sub_chan, sub, sub_args, 0); + + ast_channel_lock(sub_chan); + ast_channel_stop_defer_frames(sub_chan); + ast_channel_unlock(sub_chan); + if (!retval) { struct ast_party_connected_line saved_connected; @@ -10397,9 +10415,15 @@ int ast_channel_redirecting_sub(struct ast_channel *autoservice_chan, struct ast ast_party_redirecting_copy(ast_channel_redirecting(sub_chan), redirecting); } + ast_channel_start_defer_frames(sub_chan); ast_channel_unlock(sub_chan); retval = ast_app_run_sub(autoservice_chan, sub_chan, sub, sub_args, 0); + + ast_channel_lock(sub_chan); + ast_channel_stop_defer_frames(sub_chan); + ast_channel_unlock(sub_chan); + if (!retval) { struct ast_party_redirecting saved_redirecting; diff --git a/res/res_agi.c b/res/res_agi.c index e0eb8e2ecb..cb23a07afc 100644 --- a/res/res_agi.c +++ b/res/res_agi.c @@ -4093,23 +4093,6 @@ static enum agi_result agi_handle_command(struct ast_channel *chan, AGI *agi, ch return AGI_RESULT_SUCCESS; } -AST_LIST_HEAD_NOLOCK(deferred_frames, ast_frame); - -static void queue_deferred_frames(struct deferred_frames *deferred_frames, - struct ast_channel *chan) -{ - struct ast_frame *f; - - if (!AST_LIST_EMPTY(deferred_frames)) { - ast_channel_lock(chan); - while ((f = AST_LIST_REMOVE_HEAD(deferred_frames, frame_list))) { - ast_queue_frame_head(chan, f); - ast_frfree(f); - } - ast_channel_unlock(chan); - } -} - static enum agi_result run_agi(struct ast_channel *chan, char *request, AGI *agi, int pid, int *status, int dead, int argc, char *argv[]) { struct ast_channel *c; @@ -4128,9 +4111,6 @@ static enum agi_result run_agi(struct ast_channel *chan, char *request, AGI *agi const char *sighup_str; const char *exit_on_hangup_str; int exit_on_hangup; - struct deferred_frames deferred_frames; - - AST_LIST_HEAD_INIT_NOLOCK(&deferred_frames); ast_channel_lock(chan); sighup_str = pbx_builtin_getvar_helper(chan, "AGISIGHUP"); @@ -4192,20 +4172,8 @@ static enum agi_result run_agi(struct ast_channel *chan, char *request, AGI *agi /* Write, ignoring errors */ if (write(agi->audio, f->data.ptr, f->datalen) < 0) { } - ast_frfree(f); - } else if (ast_is_deferrable_frame(f)) { - struct ast_frame *dup_f; - - if ((dup_f = ast_frisolate(f))) { - AST_LIST_INSERT_HEAD(&deferred_frames, dup_f, frame_list); - } - - if (dup_f != f) { - ast_frfree(f); - } - } else { - ast_frfree(f); } + ast_frfree(f); } } else if (outfd > -1) { size_t len = sizeof(buf); @@ -4253,8 +4221,6 @@ static enum agi_result run_agi(struct ast_channel *chan, char *request, AGI *agi buf[buflen - 1] = '\0'; } - queue_deferred_frames(&deferred_frames, chan); - if (agidebug) ast_verbose("<%s>AGI Rx << %s\n", ast_channel_name(chan), buf); cmd_status = agi_handle_command(chan, agi, buf, dead); @@ -4277,8 +4243,6 @@ static enum agi_result run_agi(struct ast_channel *chan, char *request, AGI *agi } } - queue_deferred_frames(&deferred_frames, chan); - if (agi->speech) { ast_speech_destroy(agi->speech); } From 0288fba2f0c0d208b3e6d563f5f77badf15cd3a7 Mon Sep 17 00:00:00 2001 From: Mark Michelson Date: Thu, 3 Nov 2016 16:46:03 -0500 Subject: [PATCH 3/4] autoservice: Use frame deferral API Rather than use manual frame deferral, just let the channel API do it for us. ASTERISK-26343 Change-Id: I688386f36e765dbc07be863943a43f26bd5eac49 (cherry picked from commit 8ba3e2fc27f9966b8c7ce75c1eca6208613a9315) --- main/autoservice.c | 66 +++------------------------------------------- 1 file changed, 4 insertions(+), 62 deletions(-) diff --git a/main/autoservice.c b/main/autoservice.c index 1af052d088..a9a887e688 100644 --- a/main/autoservice.c +++ b/main/autoservice.c @@ -61,10 +61,6 @@ struct asent { unsigned int use_count; unsigned int orig_end_dtmf_flag:1; unsigned int ignore_frame_types; - /*! Frames go on at the head of deferred_frames, so we have the frames - * from newest to oldest. As we put them at the head of the readq, we'll - * end up with them in the right order for the channel's readq. */ - AST_LIST_HEAD_NOLOCK(, ast_frame) deferred_frames; AST_LIST_ENTRY(asent) list; }; @@ -79,19 +75,13 @@ static int as_chan_list_state; static void *autoservice_run(void *ign) { ast_callid callid = 0; - struct ast_frame hangup_frame = { - .frametype = AST_FRAME_CONTROL, - .subclass.integer = AST_CONTROL_HANGUP, - }; while (!asexit) { struct ast_channel *mons[MAX_AUTOMONS]; - struct asent *ents[MAX_AUTOMONS]; struct ast_channel *chan; struct asent *as; - int i, x = 0, ms = 50; + int x = 0, ms = 50; struct ast_frame *f = NULL; - struct ast_frame *defer_frame = NULL; AST_LIST_LOCK(&aslist); @@ -106,7 +96,6 @@ static void *autoservice_run(void *ign) AST_LIST_TRAVERSE(&aslist, as, list) { if (!ast_check_hangup(as->chan)) { if (x < MAX_AUTOMONS) { - ents[x] = as; mons[x++] = as->chan; } else { ast_log(LOG_WARNING, "Exceeded maximum number of automatic monitoring events. Fix autoservice.c\n"); @@ -134,51 +123,9 @@ static void *autoservice_run(void *ign) ast_callid_threadassoc_change(callid); f = ast_read(chan); - - if (!f) { - /* No frame means the channel has been hung up. - * A hangup frame needs to be queued here as ast_waitfor() may - * never return again for the condition to be detected outside - * of autoservice. So, we'll leave a HANGUP queued up so the - * thread in charge of this channel will know. */ - - defer_frame = &hangup_frame; - } else if (ast_is_deferrable_frame(f)) { - defer_frame = f; - } else { - /* Can't defer. Discard and continue with next. */ + if (f) { ast_frfree(f); - continue; } - - for (i = 0; i < x; i++) { - struct ast_frame *dup_f; - - if (mons[i] != chan) { - continue; - } - - if (!f) { /* defer_frame == &hangup_frame */ - if ((dup_f = ast_frdup(defer_frame))) { - AST_LIST_INSERT_HEAD(&ents[i]->deferred_frames, dup_f, frame_list); - } - } else { - if ((dup_f = ast_frisolate(defer_frame))) { - AST_LIST_INSERT_HEAD(&ents[i]->deferred_frames, dup_f, frame_list); - } - if (dup_f != defer_frame) { - ast_frfree(defer_frame); - } - } - - break; - } - /* The ast_waitfor_n() call will only read frames from - * the channels' file descriptors. If ast_waitfor_n() - * returns non-NULL, then one of the channels in the - * mons array must have triggered the return. It's - * therefore impossible that we got here while (i >= x). - * If we did, we'd need to ast_frfree(f) if (f). */ } ast_callid_threadassoc_change(0); @@ -217,6 +164,7 @@ int ast_autoservice_start(struct ast_channel *chan) as->orig_end_dtmf_flag = ast_test_flag(ast_channel_flags(chan), AST_FLAG_END_DTMF_ONLY) ? 1 : 0; if (!as->orig_end_dtmf_flag) ast_set_flag(ast_channel_flags(chan), AST_FLAG_END_DTMF_ONLY); + ast_channel_start_defer_frames(chan); ast_channel_unlock(chan); AST_LIST_LOCK(&aslist); @@ -250,7 +198,6 @@ int ast_autoservice_stop(struct ast_channel *chan) { int res = -1; struct asent *as, *removed = NULL; - struct ast_frame *f; int chan_list_state; AST_LIST_LOCK(&aslist); @@ -302,12 +249,7 @@ int ast_autoservice_stop(struct ast_channel *chan) } ast_channel_lock(chan); - while ((f = AST_LIST_REMOVE_HEAD(&as->deferred_frames, frame_list))) { - if (!((1 << f->frametype) & as->ignore_frame_types)) { - ast_queue_frame_head(chan, f); - } - ast_frfree(f); - } + ast_channel_stop_defer_frames(chan); ast_channel_unlock(chan); ast_free(as); From 1db9e7886cc72355663123e906cf5f2dc4a2ef84 Mon Sep 17 00:00:00 2001 From: Mark Michelson Date: Thu, 3 Nov 2016 16:46:41 -0500 Subject: [PATCH 4/4] channel: Use frame deferral API for safe sleep. This is another case where manual frame deferral can be replaced with centralized routines instead. Change-Id: I42cdf205f8f29a7977e599751a57efbaac07c30e (cherry picked from commit d149c4b9e07eeb880d8428ad52c6fdb315cc15f5) --- main/channel.c | 33 ++++++--------------------------- 1 file changed, 6 insertions(+), 27 deletions(-) diff --git a/main/channel.c b/main/channel.c index 9ac8a640f0..0d13be13b8 100644 --- a/main/channel.c +++ b/main/channel.c @@ -1546,19 +1546,18 @@ int ast_safe_sleep_conditional(struct ast_channel *chan, int timeout_ms, int (*c int res = 0; struct timeval start; int ms; - AST_LIST_HEAD_NOLOCK(, ast_frame) deferred_frames; - - AST_LIST_HEAD_INIT_NOLOCK(&deferred_frames); /* If no other generator is present, start silencegen while waiting */ if (ast_opt_transmit_silence && !ast_channel_generatordata(chan)) { silgen = ast_channel_start_silence_generator(chan); } + ast_channel_lock(chan); + ast_channel_start_defer_frames(chan); + ast_channel_unlock(chan); + start = ast_tvnow(); while ((ms = ast_remaining_ms(start, timeout_ms))) { - struct ast_frame *dup_f = NULL; - if (cond && ((*cond)(data) == 0)) { break; } @@ -1573,18 +1572,7 @@ int ast_safe_sleep_conditional(struct ast_channel *chan, int timeout_ms, int (*c res = -1; break; } - - if (!ast_is_deferrable_frame(f)) { - ast_frfree(f); - continue; - } - - if ((dup_f = ast_frisolate(f))) { - if (dup_f != f) { - ast_frfree(f); - } - AST_LIST_INSERT_HEAD(&deferred_frames, dup_f, frame_list); - } + ast_frfree(f); } } @@ -1593,17 +1581,8 @@ int ast_safe_sleep_conditional(struct ast_channel *chan, int timeout_ms, int (*c ast_channel_stop_silence_generator(chan, silgen); } - /* We need to free all the deferred frames, but we only need to - * queue the deferred frames if there was no error and no - * hangup was received - */ ast_channel_lock(chan); - while ((f = AST_LIST_REMOVE_HEAD(&deferred_frames, frame_list))) { - if (!res) { - ast_queue_frame_head(chan, f); - } - ast_frfree(f); - } + ast_channel_stop_defer_frames(chan); ast_channel_unlock(chan); return res;