<p style="white-space: pre-wrap; word-wrap: break-word;">-1 for question visibility</p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14274">View Change</a></p><p>2 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14274/1/bridges/bridge_simple.c">File bridges/bridge_simple.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14274/1/bridges/bridge_simple.c@109">Patch Set #1, Line 109:</a> <code style="font-family:monospace,monospace">ast_stream_get_formats(stream)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If the stream's format structure should be immutable, should we make ast_stream_get_formats return a const pointer instead thus avoiding this potential situation from happening again?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14274/1/res/res_pjsip_session.c">File res/res_pjsip_session.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14274/1/res/res_pjsip_session.c@1733">Patch Set #1, Line 1733:</a> <code style="font-family:monospace,monospace">ast_format_cap_get_compatible(ast_stream_get_formats(stream), session->endpoint->media.codecs, joint_cap);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Shouldn't ast_stream_get_formats need to be checked here too?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I believe the idea here is in these cases it should never be NULL. Which was part of the problem this patch is trying to solve (some off nominal fax path). That said I think we do NULL checks for this elsewhere, and I believe it has been a problem in the past, and could be again moving forward.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I propose instead of checking we allocate (if needed) an empty format at stream allocation time. That way it's never NULL, and we can avoid NULL checks now and moving forward for this. "empty" format bugs may arise, but at least things shouldn't crash. Thoughts?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/14274">change 14274</a>. To unsubscribe, or for help writing mail filters, 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/c/asterisk/+/14274"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: I8ed5924b53be9fe06a385c58817e5584b0f25cc2 </div>
<div style="display:none"> Gerrit-Change-Number: 14274 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-CC: Alexei Gradinari <alex2grad@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 20 Apr 2020 21:24:32 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: Alexei Gradinari <alex2grad@gmail.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>