Add NAT traversal

[Ansa89]

Requested in #195

Requested by @GrayHatter in #209 (review)

Previous PR: #209

Related issues:

• Deduplicate enums and typedefs from tox.h and lower layers #214
• NAT traversal: cleanup old functions #219
• Feature request: provide an async API miniupnp/miniupnp#223

CC: @GrayHatter @zetok @iphydf

---

This change is

[GrayHatter]

Reviewed 8 of 24 files at r1.
Review status: 8 of 24 files reviewed at latest revision, 7 unresolved discussions.

---

INSTALL.md, line 577 at r1 (raw file):

Install from source (example for most unix-like OS's):

can arch linux get some love?

pacman -Syu miniupnpc

---

INSTALL.md, line 603 at r1 (raw file):

Install from source (example for most unix-like OS's):

can arch linux get some love?

pacman -Syu libnatpmp

---

auto_tests/onion_test.c, line 147 at r1 (raw file):

    ip_init(&ip, 1);
    ip.ip6.uint8[15] = 1;
    // TODO(#219)

please use TODO(user) suggestion/context

here and throughout

---

toxcore/Messenger.h, line 77 at r1 (raw file):

    uint8_t ipv6enabled;
    uint8_t udp_disabled;
    TOX_TRAVERSAL_TYPE traversal_type;

Per the previous discussion, I'd rather have this be defined in another file, let tox.h be a wrapper/reference to expose it to the client if needed, and use the real name here.

---

toxcore/nat_traversal.c, line 1 at r1 (raw file):

/* nat_traversal.c -- Functions to traverse a NAT (UPnP, NAT-PMP).

@iphydf is going to ask you to remove the file name from here...

---

toxcore/nat_traversal.c, line 31 at r1 (raw file):

#include <string.h>

#ifdef HAVE_LIBMINIUPNPC

Is one better than the other, is there a reason we want both?

---

toxcore/tox.api.h, line 379 at r1 (raw file):
original comment from @iphydf

What do you think about making this a bitfield instead of an enum class? It currently is a bitfield by accident: UPNP | NATPMP == ALL.

Note that this is a question, don't change things before discussing.

---

Comments from Reviewable

[Ansa89]

Review status: 8 of 24 files reviewed at latest revision, 7 unresolved discussions.

---

auto_tests/onion_test.c, line 147 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

please use TODO(user) suggestion/context

here and throughout

@iphydf told me to use that naming...

---

toxcore/Messenger.h, line 77 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Per the previous discussion, I'd rather have this be defined in another file, let tox.h be a wrapper/reference to expose it to the client if needed, and use the real name here.

I think this is the same/related to #214.

---

toxcore/nat_traversal.c, line 1 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

@iphydf is going to ask you to remove the file name from here...

I just kept the same format as all other project files...

---

toxcore/nat_traversal.c, line 31 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Is one better than the other, is there a reason we want both?

Long story short: NAT-PMP is for Apple people, UPnP is for the rest of the world.

---

toxcore/tox.api.h, line 379 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

original comment from @iphydf

What do you think about making this a bitfield instead of an enum class? It currently is a bitfield by accident: UPNP | NATPMP == ALL.

Note that this is a question, don't change things before discussing.

`TOX_TRAVERSAL_TYPE` can be a bitfield, but I never used them so expect bad/incorrect code.

---

Comments from Reviewable

[GrayHatter]

Reviewed 9 of 24 files at r1, 1 of 3 files at r2.
Review status: 17 of 24 files reviewed at latest revision, 11 unresolved discussions.

---

INSTALL.md, line 577 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

can arch linux get some love?

pacman -Syu miniupnpc

lgtm

---

INSTALL.md, line 603 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

can arch linux get some love?

pacman -Syu libnatpmp

lgtm

---

auto_tests/onion_test.c, line 147 at r1 (raw file):

Previously, Ansa89 wrote…

@iphydf told me to use that naming...

Ok that's what we'll do in this case then :/

---

toxcore/Messenger.h, line 77 at r1 (raw file):

Previously, Ansa89 wrote…

I think this is the same/related to #214.

yeah, if you want to wait until we decide how we _want_ to do this. We can fix it the right way.

If you don't want to wait, make this a different type in a new/seperate .h, and alias the new type in tox.h

It's ugly but it'll allow us to merge this while deferring any decisions until later.

@iphydf will likely need to comment on this as well.

---

toxcore/nat_traversal.c, line 225 at r2 (raw file):

    bool natpmp = true;

    switch (traversal_type) {

what are we doing with this?

---

toxcore/nat_traversal.c, line 236 at r2 (raw file):

    }

#ifdef HAVE_LIBMINIUPNPC

It'll more efficient to move the #ifdef inside the if's so that they're only run once and either or both will be called if available.

---

toxcore/network.c, line 47 at r2 (raw file):

#include "nat_traversal.h"
#include "network.h"

why is "network.h" here and above?

---

toxcore/network.h, line 39 at r2 (raw file):

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

do we need stdbool here?

---

toxcore/TCP_server.c, line 1105 at r2 (raw file):

#endif

            nat_map_port(log, traversal_type, NAT_TRAVERSAL_TCP, ntohs(ports[i]), NULL);

is ntohs() fixing something?

---

toxcore/TCP_server.h, line 30 at r2 (raw file):

#include "logger.h"
#include "onion.h"
#include "tox.h"

needing to include tox.h in TCP_server makes me feel uncomfortable Messenger is one thing but TCP, (and network.c/h) is another!

---

toxcore/tox.api.h, line 379 at r1 (raw file):

Previously, Ansa89 wrote…

TOX_TRAVERSAL_TYPE can be a bitfield, but I never used them so expect bad/incorrect code.

It's pretty simple

just give each option in's own bit

NONE = 0,        // binary 0b0000
UPNP = 1 << 0,   // binary 0b0001
NATPMP = 1 << 1, // binary 0b0010
ALL = ~0,        // binary 0b1111 in otherwords 0b0001 
                                              + 0b0010

---

Comments from Reviewable

[Ansa89]

Review status: 17 of 24 files reviewed at latest revision, 10 unresolved discussions.

---

toxcore/nat_traversal.c, line 225 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

what are we doing with this?

Checking for a valid `traversal_type`.

---

toxcore/nat_traversal.c, line 236 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

It'll more efficient to move the #ifdef inside the if's so that they're only run once and either or both will be called if available.

I don't think so: `#ifdef` is a preprocessor instruction used to allow/deny compilation of the code between `#ifdef` and `#endif`. Putting it after the `if`, will introduce unnecessary code.

---

toxcore/TCP_server.c, line 1105 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

is ntohs() fixing something?

I don't know, copy/pasted from `network.c`. Should I remove it?

---

toxcore/tox.api.h, line 379 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

It's pretty simple

just give each option in's own bit

NONE = 0,        // binary 0b0000
UPNP = 1 << 0,   // binary 0b0001
NATPMP = 1 << 1, // binary 0b0010
ALL = ~0,        // binary 0b1111 in otherwords 0b0001 
                                              + 0b0010

Done, but `other/astyle/format-source` now generates an empty `tox.h`. Probably it has to do with setting values inside an enum type and the apidls parser, but I don't know how to solve this. Maybe @iphydf has some hints.

---

Comments from Reviewable

[Ansa89]

Review status: 13 of 24 files reviewed at latest revision, 10 unresolved discussions.

---

toxcore/tox.api.h, line 379 at r1 (raw file):

Previously, Ansa89 wrote…

Done, but other/astyle/format-source now generates an empty tox.h.
Probably it has to do with setting values inside an enum type and the apidls parser, but I don't know how to solve this.
Maybe @iphydf has some hints.

Fixed. Removed `ALL`, instead use `UPNP | NATPMP`.

---

Comments from Reviewable

[iphydf]

Review status: 13 of 24 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

auto_tests/onion_test.c, line 147 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Ok that's what we'll do in this case then :/

Suggestion/context is good. Generally, I prefer to have TODOs bound to bugs, not people, because people can leave while bugs are tracked. With bugs, we can even have a travis check that there are no TODOs about closed or non-existent bugs. Bugs can be assigned and reassigned, while TODOs with names can't easily be reassigned. The format should be: `TODO(#NNN): what to do` or `TODO(user): what to do`.

---

toxcore/nat_traversal.c, line 1 at r1 (raw file):

Previously, Ansa89 wrote…

I just kept the same format as all other project files...

*shrug*

---

Comments from Reviewable

[GrayHatter]

Reviewed 5 of 10 files at r3, 5 of 6 files at r4.
Review status: 22 of 24 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

toxcore/Messenger.h, line 77 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

yeah, if you want to wait until we decide how we want to do this. We can fix it the right way.

If you don't want to wait, make this a different type in a new/seperate .h, and alias the new type in tox.h

It's ugly but it'll allow us to merge this while deferring any decisions until later.

@iphydf will likely need to comment on this as well.

LGTM

---

toxcore/nat_traversal.c, line 1 at r1 (raw file):

Previously, iphydf wrote…

shrug

lol, LGTM then

---

toxcore/nat_traversal.c, line 225 at r2 (raw file):

Previously, Ansa89 wrote…

Checking for a valid traversal_type.

How about `if ( type > ALL) { return false;` with a comment instead?

I'd rather leave switches for logic, instead of safety checks.

---

toxcore/nat_traversal.c, line 236 at r2 (raw file):

Previously, Ansa89 wrote…

I don't think so: #ifdef is a preprocessor instruction used to allow/deny compilation of the code between #ifdef and #endif.
Putting it after the if, will introduce unnecessary code.

It shouldn't an average to good compiler will optimize out an empty if block, (if both ifdefs are false, and the block does become empty) Whereas if both are true, a single if block will be faster / more efficient.

---

toxcore/network.h, line 39 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

do we need stdbool here?

LGTM

---

toxcore/TCP_server.c, line 1105 at r2 (raw file):

Previously, Ansa89 wrote…

I don't know, copy/pasted from network.c.
Should I remove it?

It's all ready a uint16 which is what a port should be. and it 'should' be coming from the host already?

I can trace the backtrack for you to be sure. But either way, this is the wrong place for it. By the time toxcore is calling this function, it should already have translated from net into host. So if it needs to be anywhere, it's not here.

Where did you copy the code from, because we might need to fix it there as well.

@iphydf I'd like your comment on this one as well.

---

toxcore/TCP_server.h, line 30 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

needing to include tox.h in TCP_server makes me feel uncomfortable Messenger is one thing but TCP, (and network.c/h) is another!

LGTM

---

toxcore/tox.api.h, line 368 at r4 (raw file):

 * Type of technology used to try to traverse a NAT.
 */
bitmask TRAVERSAL_TYPE {

@iphydf this says bitmask, but the generated code doesn't offer any obvious information to confirm this to developers. Do we need to and any?

---

Comments from Reviewable

[Ansa89]

Review status: 22 of 24 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

toxcore/nat_traversal.c, line 225 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

How about if ( type > ALL) { return false; with a comment instead?

I'd rather leave switches for logic, instead of safety checks.

Nevermind, I removed that check and kept only the one in `tox.c:224`. Moreover the `ALL` label doesn't exist anymore, instead we must use `UPNP | NATPMP`.

---

toxcore/nat_traversal.c, line 236 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

It shouldn't an average to good compiler will optimize out an empty if block, (if both ifdefs are false, and the block does become empty) Whereas if both are true, a single if block will be faster / more efficient.

I'm sorry, but I can't understand how do you want it done. Moving the `#ifdef` inside the `if` would result in

if (traversal_type & TRAVERSAL_TYPE_UPNP) {
#ifdef HAVE_LIBMINIUPNPC
...
#endif
}
if (traversal_type & TRAVERSAL_TYPE_NATPMP) {
#ifdef HAVE_LIBNATPMP
...
#endif
}

I don't see the benefits in doing this.

---

toxcore/network.c, line 47 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

why is "network.h" here and above?

Fixed.

---

toxcore/TCP_server.c, line 1105 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

It's all ready a uint16 which is what a port should be. and it 'should' be coming from the host already?

I can trace the backtrack for you to be sure. But either way, this is the wrong place for it. By the time toxcore is calling this function, it should already have translated from net into host. So if it needs to be anywhere, it's not here.

Where did you copy the code from, because we might need to fix it there as well.

@iphydf I'd like your comment on this one as well.

The `ntohs` function is present in `network.c:288`, `network.c:293`, `network.c:298`, `network.c:706` and `network.c:719`. Personally I saw it in `network.c:706`, then used it in `network.c:719` and copied to `TCP_server.c`.

---

Comments from Reviewable

[GrayHatter]

Review status: 18 of 24 files reviewed at latest revision, 9 unresolved discussions.

---

toxcore/nat_traversal.c, line 236 at r2 (raw file):

Previously, Ansa89 wrote…

I'm sorry, but I can't understand how do you want it done.
Moving the #ifdef inside the if would result in

if (traversal_type & TRAVERSAL_TYPE_UPNP) {
#ifdef HAVE_LIBMINIUPNPC
...
#endif
}
if (traversal_type & TRAVERSAL_TYPE_NATPMP) {
#ifdef HAVE_LIBNATPMP
...
#endif
}

I don't see the benefits in doing this.

Ahh, I'm sorry, I didn't explain what I wanted very cleary

if (traversal_type ) {
#ifdef HAVE_LIBMINIUPNPC
...
#endif
#ifdef HAVE_LIBNATPMP
...
#endif
}

that way if compiled with both, the compiler will drop the whole section, if we have exactly one, we only have to if, once, and if we have both, we still only have to if once.

---

Comments from Reviewable

[zetok]

Review status: 18 of 24 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

INSTALL.md, line 558 at r6 (raw file):

<a name="nat-traversal" />

This is unnecessary, just remove the : from NAT traversal.

Same goes for other headers.

---

libtoxcore.pc.in, line 10 at r6 (raw file):

Requires:
Version: @PACKAGE_VERSION@
Libs: @NACL_OBJECTS_PKGCONFIG@ -L${libdir} @NACL_LDFLAGS@ -ltoxdns -ltoxencryptsave -ltoxcore @NACL_LIBS@ @MINIUPNP_LIBS@ @NATPMP_LIBS@ @LIBS@ @MATH_LDFLAGS@ @PTHREAD_LDFLAGS@

Can this be vertically "split" using \ ?

---

toxcore/tox.api.h, line 368 at r4 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

@iphydf this says bitmask, but the generated code doesn't offer any obvious information to confirm this to developers. Do we need to and any?

Is there a reason why this can't be an enum?

And since the generated API is an enum anyway, why is TRAVERSAL_TYPE_NONE outside of it?

---

Comments from Reviewable

[GrayHatter]

@cleverca22 can you verify that the nat packets are getting sent correctly with the ntohs() function removed?

[iphydf]

Reviewed 9 of 24 files at r1, 1 of 3 files at r2, 3 of 10 files at r3, 6 of 6 files at r4, 3 of 5 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

INSTALL.md, line 558 at r6 (raw file):

Previously, zetok wrote…

<a name="nat-traversal" />

This is unnecessary, just remove the : from NAT traversal.

Same goes for other headers.

All headers in this file do this. For consistency, let's keep it like this. In a future cleanup, we can remove the manual anchors.

---

toxcore/tox.api.h, line 368 at r4 (raw file):

Previously, zetok wrote…

Is there a reason why this can't be an enum?

And since the generated API is an enum anyway, why is TRAVERSAL_TYPE_NONE outside of it?

NONE is not part of the public API. This is good. We can discuss adding NONE to the public API later, probably by adding something to apidsl.

---

Comments from Reviewable

[Ansa89]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

INSTALL.md, line 558 at r6 (raw file):

Previously, iphydf wrote…

All headers in this file do this. For consistency, let's keep it like this. In a future cleanup, we can remove the manual anchors.

I don't think he was talking about the manual anchors, but the `:` in the header (`###NAT traversal:`).

---

libtoxcore.pc.in, line 10 at r6 (raw file):

Previously, zetok wrote…

Can this be vertically "split" using \ ?

I don't know if ".pc" files allow that syntax.

---

toxcore/nat_traversal.c, line 236 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Ahh, I'm sorry, I didn't explain what I wanted very cleary

if (traversal_type ) {
#ifdef HAVE_LIBMINIUPNPC
...
#endif
#ifdef HAVE_LIBNATPMP
...
#endif
}

that way if compiled with both, the compiler will drop the whole section, if we have exactly one, we only have to if, once, and if we have both, we still only have to if once.

This is wrong because it doesn't check what type of traversal the user has requested.

---

toxcore/tox.api.h, line 368 at r4 (raw file):

Previously, iphydf wrote…

NONE is not part of the public API. This is good. We can discuss adding NONE to the public API later, probably by adding something to apidsl.

@zetok: `TRAVERSAL_TYPE_NONE` is outside of it because of apids.

@iphydf: TRAVERSAL_TYPE_NONE is part of the public API (tox.api.h:292).

---

Comments from Reviewable

[Ansa89]

@cleverca22 can you verify that the nat packets are getting sent correctly with the ntohs()
function removed?

@cleverca22: please consider the following table for your tests

    |     UPnP    | NAT-PMP |
----|-------------|---------|
UDP | probably ok | unknown |
----|-------------|---------|
TCP |   unknown   | unknown |
----|-------------|---------|

[iphydf]

Review status: 4 of 29 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

libtoxcore.pc.in, line 10 at r6 (raw file):

Previously, Ansa89 wrote…

I don't know if ".pc" files allow that syntax.

A 2-minute web search didn't yield anything indicating that it can. I've also never seen it.

---

Comments from Reviewable

[iphydf]

Reviewed 1 of 16 files at r12, 1 of 4 files at r17, 1 of 9 files at r20, 2 of 5 files at r24, 8 of 16 files at r25, 15 of 15 files at r26.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

What's the plan for this one?

[iphydf]

Let's revisit this idea at some point. As it is, the PR can't be merged easily. The library can't be used without threads. It's not a very complicated library, so maybe we can at some point adopt the logic from there into toxcore.