[Asterisk-code-review] res rtp asterisk: Add support for receiving and handling NAC... (asterisk[15])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/8773 )

Change subject: res_rtp_asterisk: Add support for receiving and handling NACK requests.
......................................................................


Patch Set 3: Code-Review-1

(2 comments)

Minor nits

https://gerrit.asterisk.org/#/c/8773/3/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/#/c/8773/3/res/res_rtp_asterisk.c@4413
PS3, Line 4413: 			if ((payload = ast_calloc(1, sizeof(*payload) + packet_len))) {
Use ast_malloc() instead since you are immediately setting all members in payload manually so there is no need for ast_calloc() to set them to zero first.

Also you should avoid doing assignments in if statements.  There are better line break opportunities when it is done as an assignment followed by an allocation check.  It is also easier to read the code.

payload = ast_malloc(sizeof(*payload) + packet_len);
if (payload) {
}


https://gerrit.asterisk.org/#/c/8773/3/res/res_rtp_asterisk.c@4420
PS3, Line 4420: 		if ((res = rtp_sendto(instance, (void *)rtpheader, packet_len, 0, &remote_address, &ice)) < 0) {
Might as well pull the assignment out here so the line isn't as long since you are touching the line:

res = rtp_sendto(instance...);
if (res < 0) {
}

Much easier to read and you can easily see what the if test is checking now.



-- 
To view, visit https://gerrit.asterisk.org/8773
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f7f124af3b9d5d2fd9cffc6ba8cb48a6fff06ec
Gerrit-Change-Number: 8773
Gerrit-PatchSet: 3
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Mon, 16 Apr 2018 21:15:35 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180416/a161dc26/attachment-0001.html>