Add toxcore capabilities to "online" packet sent to friends #1033

zoff99 · 2018-07-27T12:09:15Z

This change is

hugbubby

Would you mind writing a better commit description and name for this?

Reviewable status: 0 of 2 LGTMs obtained (waiting on @zugz and @robinlinden)

toxcore/Messenger.c, line 2177 at r1 (raw file):

/* get capabilities of friend's toxcore
 * return TOX_CAPABILITY_BASIC on any error

I don't like that. The function should turn an error code in case of any error, and let the caller decide whether or not to assume TOX_COMPATIBILITY_BASIC. People that call this function will have no idea if there was an error code or their compatibility is really basic.

iphydf · 2018-07-28T00:39:35Z

toxcore/Messenger.c

+
+    write_cryptpacket(m->net_crypto, friend_connection_crypt_connection_id(m->fr_c,
+                      m->friendlist[friendnumber].friendcon_id), buf, TOX_CAPABILITIES_SIZE + 1, 0);
+    // TODO: what to do if res == -1 ?


Probably return -1.

iphydf · 2018-07-28T00:42:19Z

toxcore/Messenger.c

@@ -2157,6 +2173,23 @@ static int m_handle_status(void *object, int i, uint8_t status, void *userdata)
    return 0;
 }

+/* get capabilities of friend's toxcore
+ * return TOX_CAPABILITY_BASIC on any error


That's a surprising error behaviour. If the friend doesn't exist, you say that they have basic capabilities. In fact they have no capabilities at all.

toxcore/Messenger.h

iphydf · 2018-07-28T00:43:59Z

toxcore/Messenger.c

-        if (packet_id == PACKET_ID_ONLINE && len == 1) {
+        if (packet_id == PACKET_ID_ONLINE) {
+
+            if ((len >= 1) && (len == TOX_CAPABILITIES_SIZE)) {


Remove superfluous ().

iphydf · 2018-07-28T00:44:21Z

toxcore/Messenger.c

+        if (packet_id == PACKET_ID_ONLINE) {
+
+            if ((len >= 1) && (len == TOX_CAPABILITIES_SIZE)) {
+                uint64_t received_caps = TOX_CAPABILITY_BASIC;


Dead store.

iphydf · 2018-07-28T00:44:31Z

toxcore/Messenger.c

@@ -2169,7 +2202,14 @@ static int m_handle_packet(void *object, int i, const uint8_t *temp, uint16_t le
    uint32_t data_length = len - 1;

    if (m->friendlist[i].status != FRIEND_ONLINE) {
-        if (packet_id == PACKET_ID_ONLINE && len == 1) {
+        if (packet_id == PACKET_ID_ONLINE) {
+


Remove empty line.

The line is removed, read the line numbers, github is just weird.

Line 2206 is an empty line. I checked in my editor on the actual file.

Ohhhhhhhh that line

hugbubby

Reviewable status: 0 of 2 LGTMs obtained (waiting on @zugz and @robinlinden)

toxcore/Messenger.h, line 76 at r1 (raw file):

Previously, iphydf wrote…

The size of a capabilities packet is 1 + sizeof(uint64_t).

The size of the capabilities byte is sizeof(uint64_t) though, so this definition is correct. He correctly adds one later on when specifying the packet size.

If you want he could create another definition detailing the packet size or rename this macro to something else

zoff99

Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @zoff99, @iphydf, @zugz, and @robinlinden)

toxcore/Messenger.h, line 76 at r1 (raw file):

Previously, zoff99 (Zoff) wrote…

but the pure data size of the capabilties is sizeof(uint64_t)

Done.

toxcore/Messenger.c, line 2177 at r1 (raw file):

Previously, hugbubby wrote…

I don't like that. The function should turn an error code in case of any error, and let the caller decide whether or not to assume TOX_COMPATIBILITY_BASIC. People that call this function will have no idea if there was an error code or their compatibility is really basic.

its not an API function. also there is no error (code).
if something does not work, whatever it is then we must assume the "current status" meaning we have no special capabilities

toxcore/Messenger.c, line 2177 at r1 (raw file):

Previously, iphydf wrote…

That's a surprising error behaviour. If the friend doesn't exist, you say that they have basic capabilities. In fact they have no capabilities at all.

i think thats sort of correct. if we dont get the new capabilties we must assume the status as in <=0.2.4
what else would you do?

toxcore/Messenger.c, line 2206 at r1 (raw file):

Previously, hugbubby wrote…

Ohhhhhhhh that line

Done.

toxcore/Messenger.c, line 2207 at r1 (raw file):

Previously, iphydf wrote…

Remove superfluous ().

Done.

toxcore/Messenger.c, line 2208 at r1 (raw file):

Previously, iphydf wrote…

Dead store.

my C knowledge is not that firm. can i just use a define as pointer?

net_unpack_u64(data, &TOX_CAPABILITY_BASIC);

not sure if that works?

zoff99

Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @hugbubby, @zoff99, @iphydf, @zugz, and @robinlinden)

toxcore/Messenger.c, line 170 at r1 (raw file):

Previously, iphydf wrote…

Probably return -1.

does the caller of send_online_packet() handle that correctly?

maybe we need more measures here. it could result in a horrible loop:
send new online packet -> error -> return -1
caller calls again now send "old" online packet -> error -> return -1
caller calls again now send new online packet -> error -> return -1
...

zoff99

Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @zoff99, @iphydf, @zugz, and @robinlinden)

toxcore/Messenger.c, line 2177 at r1 (raw file):

Previously, zoff99 (Zoff) wrote…

its not an API function. also there is no error (code).
if something does not work, whatever it is then we must assume the "current status" meaning we have no special capabilities

no "people" will be calling this function.
thats the point, its not allowed to decide the value. it must be set by strict rules in toxcore.
the value is hardcoded in each version / branch of toxcore sourcecode. it's not changed at runtime

maybe later we will add client capabilities aswell, then that's correct.

iphydf

Reviewable status: 0 of 2 LGTMs obtained (waiting on @zugz and @robinlinden)

toxcore/Messenger.c, line 2208 at r1 (raw file):

Previously, zoff99 (Zoff) wrote…

my C knowledge is not that firm. can i just use a define as pointer?

net_unpack_u64(data, &TOX_CAPABILITY_BASIC);

not sure if that works?

Remove the initialisation. received_caps is initialised by net_unpack_u64.

iphydf

Reviewable status: 0 of 2 LGTMs obtained (waiting on @zugz and @robinlinden)

toxcore/Messenger.c, line 2177 at r1 (raw file):

Previously, zoff99 (Zoff) wrote…

i think thats sort of correct. if we dont get the new capabilties we must assume the status as in <=0.2.4
what else would you do?

Error and handle it in the caller. The logic is the same, but the location is different.

zoff99

Reviewable status: 0 of 2 LGTMs obtained (waiting on @zugz and @robinlinden)

toxcore/Messenger.c, line 2208 at r1 (raw file):

Previously, iphydf wrote…

Remove the initialisation. received_caps is initialised by net_unpack_u64.

Done.

zoff99

Reviewable status: 0 of 2 LGTMs obtained (waiting on @zugz and @robinlinden)

toxcore/Messenger.c, line 2177 at r1 (raw file):

Previously, iphydf wrote…

Error and handle it in the caller. The logic is the same, but the location is different.

i don't think this is correct in this case.
what would an error mean actually?
you can't ask to receive the packet again.
you can't do anything.

either you get capabilities or it's "BASIC"

the only real error is "friend not valid" but even then i am not sure.
also with -1 as error you cant give the result back (doesnt fit in uint64_t)

zugz · 2018-07-29T18:43:31Z

What is the purpose of the TOX_CAPABILITY_BASIC flag? What does it mean if it is set to false? I don't think we should actually reserve flags for anything yet where we can't specify exactly what it will mean, as is the case at least for the PGC flag. Separately: perhaps it would make sense to reserve the first bit for future expansion - if it's set, it means "ignore this bitmap, and look instead at the successor method of specifying capabilities" (whatever that turns out to be).

add comment of int64_t to int conversion

zoff99

fixed both things

Reviewable status: 0 of 2 LGTMs obtained (waiting on @zugz and @robinlinden)

zoff99

Reviewable status: 0 of 2 LGTMs obtained (waiting on @zugz and @robinlinden)

toxcore/Messenger.c, line 170 at r1 (raw file):

Previously, zoff99 (Zoff) wrote…

does the caller of send_online_packet() handle that correctly?

maybe we need more measures here. it could result in a horrible loop:
send new online packet -> error -> return -1
caller calls again now send "old" online packet -> error -> return -1
caller calls again now send new online packet -> error -> return -1
...

Done.

zoff99 · 2018-07-30T14:59:42Z

@iphydf
error: ISO C restricts enumerator values to range of 'int'
how can i make the flags 64bit wide?

iphydf · 2018-08-14T22:48:51Z

@zoff99 there are open comments from @zugz.

cuz

codecov · 2018-11-04T12:23:03Z

Codecov Report

❗ No coverage uploaded for pull request base (master@aa63c13). Click here to learn what that means.
The diff coverage is 80%.

@@           Coverage Diff            @@
##             master   #1033   +/-   ##
========================================
  Coverage          ?   82.9%           
========================================
  Files             ?      82           
  Lines             ?   14697           
  Branches          ?       0           
========================================
  Hits              ?   12195           
  Misses            ?    2502           
  Partials          ?       0

Impacted Files	Coverage Δ
toxcore/Messenger.c	`86.6% <80%> (ø)`

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 aa63c13...0088abd. Read the comment docs.

add TOX_CAPABILITY_CAPABILITIES as new default

(in case of a future bug *mocking* reserved)

add toxcore capabilities

37e7e64

zoff99 assigned iphydf Jul 27, 2018

zoff99 requested review from zugz and robinlinden July 27, 2018 12:09

zoff99 added enhancement New feature for the user, not a new feature for build script messenger Messenger labels Jul 27, 2018

zoff99 added 2 commits July 27, 2018 14:22

fix pointer

257fedc

appease codacy

4547372

hugbubby previously requested changes Jul 27, 2018

View reviewed changes

iphydf requested changes Jul 28, 2018

View reviewed changes

hugbubby reviewed Jul 28, 2018

View reviewed changes

zoff99 added 2 commits July 28, 2018 09:54

remove empty line

374d5fd

remove unneded condition

8ca9e08

zoff99 commented Jul 28, 2018

View reviewed changes

iphydf added this to the v0.2.x milestone Jul 28, 2018

iphydf requested changes Jul 29, 2018

View reviewed changes

removed initialisation

0b7c473

zoff99 commented Jul 29, 2018

View reviewed changes

return toxcore_capabilities even for offline friends

63ca9fe

zoff99 added 4 commits July 30, 2018 11:28

fix ToxCapabilitiesFlags definition

3ded7ba

typo

f16015e

handle error in send_online_packet

5572a77

add comment of int64_t to int conversion

fix suggested issues with capabilities

47f6e3f

zoff99 commented Jul 30, 2018

View reviewed changes

update the condition

1c1f80e

zoff99 and others added 3 commits July 30, 2018 20:24

fix: ISO C restricts enumerator values to range of ‘int’

8b944a9

Merge branch 'master' into zoff99/capabilities_add_001

273dd1f

Merge branch 'master' into zoff99/capabilities_add_001

60c64af

iphydf assigned zugz Aug 4, 2018

Merge branch 'master' into zoff99/capabilities_add_001

d8575ad

iphydf changed the title ~~add toxcore capabilities~~ Add toxcore capabilities to "online" packet sent to friends Aug 4, 2018

iphydf removed their assignment Sep 9, 2018

zoff99 mentioned this pull request Oct 24, 2018

Proposal: multidevice and (partial) offline messages #1238

Closed

zoff99 added 3 commits November 4, 2018 13:17

small tweaks as requested

c90438e

change comment

2858d31

change comment

c0f7a40

zoff99 added 6 commits November 4, 2018 13:40

check for length of packet, ignore if not ok

5e6b3ae

wrong length of packet fixed

3b510e3

change comments

44f48ee

add TOX_CAPABILITY_CAPABILITIES as new default

remove unused line

cf926ba

(in case of a future bug *mocking* reserved)

start with the first bit

94f0785

use data_length

0088abd

zoff99 closed this Nov 4, 2018

iphydf mentioned this pull request Nov 4, 2018

Proposal: multidevice and (partial) offline messages #1259

Open

robinlinden modified the milestones: v0.2.x, v0.2.9 Jan 3, 2019

Add toxcore capabilities to "online" packet sent to friends #1033

Add toxcore capabilities to "online" packet sent to friends #1033

Uh oh!

Conversation

zoff99 commented Jul 27, 2018 • edited by iphydf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hugbubby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugbubby left a comment

Choose a reason for hiding this comment

Uh oh!

zoff99 left a comment

Choose a reason for hiding this comment

Uh oh!

zoff99 left a comment

Choose a reason for hiding this comment

Uh oh!

zoff99 left a comment

Choose a reason for hiding this comment

Uh oh!

iphydf left a comment

Choose a reason for hiding this comment

Uh oh!

iphydf left a comment

Choose a reason for hiding this comment

Uh oh!

zoff99 left a comment

Choose a reason for hiding this comment

Uh oh!

zoff99 left a comment

Choose a reason for hiding this comment

Uh oh!

zugz commented Jul 29, 2018 via email

Uh oh!

zoff99 left a comment

Choose a reason for hiding this comment

Uh oh!

zoff99 left a comment

Choose a reason for hiding this comment

Uh oh!

zoff99 commented Jul 30, 2018

Uh oh!

iphydf commented Aug 14, 2018

Uh oh!

codecov bot commented Nov 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

zoff99 commented Jul 27, 2018 •

edited by iphydf

Loading

codecov bot commented Nov 4, 2018 •

edited

Loading