<p>George Joseph <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6614">View Change</a></p><p>Patch set 5:</p><p>(10 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/6614/5/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/5/channels/chan_pjsip.c@2234">Patch Set #5, Line 2234:</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;"> data->msg = ast_msg_data_dup(msg);<br> if (!data->msg) {<br> return NULL;<br> }<br></pre></blockquote></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">data gets leaked in the off nominal path.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">fixed</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/5/channels/chan_pjsip.c@2319">Patch Set #5, Line 2319:</a> <code style="font-family:monospace,monospace"></code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Stray/extra blank line.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">fixed</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/5/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/5/include/asterisk/message.h@419">Patch Set #5, Line 419:</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;"> char from[AST_MSG_DATA_MAX_FROMTO];<br> char to[AST_MSG_DATA_MAX_FROMTO];<br></pre></blockquote></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe should define the from/to members as offsets into the body[]<br>where the string contents for the from/to start. That would<br>eliminate the arbitrary 256 max length in favor of a dynamic length<br>that could be larger. It would also reduce the amount of<br>unnecessary data copied when a frame is duped because of empty or<br>short from/to strings.</p><p style="white-space: pre-wrap; word-wrap: break-word;">struct ast_msg_data {<br>unsigned int from; /* Offset of from string from body[] */<br>unsigned int to; /* Offset of to string from body[] */<br>char body[0]; /* from/to strings stored after body string */<br>}</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Done. Kinda.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/5/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/5/main/bridge_channel.c@1002">Patch Set #5, Line 1002:</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 (fr->frametype == AST_FRAME_TEXT) {<br> ast_debug(1, "Queuing TEXT frame to '%s': %*.s\n", ast_channel_name(bridge_channel->chan),<br> fr->datalen, (char *)fr->data.ptr);<br> } else if (fr->frametype == AST_FRAME_TEXT_DATA) {<br> struct ast_msg_data *msg = fr->data.ptr;<br> ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",<br> msg->from, msg->to, ast_channel_name(bridge_channel->chan), msg->body);<br> }<br></pre></blockquote></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">This needs to be removed. This is development debug code only. No<br>other frame types are treated like this.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">New functionality that may need to be debugged later. I'll surround it with a DEBUG_ATLEAST but I don't want to remove it.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/5/main/bridge_channel.c@2358">Patch Set #5, Line 2358:</a> <code style="font-family:monospace,monospace"> ast_debug(1, "Sending TEXT frame to '%s': %*.s\n",</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Why treat TEXT and TEXT_DATA frames different than any other frames<br>with debug messages?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">See above.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/5/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/5/main/channel.c@4892">Patch Set #5, Line 4892:</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 (ast_channel_tech(chan)->write_text && (ast_format_cap_has_type(ast_channel_nativeformats(chan), AST_MEDIA_TYPE_TEXT))) {<br> struct ast_frame f;<br><br> memset(&f, 0, sizeof(f));<br> f.src = "DIALPLAN";<br> f.subclass.format = ast_format_t140;<br><br> if (ast_channel_tech(chan)->properties & AST_CHAN_TP_SEND_TEXT_DATA) {<br> f.frametype = AST_FRAME_TEXT_DATA;<br> f.datalen = sizeof(*msg) + strlen(msg->body) + 1;<br> f.data.ptr = msg;<br> } else {<br> f.frametype = AST_FRAME_TEXT;<br> f.datalen = strlen(msg->body);<br> f.data.ptr = ast_strdup(msg->body);<br> f.mallocd = AST_MALLOCD_DATA;<br> }<br><br> if (f.data.ptr) {<br> res = ast_channel_tech(chan)->write_text(chan, &f);<br> ast_frfree(&f);<br> }<br></pre></blockquote></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">This is only for sending T.140 text data. So we should always<br>convert an AST_FRAME_TEXT_DATA to an AST_FRAME_TEXT to send over<br>RTP.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I disagree. TEXT_DATA doesn't necessarily imply that it was the result of a SIP MESSAGE or that it should be delivered by SIP MESSAGE. It's just an alternate structure. The channel driver should decide what to do with it.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/5/main/channel.c@4916">Patch Set #5, Line 4916:</a> <code style="font-family:monospace,monospace"> ast_debug(1, "Sending TEXT_DATA from '%s' to %s:%s %s\n",</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Development debugging msg</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Not removing</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/5/main/channel.c@4920">Patch Set #5, Line 4920:</a> <code style="font-family:monospace,monospace"> ast_debug(1, "Sending TEXT to %s: %s\n", ast_channel_name(chan), msg->body);</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Development debugging msg</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Not removing</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6614/5/main/channel.c@4923">Patch Set #5, Line 4923:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_WARNING, "Unable to send text message on channel '%s'\n",</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think this new warning is a good idea. Seems like this<br>will spam the log when a channel doesn't support text is in a<br>confbridge exchanging text.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Ok, I'll make this a debug message. :)</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6614/5/res/res_pjsip_messaging.c">File res/res_pjsip_messaging.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/5/res/res_pjsip_messaging.c@793">Patch Set #5, Line 793:</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 (caller->id.name.valid && !ast_strlen_zero(caller->id.name.str)) {<br> ast_copy_string(msg->from, caller->id.name.str, AST_MSG_DATA_MAX_FROMTO);<br> }<br></pre></blockquote></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">How about using S_COR()</p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_copy_string(msg->from, S_COR(caller->id.name.valid,<br>caller->id.name.str, ""), AST_MSG_DATA_MAX_FROMTO);</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">why? the "if" is clearer and results in no more code or effort. Hey, at least I didn't do the assignment in the if. :)</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: 5 </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: Tue, 10 Oct 2017 01:17:11 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>