Network cleanup: reduce dependency on system-defined constants

[Diadlo]

Should be the last part of network cleanup. I can split it on few PRs if it's needed.

---

This change is 

[sudden6]

I think splitting in two or three parts would be appropriate, because right now it is huge.

[Diadlo]

@sudden6 Done

[sudden6]

Any idea why that failed https://travis-ci.org/TokTok/c-toxcore/jobs/211243880#L2144 ?

[sudden6]

osx is still broken :/

[Diadlo]

@sudden6 Sorry, my mistake. Should be fixed now

[sudden6]

Reviewed 20 of 20 files at r1, 1 of 2 files at r2.
Review status: 19 of 20 files reviewed at latest revision, 2 unresolved discussions.

---

toxcore/LAN_discovery.c, line 250 at r2 (raw file):

            ip.ip6.uint32[1] = 0x00000000;
            ip.ip6.uint32[2] = 0x0000FFFF;
            ip.ip6.uint32[3] = 0xFFFFFFFF;

Please add a comment for those "magic numbers", like in ipv6

---

toxcore/LAN_discovery.c, line 255 at r2 (raw file):

        if (family_broadcast == TOX_AF_INET) {
            ip.family = TOX_AF_INET;
            ip.ip4.uint32 = 0xFFFFFFFF;

Same

---

Comments from Reviewable

[sudden6]

---

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[sudden6]

Reviewed 7 of 7 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[Diadlo]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

toxcore/LAN_discovery.c, line 250 at r2 (raw file):

Previously, sudden6 wrote…

Please add a comment for those "magic numbers", like in ipv6

Moved to const

---

toxcore/LAN_discovery.c, line 255 at r2 (raw file):

Previously, sudden6 wrote…

Same

Done.

---

Comments from Reviewable

[Diadlo]

@nurupo Can you take a look?

[nurupo]

Sure, when I get time for it. Have been busy the entire week, haven't even joined IRC in a while now.

[iphydf]

Please enable the checkbox "Allow edits from maintainers." on the bottom right.

[robinlinden]

---

Reviewed 11 of 20 files at r1, 3 of 7 files at r4, 6 of 6 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[nurupo]

toxcore/network.c, line 144 at r5 (raw file):

#endif

const IP4 ip4_loopback = {

Shouldn't constants be in ALL_CAPS?

---

Comments from Reviewable

[nurupo]

I just quickly eyeballed the PR, without reading too much into it. A few questions popped up, which I commented on.

---

Reviewed 11 of 20 files at r1, 3 of 7 files at r4, 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

auto_tests/dht_test.c, line 100 at r5 (raw file):

    IP_Port test_ipp;
    uint8_t test_id[CRYPTO_PUBLIC_KEY_SIZE];
    uint8_t ipv6 = ip_port->ip.family == AF_INET6 ? 1 : 0;

Did you remove the file which defines AF_INET6 in this scope, so that in case you have missed one, it won't compile because AF_INET6 is not defined?

---

auto_tests/network_test.c, line 74 at r5 (raw file):

    ck_assert_msg(extra.family == TOX_AF_INET, "Expected family TOX_AF_INET (%u), got %u.", TOX_AF_INET, extra.family);
    ck_assert_msg(extra.ip4.uint32 == net_htonl(0x7F000001), "Expected 127.0.0.1, got %s.",

Hm, so you use ip6_loopback for IPv6, but net_htonl(0x7F000001) for IPv4? Why not rewrite this to use ip4_loopback? There are also other places where net_htonl(0x7F000001) is used which could be rewritten to use ip4_loopback.

---

auto_tests/TCP_test.c, line 37 at r5 (raw file):

    IP_Port ip_port_loopback;
    ip_port_loopback.ip.family = TOX_AF_INET6;
    ip_port_loopback.ip.ip6.uint64[0] = 0;

Shouldn't we use ip6_loopback in here instead of manually specifying ::1?

---

toxcore/DHT.c, line 269 at r5 (raw file):

    }

    if (ip_family == TCP_INET) {

Not TOX_TCP_INET?

---

toxcore/DHT.c, line 303 at r5 (raw file):

        is_ipv4 = true;
        net_family = TOX_AF_INET;
    } else if (ip_port->ip.family == TCP_INET) {

Not TOX_TCP_INET?

---

Comments from Reviewable

[Diadlo]

Review status: 11 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

auto_tests/dht_test.c, line 100 at r5 (raw file):

Previously, nurupo wrote…

Did you remove the file which defines AF_INET6 in this scope, so that in case you have missed one, it won't compile because AF_INET6 is not defined?

IDK how you find AF_INET6 this. But here is already TOX_AF_INET6. May be reviewable bug?

---

auto_tests/network_test.c, line 74 at r5 (raw file):

Previously, nurupo wrote…

Hm, so you use ip6_loopback for IPv6, but net_htonl(0x7F000001) for IPv4? Why not rewrite this to use ip4_loopback? There are also other places where net_htonl(0x7F000001) is used which could be rewritten to use ip4_loopback.

Done.

---

auto_tests/TCP_test.c, line 37 at r5 (raw file):

Previously, nurupo wrote…

Shouldn't we use ip6_loopback in here instead of manually specifying ::1?

Done.

---

toxcore/DHT.c, line 269 at r5 (raw file):

Previously, nurupo wrote…

Not TOX_TCP_INET?

TCP_INET defined in network.h

---

toxcore/DHT.c, line 303 at r5 (raw file):

Previously, nurupo wrote…

Not TOX_TCP_INET?

Same as above

---

toxcore/network.c, line 144 at r5 (raw file):

Previously, nurupo wrote…

Shouldn't constants be in ALL_CAPS?

Done.

---

Comments from Reviewable

[nurupo]

Review status: 11 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

auto_tests/dht_test.c, line 100 at r5 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

IDK how you find AF_INET6 this. But here is already TOX_AF_INET6. May be reviewable bug?

You misunderstood me. I didn't find AF_INET6 here. I'm asking if the AF_INET6 symbol from the system headers is still being pulled into the scope of this (and all other too) file.

---

Comments from Reviewable

[Diadlo]

Review status: 11 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

auto_tests/dht_test.c, line 100 at r5 (raw file):

Previously, nurupo wrote…

You misunderstood me. I didn't find AF_INET6 here. I'm asking if the AF_INET6 symbol from the system headers is still being pulled into the scope of this (and all other too) file.

Yes. AF_INET6 is still in the scope

---

Comments from Reviewable

[nurupo]

Reviewed 6 of 9 files at r7, 8 of 8 files at r8.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

toxcore/network.h, line 187 at r8 (raw file):

#define TOX_INET_ADDRSTRLEN 16
#define TOX_INET6_ADDRSTRLEN 46

Why is this needed? Doesn't seem like this helps with anything. INET6_ADDRSTRLEN and INET_ADDRSTRLEN seem fine as they are.

---

Comments from Reviewable

[Diadlo]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

toxcore/Messenger.c, line 2531 at r7 (raw file):

Previously, nurupo wrote…

@iphydf has pointed out that this is not portable change. You use IP4_LOOPBACK to replace INADDR_LOOPBACK, but IP4_LOOPBACK is a uint8_t array of {127, 0, 0, 1}, while INADDR_LOOPBACK is a 0x7f000001 integer. They equal on big-endian machines, but they are not equal on little-endian. IP4_LOOPBACK initialized that way stays the same in memory no matter the endianness, but INADDR_LOOPBACK changes its memory representation with the change of endianness.

Done.

---

toxcore/network.h, line 187 at r8 (raw file):

Previously, nurupo wrote…

Why is this needed? Doesn't seem like this helps with anything. INET6_ADDRSTRLEN and INET_ADDRSTRLEN seem fine as they are.

INET6_ADDRSTRLEN and INET_ADDRSTRLEN are system-dependent constants

---

Comments from Reviewable

[nurupo]

Review status: 19 of 20 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

toxcore/network.h, line 187 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

INET6_ADDRSTRLEN and INET_ADDRSTRLEN are system-dependent constants

Then add asserts in network.c to check that TOX_INET_ADDRSTRLEN constants are indeed equal to INET_ADDRSTRLEN on the target system, since we assume that to be true by using inet_ntop() this way. Those asserts might seem pointless, I myself can't imagine those constants to be re-defined to be equal to anything other than what we assume them to be, but it's something we have to check regardless.

---

Comments from Reviewable

[Diadlo]

Review status: 18 of 20 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

toxcore/network.h, line 187 at r8 (raw file):

Previously, nurupo wrote…

Then add asserts in network.c to check that TOX_INET_ADDRSTRLEN constants are indeed equal to INET_ADDRSTRLEN on the target system, since we assume that to be true by using inet_ntop() this way. Those asserts might seem pointless, I myself can't imagine those constants to be re-defined to be equal to anything other than what we assume them to be, but it's something we have to check regardless.

Replaced on constants

---

Comments from Reviewable

[Diadlo]

@nurupo Forget, that const in C not really constant. Add assert in netwok.c

[Diadlo]

@nurupo ping №2

[nurupo]

Review status: 18 of 20 files reviewed at latest revision, 1 unresolved discussion.

---

toxcore/network.h, line 187 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Replaced on constants

Why were the values changed, 16 to 22 and 46 to 66? Did the compilation started to fail once you have added the compile-time checks against systems constants?

---

Comments from Reviewable

[Diadlo]

Review status: 18 of 20 files reviewed at latest revision, 1 unresolved discussion.

---

toxcore/network.h, line 187 at r8 (raw file):

Previously, nurupo wrote…

Why were the values changed, 16 to 22 and 46 to 66? Did the compilation started to fail once you have added the compile-time checks against systems constants?

Exectly. And I have done binary search for the right value

---

Comments from Reviewable

[nurupo]

---

Reviewed 2 of 2 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

toxcore/network.h, line 187 at r8 (raw file):
Heh.

This shows that assuming things in C is dangerous. While 16 and 46 look perfectly fine for the xxx.xxx.xxx and xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx IP notations, inet_ntop() specifically says that

The buffer dst must be at least INET6_ADDRSTRLEN bytes long

so it could potentially have written 22 or 66 bytes on your system, which would result in a buffer overflow, which is why I said that it's something we have to check.

---

toxcore/onion.c, line 83 at r9 (raw file):

                 target->family == TOX_AF_INET6;

    return valid ? 0 : -1;

Hm, weird, shouldn't this function succeed for target->family == == TOX_TCP_INET? @irungentoo

@Diadlo, it's not an issue with your PR but with the original code.

---

Comments from Reviewable

[nurupo]

Please rebase on master. In 889d25f, I have edited Messenger.c to define INADDR_LOOPBACK because it wasn't defined on FreeBSD. You might want to remove all my additions from that commit because your PR replaces the only use of INADDR_LOOPBACK in Messenger.c with IP4_LOOPBACK, which would be defined on FreeBSD because we define it ourselves.

[Diadlo]

@nurupo Thank you for reminding! Done

[nurupo]

Check Travis, looks like it fails to build on FreeBSD.

[nurupo]

You could move changes from 889d25f to network.c to fix that.

[Diadlo]

@nurupo Done

[nurupo]

Looks good to me.

Would still like to see @irungentoo answer my question, but don't let that block the PR from being merged.

---

Reviewed 1 of 1 files at r10, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[robinlinden]

---

Reviewed 1 of 8 files at r6, 6 of 9 files at r7, 7 of 8 files at r8, 2 of 2 files at r9, 1 of 1 files at r10, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

toxcore/network.c, line 161 at r11 (raw file):

#if TOX_INET_ADDRSTRLEN < INET_ADDRSTRLEN
#error TOX_INET_ADDRSTRLEN should be egreater or qual to INET_ADDRSTRLEN (#INET_ADDRSTRLEN)

greater or equal

---

toxcore/network.c, line 975 at r11 (raw file):

    if (ip) {
        int family = make_family(ip->family);

const int

---

Comments from Reviewable

[Diadlo]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

toxcore/network.c, line 161 at r11 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

greater or equal

Done.

---

toxcore/network.c, line 975 at r11 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

const int

Done.

---

toxcore/onion.c, line 83 at r9 (raw file):

Previously, nurupo wrote…

Hm, weird, shouldn't this function succeed for target->family == == TOX_TCP_INET? @irungentoo

@Diadlo, it's not an issue with your PR but with the original code.

It will succeed. It shouldn't?

---

Comments from Reviewable

[robinlinden]

Reviewed 1 of 1 files at r13.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[nurupo]

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

---

toxcore/onion.c, line 83 at r9 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

It will succeed. It shouldn't?

No, it won't succeed, the function will return with -1, which is documented as a failure. I don't know whether it should or should not succeed, I have little knowledge of toxcore code, thus why I'm asking @irungentoo.

It just seems weird that the second if condition:

   if (target->family == TOX_AF_INET || target->family == TOX_TCP_INET) {
        memcpy(target->ip4.uint8, data + 1, SIZE_IP4);
    } else {
        memcpy(target->ip6.uint8, data + 1, SIZE_IP6);
    }

does accept target->family = TOX_TCP_INET and does a memcpy() on it, which seems like a success to me, but then the following code says that the function has failed for target->family = TOX_TCP_INET (unless disable_family_check is set, obviously):

    bool valid = disable_family_check ||
                 target->family == TOX_AF_INET ||
                 target->family == TOX_AF_INET6;
 
    return valid ? 0 : -1;

So, to reiterate, I'm getting mixed signals here. It looks like the function should succeed for target->family = TOX_TCP_INET because it does all the work, but then it indicated that it has failed. If it always fails for target->family = TOX_TCP_INET, then why do the work, the memcpy(), in the first place, why not just do an early return with -1? This is a smelly code.

---

Comments from Reviewable

[Diadlo]

@nurupo I think, it should be changed in another PR, because refactoring should not touch the logic

[nurupo]

I'm not asking to change it. You must have misunderstood me.