<p>Patch set 5:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/10227">View Change</a></p><p>12 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/5/include/asterisk/stasis_bridges.h">File include/asterisk/stasis_bridges.h:</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/10227/5/include/asterisk/stasis_bridges.h@502">Patch Set #5, Line 502:</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;"> * \since 13.24<br> * \since 15.6<br> * \since 16.1<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">And another one which should just be<br>\since 17.0</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/5/main/Makefile">File main/Makefile:</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/10227/5/main/Makefile@170">Patch Set #5, Line 170:</a> <code style="font-family:monospace,monospace">stasis.o: _ASTCFLAGS+=$(call get_menuselect_cflags,AO2_DEBUG)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This needs to be done in the 16 branch too. It is not necessary in the 16.0 branch.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Though I question why stasis.c should even bother to use AO2_DEBUG to conditional out that code at all. Just always include that code. If AO2_DEBUG is not enabled that code does nothing.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/5/main/manager_bridges.c">File main/manager_bridges.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/10227/5/main/manager_bridges.c@578">Patch Set #5, Line 578:</a> <code style="font-family:monospace,monospace"> snapshot = ast_bridge_snapshot_get_latest(bridge_uniqueid);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">snapshot is refleaked at both return points below</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c">File main/stasis_bridges.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/10227/5/main/stasis_bridges.c@286">Patch Set #5, Line 286:</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;">static struct ast_bridge_snapshot_update *bridge_snapshot_update_create(<br> struct ast_bridge_snapshot *old, struct ast_bridge_snapshot *new)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ownership of refs are not normally transferred to called functions in Asterisk. This non-standard scheme for Asterisk makes tracking refs more difficult as you have to trace into called functions.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It is best to use the normal convention for handling refs in Asterisk. It may be less efficient concerning the number of ref alterations but it makes balancing refs so much easier when looking for leaks.</p><p style="white-space: pre-wrap; word-wrap: break-word;">At least make a doxygen comment for this function with a note that this function does not follow the normal Asterisk ref convention. Describe what is expected for the refs.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@333">Patch Set #5, Line 333:</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;"> * and use it in the update message. It's possible though that the<br> * bridge is shutting down before any snapshot was previously written<br> * to the cache. In this case, we need to create one from the current<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The only reason it won't be in the cache when we are destroying the bridge is if it failed to get linked into the cache in the first place when bridge_register() published the initial state.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@336">Patch Set #5, Line 336:</a> <code style="font-family:monospace,monospace"> * state of teh bridge to use in the update message.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/teh/the/</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@348">Patch Set #5, Line 348:</a> <code style="font-family:monospace,monospace"> update = bridge_snapshot_update_create(old_snapshot, NULL);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Missing the comment about transferring old_snapshot ref to the function.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@1376">Patch Set #5, Line 1376:</a> <code style="font-family:monospace,monospace"> ao2_container_unregister("bridge_snapshots");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This doesn't match the corresponding register which uses "bridge_cache".</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@1380">Patch Set #5, Line 1380:</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;">static int bridge_snapshot_sort_cmp(const void *obj_left, const void *obj_right, int flags)<br>{<br> const struct ast_bridge_snapshot *bridge_left = obj_left;<br> const struct ast_bridge_snapshot *bridge_right = obj_right;<br> const char *right_key = obj_right;<br> int cmp;<br><br> switch (flags & (OBJ_POINTER | OBJ_KEY | OBJ_PARTIAL_KEY)) {<br> default:<br> case OBJ_POINTER:<br> right_key = bridge_right->uniqueid;<br> /* Fall through */<br> case OBJ_KEY:<br> cmp = strcmp(bridge_left->uniqueid, right_key);<br> break;<br> case OBJ_PARTIAL_KEY:<br> cmp = strncmp(bridge_left->uniqueid, right_key, strlen(right_key));<br> break;<br> }<br> return cmp;<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why not use<br>AO2_STRING_FIELD_SORT_FN(ast_bridge_snapshot, uniqueid)</p><p style="white-space: pre-wrap; word-wrap: break-word;">Or at least use the non-deprecated defines:<br>OBJ_SEARCH_MASK <- (OBJ_POINTER | OBJ_KEY | OBJ_PARTIAL_KEY)<br>OBJ_SEARCH_OBJECT <- OBJ_POINTER<br>OBJ_SEARCH_KEY <- OBJ_KEY<br>OBJ_SEARCH_PARTIAL_KEY <= OBJ_PARTIAL_KEY</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/5/res/ari/resource_bridges.c">File res/ari/resource_bridges.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/10227/5/res/ari/resource_bridges.c@906">Patch Set #5, Line 906:</a> <code style="font-family:monospace,monospace"> struct ast_bridge_snapshot *snapshot = obj;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why not eliminate obj and replace it with snapshot?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/5/res/stasis/app.c">File res/stasis/app.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/10227/5/res/stasis/app.c@181">Patch Set #5, Line 181:</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 (bridge) {<br> forwards->topic_forward = stasis_forward_all(ast_bridge_topic(bridge),<br> app->topic);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We used to forward all cached bridge events if we didn't have a bridge. I think we need to forward all bridge events if we don't have a bridge instead by removing the if test and just forwarding unconditionally. ast_bridge_topic() returns the all bridge topic if bridge is NULL.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/5/res/stasis/app.c@186">Patch Set #5, Line 186:</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 (!forwards->topic_forward && bridge) {<br> forwards_unsubscribe(forwards);<br> ao2_ref(forwards, -1);<br> return NULL;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">With the above change this needs to be:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (!forwards->topic_forward) {<br> ao2_ref(forwards, -1);<br> return NULL;<br>}</pre></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10227">change 10227</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/10227"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I7049b80efa88676ce5c4666f818fa18ad1985369 </div>
<div style="display:none"> Gerrit-Change-Number: 10227 </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 (1000185) </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-Comment-Date: Fri, 05 Oct 2018 23:37:24 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>