Skip to content

Improve handling of peers entering and leaving conferences#1267

Merged
iphydf merged 1 commit into
TokTok:masterfrom
zugz:minpgcFixes
Jan 6, 2019
Merged

Improve handling of peers entering and leaving conferences#1267
iphydf merged 1 commit into
TokTok:masterfrom
zugz:minpgcFixes

Conversation

@zugz

@zugz zugz commented Nov 18, 2018

Copy link
Copy Markdown

(based on #1266)

  • delete existing peers with same real_pk on adding a peer
  • send freeze packet on quit

This change is Reviewable

@hugbubby

Copy link
Copy Markdown
Member

Make sure you run astyle so Travis doesn't freak out.

@hugbubby

hugbubby commented Nov 18, 2018

Copy link
Copy Markdown
Member

You also need to fix the codefactor issues. 3 of them are about there being redundant or unnecessary newlines, one is because a method in this patch is more "complex" than codefactor likes (has too many arguments, return paths, etc.)

@codecov

codecov Bot commented Nov 22, 2018

Copy link
Copy Markdown

Codecov Report

Merging #1267 into master will increase coverage by <.1%.
The diff coverage is 96.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1267     +/-   ##
========================================
+ Coverage      83%     83%   +<.1%     
========================================
  Files          82      82             
  Lines       14928   14957     +29     
========================================
+ Hits        12392   12418     +26     
- Misses       2536    2539      +3
Impacted Files Coverage Δ
toxcore/group.c 78.7% <96.5%> (+0.3%) ⬆️
toxcore/LAN_discovery.c 84.9% <0%> (-1%) ⬇️
toxcore/onion_client.c 95.3% <0%> (-0.7%) ⬇️
toxcore/friend_connection.c 94.6% <0%> (-0.3%) ⬇️
toxav/toxav.c 67.6% <0%> (ø) ⬆️
toxcore/DHT.c 77.8% <0%> (ø) ⬆️
toxav/msi.c 65.6% <0%> (+1.1%) ⬆️

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 e08f631...e5561ad. Read the comment docs.

@anthonybilinski

Copy link
Copy Markdown

From my own use, I used to reproduce a corruption of the tox savefile frequently when running off tip. Since I switched to this PR branch, I haven't seen it once.

To recover from the corrupt save file, downgrading toxcore to before persistent conferences were introduced, loading then saving, then upgrading to this PR should fix it. Upgrading to this PR directly from a corrupt save file on tip will just fail to load completely.

@sudden6 sudden6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @zugz)


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

}

static bool delete_frozen(Group_c *g, uint32_t frozen_index)

add nullptr check or comment that g must not be nullptr


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

 * return -1 on failure
 */
static int group_leave(const Group_Chats *g_c, uint32_t groupnumber, bool permanent)

why change the return type? IIRC we're trying to use bool for success/failure return codes


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

 * return -1 on failure
 */
static int group_freeze_peer_send(const Group_Chats *g_c, uint32_t groupnumber, uint16_t peer_num)

use bool for success/error return types?

@robinlinden robinlinden left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, sudden6 wrote…

add nullptr check or comment that g must not be nullptr

That's fine for functions in the public API, but I think that's unnecessary for things that are only used internally in Toxcore.

@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.9 Jan 3, 2019
@robinlinden robinlinden mentioned this pull request Jan 3, 2019

@sudden6 sudden6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:

Reviewed 1 of 1 files at r3.
Reviewable status: 0 of 1 approvals obtained


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

Previously, robinlinden (Robin Lindén) wrote…

That's fine for functions in the public API, but I think that's unnecessary for things that are only used internally in Toxcore.

ok, ignoring this then.

@anthonybilinski

Copy link
Copy Markdown

#1285 may be related to this change.

* send freeze packet on quit
* delete existing peers with same real_pk on adding a peer
* record actual number of conference peers saved
@iphydf iphydf merged commit e5561ad into TokTok:master Jan 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants