add configurable limit on number of stored frozen peers #1309

zugz · 2019-02-17T16:39:46Z

This change is

sudden6

Reviewed 3 of 6 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @zugz)

auto_tests/conference_test.c, line 168 at r1 (raw file):

    const bool check_name_change_propagation = false;

    /* each peer should freeze at least its two friends, but freezing more

This test seems to only test the function that sets the limit, but doesn't actually check if the limit is enforced, right? I think testing for the later is the most important part of this PR, no?

toxcore/group.c, line 789 at r1 (raw file):

    qsort(g->frozen, g->numfrozen, sizeof(Group_Peer), cmp_frozen);

    g->numfrozen = g->maxfrozen;

Why is the now unused memory not freed?

toxcore/group.c, line 3306 at r1 (raw file):

        if (g->numfrozen > g->maxfrozen) {
            g->maxfrozen = g->numfrozen;

shouldn't we trim the list here using remove_old_frozen()?

nurupo · 2019-02-19T01:00:52Z

toxcore/group.c, line 1293 at r1 (raw file):

    }

    g->maxfrozen = maxfrozen;

Shouldn't the g->frozen array be resized to make sure it can fit the requested maxfrozen?

nurupo · 2019-02-19T01:02:21Z

toxcore/group.c, line 781 at r1 (raw file):

 * return true if any frozen peers are removed.
 */
static bool remove_old_frozen(Group_c *g)

The function is called remove_old_frozen, which implies some kind of deallocation. If it doesn't actually deallocate any memory, perhaps it should be renamed more appropriately, like trim_frozen or something, idk, I'm horrible at naming things.

codecov · 2019-02-19T21:51:39Z

Codecov Report

Merging #1309 into master will decrease coverage by 0.5%.
The diff coverage is 88.4%.

@@           Coverage Diff            @@
##           master   #1309     +/-   ##
========================================
- Coverage    84.6%     84%   -0.6%     
========================================
  Files          86      85      -1     
  Lines       15705   15491    -214     
========================================
- Hits        13291   13024    -267     
- Misses       2414    2467     +53

Impacted Files	Coverage Δ
auto_tests/conference_test.c	`99.3% <100%> (ø)`	⬆️
toxcore/tox.c	`65% <83.3%> (+0.8%)`	⬆️
toxcore/group.c	`91.5% <86.8%> (-0.3%)`	⬇️
toxcore/crypto_core_test.cc	`90.3% <0%> (-9.7%)`	⬇️
toxcore/state.c	`66.6% <0%> (-7.4%)`	⬇️
toxav/audio.c	`65.7% <0%> (-4.1%)`	⬇️
toxcore/tox_api.c	`61.2% <0%> (-3.6%)`	⬇️
toxcore/ping_array.c	`91.9% <0%> (-3.3%)`	⬇️
toxcore/TCP_client.c	`62.3% <0%> (-2.4%)`	⬇️
toxcore/TCP_server.c	`80.3% <0%> (-2.4%)`	⬇️
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d26b11d...306dd1f. Read the comment docs.

zugz

Reviewable status: 0 of 1 approvals obtained (waiting on @nurupo and @sudden6)

auto_tests/conference_test.c, line 168 at r1 (raw file):

Previously, sudden6 wrote…

This test seems to only test the function that sets the limit, but doesn't actually check if the limit is enforced, right? I think testing for the later is the most important part of this PR, no?

Done. Which did turn up a bug (is_friend wasn't being set on loaded peers), so good call.

toxcore/group.c, line 781 at r1 (raw file):

Previously, nurupo wrote…

The function is called remove_old_frozen, which implies some kind of deallocation. If it doesn't actually deallocate any memory, perhaps it should be renamed more appropriately, like trim_frozen or something, idk, I'm horrible at naming things.

deallocates now

toxcore/group.c, line 789 at r1 (raw file):

Previously, sudden6 wrote…

Why is the now unused memory not freed?

Right, we could be calling this as a result of reducing the max with the aim of freeing memory. Done.

toxcore/group.c, line 1293 at r1 (raw file):

Previously, nurupo wrote…

Shouldn't the g->frozen array be resized to make sure it can fit the requested maxfrozen?

I don't think so. It's just a limit, returning success isn't a guarantee that the memory will actually be available. I don't think we want to reserve the memory, quite possibly needlessly, just to be able to make that guarantee.

toxcore/group.c, line 3306 at r1 (raw file):

Previously, sudden6 wrote…

shouldn't we trim the list here using remove_old_frozen()?

No, that wouldn't do anything.

sudden6

Reviewed 1 of 6 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo and @zugz)

