<p><a href="https://gerrit.asterisk.org/10227">View Change</a></p><p>11 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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">And another one which should just be […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><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 done in the 16 branch too. It is not necessary in<br>the 16.0 branch.<br></p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">not sure why this needs to be done in 16.</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/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I think creating that ao2_debug_is_enabled function is overkill for<br>this. We don't generate the containers that frequently to make the<br>unnecessary generation of the names a burden if AO2_DEBUG is not<br>enabled.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I'm leaving it as is.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">snapshot is refleaked at both return points below</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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@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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><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<br>bridge is if it failed to get linked into the cache in the first<br>place when bridge_register() published the initial state.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">And?</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">s/teh/the/</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Missing the comment about transferring old_snapshot ref to the function.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This doesn't match the corresponding register which uses "bridge_cache".</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><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></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">was a copy paste from bridges. changed.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why not eliminate obj and replace it with snapshot?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">We used to forward all cached bridge events if we didn't have a<br>bridge. I think we need to forward all bridge events if we don't<br>have a bridge instead by removing the if test and just forwarding<br>unconditionally. ast_bridge_topic() returns the all bridge topic<br>if bridge is NULL.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think we needed to forward all cached events in the first place. Was there a specific consumer you're concerned about?</p></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: Corey Farrell <git@cfware.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: Wed, 17 Oct 2018 14:30:17 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>