[Asterisk-code-review] res stasis: Reduce RAII VAR usage. (asterisk[13])
Richard Mudgett
asteriskteam at digium.com
Sat Jan 6 18:28:01 CST 2018
Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/7833 )
Change subject: res_stasis: Reduce RAII_VAR usage.
......................................................................
Patch Set 1: Code-Review-1
(15 comments)
https://gerrit.asterisk.org/#/c/7833/1/res/res_stasis.c
File res/res_stasis.c:
https://gerrit.asterisk.org/#/c/7833/1/res/res_stasis.c@1615
PS1, Line 1615: ao2_ref(app, -1);
It might be best if this unref was done after cleanup() just in case there is some unexpected locking relationship with the apps_registry container.
https://gerrit.asterisk.org/#/c/7833/1/res/res_stasis.c@1625
PS1, Line 1625: AST_RWLIST_RDLOCK(&event_sources);
need write lock not read lock
https://gerrit.asterisk.org/#/c/7833/1/res/res_stasis.c@1778
PS1, Line 1778: if (handler) {
: res = handler(app, uri, event_source);
: }
* The handling of res is not equivalent. Previous code aborted the loop by returning res if res was not 0 (STASIS_ASR_OK).
* The test for handler != NULL is loop invariant and could be made a condition of even executing the loop.
if (handler) {
for () {
...
res = handler();
if (res != STASIS_ASR_OK) {
break;
}
}
}
or instead of breaking out of the loop and retesting res you could exit early:
ao2_ref(app, -1);
return res;
https://gerrit.asterisk.org/#/c/7833/1/res/res_stasis.c@1926
PS1, Line 1926: if (json_variables) {
: struct ast_json *json_value = ast_json_string_create(event_name);
:
: if (json_value && !ast_json_object_set(blob, "eventname", json_value)) {
: blob = ast_json_ref(json_variables);
: }
: } else {
: blob = ast_json_pack("{ss}", "eventname", event_name);
: }
:
: if (!blob) {
: ast_log(LOG_ERROR, "Failed to initialize blob\n");
:
: return res;
: }
This does not appear to be equivalent code to setup blob from json_variables.
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c
File res/stasis/app.c:
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@932
PS1, Line 932: struct stasis_app *app_create(const char *name, stasis_app_cb handler, void *data, enum stasis_app_subscription_model subscription_model)
A follow on patch is needed to fix app_create() leaking a partially created app if it fails to get fully initialized. See app_shutdown() for cleanup.
In fact, the circular refs in app->bridge_router and app->router could likely cause app to leak on many off-nominal failure paths by callers even on successful creation. Not sure it is even possible to clean up in these cases.
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@956
PS1, Line 956: if (!app->forwards || !app->topic || !app->bridge_router) {
You can combine app->forwards and app->topic this way.
However, app->bridge_router supposedly has its own circular ref to app so app_dtor() asserts that app->bridge_router == NULL as well as app->router == NULL.
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1041
PS1, Line 1041: if (app->data) {
: ao2_ref(app->data, +1);
: data = app->data;
: }
This is
data = ao2_bump(app->data);
so data won't need initializing to NULL either.
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1054
PS1, Line 1054: ao2_ref(data, -1);
data could be NULL so
ao2_cleanup(data);
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1237
PS1, Line 1237: id = ast_json_string_create(forwards->id);
:
: switch (forwards->forward_type) {
id is leaked if no switch case is taken. Adding a default case would remove the compile time safety of an enum as a switch selector.
id = ast_json_string_create()
switch (forwards->forward_type) {
case FORWARD_CHANNEL:
appen_res = ast_json_array_apend(channels, ast_json_ref(id));
break;
...
}
ast_json_unref(id);
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1265
PS1, Line 1265: int app_subscribe_channel(struct stasis_app *app, struct ast_channel *chan)
Missed this function. It corresponds to the bridge and endpoint versions done in this patch.
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1336
PS1, Line 1336: return -1;
Missed the unlock here
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1441
PS1, Line 1441: ao2_ref(forwards, -1);
Unlock app->forwards before unref here instead of earlier so the manipulation of forwards is done under the lock.
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1469
PS1, Line 1469: int app_is_subscribed_bridge_id(struct stasis_app *app, const char *bridge_id)
: {
: struct app_forwards *forwards;
:
: if (ast_strlen_zero(bridge_id)) {
: bridge_id = BRIDGE_ALL;
: }
:
: forwards = ao2_find(app->forwards, bridge_id, OBJ_SEARCH_KEY);
: ao2_cleanup(forwards);
:
: return forwards != NULL;
: }
I think this bug fix change deserves special mention in the commit message as we can search the container twice if it isn't found. Either that or put it in a separate patch.
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1499
PS1, Line 1499: SCOPED_AO2LOCK(lock, app->forwards);
This was left in
https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1539
PS1, Line 1539: ao2_ref(forwards, -1);
Unlock app->forwards before unref here instead of earlier so the manipulation of forwards is done under the lock.
--
To view, visit https://gerrit.asterisk.org/7833
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ce67120583a446babf9adeec678b71d37fcd9e5
Gerrit-Change-Number: 7833
Gerrit-PatchSet: 1
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Sun, 07 Jan 2018 00:28:01 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180106/bca7a59f/attachment.html>
More information about the asterisk-code-review
mailing list