Separate ip_port packing from pack_nodes() and unpack_nodes()

[JFreegman]

This change is

[iphydf]

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

---

toxcore/DHT.c, line 377 [r1] (raw file):

        memcpy(data + packed_length, nodes[i].public_key, crypto_box_PUBLICKEYBYTES);
        packed_length += crypto_box_PUBLICKEYBYTES;

Perhaps add an assert to check that the packed length is increased by either PACKED_NODE_SIZE_IP6 or PACKED_NODE_SIZE_IP4 in each iteration. I can statically verify it, but I like defensive checks :).

---

Comments from Reviewable

[iphydf]

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

---

toxcore/DHT.c, line 30 [r2] (raw file):

#endif

#ifdef TOX_DEBUG

Remove the ifdef.

---

Comments from Reviewable

[nurupo]

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

---

toxcore/DHT.c, line 30 [r2] (raw file):

Previously, iphydf wrote…

Remove the ifdef.

Are you talking about `#ifdef TOX_DEBUG`? Well, it doesn't make much sense for you to talk about `#ifdef HAVE_CONFIG_H`, so I assume it's about `#ifdef TOX_DEBUG`. See the image if you wonder why I'm asking,

---

toxcore/DHT.c, line 240 [r3] (raw file):
I'd do it a bit differently. I'd require the data pointer passed into pack_ip_port() to be already advanced to the position of where you want this function to pack the IP_Port, you'd also need to adjust the passed length accordingly. So, instead of

here is a data pointer, but please don't write directly at it, write at data+packed_length

it would be

here is a pointer to where I want you to write the stuff

---

toxcore/DHT.c, line 282 [r3] (raw file):

        data[packed_length] = net_family;
        memcpy(data + packed_length + 1, &ip_port->ip.ip6, SIZE_IP6);
        memcpy(data + packed_length + 1 + SIZE_IP6, &ip_port->port, sizeof(uint16_t));

Not of scope of this PR, but such memcpy() pattern is used in quite a few place in toxcore. What about writing some Data_Packer thing that takes an existing uin8_t array and allows to append all sorts of data to it easily, keeping track of the current offset? It would be a lot nicer in C++ than in C, with templates (even recursive variadic templates) and overloads, but it's still would provide some convenience even in C and prevent possible copy-paste errors.

So, instead of writing something like this

int foo(uint8_t *data, uint16_t length)
{
    uint8_t  a = 0;
    uint16_t b = 1;
    uint32_t c = 2;
    uint64_t d = 3;

    if (length < data + sizeof(a) + sizeof(b) + sizeof(c)) {
        fail;
    }

    memcpy(data,                                     a, sizeof(a));
    memcpy(data + sizeof(a),                         b, sizeof(b));
    memcpy(data + sizeof(a) + sizeof(b),             c, sizeof(c));
    memcpy(data + sizeof(a) + sizeof(b) + sizeof(c), d, sizeof(d));

    return data + sizeof(a) + sizeof(b) + sizeof(c);
}

You could write something like that in C

int foo(uint8_t *data, uint16_t length)
{
    uint8_t  a = 0;
    uint16_t b = 1;
    uint32_t c = 2;
    uint64_t d = 3;

    Data_Packer packer = packer_init(data, length);

    // assumes uint8_t alignment of `a`, `b`, `c` and `d`
    pack_as_uint8t_arr(&a, sizeof(a));
    pack_as_uint8t_arr(&b, sizeof(b));
    pack_as_uint8t_arr(&c, sizeof(c));
    pack_as_uint8t_arr(&d, sizeof(d));

    // maybe you could check return value of `pack_as_uint8t_arr` instead
    if (paker.ok(&packer) != SUCCESS) {
        fail;
    }

    return packer_offset(&packer);
}

In C++ you could even do

int foo(uint8_t *data, uint16_t length)
{
    uint8_t  a = 0;
    uint16_t b = 1;
    uint32_t c = 2;
    uint64_t d = 3;

    Data_Packer packer(data, length);

    // doesn't assume any alignment, powered by variadic templates and overloads
    packer.pack(a, b, c, d);

    // maybe you could check return value of `Data_Packer::pack` instead
    if (!packer) {
        fail;
    }

    return packer.offset();
}

