Fix problems with initial connections and name-setting in conferences#1035
Conversation
iphydf
left a comment
There was a problem hiding this comment.
Reviewed 1 of 6 files at r1.
Reviewable status: 0 of 1 LGTMs obtained
auto_tests/conference_test.c, line 105 at r1 (raw file):
for (uint16_t i = 0; i < NUM_GROUP_TOX; ++i) { for (uint16_t j = 0; j < NUM_GROUP_TOX; ++j) { const int len = tox_conference_peer_get_name_size(toxes[i], 0, j, nullptr);
size_t or uint32_t
auto_tests/conference_test.c, line 154 at r1 (raw file):
char name[NAMELEN+1]; snprintf(name,NAMELEN+1,"Tox #%4d",tox_index[i]);
Run format-source.
toxcore/group.c, line 2043 at r1 (raw file):
static bool check_message_info(uint32_t message_number, uint8_t message_id, Group_Peer *peer) { bool ignore_older = (message_id == GROUP_MESSAGE_NAME_ID || message_id == GROUP_MESSAGE_TITLE_ID);
const bool
toxcore/group.c, line 2046 at r1 (raw file):
Message_Info *i; for (i = peer->last_message_infos; i < peer->last_message_infos + peer->num_last_message_infos; ++i) {
Can you factor out this for-loop into a separate function returning Message_Info* or nullptr?
toxcore/group.c, line 2067 at r1 (raw file):
for (Message_Info *j = peer->last_message_infos + peer->num_last_message_infos - 1; j > i; --j) { *j = *(j-1);
This looks like memmove. Use memmove?
iphydf
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
auto_tests/conference_test.c, line 155 at r1 (raw file):
char name[NAMELEN+1]; snprintf(name,NAMELEN+1,"Tox #%4d",tox_index[i]); tox_self_set_name(toxes[i], name, NAMELEN, nullptr);
name needs a cast to 'const uint8_t *`.
iphydf
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
auto_tests/conference_test.c, line 105 at r1 (raw file):
Previously, iphydf wrote…
size_t or uint32_t
Still (this comment is on line 105, regarding len).
auto_tests/conference_test.c, line 114 at r2 (raw file):
snprintf(expected_name, NAMELEN + 1, NAME_FORMAT_STR, tox_index[j]); ck_assert_msg(memcmp(name, expected_name, NAMELEN) == 0, "name of #%d according to #%d is \"" NAME_FORMAT "\"; expected \"%s\"",
%u since tox_indexes are unsigned.
auto_tests/conference_test.c, line 164 at r2 (raw file):
char name[NAMELEN + 1]; snprintf(name, NAMELEN + 1, NAME_FORMAT_STR, tox_index[i]); tox_self_set_name(toxes[i], (const uint8_t *) name, NAMELEN, nullptr);
No space after cast.
toxcore/group.c, line 2042 at r2 (raw file):
} Message_Info *find_message_slot_or_reject(uint32_t message_number, uint8_t message_id, Group_Peer *peer)
Make this internal linkage.
toxcore/group.c, line 2048 at r2 (raw file):
Message_Info *i; for (i = peer->last_message_infos; i < peer->last_message_infos + peer->num_last_message_infos; ++i) {
You can write for (Message_Info *i = ...) { and instead of break, write return i and instead of return nullptr, write break, and at the bottom return nullptr.
toxcore/group.c, line 2072 at r2 (raw file):
static bool check_message_info(uint32_t message_number, uint8_t message_id, Group_Peer *peer) { Message_Info *i = find_message_slot_or_reject(message_number, message_id, peer);
Message_Info *const i = ...
toxcore/group.c, line 2121 at r2 (raw file):
if (g->number_joined != -1 && count_close_connected(g) >= DESIRED_CLOSE_CONNECTIONS) { int fr_close_index = friend_in_close(g, g->number_joined);
const
|
* Sunday, 2018-07-29 at 17:13 +0000 - iphydf <notifications@github.com>:
You can write `for (Message_Info *i = ...) {` and instead of `break`,
write `return i` and instead of `return nullptr`, write `break`, and at
the bottom `return nullptr`.
That doesn't work, sadly - we're also interested in the case that the
condition in the 'for' fails.
Done the rest, except for fixing the other conference tests.
|
iphydf
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
auto_tests/conference_test.c, line 21 at r3 (raw file):
#define GROUP_MESSAGE "Install Gentoo" #define NAME_FORMAT_STR "Tox #%4d"
%4u
toxcore/group.c, line 2143 at r3 (raw file):
uint32_t message_number; memcpy(&message_number, data + sizeof(uint16_t), sizeof(message_number));
Can you use net_unpack_u32 here?
iphydf
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
auto_tests/conference_test.c, line 108 at r3 (raw file):
for (uint16_t j = 0; j < NUM_GROUP_TOX; ++j) { const size_t len = tox_conference_peer_get_name_size(toxes[i], 0, j, nullptr); ck_assert_msg(len == NAMELEN, "name of #%u according to #%u has incorrect length %u", tox_index[j], tox_index[i], len);
Now you need to cast len to (unsigned int) to fit into %u.
|
* Sunday, 2018-07-29 at 21:34 +0000 - iphydf <notifications@github.com>:
*[toxcore/group.c, line 2143 at r3](https://reviewable.io/reviews/toktok/c-toxcore/1035#-LIbxWo1AMSHkR82qOPX:-LIbxWo1AMSHkR82qOPY:bllger9) ([raw file](https://github.com/toktok/c-toxcore/blob/ed89dc79aaac37132a21dd5eb8a4e2b681366235/toxcore/group.c#L2143)):*
> ```C
>
> uint32_t message_number;
> memcpy(&message_number, data + sizeof(uint16_t), sizeof(message_number));
> ```
Can you use `net_unpack_u32` here?
This was existing code, just moved. I can change it, but maybe it would
make more sense to convert the whole codebase to use net_unpack_* in one
go, in a separate PR?
|
iphydf
left a comment
There was a problem hiding this comment.
Reviewed 1 of 3 files at r2, 6 of 6 files at r4.
Reviewable status:complete! 1 of 1 LGTMs obtained
robinlinden
left a comment
There was a problem hiding this comment.
Reviewed 1 of 3 files at r2, 6 of 6 files at r4.
Reviewable status:complete! 2 of 1 LGTMs obtained
auto_tests/conference_test.c, line 132 at r4 (raw file):
const size_t len = tox_conference_peer_get_name_size(toxes[i], 0, j, nullptr); ck_assert_msg(len == NAMELEN, "name of #%u according to #%u has incorrect length %u", state[j].id, state[i].id, (unsigned int) len);
Nit: no space between cast and variable.
|
* Wednesday, 2018-08-01 at 19:35 +0000 - Robin Lindén <notifications@github.com>:
Nit: no space between cast and variable.
Done.
Also improved connectedness detetection, and signed the commit.
|
* test names in conference_test
* raise error on attempt to invite friend to group before we are connected
* revise handling of temporary invited connections
We are now careful not to prematurely delete a connection to a peer
established during the invitation process; namely, before we have sufficient
other connections and have confirmed that we have an alternative route to the
peer.
* process out-of-order messages from a peer
* don't reset names when handling a Peer Response
robinlinden
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5.
Reviewable status:complete! 2 of 1 LGTMs obtained
This change is