<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6614">View Change</a></p><p>Patch set 7:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><ul><li>app_sendtext.c needs to be updated to support the new tech callback. Otherwise we are breaking it for chan_pjsip.</li></ul><ul><li>Local channels/core_local/core_unreal need to support the new frame type or local channels in confbridges going to pjsip channels will have unsatisfactory results.</li></ul><ul><li>CLI "core show channeltype" in channel.c needs to be updated to deal with the new tech callback.</li></ul><ul><li>ast_write() should be updated to handle the new frame type.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">Probably the best way to handle these cases is to have chan_pjsip still implement a send_text callback and the places where it matters prefer using the new callback.</p><p>(20 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG@19">Patch Set #7, Line 19:</a> <code style="font-family:monospace,monospace">will send an MESSAGE when it gets a TEXT frame. On a normal</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/an/a/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG@21">Patch Set #7, Line 21:</a> <code style="font-family:monospace,monospace">correctly. bridge_softmix was not though so messages weren't</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/though //</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG@33">Patch Set #7, Line 33:</a> <code style="font-family:monospace,monospace">setting the new AST_CHAN_TP_SEND_TEXT_DATA flag in its tech</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The master version of this patch should remove the need for the AST_CHAN_TP_SEND_TEXT_DATA flag. The presence or absence of the tech callback is sufficient going forward.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This could be done in a follow on patch for master.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG@50">Patch Set #7, Line 50:</a> <code style="font-family:monospace,monospace">* A new function "ast_sendtext_data" was added to channel which</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/channel/channel.c/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG@55">Patch Set #7, Line 55:</a> <code style="font-family:monospace,monospace">* bridge_channel now calls ast_sendtext_data for TEXT_DATA frame</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/bridge_channel/bridge_channel.c/</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/7/CHANGES">File CHANGES:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/CHANGES@25">Patch Set #7, Line 25:</a> <code style="font-family:monospace,monospace"> other participants in the call. </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">de blob</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/7/bridges/bridge_softmix.c">File bridges/bridge_softmix.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/bridges/bridge_softmix.c@685">Patch Set #7, Line 685:</a> <code style="font-family:monospace,monospace"> ast_debug(1, "Received %s frame from '%s:%s': %s\n", frame_type,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use ast_log(LOG_DEBUG,... instead. ast_debug() includes the DEBUG_ATLEAST() test.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/bridges/bridge_softmix.c@686">Patch Set #7, Line 686:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ast_msg_data_get_from(msg), ast_channel_name(bridge_channel->chan),<br> ast_msg_data_get_body(msg));<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The debug message parameters assumes that the frame type is always TEXT_DATA when it could also be simply TEXT.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/7/channels/chan_pjsip.c">File channels/chan_pjsip.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/channels/chan_pjsip.c@114">Patch Set #7, Line 114:</a> <code style="font-family:monospace,monospace"> .send_text_data = chan_pjsip_sendtext_data,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should keep the .send_text callback for other parts of the system that check to see if the channel driver supports text. (e.g., app_sendtext)</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/7/include/asterisk/message.h">File include/asterisk/message.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/include/asterisk/message.h@441">Patch Set #7, Line 441:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * \param from The display name of the sender<br> * \param to The display name of destination<br> * \param body The message body<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">From, to, and body can be NULL.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/7/main/bridge_channel.c">File main/bridge_channel.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/main/bridge_channel.c@1004">Patch Set #7, Line 1004:</a> <code style="font-family:monospace,monospace"> ast_debug(1, "Queuing TEXT frame to '%s': %*.s\n", ast_channel_name(bridge_channel->chan),</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use ast_log(LOG_DEBUG,... instead. ast_debug() includes the DEBUG_ATLEAST() test.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/main/bridge_channel.c@1008">Patch Set #7, Line 1008:</a> <code style="font-family:monospace,monospace"> ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/main/bridge_channel.c@2366">Patch Set #7, Line 2366:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (DEBUG_ATLEAST(1)) {<br> ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",<br> ast_msg_data_get_from((struct ast_msg_data *)fr->data.ptr),<br> ast_msg_data_get_to((struct ast_msg_data *)fr->data.ptr),<br> ast_channel_name(bridge_channel->chan),<br> ast_msg_data_get_body((struct ast_msg_data *)fr->data.ptr));<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">DEBUG_ATLEAST isn't needed as ast_debug(1... already does it.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/main/bridge_channel.c@2360">Patch Set #7, Line 2360:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> case AST_FRAME_TEXT:<br> ast_debug(1, "Sending TEXT frame to '%s': %*.s\n",<br> ast_channel_name(bridge_channel->chan), fr->datalen, (char *)fr->data.ptr);<br> ast_sendtext(bridge_channel->chan, fr->data.ptr);<br> break;<br> case AST_FRAME_TEXT_DATA:<br> if (DEBUG_ATLEAST(1)) {<br> ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",<br> ast_msg_data_get_from((struct ast_msg_data *)fr->data.ptr),<br> ast_msg_data_get_to((struct ast_msg_data *)fr->data.ptr),<br> ast_channel_name(bridge_channel->chan),<br> ast_msg_data_get_body((struct ast_msg_data *)fr->data.ptr));<br> }<br> ast_sendtext_data(bridge_channel->chan, (struct ast_msg_data *)fr->data.ptr);<br> break;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why not just push this code down into ast_write()?</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/7/main/channel.c">File main/channel.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/main/channel.c@4901">Patch Set #7, Line 4901:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> } else {<br> f.frametype = AST_FRAME_TEXT;<br> f.datalen = body_len;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Move body_len definition and initialization to here since this is the only place where it is used.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/main/channel.c@4934">Patch Set #7, Line 4934:</a> <code style="font-family:monospace,monospace"> msg = ast_msg_data_create(AST_MSG_DATA_SOURCE_TYPE_UNKNOWN, "", "", text);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You can pass NULL instead of "" for the from and to parameters.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/main/channel.c@4938">Patch Set #7, Line 4938:</a> <code style="font-family:monospace,monospace"> return ast_sendtext_data(chan, msg);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">msg is leaked</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/7/main/message.c">File main/message.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/main/message.c@1361">Patch Set #7, Line 1361:</a> <code style="font-family:monospace,monospace"> enum ast_msg_data_source_type source;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">To avoid possible padding, move source to between body_offset and buf. An enum base type could be char, short, or int depending on compiler options and the number of enum values defined or enum values assigned.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/main/message.c@1407">Patch Set #7, Line 1407:</a> <code style="font-family:monospace,monospace"> if (!body) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add a comment that empty bodies are specifically allowed:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/*<br> * Zero length messages are allowed for SIP by<br> * RFC3428 (See the end of section 9).<br> */</pre></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/7/main/message.c@1418">Patch Set #7, Line 1418:</a> <code style="font-family:monospace,monospace"> dest = ast_calloc(1, len);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">calloc isn't necessary since you manually set all members anyway.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/6614">change 6614</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/6614"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Idacf5900bfd5f22ab8cd235aa56dfad090d18489 </div>
<div style="display:none"> Gerrit-Change-Number: 6614 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@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>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 11 Oct 2017 18:22:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>