Something similar could be done to unpack things.

---

Comments from Reviewable

[iphydf]

---

Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions.

---

toxcore/DHT.c, line 30 [r2] (raw file):

Previously, nurupo wrote…

Are you talking about #ifdef TOX_DEBUG? Well, it doesn't make much sense for you to talk about #ifdef HAVE_CONFIG_H, so I assume it's about #ifdef TOX_DEBUG. See the image if you wonder why I'm asking,

Yes, it was about TOX_DEBUG. It's done.

---

toxcore/DHT.c, line 282 [r3] (raw file):

Previously, nurupo wrote…

Not of scope of this PR, but such memcpy() pattern is used in quite a few place in toxcore. What about writing some Data_Packer thing that takes an existing uin8_t array and allows to append all sorts of data to it easily, keeping track of the current offset? It would be a lot nicer in C++ than in C, with templates (even recursive variadic templates) and overloads, but it's still would provide some convenience even in C and prevent possible copy-paste errors.

So, instead of writing something like this

int foo(uint8_t *data, uint16_t length)
{
    uint8_t  a = 0;
    uint16_t b = 1;
    uint32_t c = 2;
    uint64_t d = 3;

    if (length < data + sizeof(a) + sizeof(b) + sizeof(c)) {
        fail;
    }

    memcpy(data,                                     a, sizeof(a));
    memcpy(data + sizeof(a),                         b, sizeof(b));
    memcpy(data + sizeof(a) + sizeof(b),             c, sizeof(c));
    memcpy(data + sizeof(a) + sizeof(b) + sizeof(c), d, sizeof(d));

    return data + sizeof(a) + sizeof(b) + sizeof(c);
}

You could write something like that in C

int foo(uint8_t *data, uint16_t length)
{
    uint8_t  a = 0;
    uint16_t b = 1;
    uint32_t c = 2;
    uint64_t d = 3;

    Data_Packer packer = packer_init(data, length);

    // assumes uint8_t alignment of `a`, `b`, `c` and `d`
    pack_as_uint8t_arr(&a, sizeof(a));
    pack_as_uint8t_arr(&b, sizeof(b));
    pack_as_uint8t_arr(&c, sizeof(c));
    pack_as_uint8t_arr(&d, sizeof(d));

    // maybe you could check return value of `pack_as_uint8t_arr` instead
    if (paker.ok(&packer) != SUCCESS) {
        fail;
    }

    return packer_offset(&packer);
}

In C++ you could even do

int foo(uint8_t *data, uint16_t length)
{
    uint8_t  a = 0;
    uint16_t b = 1;
    uint32_t c = 2;
    uint64_t d = 3;

    Data_Packer packer(data, length);

    // doesn't assume any alignment, powered by variadic templates and overloads
    packer.pack(a, b, c, d);

    // maybe you could check return value of `Data_Packer::pack` instead
    if (!packer) {
        fail;
    }

    return packer.offset;
}

Something similar could be done to unpack things.

I like the idea of a packer/unpacker abstraction. Alternatively, we could increment the pointer itself, and compare with an end-pointer (like iterators) to see if we have enough space. That way, you also don't need to add more and more numbers together for the next memcpy call (which is as you correctly noted, error-prone).

---

toxcore/DHT.c, line 382 [r4] (raw file):

        packed_length += crypto_box_PUBLICKEYBYTES;

        uint32_t increment = ipp_size + crypto_box_PUBLICKEYBYTES;

Alternatively, save packed_length at the beginning of the iteration and compare with the end. Currently, you have the addition twice, so if someone changes something in between, this increment assignment needs to be updated. If you just do before - after == 4 or 6, this doesn't need to be updated, and (imo) expresses the intent a bit more clearly.

---

Comments from Reviewable

[nurupo]

---

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

---

Comments from Reviewable