[Asterisk-code-review] channel.c: Fix usage of CHECK BLOCKING() (asterisk[15])
Richard Mudgett
asteriskteam at digium.com
Tue Jun 19 15:04:36 CDT 2018
Richard Mudgett has uploaded this change for review. ( https://gerrit.asterisk.org/9217
Change subject: channel.c: Fix usage of CHECK_BLOCKING()
......................................................................
channel.c: Fix usage of CHECK_BLOCKING()
The CHECK_BLOCKING() macro is used to indicate if a channel's handling
thread is about to do a blocking operation (poll, read, or write) of
media. A few operations such as ast_queue_frame(), soft hangup, and
masquerades use the indication to wake up the blocked thread to reevaluate
what is going on.
ASTERISK-27625
Change-Id: I4dfc33e01e60627d962efa29d0a4244cf151a84d
---
M include/asterisk/channel.h
M main/channel.c
2 files changed, 44 insertions(+), 23 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/17/9217/1
diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h
index 16b8a7b..d0c0dbf 100644
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -2758,15 +2758,30 @@
return state;
}
-#define CHECK_BLOCKING(c) do { \
- if (ast_test_flag(ast_channel_flags(c), AST_FLAG_BLOCKING)) {\
- ast_debug(1, "Thread %p is blocking '%s', already blocked by thread %p in procedure %s\n", \
- (void *) pthread_self(), ast_channel_name(c), (void *) ast_channel_blocker(c), ast_channel_blockproc(c)); \
- } else { \
+/*!
+ * \brief Set the blocking indication on the channel.
+ *
+ * \details
+ * Indicate that the thread handling the channel is about to do a blocking
+ * operation to wait for media on the channel. (poll, read, or write)
+ *
+ * Masquerading and ast_queue_frame() use this indication to wake up the thread.
+ *
+ * \pre The channel needs to be locked
+ */
+#define CHECK_BLOCKING(c) \
+ do { \
+ if (ast_test_flag(ast_channel_flags(c), AST_FLAG_BLOCKING)) { \
+ /* This should not happen as there should only be one thread handling a channel's media at a time. */ \
+ ast_log(LOG_DEBUG, "Thread %p is blocking '%s', already blocked by thread %p in procedure %s\n", \
+ (void *) pthread_self(), ast_channel_name(c), \
+ (void *) ast_channel_blocker(c), ast_channel_blockproc(c)); \
+ ast_assert(0); \
+ } \
ast_channel_blocker_set((c), pthread_self()); \
ast_channel_blockproc_set((c), __PRETTY_FUNCTION__); \
ast_set_flag(ast_channel_flags(c), AST_FLAG_BLOCKING); \
- } } while (0)
+ } while (0)
ast_group_t ast_get_group(const char *s);
diff --git a/main/channel.c b/main/channel.c
index a29c968..6386de2 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -5055,11 +5055,9 @@
/* There is a generator running while we're in the middle of a digit.
* It's probably inband DTMF, so go ahead and pass it so it can
* stop the generator */
- ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
ast_channel_unlock(chan);
res = ast_senddigit_end(chan, fr->subclass.integer, fr->len);
ast_channel_lock(chan);
- CHECK_BLOCKING(chan);
} else if (fr->frametype == AST_FRAME_CONTROL
&& fr->subclass.integer == AST_CONTROL_UNHOLD) {
/*
@@ -5077,7 +5075,6 @@
/* High bit prints debugging */
if (ast_channel_fout(chan) & DEBUGCHAN_FLAG)
ast_frame_dump(ast_channel_name(chan), fr, ">>");
- CHECK_BLOCKING(chan);
switch (fr->frametype) {
case AST_FRAME_CONTROL:
indicate_data_internal(chan, fr->subclass.integer, fr->data.ptr, fr->datalen);
@@ -5091,11 +5088,9 @@
f = fr;
}
send_dtmf_begin_event(chan, DTMF_SENT, fr->subclass.integer);
- ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
ast_channel_unlock(chan);
res = ast_senddigit_begin(chan, fr->subclass.integer);
ast_channel_lock(chan);
- CHECK_BLOCKING(chan);
break;
case AST_FRAME_DTMF_END:
if (ast_channel_audiohooks(chan)) {
@@ -5107,13 +5102,12 @@
}
}
send_dtmf_end_event(chan, DTMF_SENT, fr->subclass.integer, fr->len);
- ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
ast_channel_unlock(chan);
res = ast_senddigit_end(chan, fr->subclass.integer, fr->len);
ast_channel_lock(chan);
- CHECK_BLOCKING(chan);
break;
case AST_FRAME_TEXT:
+ CHECK_BLOCKING(chan);
if (ast_format_cmp(fr->subclass.format, ast_format_t140) == AST_FORMAT_CMP_EQUAL) {
res = (ast_channel_tech(chan)->write_text == NULL) ? 0 :
ast_channel_tech(chan)->write_text(chan, fr);
@@ -5121,13 +5115,17 @@
res = (ast_channel_tech(chan)->send_text == NULL) ? 0 :
ast_channel_tech(chan)->send_text(chan, (char *) fr->data.ptr);
}
+ ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
break;
case AST_FRAME_HTML:
+ CHECK_BLOCKING(chan);
res = (ast_channel_tech(chan)->send_html == NULL) ? 0 :
ast_channel_tech(chan)->send_html(chan, fr->subclass.integer, (char *) fr->data.ptr, fr->datalen);
+ ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
break;
case AST_FRAME_VIDEO:
/* XXX Handle translation of video codecs one day XXX */
+ CHECK_BLOCKING(chan);
if (ast_channel_tech(chan)->write_stream) {
if (stream) {
res = ast_channel_tech(chan)->write_stream(chan, ast_stream_get_position(stream), fr);
@@ -5139,8 +5137,10 @@
} else {
res = 0;
}
+ ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
break;
case AST_FRAME_MODEM:
+ CHECK_BLOCKING(chan);
if (ast_channel_tech(chan)->write_stream) {
if (stream) {
res = ast_channel_tech(chan)->write_stream(chan, ast_stream_get_position(stream), fr);
@@ -5152,6 +5152,7 @@
} else {
res = 0;
}
+ ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
break;
case AST_FRAME_VOICE:
if (ast_opt_generic_plc && ast_format_cmp(fr->subclass.format, ast_format_slin) == AST_FORMAT_CMP_EQUAL) {
@@ -5201,15 +5202,15 @@
if (ast_channel_writetrans(chan)) {
struct ast_frame *trans_frame = ast_translate(ast_channel_writetrans(chan), f, 0);
- if (trans_frame != f && f != fr) {
- /*
- * If translate gives us a new frame and so did the audio
- * hook then we need to free the one from the audio hook.
- */
- ast_frfree(f);
- }
- f = trans_frame;
- }
+ if (trans_frame != f && f != fr) {
+ /*
+ * If translate gives us a new frame and so did the audio
+ * hook then we need to free the one from the audio hook.
+ */
+ ast_frfree(f);
+ }
+ f = trans_frame;
+ }
}
if (!f) {
@@ -5307,6 +5308,7 @@
/* the translator on chan->writetrans may have returned multiple frames
from the single frame we passed in; if so, feed each one of them to the
channel, freeing each one after it has been written */
+ CHECK_BLOCKING(chan);
if ((f != fr) && AST_LIST_NEXT(f, frame_list)) {
struct ast_frame *cur, *next = NULL;
unsigned int skip = 0;
@@ -5345,6 +5347,7 @@
res = 0;
}
}
+ ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
break;
case AST_FRAME_NULL:
case AST_FRAME_IAX:
@@ -5353,23 +5356,26 @@
break;
case AST_FRAME_RTCP:
/* RTCP information is on a per-stream basis and only available on multistream capable channels */
+ CHECK_BLOCKING(chan);
if (ast_channel_tech(chan)->write_stream && stream) {
res = ast_channel_tech(chan)->write_stream(chan, ast_stream_get_position(stream), fr);
} else {
res = 0;
}
+ ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
break;
default:
/* At this point, fr is the incoming frame and f is NULL. Channels do
* not expect to get NULL as a frame pointer and will segfault. Hence,
* we output the original frame passed in. */
+ CHECK_BLOCKING(chan);
res = ast_channel_tech(chan)->write(chan, fr);
+ ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
break;
}
if (f && f != fr)
ast_frfree(f);
- ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING);
/* Consider a write failure to force a soft hangup */
if (res < 0) {
--
To view, visit https://gerrit.asterisk.org/9217
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4dfc33e01e60627d962efa29d0a4244cf151a84d
Gerrit-Change-Number: 9217
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180619/488d331a/attachment-0001.html>
More information about the asterisk-code-review
mailing list