toxcore/group.c, line 3306 at r1 (raw file):

Previously, zugz (zugz) wrote…

No, that wouldn't do anything.

Why not? Imagine the following scenario: Client A saves a conference with 200 numfrozen peers, now Client B loads this .tox save
and never reduces the file size to the intended maximum (default 128). IMO that's not the desired outcome?

zugz

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo and @sudden6)

toxcore/group.c, line 3306 at r1 (raw file):

Previously, sudden6 wrote…

Why not? Imagine the following scenario: Client A saves a conference with 200 numfrozen peers, now Client B loads this .tox save
and never reduces the file size to the intended maximum (default 128). IMO that's not the desired outcome?

That was intended. The alternative is to throw away information that's in the save file. I don't think that should happen unless it's explicitly asked for by resetting the maximum. There's also the problem that, currently, we can't increase the maximum until after the group is loaded, at which point it's too late.

sudden6

Reviewed 1 of 2 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @nurupo)

toxcore/group.c, line 3306 at r1 (raw file):

Previously, zugz (zugz) wrote…

That was intended. The alternative is to throw away information that's in the save file. I don't think that should happen unless it's explicitly asked for by resetting the maximum. There's also the problem that, currently, we can't increase the maximum until after the group is loaded, at which point it's too late.

ok, with that reasoning it makes sense.

sudden6

from my side

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @nurupo)

sudden6 · 2019-03-09T13:53:39Z

@nurupo what's your state on this? If there are no further issues, I'll go ahead an merge it.

nurupo

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo and @zugz)

toxcore/group.c, line 789 at r2 (raw file):

    qsort(g->frozen, g->numfrozen, sizeof(Group_Peer), cmp_frozen);

    g->numfrozen = g->maxfrozen;

You should update g->numfrozen after you actually shrunk the array, so I suggest to move this assignment to right after the if-statement and accordingly change g->numfrozen to g->maxfrozen in the realloc. The reasoning is that if realloc does fail and the function returns early, this g->numfrozen = g->maxfrozen; causes an inconsistent state, you basically say "hey, this array is 10 items less now!" but then you failed to remove those 10 items.

toxcore/group.c, line 791 at r2 (raw file):

    g->numfrozen = g->maxfrozen;

    Group_Peer *temp = (Group_Peer *)realloc(g->frozen, sizeof(Group_Peer) * g->numfrozen);

Just to make sure, can the 2nd arg here ever be 0? Because realloc() with new size being 0 is an undefined behavior.

toxcore/group.c, line 3312 at r2 (raw file):

            data += peer->nick_len;

            // XXX: this relies on friends being loaded before conferences.

What's "XXX"?

zugz · 2019-03-17T18:40:34Z

Reviewables isn't working for me at the moment, so I'm responding here by email to nurupo's review. * Saturday, 2019-03-09 at 21:26 -0800 - nurupo <[email protected]>:

You should update `g->numfrozen` after you actually shrunk the array,

Done.

Just to make sure, can the 2nd arg here ever be 0? Because `realloc()` with new size being 0 is an undefined behavior.

According to the manpage, realloc is equivalent to free if size is 0, which is what we want.

What's "XXX"?

It's sometimes used to warn about something surprising/unpleasant. But I've changed it to "NOTE".

zugz · 2019-03-17T19:28:05Z

* Sunday, 2019-03-17 at 11:40 -0700 - zugz <[email protected]>:

* Saturday, 2019-03-09 at 21:26 -0800 - nurupo ***@***.***>: >Just to make sure, can the 2nd arg here ever be 0? Because `realloc()` >with new size being 0 is an undefined behavior. According to the manpage, realloc is equivalent to free if size is 0, which is what we want.

Sorry, that only goes for the glibc implementation. Thanks to robinli for setting me right on this. maxfrozen can indeed be set to 0. I've pushed a commit which handles this, and also correctly handles numfrozen being 0 on load. Thanks for pointing this out!

nurupo

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @zugz)

nurupo · 2019-04-05T11:18:39Z

Looks good to me. At least just the the code you have added, when considered locally as in the diff. Can't say anything about the global picture, about how it ties in with the whole conferences thing, if the changes you have made are complete and are done right, since I'm not familiar with conferences code.

zugz · 2019-04-06T14:12:00Z

@nurupo: I realised freeze_peer wasn't correctly handling the case that delpeer fails (which would be due to a realloc failure). I've just pushed a commit to fix this; would you mind checking your approval still applies?

nurupo

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @zugz)

toxcore/group.c, line 831 at r4 (raw file):

    }

    ++g->numfrozen;

