[Asterisk-code-review] bridges: Remove reliance on stasis caching (asterisk[master])
Richard Mudgett
asteriskteam at digium.com
Fri Oct 5 18:37:24 CDT 2018
Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/10227 )
Change subject: bridges: Remove reliance on stasis caching
......................................................................
Patch Set 5: Code-Review-1
(12 comments)
https://gerrit.asterisk.org/#/c/10227/5/include/asterisk/stasis_bridges.h
File include/asterisk/stasis_bridges.h:
https://gerrit.asterisk.org/#/c/10227/5/include/asterisk/stasis_bridges.h@502
PS5, Line 502: * \since 13.24
: * \since 15.6
: * \since 16.1
And another one which should just be
\since 17.0
https://gerrit.asterisk.org/#/c/10227/5/main/Makefile
File main/Makefile:
https://gerrit.asterisk.org/#/c/10227/5/main/Makefile@170
PS5, Line 170: stasis.o: _ASTCFLAGS+=$(call get_menuselect_cflags,AO2_DEBUG)
This needs to be done in the 16 branch too. It is not necessary in the 16.0 branch.
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.
https://gerrit.asterisk.org/#/c/10227/5/main/manager_bridges.c
File main/manager_bridges.c:
https://gerrit.asterisk.org/#/c/10227/5/main/manager_bridges.c@578
PS5, Line 578: snapshot = ast_bridge_snapshot_get_latest(bridge_uniqueid);
snapshot is refleaked at both return points below
https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c
File main/stasis_bridges.c:
https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@286
PS5, Line 286: static struct ast_bridge_snapshot_update *bridge_snapshot_update_create(
: struct ast_bridge_snapshot *old, struct ast_bridge_snapshot *new)
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.
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.
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.
https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@333
PS5, Line 333: * and use it in the update message. It's possible though that the
: * bridge is shutting down before any snapshot was previously written
: * to the cache. In this case, we need to create one from the current
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.
https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@336
PS5, Line 336: * state of teh bridge to use in the update message.
s/teh/the/
https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@348
PS5, Line 348: update = bridge_snapshot_update_create(old_snapshot, NULL);
Missing the comment about transferring old_snapshot ref to the function.
https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@1376
PS5, Line 1376: ao2_container_unregister("bridge_snapshots");
This doesn't match the corresponding register which uses "bridge_cache".
https://gerrit.asterisk.org/#/c/10227/5/main/stasis_bridges.c@1380
PS5, Line 1380: static int bridge_snapshot_sort_cmp(const void *obj_left, const void *obj_right, int flags)
: {
: const struct ast_bridge_snapshot *bridge_left = obj_left;
: const struct ast_bridge_snapshot *bridge_right = obj_right;
: const char *right_key = obj_right;
: int cmp;
:
: switch (flags & (OBJ_POINTER | OBJ_KEY | OBJ_PARTIAL_KEY)) {
: default:
: case OBJ_POINTER:
: right_key = bridge_right->uniqueid;
: /* Fall through */
: case OBJ_KEY:
: cmp = strcmp(bridge_left->uniqueid, right_key);
: break;
: case OBJ_PARTIAL_KEY:
: cmp = strncmp(bridge_left->uniqueid, right_key, strlen(right_key));
: break;
: }
: return cmp;
: }
Why not use
AO2_STRING_FIELD_SORT_FN(ast_bridge_snapshot, uniqueid)
Or at least use the non-deprecated defines:
OBJ_SEARCH_MASK <- (OBJ_POINTER | OBJ_KEY | OBJ_PARTIAL_KEY)
OBJ_SEARCH_OBJECT <- OBJ_POINTER
OBJ_SEARCH_KEY <- OBJ_KEY
OBJ_SEARCH_PARTIAL_KEY <= OBJ_PARTIAL_KEY
https://gerrit.asterisk.org/#/c/10227/5/res/ari/resource_bridges.c
File res/ari/resource_bridges.c:
https://gerrit.asterisk.org/#/c/10227/5/res/ari/resource_bridges.c@906
PS5, Line 906: struct ast_bridge_snapshot *snapshot = obj;
Why not eliminate obj and replace it with snapshot?
https://gerrit.asterisk.org/#/c/10227/5/res/stasis/app.c
File res/stasis/app.c:
https://gerrit.asterisk.org/#/c/10227/5/res/stasis/app.c@181
PS5, Line 181: if (bridge) {
: forwards->topic_forward = stasis_forward_all(ast_bridge_topic(bridge),
: app->topic);
: }
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.
https://gerrit.asterisk.org/#/c/10227/5/res/stasis/app.c@186
PS5, Line 186: if (!forwards->topic_forward && bridge) {
: forwards_unsubscribe(forwards);
: ao2_ref(forwards, -1);
: return NULL;
: }
With the above change this needs to be:
if (!forwards->topic_forward) {
ao2_ref(forwards, -1);
return NULL;
}
--
To view, visit https://gerrit.asterisk.org/10227
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7049b80efa88676ce5c4666f818fa18ad1985369
Gerrit-Change-Number: 10227
Gerrit-PatchSet: 5
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 05 Oct 2018 23:37:24 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181005/ebd99b5e/attachment.html>
More information about the asterisk-code-review
mailing list