[Asterisk-code-review] bridges: Remove reliance on stasis caching (asterisk[master])
George Joseph
asteriskteam at digium.com
Wed Oct 17 09:30:17 CDT 2018
George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/10227 )
Change subject: bridges: Remove reliance on stasis caching
......................................................................
Patch Set 5:
(11 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 […]
Done
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.
>
not sure why this needs to be done in 16.
https://gerrit.asterisk.org/#/c/10227/5/main/Makefile@170
PS5, Line 170: stasis.o: _ASTCFLAGS+=$(call get_menuselect_cflags,AO2_DEBUG)
> I think creating that ao2_debug_is_enabled function is overkill for
> this. We don't generate the containers that frequently to make the
> unnecessary generation of the names a burden if AO2_DEBUG is not
> enabled.
I'm leaving it as is.
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
Done
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@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.
And?
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/
Done
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.
Done
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".
Done
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
was a copy paste from bridges. changed.
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?
Done
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.
I don't think we needed to forward all cached events in the first place. Was there a specific consumer you're concerned about?
--
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: Corey Farrell <git at cfware.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: Wed, 17 Oct 2018 14:30:17 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181017/4430fae7/attachment-0001.html>
More information about the asterisk-code-review
mailing list