[Asterisk-code-review] res rtp asterisk.c: Initialize ourip passed to ast find ouri... (asterisk[master])

[Mark Michelson]

Mark Michelson has posted comments on this change. ( https://gerrit.asterisk.org/4664 )

Change subject: res_rtp_asterisk.c: Initialize ourip passed to ast_find_ourip().
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

I kind of agree with Martin here that it is unexpected that an output parameter would be used by ast_find_ourip(). It appears that ast_find_ourip() will read the port that is set on the output address and then make sure to preserve that after finding the appropriate IP address to set.

IMO, this is unexpected behavior, especially since the function name and documentation only mention IP addresses and say nothing of ports. You can see that res_rtp_asterisk has no clue this is going on since it attempts to set the port after the call to ast_find_ourip().

I think that for 13/14, we can keep the change as you have it here: initialize the local_addr so that we do not reference an uninitialized value in ast_find_ourip(). For master, though, I think something more intuitive should be done. For instance, change ast_find_ourip() to only set the IP address on the output parameter, and don't touch the port at all. Since ast_find_ourip() is only used in two places in Asterisk, it should be easy to make these work as expected.

https://gerrit.asterisk.org/#/c/4664/1/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

PS1, Line 4919: 				/* Failed to get local address reset to use default. */
              : 				ast_sockaddr_copy(&local_addr, &rtp->rtcp->us);
You shouldn't need to do this if ast_find_ourip() failed, since you've already set local_addr to have the correct information in it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35507ea1ad7455d2be188f6ccdd4add7bd150e15
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Martin Tomec <tomec.martin at gmail.com>
Gerrit-HasComments: Yes