Shouldn't that increment be above the if-statement? Otherwise you grow g->frozen by one but don't increment g->numfrozen accordingly.

zugz · 2019-04-07T20:31:10Z

nurupo requested changes on this pull request. Shouldn't that increment be above the if-statement? Otherwise you grow `g->frozen` by one but don't increment `g->numfrozen` accordingly.

g->numfrozen is the number of frozen peers. If we can't delete the peer, we shouldn't freeze it. This does mean that g->numfrozen will be less than the allocated size of g->frozen in this case. But we never guaranteed that it would be, so I don't see why that would be a problem.

nurupo

Oh, so you don't track the size of the array. I guess this makes sense.

Reviewed 4 of 6 files at r1, 2 of 2 files at r4.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @zugz)

sudden6

reviewed again, ping @robinli to merge

Reviewed 1 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @zugz)

sudden6 · 2019-05-19T16:48:09Z

@zugz I think you need to rebase this on master in order to get CI fixed.

zugz force-pushed the limitFrozen branch from ac898ec to 170f99b Compare February 17, 2019 17:27

sudden6 requested changes Feb 18, 2019

View reviewed changes

zugz commented Feb 19, 2019

View reviewed changes

sudden6 requested changes Feb 19, 2019

View reviewed changes

zugz commented Feb 20, 2019

View reviewed changes

sudden6 reviewed Feb 20, 2019

View reviewed changes

sudden6 approved these changes Feb 20, 2019

View reviewed changes

nurupo requested changes Mar 10, 2019

View reviewed changes

nurupo approved these changes Apr 5, 2019

View reviewed changes

zugz force-pushed the limitFrozen branch from 44912ff to b5a97d0 Compare April 6, 2019 14:10

nurupo requested changes Apr 7, 2019

View reviewed changes

nurupo approved these changes Apr 23, 2019

View reviewed changes

zugz mentioned this pull request May 4, 2019

Some small fixes to groups. #1321

Merged

sudden6 approved these changes May 16, 2019

View reviewed changes

add configurable limit on number of stored frozen peers

306dd1f

zugz force-pushed the limitFrozen branch from ad2b30d to 306dd1f Compare May 19, 2019 16:54

robinlinden merged commit 306dd1f into TokTok:master May 20, 2019

robinlinden added this to the v0.2.10 milestone May 20, 2019

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add configurable limit on number of stored frozen peers #1309

add configurable limit on number of stored frozen peers #1309

zugz commented Feb 17, 2019 •

edited by iphydf

Loading

sudden6 left a comment

nurupo commented Feb 19, 2019

nurupo commented Feb 19, 2019

codecov bot commented Feb 19, 2019 •

edited

Loading

zugz left a comment

sudden6 left a comment

zugz left a comment

sudden6 left a comment

sudden6 left a comment

sudden6 commented Mar 9, 2019

nurupo left a comment

zugz commented Mar 17, 2019 via email

zugz commented Mar 17, 2019 via email

nurupo left a comment

nurupo commented Apr 5, 2019

zugz commented Apr 6, 2019 via email

nurupo left a comment

zugz commented Apr 7, 2019 via email

nurupo left a comment

sudden6 left a comment

sudden6 commented May 19, 2019

add configurable limit on number of stored frozen peers #1309

add configurable limit on number of stored frozen peers #1309

Conversation

zugz commented Feb 17, 2019 • edited by iphydf Loading

sudden6 left a comment

Choose a reason for hiding this comment

nurupo commented Feb 19, 2019

nurupo commented Feb 19, 2019

codecov bot commented Feb 19, 2019 • edited Loading

Codecov Report

zugz left a comment

Choose a reason for hiding this comment

sudden6 left a comment

Choose a reason for hiding this comment

zugz left a comment

Choose a reason for hiding this comment

sudden6 left a comment

Choose a reason for hiding this comment

sudden6 left a comment

Choose a reason for hiding this comment

sudden6 commented Mar 9, 2019

nurupo left a comment

Choose a reason for hiding this comment

zugz commented Mar 17, 2019 via email

zugz commented Mar 17, 2019 via email

nurupo left a comment

Choose a reason for hiding this comment

nurupo commented Apr 5, 2019

zugz commented Apr 6, 2019 via email

nurupo left a comment

Choose a reason for hiding this comment

zugz commented Apr 7, 2019 via email

nurupo left a comment

Choose a reason for hiding this comment

sudden6 left a comment

Choose a reason for hiding this comment

sudden6 commented May 19, 2019

zugz commented Feb 17, 2019 •

edited by iphydf

Loading

codecov bot commented Feb 19, 2019 •

edited

Loading