Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DHT refactoring #498

Merged
merged 15 commits into from
Jun 5, 2017
Merged

DHT refactoring #498

merged 15 commits into from
Jun 5, 2017

Conversation

Diadlo
Copy link

@Diadlo Diadlo commented Mar 4, 2017 •
edited by iphydf
Loading

This change is Reviewable

@Diadlo
Copy link
Author

Diadlo commented Mar 6, 2017

I have splitted on 4 commits to make review easier

@nurupo
Copy link
Member

nurupo commented Mar 6, 2017

Do we want to expose AF_INET6 and AF_INET outside network.c? They require including system-specific network headers.

@Diadlo
Copy link
Author

Diadlo commented Mar 6, 2017 •
edited
Loading

@nurupo It will be fixed in another PR. I have started this PR because during update AF_INET I found a lot of not good (ok, sometimes it's bad 😄) code in DHT and decide to make refactor it in separate RP

@nurupo
Copy link
Member

nurupo commented Mar 6, 2017

Just make sure you don't change the behaviour of DHT code.

@Diadlo
Copy link
Author

Diadlo commented Mar 6, 2017

AFAIK this is the logic of refactoring :)

@zugz
Copy link

zugz commented Mar 7, 2017

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

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

    uint32_t i; \
    for (i = 0; i < size; i++) { \
        if (id_equal(array[i].public_key, pk)) { \

This replaces use of public_key_cmp in the original with use of id_equal. This is good. Why not replace the others too?

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

INDEX_OF_PK(DHT_Friend)
INDEX_OF_PK(Node_format)

Using macros to define functions makes it hard to look up the definition. Maybe only use a macro for the body, or keep it as is but add explicit function declarations?

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

     * and the one who is the actual friend's public_key/address set?
     * MAYBE: check the other address, if valid, don't nuke? */
    index = index_of_Client_data_ip_port(list, length, &ip_port);

The original code handled the case that an ip_port occurs multiple times in the list, updating the public key for each occurence, while the revised version does not. The same goes for repeated public keys. It is actually possible to have duplicate ip_ports, because the ip_port of an existing pk could be replaced by an ip_port which is already in the list, associated to a different pk.

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

    if (ip_port.ip.family == TOX_AF_INET) {
        assoc = &data->assoc4;
    } else if (ip_port.ip.family == TOX_AF_INET6) {

Why did AF_INET become TOX_AF_INET?

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

    assoc->ret_ip_port = ip_port;
    assoc->ret_timestamp = temp_time;

Why not use unix_time() directly here?

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

    uint32_t i;

    for (i = 0; i < dht->num_friends; ++i) {

Could define i here, i.e. "for (uint32_t i = 0;".

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

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

    int crypto_size = 1 + CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_NONCE_SIZE + CRYPTO_MAC_SIZE;

Here crypto_size includes CRYPTO_MAC_SIZE, but in earlier definitions it didn't. Better to be consistent?

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

static int handle_getnodes(void *object, IP_Port source, const uint8_t *packet, uint16_t length, void *userdata)
{
    int crypto_size = 1 + CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_NONCE_SIZE + CRYPTO_MAC_SIZE;

Why not include the second public key in crypto_size?

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

    DHT_Friend *frnd = &dht->friends_list[index];
    index = index_of_Client_data_pk(frnd->client_list, MAX_FRIEND_CLIENTS, public_key);

Need to check that this index isn't -1! Return 0 if it is.

Comments from Reviewable

@zugz
Copy link

zugz commented Mar 7, 2017

Above issues aside, this refactoring makes quite a bit of code substantially cleaner.

@Diadlo
Copy link
Author

Diadlo commented Mar 7, 2017

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

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

Previously, zugz (zugz) wrote…

This replaces use of public_key_cmp in the original with use of id_equal. This is good. Why not replace the others too?

Done.

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

Previously, zugz (zugz) wrote…

Using macros to define functions makes it hard to look up the definition. Maybe only use a macro for the body, or keep it as is but add explicit function declarations?

Yes, I thought about it. But I'm nooby in C and don't know how to do it right. Done

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

Previously, zugz (zugz) wrote…

The original code handled the case that an ip_port occurs multiple times in the list, updating the public key for each occurence, while the revised version does not. The same goes for repeated public keys. It is actually possible to have duplicate ip_ports, because the ip_port of an existing pk could be replaced by an ip_port which is already in the list, associated to a different pk.

Hm... As far as I understand, in old code, both if had return in the end, so it will update only first occurrence. Am I right?

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

Previously, zugz (zugz) wrote…

Why did AF_INET become TOX_AF_INET?

Accidentally copied from another PR. Reverted

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

Previously, zugz (zugz) wrote…

Why not use unix_time() directly here?

I thought about it. For some reason it was calculated in the start of the function (maybe to avoid including index_of time in timestamp).
I'm trying to avoid logic changes. If someone can confirm, that index_of time can be ignored, I will move it

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

Previously, zugz (zugz) wrote…

Could define i here, i.e. "for (uint32_t i = 0;".

IDK, which version of the C standard toxcore are using, but in old version you can't define variable in for init block

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

Previously, zugz (zugz) wrote…

Here crypto_size includes CRYPTO_MAC_SIZE, but in earlier definitions it didn't. Better to be consistent?

  1. Here crypto_size has different meaning in different functions. Maybe rename? (Are there any suggestions for a new name?)
  2. Here difference not only in the CRYPTO_MAC_SIZE. It's absolutely different formula
  3. Made const

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

Previously, zugz (zugz) wrote…

Why not include the second public key in crypto_size?

Did not see first. Thank you 👍
Maybe define it? If so, we need better name, than crypto_size

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

Previously, zugz (zugz) wrote…

Need to check that this index isn't -1! Return 0 if it is.

Done.

Comments from Reviewable

@Diadlo
Copy link
Author

Diadlo commented Mar 7, 2017 •
edited
Loading

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

toxcore/DHT.c, line 1170 at r1 (raw file):
From IRC:

<robinli> Diadlo: We recently switched Toxcore to C99.
<robinli> So for (int i; i < 10; i++) {} is okay now.

Done

Comments from Reviewable

@zugz
Copy link

zugz commented Mar 8, 2017

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

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

Previously, Diadlo (Polshakov Dmitry) wrote…

Hm... As far as I understand, in old code, both if had return in the end, so it will update only first occurrence. Am I right?

Sorry yes, you're quite right./IPP/

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

 *  return index or (uint32_t)(-1) if not found.
 */
#define INDEX_OF_PK(type) \

You can drop the argument now (also in INDEX_OF_PORT_IP below)

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

        Client_data *client = &list[i];

        for (IPPTsPng *assoc = &client->assoc4; assoc <= &client->assoc6; assoc++) {

This is cute, but is it really a good idea? At least a comment in the Client_data struct would be in order, warning that assoc4 and assoc6 must be kept next to each other.

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

     * is *usually* good(tm) (bites us in the behind in this case though) */

    for (uint32_t i = 0; i < MAX_FRIEND_CLIENTS; ++i) {

You've inverted the loops. Unlikely to matter, but this is meant to be purely a refactoring.

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

     * is *usually* good(tm) (bites us in the behind in this case though) */

    for (uint32_t i = 0; i < MAX_FRIEND_CLIENTS; ++i) {

As above.

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

        send_ping_request(dht->ping, pinging, dht->friends_list[friend_num].public_key);
    } else {
        for (i = 0; i != MAX_PUNCHING_PORTS; ++i) {

Can make it "i < MAX_PUNCHING_PORTS".

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

            uint32_t delta = sign * it / (2 * numports);
            uint32_t index = (it / 2) % numports;
            uint16_t port = port_list[index] + delta;

Problems with integer types here... -1 for the uint8_t sign ends up being used as 255.

Comments from Reviewable

@Diadlo
Copy link
Author

Diadlo commented Mar 9, 2017

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

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

Previously, zugz (zugz) wrote…

You can drop the argument now (also in INDEX_OF_PORT_IP below)

Done.

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

Previously, zugz (zugz) wrote…

This is cute, but is it really a good idea? At least a comment in the Client_data struct would be in order, warning that assoc4 and assoc6 must be kept next to each other.

Comment with warning added. Is it ok?

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

Previously, zugz (zugz) wrote…

You've inverted the loops. Unlikely to matter, but this is meant to be purely a refactoring.

Done.

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

Previously, zugz (zugz) wrote…

As above.

Done.

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

Previously, zugz (zugz) wrote…

Can make it "i < MAX_PUNCHING_PORTS".

Done.

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

Previously, zugz (zugz) wrote…

Problems with integer types here... -1 for the uint8_t sign ends up being used as 255.

Done.

Comments from Reviewable

@zugz
Copy link

zugz commented Mar 9, 2017

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

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

Previously, Diadlo (Polshakov Dmitry) wrote…

I thought about it. For some reason it was calculated in the start of the function (maybe to avoid including index_of time in timestamp).
I'm trying to avoid logic changes. If someone can confirm, that index_of time can be ignored, I will move it

I'm not sure what you mean. There's nothing in between the definition of temp_time and its use which could call update_unix_time().

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

Previously, Diadlo (Polshakov Dmitry) wrote…

Comment with warning added. Is it ok?

I meant a warning in the definition of the struct, in DHT.h.

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

Previously, Diadlo (Polshakov Dmitry) wrote…

Done.

Not pushed?

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

Previously, Diadlo (Polshakov Dmitry) wrote…

Done.

This still doesn't work. If you also make each uint32_t into uint16_t, then it should nearly agree with the original formula, except that delta should be
uint16_t delta = sign * (it / (2 * numports));
Arithmetic in C is tricky!

Comments from Reviewable

@zugz
Copy link

zugz commented Mar 9, 2017 via email

@Diadlo
Copy link
Author

Diadlo commented Mar 9, 2017

@zugz About whitespace. It's not from my PR
From header: "Files changed 2". It's DHT.h and DHT.c

@zugz
Copy link

zugz commented Mar 9, 2017 via email

@Diadlo
Copy link
Author

Diadlo commented Mar 9, 2017

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

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

Previously, zugz (zugz) wrote…

I'm not sure what you mean. There's nothing in between the definition of temp_time and its use which could call update_unix_time().

index_of_client_pk between

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

Previously, zugz (zugz) wrote…

I meant a warning in the definition of the struct, in DHT.h.

Done.

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

Previously, zugz (zugz) wrote…

Not pushed?

Done.

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

Previously, zugz (zugz) wrote…

This still doesn't work. If you also make each uint32_t into uint16_t, then it should nearly agree with the original formula, except that delta should be
uint16_t delta = sign * (it / (2 * numports));
Arithmetic in C is tricky!

punching_index is uint32_t, so it should be uint32_t too.
I think, that uint32_t / (2 * uint16_t) is uint32_t. Isn't it?
And why here should be extra braces?

Comments from Reviewable

@zugz
Copy link

zugz commented Mar 9, 2017 via email

@Diadlo
Copy link
Author

Diadlo commented Mar 9, 2017

But abs(sign) == 1 so we can use * with direct order

@zugz
Copy link

zugz commented Mar 10, 2017 via email

@Diadlo
Copy link
Author

Diadlo commented Mar 10, 2017

@zugz Thanks for explanation. I change formulas a bit

@iphydf iphydf modified the milestone: v0.1.8 Mar 26, 2017
@iphydf
Copy link
Member

iphydf commented Apr 9, 2017

What is the status of this PR?

@Diadlo
Copy link
Author

Diadlo commented Apr 9, 2017

@iphydf Waiting for review?

@zugz
Copy link

zugz commented Apr 9, 2017 via email

@Diadlo
Copy link
Author

Diadlo commented Apr 9, 2017

@zugz Sorry, reviewable misplaced your comment and I didn't see it. Fixed

@zugz
Copy link

zugz commented Apr 9, 2017 via email •
edited by iphydf
Loading

@Diadlo
Copy link
Author

Diadlo commented Apr 14, 2017

Should I split this PR into smaller ones?

<grayhatter> but robinli would know if you should split the pull request

@robinlinden ?

@Diadlo
Copy link
Author

Diadlo commented May 7, 2017

Is there anything else?

@CLAassistant
Copy link

CLAassistant commented Jun 3, 2017 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@iphydf
Copy link
Member

iphydf commented Jun 4, 2017

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

toxcore/DHT.c, line 1029 at r6 (raw file):

    unsigned int *num = &dht->num_to_bootstrap;
    uint32_t index = index_of_node_pk(dht->to_bootstrap, *num, public_key);
    bool in_clise_list = is_pk_in_close_list(dht, public_key, ip_port);

in_close_list

Comments from Reviewable

@Diadlo
Copy link
Author

Diadlo commented Jun 4, 2017

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

toxcore/DHT.c, line 1029 at r6 (raw file):

Previously, iphydf wrote…

in_close_list

Done.

Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Jun 4, 2017

@Diadlo can you squash the commits?

@Diadlo
Copy link
Author

Diadlo commented Jun 4, 2017

@iphydf All in one?

@Diadlo
Copy link
Author

Diadlo commented Jun 4, 2017

I can, but if some of changes will provide regression, it will be very hard to bisect it

@iphydf
Copy link
Member

iphydf commented Jun 4, 2017

Having commits called 1, 2, 3, 4, is not useful. Either squash them into a single commit with a good description, or several commits, also with good descriptions.

@iphydf
Copy link
Member

iphydf commented Jun 4, 2017

Ok, if they are separately useful commits then do keep them separate, but rename them from 1, 2, 3 to something that explains what that commit does.

@Diadlo
Copy link
Author

Diadlo commented Jun 4, 2017

@iphydf Ok. It will take some time

@Diadlo
Copy link
Author

Diadlo commented Jun 4, 2017

@iphydf Is it better now?

Diadlo added 14 commits June 5, 2017 01:51
@Diadlo
Extract SharedKey struct and use it as var instead of indexing
b3174bb
@Diadlo
Add 'index_of' functions
db7b422 
- Replace 'for' with long body
- Replace friend_number function on index_of_friend_pk
- Replace client_in_nodelist on index_of_node_pk
@Diadlo
Add using macros to improve code
6197494
@Diadlo
Change return type to be the same as return value type
69c71dd 
Return value of 'addto_list' function is 'used' counter, which is uint32_t.
It will be better to use the same return type.
@Diadlo
Extract variables
71260e3
@Diadlo
Extract 'update_client' function
0eaa446 
To make 'client_or_ip_port_in_list' code easy to read
@Diadlo
Extract 'update_client_with_reset' function to reduce code duplication
ffcee69
@Diadlo
Extract 'update_client_data' to reduce code duplication
d950376
@Diadlo
Update crypto_size and check index
7864f8d
@Diadlo
Add using id_equal
3902fd3
@Diadlo
Cleanup punch_holes
6a69e07
@Diadlo
Improve formatting and syntax
91e3eb0
@Diadlo
Make work with assoc more generic
f9607bc
@Diadlo
Change way to iterate through assoc
ced07d6
@iphydf
Copy link
Member

iphydf commented Jun 5, 2017

:lgtm_strong:

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

Comments from Reviewable

@iphydf iphydf merged commit ced07d6 into TokTok:master Jun 5, 2017
@Diadlo Diadlo deleted the dht_refactor branch June 5, 2017 06:42
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@zugz zugz

@iphydf iphydf

Labels
None yet
Projects
None yet
Milestone
v0.1.9
Development

Successfully merging this pull request may close these issues.
6 participants
@Diadlo @nurupo @zugz @iphydf @CLAassistant @SkyzohKey