Replace part of network functions on platform-independent implementation

[Diadlo]

This change is 

[sudden6]

There are many tabs where spaces should be.

---

Comments from Reviewable

[Diadlo]

@sudden6 Sorry. Can you check now?

[sudden6]

There are still tabs in:
crypto_test.c
network_test.c
bootstrap_node_packets.c
DHT_bootstrap.c

---

Comments from Reviewable

[Diadlo]

@sudden6 Thanks a lot! Fixed.

[sudden6]

I think you forgot to upload the changes^^

[Diadlo]

@sudden6 Hm.. Seems updated

[sudden6]

ok, I see it now too, maybe missed it before

[sudden6]

Reviewed 2 of 29 files at r1, 2 of 7 files at r2, 2 of 13 files at r3.
Review status: 6 of 29 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

toxcore/network.c, line 1239 at r3 (raw file):

    }

    res = malloc(sizeof(IP_Port) * count);

check return value?

---

toxcore/network.c, line 1348 at r3 (raw file):

    target.sin_family = make_family(ip_port.ip.family);
    target.sin_port = net_htons(ip_port.port);
    fill_addr4(ip_port.ip.ip4, &target.sin_addr);

only IPv4?

---

toxcore/network.h, line 388 at r3 (raw file):

/* High-level getaddrinfo implementation.
 * Given node, which identify an Internet host. net_getipport() fill array with

which identifies an Internet host. I don't really understand the rest of the paragraph, can you rephrase/explain it?

---

toxcore/network.h, line 392 at r3 (raw file):

 * that can be specified in a call to net_connect().
 *
 * Skip all addresses with socktype != type (use type = -1 to grep all address)

to grep all addresses ?

---

toxcore/network.h, line 393 at r3 (raw file):

 *
 * Skip all addresses with socktype != type (use type = -1 to grep all address)
 * To correct deallocate array memory use net_freeipport()

To correctly deallocate?

---

Comments from Reviewable

[sudden6]

Reviewed 9 of 29 files at r1, 2 of 7 files at r2, 10 of 13 files at r3.
Review status: 27 of 29 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[Diadlo]

Review status: 25 of 30 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

toxcore/network.c, line 1239 at r3 (raw file):

Previously, sudden6 wrote…

check return value?

Done.

---

toxcore/network.c, line 1348 at r3 (raw file):

Previously, sudden6 wrote…

only IPv4?

Yes. Function renamed and TODO added.

---

toxcore/network.h, line 388 at r3 (raw file):

Previously, sudden6 wrote…

which identifies an Internet host. I don't really understand the rest of the paragraph, can you rephrase/explain it?

Better now?

---

toxcore/network.h, line 392 at r3 (raw file):

Previously, sudden6 wrote…

to grep all addresses ?

Done.

---

toxcore/network.h, line 393 at r3 (raw file):

Previously, sudden6 wrote…

To correctly deallocate?

Done.

---

Comments from Reviewable

[sudden6]

Reviewed 1 of 7 files at r4.
Review status: 24 of 30 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

auto_tests/network_test.c, line 58 at r4 (raw file):

    if (localhost_split) {
        printf("Localhost seems to be split in two.\n");

This error message doesn't make sense to me, what does split in two mean?

---

toxcore/crypto_core.c, line 217 at r4 (raw file):

#if BYTE_ORDER == LITTLE_ENDIAN
    uint8_t *s = (uint8_t *)&x;
    return (uint32_t)(s[0] << 24 | s[1] << 16 | s[2] << 8 | s[3]);

Is there a reason why you used the way with casts instead of the "shift and or" way to change the byte order? Also why does crypto_core need it's own implementation of byte order swapping? If there's a good reason for it, can you add a comment?

---

toxcore/network.c, line 1236 at r4 (raw file):

    *res = (IP_Port*)malloc(sizeof(IP_Port) * count);
    if (*res == NULL) {
        return -1;

returning -1 from an unsigned int function is wrong

---

toxcore/network.h, line 388 at r3 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Better now?

Better but not quite there yet. Which format does node have?

---

toxcore/network.h, line 392 at r4 (raw file):

 * To correctly deallocate array memory use net_freeipport()
 *
 * return size of res array.

size in bytes or number of entries?

---

Comments from Reviewable

[Diadlo]

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

---

auto_tests/network_test.c, line 58 at r4 (raw file):

Previously, sudden6 wrote…

This error message doesn't make sense to me, what does split in two mean?

For me too. But it's out of the scope of this PR.

---

toxcore/crypto_core.c, line 217 at r4 (raw file):

Is there a reason why you used the way with casts instead of the "shift and or" way to change the byte order?

I use cast and "shift and or". Or I don't understand you.

Also why does crypto_core need it's own implementation of byte order swapping?

Because htonl is implemented in #include <arpa/inet.h> we don't want to depend of it

---

toxcore/network.c, line 1236 at r4 (raw file):

Previously, sudden6 wrote…

returning -1 from an unsigned int function is wrong

-1 is invalid value and indicate invalid function result. It still can be checked withres != -1
I don't want to lose half of integer just because we need indicator for invalid result

---

toxcore/network.h, line 388 at r3 (raw file):

Previously, sudden6 wrote…

Better but not quite there yet. Which format does node have?

It's part of getaddrinfo docs. So it will be retranslated to getaddrinfo with their node meaning

---

toxcore/network.h, line 392 at r4 (raw file):

Previously, sudden6 wrote…

size in bytes or number of entries?

AFAIK 'size of array' is alwais mean number of elements (exclude sizeof in C)

---

Comments from Reviewable

[sudden6]

Reviewed 3 of 7 files at r4.
Review status: 27 of 30 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

toxcore/crypto_core.c, line 217 at r4 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Is there a reason why you used the way with casts instead of the "shift and or" way to change the byte order?

I use cast and "shift and or". Or I don't understand you.

Also why does crypto_core need it's own implementation of byte order swapping?

Because htonl is implemented in #include <arpa/inet.h> we don't want to depend of it

I would have done it like this

                    ((num<<8)&0x00FF0000) | // move byte 1 to byte 2
                    ((num>>8)&0x0000FF00) | // move byte 2 to byte 1
                    ((num<<24)&0xFF000000); // byte 0 to byte 3 ```

this way there's no need to cast anything, which is better I think

---

toxcore/network.c, line 1236 at r4 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

-1 is invalid value and indicate invalid function result. It still can be checked withres != -1
I don't want to lose half of integer just because we need indicator for invalid result

Then you'd need to make the invalid value UINT_MAX or use 0 as error value, or use an error paramenter. I think the way you're doing it currently is not a good practice or defined in the C standard, but prove me wrong :)

---

toxcore/network.h, line 392 at r4 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

AFAIK 'size of array' is alwais mean number of elements (exclude sizeof in C)

I'd prefer number of elements in res array to avoid any ambiguity, but I'll leave it up to you.

---

Comments from Reviewable

[Diadlo]

Review status: 27 of 30 files reviewed at latest revision, 4 unresolved discussions.

---

toxcore/crypto_core.c, line 217 at r4 (raw file):

Previously, sudden6 wrote…

I would have done it like this

                    ((num<<8)&0x00FF0000) | // move byte 1 to byte 2
                    ((num>>8)&0x0000FF00) | // move byte 2 to byte 1
                    ((num<<24)&0xFF000000); // byte 0 to byte 3

this way there's no need to cast anything, which is better I think

I think, previous variant was more readable. But done.

---

toxcore/network.c, line 1236 at r4 (raw file):

Previously, sudden6 wrote…

Then you'd need to make the invalid value UINT_MAX or use 0 as error value, or use an error paramenter. I think the way you're doing it currently is not a good practice or defined in the C standard, but prove me wrong :)

Changed return type. Done.

---

toxcore/network.h, line 392 at r4 (raw file):

Previously, sudden6 wrote…

I'd prefer number of elements in res array to avoid any ambiguity, but I'll leave it up to you.

Done.

---

Comments from Reviewable

[sudden6]

toxcore/crypto_core.c, line 217 at r4 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

I think, previous variant was more readable. But done.

sorry I forgot one line, ((num>>24)&0x000000FF) | // move byte 3 to byte 0 has to be the first one

---

Comments from Reviewable

[sudden6]

Reviewed 3 of 4 files at r5.
Review status: 29 of 30 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

toxcore/crypto_core.c, line 219 at r6 (raw file):

        ((x >> 24) & 0x000000FF) | // move byte 3 to byte 0
        ((x << 8)  & 0x00FF0000) | // move byte 1 to byte 2
        ((x >> 8)  & 0x0000FF00) | // move byte 2 to byte 1

maybe you could switch this line and the line above, so it's 3,2,1,0

---

toxcore/network.h, line 388 at r3 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

It's part of getaddrinfo docs. So it will be retranslated to getaddrinfo with their node meaning

There should be a , after host. Also mention that only hostname lookup is done and the port is ignored.

---

Comments from Reviewable

[Diadlo]

Review status: 28 of 30 files reviewed at latest revision, 2 unresolved discussions.

---

toxcore/crypto_core.c, line 217 at r4 (raw file):

Previously, sudden6 wrote…

sorry I forgot one line, ((num>>24)&0x000000FF) | // move byte 3 to byte 0 has to be the first one

Done.

---

toxcore/crypto_core.c, line 219 at r6 (raw file):

Previously, sudden6 wrote…

maybe you could switch this line and the line above, so it's 3,2,1,0

Done.

---

toxcore/network.h, line 388 at r3 (raw file):

Previously, sudden6 wrote…

There should be a , after host. Also mention that only hostname lookup is done and the port is ignored.

Done.

---

Comments from Reviewable

[sudden6]

Reviewed 1 of 2 files at r7.
Review status: 29 of 30 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[sudden6]

---

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[Diadlo]

@iphydf ping

[iphydf]

Ok, can you rebase and squash?

[Diadlo]

@iphydf Ok. Will do it a bit later today

[Diadlo]

@iphydf Done

[nurupo]

@Diadlo you didn't astyle your code, please fix https://travis-ci.org/TokTok/c-toxcore/jobs/205594073#L2043-L2247.

[nurupo]

other/bootstrap_daemon/src/tox-bootstrapd.c, line 315 at r9 (raw file):

    uint64_t last_LANdiscovery = 0;
    const uint16_t net_htons_port = net_htons(port);

The variable shouldn't have been renamed.

---

Comments from Reviewable