[Asterisk-code-review] res stasis: Reduce RAII VAR usage. (asterisk[13])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/7833 )

Change subject: res_stasis: Reduce RAII_VAR usage.
......................................................................


Patch Set 1:

(7 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@1778
PS1, Line 1778: 		if (handler) {
              : 			res = handler(app, uri, event_source);
              : 		}
> The test for handler != NULL is loop invariant and could be made a condition of even executing the loop.

This function is static and both callers provide a handler so I'm going to use an assert at the start of the function instead.


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_variable
ast_json_object_set should run on json_variables since blob is not yet set.  Otherwise I believe the code is equivalent.


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
I'm going to strip app_create changes from this patch and work on it separately, it seems to have many error handling issues.


https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1226
PS1, Line 1226: 	if (!channels || !bridges || !endpoints) {
These checks are pointless, if json != NULL these must exist too.


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 remov
Hmm I see that now.  I have another approach which will resolve all your concerns but still avoid the extra ref/unref.


https://gerrit.asterisk.org/#/c/7833/1/res/stasis/app.c@1292
PS1, Line 1292: 		res = ao2_link_flags(app->forwards, forwards,
The bridge/endpoint functions do not check this result.  I'll add it to the bridge/endpoint functions.


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 
Yeah I'll do this in a separate patch so it gets mention in 'git log --oneline'.



-- 
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: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Sun, 07 Jan 2018 18:06:53 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180107/f8cfe5fe/attachment.html>