Finish the messenger state plugin system #1046

e0ff · 2018-08-03T21:45:46Z

I have wrote all the plugins for them and set them up. ~~but it doesn't work yet, there is an issue with load and/or saving.~~

It seems to be working now, it passes the tests and I've tested it with uTox.

Depends on #1051.

This change is

iphydf · 2018-08-04T09:27:00Z

toxcore/DHT.c

 /* Save the DHT in data where data is an array of size dht_size(). */
 void dht_save(const DHT *dht, uint8_t *data)
 {
-    host_to_lendian32(data,  DHT_STATE_COOKIE_GLOBAL);
+    host_to_lendian32(data, DHT_STATE_COOKIE_GLOBAL);


Why doesn't this use write_cookie?

iphydf · 2018-08-04T09:27:34Z

toxcore/Messenger.c

+bool m_register_state_plugin(Messenger *m, Messenger_State_Type type, m_state_size_cb size_cb, m_state_load_cb load_cb,
+                             m_state_save_cb save_cb)
+{
+    void *temp = realloc(m->options.state_plugins, sizeof(Messenger_State_Plugin) * (m->options.state_plugins_length + 1));


Messenger_State_Plugin *temp = (Mess..

e0ff

Reviewable status: 0 of 1 LGTMs obtained

toxcore/DHT.c, line 2819 at r2 (raw file):

Previously, iphydf wrote…

Why doesn't this use write_cookie?

The DHT cookie is written differently, it has less padding. This is how the messenger cookie is written:

memset(data, 0, size32);
data += size32;
host_to_lendian32(data, MESSENGER_STATE_COOKIE_GLOBAL);
data += size32;

I was going to ask about this. Do you want me to get rid of the cookie function or I could add the extra padding before writing the messenger cookie.

toxcore/Messenger.c, line 2933 at r2 (raw file):

Previously, iphydf wrote…

Messenger_State_Plugin *temp = (Mess..

Done.

iphydf

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

toxcore/Messenger.c, line 2940 at r4 (raw file):

    const uint8_t index = m->options.state_plugins_length - 1;
    memset(&m->options.state_plugins[index], 0, sizeof(Messenger_State_Plugin));

memset on structures containing pointers does not necessarily yield a valid structure. The pointers inside may be messed up. You probably don't want a memset here at all, since you initialise all members, and you want to know about it if you don't.

toxcore/Messenger.c, line 2993 at r4 (raw file):

static uint32_t nospam_keys_size(const Messenger *m)
{
    (void) m;

No need for this. We don't warn on unused params.

toxcore/Messenger.c, line 2994 at r4 (raw file):

{
    (void) m;
    return (sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_SECRET_KEY_SIZE);

Remove outer parentheses.

toxcore/Messenger.c, line 3003 at r4 (raw file):

        load_secret_key(m->net_crypto, (&data[sizeof(uint32_t)]) + CRYPTO_PUBLIC_KEY_SIZE);

        if (public_key_cmp((&data[sizeof(uint32_t)]), nc_get_self_public_key(m->net_crypto)) != 0) {

Replace (&data[sizeof(uint32_t)]) with data + sizeof(uint32_t). The former expresses "pointer to that one element" while the latter expresses "data at offset ...".

toxcore/Messenger.c, line 3007 at r4 (raw file):

        }
    } else {
        return STATE_LOAD_STATUS_ERROR;    /* critical */

If you invert the condition, you can use early error return.

toxcore/Messenger.c, line 3167 at r4 (raw file):

static State_Load_Status load_name(Messenger *m, const uint8_t *data, uint32_t length)
{
    if ((length > 0) && (length <= MAX_NAME_LENGTH)) {

Remove superfluous parentheses.

toxcore/Messenger.c, line 3191 at r4 (raw file):

static State_Load_Status load_status_message(Messenger *m, const uint8_t *data, uint32_t length)
{
    if ((length > 0) && (length <= MAX_STATUSMESSAGE_LENGTH)) {

Same here.

toxcore/Messenger.c, line 3201 at r4 (raw file):

static uint32_t status_size(const Messenger *m)
{
    (void) m;

Remove void-cast.

toxcore/Messenger.c, line 3226 at r4 (raw file):

static uint32_t tcp_relay_size(const Messenger *m)
{
    (void) m;

Remove.

toxcore/Messenger.c, line 3260 at r4 (raw file):

static uint32_t path_node_size(const Messenger *m)
{
    (void) m;

Remove.

toxcore/Messenger.c, line 3270 at r4 (raw file):

    data = state_write_section_header(data, MESSENGER_STATE_COOKIE_TYPE, 0, MESSENGER_STATE_TYPE_PATH_NODE);
    memset(nodes, 0, sizeof(nodes));
    unsigned int num = onion_backup_nodes(m->onion_c, nodes, NUM_SAVED_PATH_NODES);

const

toxcore/Messenger.c, line 3271 at r4 (raw file):

    memset(nodes, 0, sizeof(nodes));
    unsigned int num = onion_backup_nodes(m->onion_c, nodes, NUM_SAVED_PATH_NODES);
    int l = pack_nodes(data, NUM_SAVED_PATH_NODES * packed_node_size(net_family_tcp_ipv6), nodes, num);

const

toxcore/Messenger.c, line 3287 at r4 (raw file):

    if (length != 0) {
        int num = unpack_nodes(nodes, NUM_SAVED_PATH_NODES, nullptr, data, length, 0);

const

toxcore/Messenger.c, line 3300 at r4 (raw file):

static uint32_t end_size(const Messenger *m)
{
    (void) m;

Remove all these (I'm going to stop commenting on them now).

toxcore/state.c, line 73 at r4 (raw file):

}

uint8_t *state_write_cookie(uint8_t *data, uint32_t cookie, uint32_t length)

This function is used exactly once. Perhaps better inline it.

e0ff

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

toxcore/Messenger.c, line 2940 at r4 (raw file):

Previously, iphydf wrote…

memset on structures containing pointers does not necessarily yield a valid structure. The pointers inside may be messed up. You probably don't want a memset here at all, since you initialise all members, and you want to know about it if you don't.

Done.

toxcore/Messenger.c, line 2993 at r4 (raw file):

Previously, iphydf wrote…

No need for this. We don't warn on unused params.

Done.

toxcore/Messenger.c, line 2994 at r4 (raw file):

Previously, iphydf wrote…

Remove outer parentheses.

Done.

toxcore/Messenger.c, line 3003 at r4 (raw file):

Previously, iphydf wrote…

Replace (&data[sizeof(uint32_t)]) with data + sizeof(uint32_t). The former expresses "pointer to that one element" while the latter expresses "data at offset ...".

Done.

toxcore/Messenger.c, line 3007 at r4 (raw file):

Previously, iphydf wrote…

If you invert the condition, you can use early error return.

Done.

toxcore/Messenger.c, line 3167 at r4 (raw file):

Previously, iphydf wrote…

Remove superfluous parentheses.

Done.

toxcore/Messenger.c, line 3191 at r4 (raw file):

Previously, iphydf wrote…

Same here.

Done.

toxcore/Messenger.c, line 3201 at r4 (raw file):

Previously, iphydf wrote…

Remove void-cast.

Done.

toxcore/Messenger.c, line 3226 at r4 (raw file):

Previously, iphydf wrote…

Remove.

Done.

toxcore/Messenger.c, line 3260 at r4 (raw file):

Previously, iphydf wrote…

Remove.

Done.

toxcore/Messenger.c, line 3270 at r4 (raw file):

Previously, iphydf wrote…

const

Done.

toxcore/Messenger.c, line 3271 at r4 (raw file):

Previously, iphydf wrote…

const

Done.

toxcore/Messenger.c, line 3287 at r4 (raw file):

Previously, iphydf wrote…

const

Done.

toxcore/Messenger.c, line 3300 at r4 (raw file):

Previously, iphydf wrote…

Remove all these (I'm going to stop commenting on them now).

Done.

toxcore/state.c, line 73 at r4 (raw file):

Previously, iphydf wrote…

This function is used exactly once. Perhaps better inline it.

Done.

iphydf

Reviewed 5 of 6 files at r5.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf and @endoffile78)

toxcore/Messenger.c, line 2940 at r4 (raw file):

Previously, endoffile78 (Endoffile) wrote…

Done.

What is done?

toxcore/Messenger.c, line 3003 at r4 (raw file):

Previously, endoffile78 (Endoffile) wrote…

Done.

Ok. When I put a comment, I generally assume that you'll fix similar issues around the code. Please also change the one 2 lines above this one.

toxcore/Messenger.c, line 3191 at r4 (raw file):

Previously, endoffile78 (Endoffile) wrote…

Done.

It doesn't seem so.

toxcore/Messenger.c, line 48 at r5 (raw file):

// TODO(endoffile78): Remove these, currently they need to be up here so they can be called in new_messenger
// Decide whether to move new_messenger or the state plugins + what they need
static uint32_t nospam_keys_size(const Messenger *m);

You could also remove all of these and have a single m_register_default_plugins or something below all the plugin functions.

toxcore/Messenger.c, line 2164 at r5 (raw file):

    friendreq_kill(m->fr);

    if (m->options.state_plugins) {

No need for the if. free(0) is valid.

toxcore/Messenger.c, line 2930 at r5 (raw file):

                             m_state_save_cb save_callback)
{
    Messenger_State_Plugin *temp = (Messenger_State_Plugin *) realloc(m->options.state_plugins,

Remove space after cast.

toxcore/Messenger.c, line 3010 at r5 (raw file):

    load_secret_key(m->net_crypto, (&data[sizeof(uint32_t)]) + CRYPTO_PUBLIC_KEY_SIZE);

    if (public_key_cmp((data + sizeof(uint32_t)), nc_get_self_public_key(m->net_crypto)) != 0) {

Remove parentheses around data + sizeof(uint32_t).

toxcore/Messenger.c, line 3022 at r5 (raw file):

    assert(sizeof(get_nospam(m->fr)) == sizeof(uint32_t));
    data = state_write_section_header(data, MESSENGER_STATE_COOKIE_TYPE, len, MESSENGER_STATE_TYPE_NOSPAMKEYS);
    *(uint32_t *)data = get_nospam(m->fr);

Maybe while we're at it, we can make this unportable code portable.

toxcore/Messenger.c, line 3323 at r5 (raw file):

    for (uint8_t i = 0; i < m->options.state_plugins_length; ++i) {
        const Messenger_State_Plugin plugin = m->options.state_plugins[i];

Copying it is probably not necessary. A ptr-to-const would be fine here.

e0ff

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

toxcore/Messenger.c, line 48 at r5 (raw file):

Previously, iphydf wrote…

You could also remove all of these and have a single m_register_default_plugins or something below all the plugin functions.

Done.

toxcore/Messenger.c, line 2164 at r5 (raw file):

Previously, iphydf wrote…

No need for the if. free(0) is valid.

Done.

toxcore/Messenger.c, line 2930 at r5 (raw file):

Previously, iphydf wrote…

Remove space after cast.

Done.

toxcore/Messenger.c, line 3010 at r5 (raw file):

Previously, iphydf wrote…

Remove parentheses around data + sizeof(uint32_t).

Done.

toxcore/Messenger.c, line 3022 at r5 (raw file):

Previously, iphydf wrote…

Maybe while we're at it, we can make this unportable code portable.

How would I do that?

toxcore/Messenger.c, line 3323 at r5 (raw file):

Previously, iphydf wrote…

Copying it is probably not necessary. A ptr-to-const would be fine here.

Is that what you wanted?

codecov · 2018-08-25T18:20:08Z

Codecov Report

Merging #1046 into master will increase coverage by <.1%.
The diff coverage is 95.8%.

@@           Coverage Diff            @@
##           master   #1046     +/-   ##
========================================
+ Coverage    82.8%   82.8%   +<.1%     
========================================
  Files          82      82             
  Lines       14594   14666     +72     
========================================
+ Hits        12084   12158     +74     
+ Misses       2510    2508      -2

Impacted Files	Coverage Δ
toxcore/DHT.c	`76.6% <100%> (-0.2%)`	⬇️
toxcore/state.c	`64.7% <83.3%> (-6.8%)`	⬇️
toxcore/Messenger.c	`86.7% <96.1%> (+0.7%)`	⬆️
toxcore/LAN_discovery.c	`83.9% <0%> (-1.9%)`	⬇️
toxcore/onion_client.c	`95.5% <0%> (-0.7%)`	⬇️
toxcore/tox.c	`65.7% <0%> (-0.1%)`	⬇️
toxav/toxav.c	`69.1% <0%> (+0.9%)`	⬆️
toxav/msi.c	`65.6% <0%> (+1.9%)`	⬆️

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 1cd1836...08cc418. Read the comment docs.

iphydf · 2018-08-25T22:53:52Z

Remove "WIP" from the title if this is ready for review.

iphydf

Reviewed 1 of 6 files at r5, 1 of 2 files at r6.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)

toxcore/Messenger.c, line 3022 at r5 (raw file):

Previously, endoffile78 wrote…

How would I do that?

Use lendian_from_host or something. Right now this code makes saves on big endian systems behave differently on little endian systems.

toxcore/Messenger.c, line 3323 at r5 (raw file):

Previously, endoffile78 wrote…

Did I do what you are talking about?

Yes, that's correct, but you can keep the const at the beginning (also keep the second const at the end).

e0ff

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

toxcore/Messenger.c, line 3022 at r5 (raw file):

Previously, iphydf wrote…

Use lendian_from_host or something. Right now this code makes saves on big endian systems behave differently on little endian systems.

Done.

toxcore/Messenger.c, line 3323 at r5 (raw file):

Previously, iphydf wrote…

Yes, that's correct, but you can keep the const at the beginning (also keep the second const at the end).

Done.

iphydf

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

toxcore/Messenger.c, line 2963 at r7 (raw file):

    }

    set_nospam(m->fr, *(const uint32_t *)data);

Also fix this one.

iphydf

Reviewed 1 of 2 files at r7.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @endoffile78)

e0ff

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

toxcore/Messenger.c, line 2963 at r7 (raw file):

Previously, iphydf wrote…

Also fix this one.

Done.

iphydf

Reviewed 1 of 1 files at r8.
Reviewable status: 1 change requests, 0 of 1 approvals obtained

iphydf · 2018-08-26T10:06:48Z

Let's hold off on this one until @ingvar1995 made a PR for NGC.

iphydf · 2018-09-04T23:46:57Z

Ingvar is not making a PR anytime soon, so let's go ahead with this.

This is for modules like groups to hook into to have their own state management in the `tox_savedata` format.

iphydf

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

iphydf · 2018-09-08T07:31:23Z

@sudden6 please still take a look when you have the chance.

iphydf

Reviewable status: complete! 1 of 1 approvals obtained

e0ff force-pushed the saving branch from 4567999 to 82a3c80 Compare August 4, 2018 05:56

e0ff changed the title ~~WIP: Finisht the messenger state plugin system~~ WIP: Finish the messenger state plugin system Aug 4, 2018

iphydf requested changes Aug 4, 2018

View reviewed changes

iphydf added this to the v0.2.x milestone Aug 4, 2018

e0ff commented Aug 4, 2018

View reviewed changes

e0ff force-pushed the saving branch 2 times, most recently from 6279821 to e37a603 Compare August 4, 2018 18:53

e0ff mentioned this pull request Aug 4, 2018

Add save file generator, compatibility test, and generate a savefile #1051

Merged

e0ff force-pushed the saving branch from 490fc23 to 0c88cd6 Compare August 11, 2018 18:06

iphydf requested changes Aug 12, 2018

View reviewed changes

e0ff force-pushed the saving branch from 0c88cd6 to 41dabc9 Compare August 13, 2018 05:59

e0ff commented Aug 13, 2018

View reviewed changes

e0ff force-pushed the saving branch 5 times, most recently from 8ad1316 to f97ed73 Compare August 19, 2018 20:21

iphydf requested changes Aug 19, 2018

View reviewed changes

e0ff commented Aug 25, 2018

View reviewed changes

e0ff force-pushed the saving branch from 7de3052 to 412ab82 Compare August 25, 2018 18:16

e0ff changed the title ~~WIP: Finish the messenger state plugin system~~ Finish the messenger state plugin system Aug 25, 2018

iphydf requested changes Aug 25, 2018

View reviewed changes

e0ff commented Aug 25, 2018

View reviewed changes

e0ff force-pushed the saving branch from 2d2e868 to 93729c8 Compare August 25, 2018 23:24

iphydf requested changes Aug 25, 2018

View reviewed changes

iphydf reviewed Aug 25, 2018

View reviewed changes

e0ff force-pushed the saving branch from 32ad14b to 0b662fd Compare August 26, 2018 00:03

e0ff commented Aug 26, 2018

View reviewed changes

iphydf approved these changes Aug 26, 2018

View reviewed changes

iphydf changed the title ~~Finish the messenger state plugin system~~ WIP: Finish the messenger state plugin system Aug 26, 2018

e0ff force-pushed the saving branch 2 times, most recently from 2c9ace7 to 804881b Compare September 1, 2018 16:44

iphydf changed the title ~~WIP: Finish the messenger state plugin system~~ Finish the messenger state plugin system Sep 4, 2018

iphydf assigned sudden6 Sep 4, 2018

e0ff force-pushed the saving branch from 804881b to c055134 Compare September 6, 2018 03:53

Add messenger state plugin system.

08cc418

This is for modules like groups to hook into to have their own state management in the `tox_savedata` format.

e0ff force-pushed the saving branch from c055134 to 08cc418 Compare September 7, 2018 17:22

iphydf approved these changes Sep 7, 2018

View reviewed changes

iphydf unassigned sudden6 Sep 8, 2018

iphydf approved these changes Sep 8, 2018

View reviewed changes

iphydf self-assigned this Sep 8, 2018

iphydf merged commit 08cc418 into TokTok:master Sep 8, 2018

robinlinden modified the milestones: v0.2.x, v0.2.8 Oct 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish the messenger state plugin system #1046

Finish the messenger state plugin system #1046

e0ff commented Aug 3, 2018 •

edited

Loading

iphydf Aug 4, 2018

iphydf Aug 4, 2018

e0ff left a comment

iphydf left a comment

e0ff left a comment

iphydf left a comment

e0ff left a comment •

edited

Loading

codecov bot commented Aug 25, 2018 •

edited

Loading

iphydf commented Aug 25, 2018

iphydf left a comment

e0ff left a comment

iphydf left a comment

iphydf left a comment

e0ff left a comment

iphydf left a comment

iphydf commented Aug 26, 2018

iphydf commented Sep 4, 2018

iphydf left a comment

iphydf commented Sep 8, 2018

iphydf left a comment

Finish the messenger state plugin system #1046

Finish the messenger state plugin system #1046

Conversation

e0ff commented Aug 3, 2018 • edited Loading

iphydf Aug 4, 2018

Choose a reason for hiding this comment

iphydf Aug 4, 2018

Choose a reason for hiding this comment

e0ff left a comment

Choose a reason for hiding this comment

iphydf left a comment

Choose a reason for hiding this comment

e0ff left a comment

Choose a reason for hiding this comment

iphydf left a comment

Choose a reason for hiding this comment

e0ff left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 25, 2018 • edited Loading

Codecov Report

iphydf commented Aug 25, 2018

iphydf left a comment

Choose a reason for hiding this comment

e0ff left a comment

Choose a reason for hiding this comment

iphydf left a comment

Choose a reason for hiding this comment

iphydf left a comment

Choose a reason for hiding this comment

e0ff left a comment

Choose a reason for hiding this comment

iphydf left a comment

Choose a reason for hiding this comment

iphydf commented Aug 26, 2018

iphydf commented Sep 4, 2018

iphydf left a comment

Choose a reason for hiding this comment

iphydf commented Sep 8, 2018

iphydf left a comment

Choose a reason for hiding this comment

e0ff commented Aug 3, 2018 •

edited

Loading

e0ff left a comment •

edited

Loading

codecov bot commented Aug 25, 2018 •

edited

Loading