Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix warning about being deployed on private IP #2386

Merged
merged 3 commits into from
Oct 13, 2020

Conversation

npikimasu
Copy link
Contributor

When no STUN or TURN server is configured and janus is on a NAT, a warning about being deployed on a private IP appears, even when nat_1_1_mapping is set in janus.jcfg.

Not sure when the warning starting popping up--sometime after v0.9.5.

There is a variable "const char* nat_1_1_mapping". Before support for multiple IPs was added, this variable was set to the value of the nat_1_1_mapping config value. However, while the const char* variable still exists, it is not used and not set. That causes the code that checks for private IPs to ignore the nat_1_1_mapping config setting, and issue the warning incorrectly.

This fix removes the 'const char* nat_1_1_mapping' variable, and replaces the single private-IP test with a test of each public IP address.

The warning can be safely ignored, so not a big deal in any case.

@januscla
Copy link

januscla commented Oct 3, 2020

Thanks for your contribution, @npikimasu! Please make sure you sign our CLA, as it's a required step before we can merge this.

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, we must have missed the orphaned variable when accepting that multiple nat-1-1 contribution.
I've added some notes inline, basically all code style related. Once they're addressed I'll test it, and in case it works merge it 👍

janus.c Outdated
private_address = TRUE;
}
int num_ips = janus_get_public_ip_count();
if(num_ips == 0) num_ips++; /* if nat_1_1_mapping is off, the first (and only) public IP is the local_ip */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code style: if action must be on a separate line, and comments must start with a capitalized letter.

janus.c Outdated
int num_ips = janus_get_public_ip_count();
if(num_ips == 0) num_ips++; /* if nat_1_1_mapping is off, the first (and only) public IP is the local_ip */
/* check each public IP */
for (int i = 0; i < num_ips; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code style: no whitespace between for and the bracket.
Besides, the i variable must be initialized before the for loop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above on comments style too (there are other comments that need to be addressed the same way).

janus.c Outdated
/* check each public IP */
for (int i = 0; i < num_ips; i++) {
gboolean private_address = FALSE;
const gchar* test_ip = janus_get_public_ip(i);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code style: the asterisk pointer must be next to the variable, not the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger! I've corrected the styling issues.

@lminiero
Copy link
Member

Apologies for this late reply, busy week... I've just tested quickly and it seems to work as expected, thanks for the quick fixes on code style too! Merging 👍

@lminiero lminiero merged commit c545cac into meetecho:master Oct 13, 2020
PauKerr pushed a commit to caffeinetv/janus-gateway that referenced this pull request Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants