Add DHT_create_packet, an abstraction for DHT RPC packets

[jhert0]

This change is

[iphydf]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

---

toxcore/DHT.c, line 1227 at r1 (raw file):

    size_t len = DHT_create_packet(dht->self_public_key, shared_key, NET_PACKET_GET_NODES, plain, sizeof(plain), data);
    if (len == -1) {

size_t can not be negative.

---

Comments from Reviewable

[iphydf]

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

---

toxcore/DHT.c, line 292 at r2 (raw file):

static int DHT_create_packet(const uint8_t public_key[crypto_box_PUBLICKEYBYTES],
                             const uint8_t *shared_key, const uint8_t type, uint8_t *plain, size_t length, uint8_t *packet){

Run other/astyle/format-source.

---

toxcore/DHT.c, line 1226 at r2 (raw file):

    DHT_get_shared_key_sent(dht, shared_key, public_key);

    ssize_t len = DHT_create_packet(dht->self_public_key, shared_key, NET_PACKET_GET_NODES, plain, sizeof(plain), data);

DHT_create_packet returns int. Unless you have a very good reason for an implicit conversion here, please use consistent types.

---

Comments from Reviewable

[GrayHatter]

will re-review once @iphydf's comments are resolved

[GrayHatter]

@tux3 welcome back mate

[tux3]

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.

---

toxcore/DHT.c, line 311 at r2 (raw file):

    memcpy(packet + 1, public_key, crypto_box_PUBLICKEYBYTES);
    memcpy(packet + 1 + crypto_box_PUBLICKEYBYTES, nonce, crypto_box_NONCEBYTES);
    memcpy(packet + 1 + crypto_box_PUBLICKEYBYTES + crypto_box_NONCEBYTES, encrypt, length);

Is the MAC being discarded intentionally here?

---

Comments from Reviewable

[jhert0]

toxcore/DHT.c, line 311 at r2 (raw file):

Previously, tux3 (Tux3) wrote…

Is the MAC being discarded intentionally here?

That is how it was done originally.

---

Comments from Reviewable

[GrayHatter]

Review status: 0 of 1 files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

---

toxcore/DHT.c, line 311 at r2 (raw file):

Previously, endoffile78 (Endoffile) wrote…

That is how it was done originally.

No, @endoffile78 you're returning the wrong `length` var.

the original uses len, ( the result of encrypt_data_symmetric() ) , you're using length, which is the clear text length.

---

toxcore/DHT.c, line 298 at r3 (raw file):

    new_nonce(nonce);

    int len = encrypt_data_symmetric(shared_key,

this can all be one line.

---

toxcore/DHT.c, line 313 at r3 (raw file):

    memcpy(packet + 1 + crypto_box_PUBLICKEYBYTES + crypto_box_NONCEBYTES, encrypt, length);

    return 1 + crypto_box_MACBYTES + crypto_box_NONCEBYTES + length;

This needs to be len, not length

---

Comments from Reviewable

[iphydf]

Review status: 0 of 1 files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

---

toxcore/DHT.c, line 292 at r2 (raw file):

Previously, iphydf wrote…

Run other/astyle/format-source.

If that didn't do anything, please add a space between `)` and `{` manually.

---

toxcore/DHT.c, line 298 at r3 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

this can all be one line.

Yes, but it doesn't have to be. It can be on two lines. It's mildly annoying to have >80 characters, because of line breaks in both github side by side diff views and reviewable.

---

toxcore/DHT.c, line 313 at r3 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

This needs to be len, not length

@endoffile78 can you write a unit test for this function that fails if you do this wrong?

---

Comments from Reviewable

[GrayHatter]

toxcore/DHT.c, line 313 at r3 (raw file):

Previously, iphydf wrote…

@endoffile78 can you write a unit test for this function that fails if you do this wrong?

the last test in my tox-named pull can serve as an example for what the test could look like

---

Comments from Reviewable

[GrayHatter]

Review status: 0 of 1 files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

---

toxcore/DHT.c, line 298 at r3 (raw file):

Previously, iphydf wrote…

Yes, but it doesn't have to be. It can be on two lines. It's mildly annoying to have >80 characters, because of line breaks in both github side by side diff views and reviewable.

1 or 2 I'm okay with. 6 I'm not.

Your call @endoffile78

---

Comments from Reviewable

[iphydf]

@endoffile78: I'd like to release 0.0.3 this Sunday. Will this change be ready by then?

[jhert0]

@iphydf Yes. Sorry I've been kind of busy I just need to finish the test.

[GrayHatter]

Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

auto_tests/dht_test.c, line 662 at r4 (raw file):

    ck_assert_msg(pkt[0] == NET_PACKET_GET_NODES, "Malformed packet.");
    ck_assert_msg(length != sizeof(pkt), "Invalid size. Should be %d got %d", sizeof(pkt), length);
    ck_assert_msg(memcmp(pkt + 1, key, crypto_box_KEYBYTES) == 0, "Malformed packet.");

You're comparing the key to the nonce, this should fail. Don't fix it until Travis finished please

---

Comments from Reviewable

[jhert0]

@GrayHatter the key is added before the nonce.

packet[0] = type;
memcpy(packet + 1, public_key, crypto_box_PUBLICKEYBYTES);
memcpy(packet + 1 + crypto_box_PUBLICKEYBYTES, nonce, crypto_box_NONCEBYTES);
memcpy(packet + 1 + crypto_box_PUBLICKEYBYTES + crypto_box_NONCEBYTES, encrypt, len);

[GrayHatter]

I've unassigned myself, as I've contributed code I can't review this.

@tux3 has signed off on this via IRC, with any luck I can pester him to confirm in reviewable as well. (If not I'll signoff for him.

[iphydf]

Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

toxcore/DHT.c, line 379 at r7 (raw file):

    new_nonce(nonce);

    int len = encrypt_data_symmetric(shared_key, nonce,

Can you rename this? Now we have a len and a length, so it's easy to mix them up. What is this the length of? Name it according to that. Perhaps encrypted_length, and rename encrypt to encrypted, since encrypt is a verb, which makes it look like a function when it's not. You could also rename length to plain_length for consistency and clarity.

---

toxcore/DHT.c, line 1309 at r7 (raw file):

        return -1;
    }
    return sendpacket(dht->net, ip_port, data, len);

Is len not equal to sizeof data here? Please add an assert above this line expressing the relationship between the two.

---

Comments from Reviewable

[tux3]

---

Reviewed 2 of 2 files at r6, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

---

Reviewed 1 of 2 files at r6, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[jhert0]

Review status: 0 of 18 files reviewed at latest revision, 8 unresolved discussions.

---

toxcore/DHT.c, line 1227 at r1 (raw file):

Previously, iphydf wrote…

size_t can not be negative.

Done.

---

toxcore/DHT.c, line 292 at r2 (raw file):

Previously, iphydf wrote…

If that didn't do anything, please add a space between ) and { manually.

Done.

---

toxcore/DHT.c, line 311 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

No, @endoffile78 you're returning the wrong length var.

the original uses len, ( the result of encrypt_data_symmetric() ) , you're using length, which is the clear text length.

Done.

---

toxcore/DHT.c, line 1226 at r2 (raw file):

Previously, iphydf wrote…

DHT_create_packet returns int. Unless you have a very good reason for an implicit conversion here, please use consistent types.

Done.

---

toxcore/DHT.c, line 298 at r3 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

1 or 2 I'm okay with. 6 I'm not.

Your call @endoffile78

Done.

---

toxcore/DHT.c, line 313 at r3 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

the last test in my tox-named pull can serve as an example for what the test could look like

Done.

---

toxcore/DHT.c, line 379 at r7 (raw file):

Previously, iphydf wrote…

Can you rename this? Now we have a len and a length, so it's easy to mix them up. What is this the length of? Name it according to that. Perhaps encrypted_length, and rename encrypt to encrypted, since encrypt is a verb, which makes it look like a function when it's not. You could also rename length to plain_length for consistency and clarity.

Done.

---

toxcore/DHT.c, line 1309 at r7 (raw file):

Previously, iphydf wrote…

Is len not equal to sizeof data here? Please add an assert above this line expressing the relationship between the two.

Done.

---

Comments from Reviewable

[nurupo]

Reviewed 16 of 18 files at r10.
Review status: 16 of 18 files reviewed at latest revision, 9 unresolved discussions.

---

toxcore/DHT.c, line 1211 at r10 (raw file):

                                     encrypt);

    if (len != sizeof(encrypt)) {

Why was this check replaced with an assert?

---

toxcore/DHT.c, line 1266 at r10 (raw file):

                                     encrypt);

    if (len != 1 + nodes_length + length + crypto_box_MACBYTES) {

Why was this check replaced with an assert?

---

toxcore/DHT.c, line 1269 at r10 (raw file):

    memcpy(plain + 1 + nodes_length, sendback_data, length);

    uint8_t data[2 + crypto_box_PUBLICKEYBYTES + crypto_box_NONCEBYTES

That shouldn't be 2 + but 1 + and 1 + later. The order of sizes should match the order of things being written into the array. Also, why not just use sizeof(plain) for 1 + nodes_length + length?

---

Comments from Reviewable

[jhert0]

Review status: 0 of 2 files reviewed at latest revision, 8 unresolved discussions.

---

toxcore/DHT.c, line 1211 at r10 (raw file):

Previously, nurupo wrote…

Why was this check replaced with an assert?

Done.

---

toxcore/DHT.c, line 1266 at r10 (raw file):

Previously, nurupo wrote…

Why was this check replaced with an assert?

Done.

---

toxcore/DHT.c, line 1269 at r10 (raw file):

Previously, nurupo wrote…

That shouldn't be 2 + but 1 + and 1 + later. The order of sizes should match the order of things being written into the array. Also, why not just use sizeof(plain) for 1 + nodes_length + length?

I reorder the length, but we can't use sizeof(plain) it could potentially be bigger than we need. We only need `1 + node_length + length` but plain is`1 + Node_format_size * MAX_SENT_NODES + length`.

---

Comments from Reviewable

[GrayHatter]

replaced nurupo with @robinlinden because nurupo is away, hopefully doing something fun

[robinlinden]

---

Reviewed 1 of 18 files at r10, 1 of 3 files at r11.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

---

Reviewed 2 of 18 files at r10.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[GrayHatter]

[nurupo]

---

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

---

Comments from Reviewable