[Asterisk-code-review] res rtp asterisk: Allow ICE host candidates to be overriden (asterisk[13])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: res_rtp_asterisk: Allow ICE host candidates to be overriden
......................................................................


Patch Set 2: Code-Review-1

(3 comments)

In addition to the findings I have, I'm also just a bit lost in understanding what the intended use for this is, specifically with regards to the advertised host addresses. If you replace a private internal address with a public address from the Asterisk server, then you'd presumably result in sending duplicate host candidates in your SDP. If you replace the internal address with an address from the edge of your network, then you presumably would be advertising a srflx address as a host address, and you'd also presumably have the address duplicated again.

If the goal is to hide internal details of your network, then wouldn't it be more useful to outright remove host candidates from the list rather than replacing them with something?

And feel free to disagree with me on this one, but would it be worthwhile to specify host candidates to replace in CIDR form?
    
    interface 10.0.0.0/8

If you're replacing host candidates with alternatives, then this suggestion wouldn't make much sense. But if you're instead specifying host candidates to remove from host candidate lists, then specifying an entire subnet seems like a convenience.

https://gerrit.asterisk.org/#/c/2101/2/configs/samples/rtp.conf.sample
File configs/samples/rtp.conf.sample:

Line 76: ; negoitation:
s/negoitation/negotiation/


https://gerrit.asterisk.org/#/c/2101/2/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

Line 5418: 			if (!strncmp(var->name, "interface", sizeof("interface") - 1)) {
Typically with config file options, we do case insensitive comparisons.

Also, any particular reason that the word "interface" is even needed for this option? Wouldn't

    192.168.0.1 => 1.2.3.4

work?


Line 5439: 				candidate = ast_calloc(1, sizeof(*candidate));
Check for allocation failure.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1c9541af97b83a4c690c8150d19bf7202c8bff1f
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes