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.

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

Make DHT a module-private type. #688

Merged
merged 1 commit into from
Jan 17, 2018
Merged

Make DHT a module-private type. #688

merged 1 commit into from
Jan 17, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jan 15, 2018

This change is Reviewable

@iphydf iphydf added this to the v0.2.0 milestone Jan 15, 2018
@jhert0
Copy link
Member

jhert0 commented Jan 15, 2018

:lgtm_strong:


Reviewed 15 of 15 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@sudden6
Copy link

sudden6 commented Jan 15, 2018

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


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

const Client_data *dht_get_close_client(const DHT *dht, uint32_t client_num)
{
    return &dht->close_clientlist[client_num];

check for out of bounds read necessary? If not add a comment stating that this function doesn't check bounds


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

DHT_Friend *dht_get_friend(DHT *dht, uint32_t friend_num)
{
    return &dht->friends_list[friend_num];

same


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

const uint8_t *dht_get_friend_public_key(const DHT *dht, uint32_t friend_num)
{
    return dht->friends_list[friend_num].public_key;

same


toxcore/LAN_discovery.c, line 359 at r1 (raw file):

    char ip_str[IP_NTOA_LEN] = { 0 };
    ip_ntoa(&source.ip, ip_str, sizeof(ip_str));
    LOGGER_DEBUG(dht->log, "Found node in LAN: %s", ip_str);

Intended change or accidentally removed?


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Jan 16, 2018

Review status: 12 of 16 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.


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

Previously, sudden6 wrote…

check for out of bounds read necessary? If not add a comment stating that this function doesn't check bounds

Added an assert. Proper bound checking can be added later, but it requires the caller to check the return type. In this PR, I'm avoiding logic changes.


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

Previously, sudden6 wrote…

same

Done.


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

Previously, sudden6 wrote…

same

Done.


toxcore/LAN_discovery.c, line 359 at r1 (raw file):

Previously, sudden6 wrote…

Intended change or accidentally removed?

Intentional, and also removed the filter in auto_tests/helpers.h. Why: because this is complete spam and doesn't help regular debugging at all.


Comments from Reviewable

@jhert0
Copy link
Member

jhert0 commented Jan 16, 2018

:lgtm_strong:


Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

@iphydf iphydf force-pushed the adt-dht branch 2 times, most recently from 54be86a to 6ce70cd Compare January 16, 2018 17:24
@sudden6
Copy link

sudden6 commented Jan 16, 2018

:lgtm_strong:


Review status: 4 of 16 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf merged commit 643eea6 into TokTok:master Jan 17, 2018
@iphydf iphydf deleted the adt-dht branch January 17, 2018 19:07
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants