[Asterisk-code-review] res/res_ari: Added ARI resource /ari/channels/{channelId}/rtp_statistics (...asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11083 )

Change subject: res/res_ari: Added ARI resource /ari/channels/{channelId}/rtp_statistics
......................................................................


Patch Set 7: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/11083/7/main/rtp_engine.c 
File main/rtp_engine.c:

https://gerrit.asterisk.org/#/c/11083/7/main/rtp_engine.c@3767 
PS7, Line 3767: 	SET_AST_JSON_OBJ(j_res, "txcount", ast_json_integer_create(stats->txcount));
I think jj_res should just not be created if any of these fail. This is because you've marked some as required in the JSON API definition, but with this logic some that are required may not actually be present if they can't be allocated. In developer mode this will cause a failure, outside of developer mode it goes against the JSON API spec.


https://gerrit.asterisk.org/#/c/11083/7/res/ari/resource_channels.c 
File res/ari/resource_channels.c:

https://gerrit.asterisk.org/#/c/11083/7/res/ari/resource_channels.c@1969 
PS7, Line 1969: 
Channel is not locked during this period of time and should be, otherwise you have no guarantee that it won't go away.


https://gerrit.asterisk.org/#/c/11083/7/res/ari/resource_channels.c@1984 
PS7, Line 1984: 	ret = glue->get_rtp_info(chan, &rtp);
You need to ensure ao2_cleanup is called on rtp or else the RTP instance will leak.


https://gerrit.asterisk.org/#/c/11083/7/res/ari/resource_channels.c@1985 
PS7, Line 1985: 	if (ret == AST_RTP_GLUE_RESULT_FORBID) {
              : 		ast_ari_response_error(response, 404, "Not Found",
              : 			"RTP info not found");
              : 		return;
              : 	}
Even if forbid there are cases where rtp will be present, so check for that instead.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11083
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I4343eec070438cec13f2a4f22e7fd9e574381376
Gerrit-Change-Number: 11083
Gerrit-PatchSet: 7
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 11:25:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190307/a30292e9/attachment.html>