Skip to content

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

Merged
iphydf merged 1 commit into
TokTok:masterfrom
e0ff:save-generator
Aug 19, 2018
Merged

Add save file generator, compatibility test, and generate a savefile#1051
iphydf merged 1 commit into
TokTok:masterfrom
e0ff:save-generator

Conversation

@e0ff
Copy link
Copy Markdown
Member

@e0ff e0ff commented Aug 4, 2018

The save file will be used to ensure that #1046 is compatible with older save files. The output of the save generator was:

Connected to the tox network.
Wrote tox save to save.tox
INFORMATION
----------------------------------
Tox ID: B70E97D41F69B7F4C42A5BC7BD7A76B95B8030BE1B7C0E9E6FC19FC4ABEB195B4C762C7D800B.
Nospam: 4C762C7D.
Name: name.
Status message: Hello World.
Number of friends: 1.
----------------------------------

The added friend was toxirc: A922A51E1C91205B9F7992E2273107D47C72E8AE909C61C28A77A4A2A115431B14592AB38A3B.


This change is Reviewable

@e0ff e0ff force-pushed the save-generator branch 3 times, most recently from dc14553 to fcec1b3 Compare August 4, 2018 21:32
@iphydf iphydf added this to the v0.2.x milestone Aug 8, 2018
Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Please don't submit the savefile in this PR. It's not used, so it serves no purpose. Once you add a test for loading it, you can submit the data for it as well.

Comment thread other/save-generator.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if we could make this tool a little more versatile, maybe have some way of generating a savefile with multiple friends, groups, etc. I'm happy to have this version submitted as an initial version, but I think the interesting bits of save parsing happen when you have more than 1 of a thing.

Comment thread other/save-generator.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

const char *const

Comment thread other/save-generator.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are these macros and the constants below are variables?

Comment thread other/save-generator.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Prefer callbacks. This main function is quite long. Given that we'll want to extend it in the future, it's probably good to split it into a few functions.

Comment thread other/save-generator.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Prefer error pointers.

Comment thread other/save-generator.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No error checking done.

Comment thread other/save-generator.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Friend number in case of failure is unspecified. This is relying on knowledge of internals. Prefer error enums.

Comment thread other/save-generator.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not enough space for the null terminator.

Copy link
Copy Markdown
Member Author

@e0ff e0ff left a comment

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 LGTMs obtained


other/save-generator.c, line 10 at r1 (raw file):

Previously, iphydf wrote…

Why are these macros and the constants below are variables?

Done.


other/save-generator.c, line 13 at r1 (raw file):

Previously, iphydf wrote…

const char *const

Done.


other/save-generator.c, line 37 at r1 (raw file):

Previously, iphydf wrote…

I wonder if we could make this tool a little more versatile, maybe have some way of generating a savefile with multiple friends, groups, etc. I'm happy to have this version submitted as an initial version, but I think the interesting bits of save parsing happen when you have more than 1 of a thing.

I added in adding multiple friends. I'll add in groupchats when persistent groupchats is merged.


other/save-generator.c, line 55 at r1 (raw file):

Previously, iphydf wrote…

No error checking done.

Done.


other/save-generator.c, line 61 at r1 (raw file):

Previously, iphydf wrote…

Prefer callbacks. This main function is quite long. Given that we'll want to extend it in the future, it's probably good to split it into a few functions.

Done.


other/save-generator.c, line 70 at r1 (raw file):

Previously, iphydf wrote…

Prefer error pointers.

Done.


other/save-generator.c, line 87 at r1 (raw file):

Previously, iphydf wrote…

Friend number in case of failure is unspecified. This is relying on knowledge of internals. Prefer error enums.

Done.


other/save-generator.c, line 117 at r1 (raw file):

Previously, iphydf wrote…

Not enough space for the null terminator.

I just use the length to write the nospam out same with the tox id.

@nurupo
Copy link
Copy Markdown
Member

nurupo commented Aug 11, 2018

I think such things should go into other/fun directory, not just other.

@iphydf Btw, I have written a bit different save file generator several weeks ago, do you think it makes sense to add it to the repo too? It doesn't use toxcore at all though, it crafts the save file using the current save file binary format, much like other/fun/make-funny-savefile.py does.

Copy link
Copy Markdown
Member Author

@e0ff e0ff left a comment

Choose a reason for hiding this comment

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

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


other/save-generator.c, line 117 at r1 (raw file):

Previously, endoffile78 (Endoffile) wrote…

I just use the length to write the nospam out same with the tox id.

Done.

@iphydf
Copy link
Copy Markdown
Member

iphydf commented Aug 12, 2018

@nurupo yes, and it seems like a good idea to put these things into other/fun.

Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

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


other/save-generator.c, line 50 at r2 (raw file):

    if (err != TOX_ERR_BOOTSTRAP_OK) {
        printf("Failed to bootstrap. Error number: %u", err);

Error numbers are signed ints (enum values), so %d.


other/save-generator.c, line 89 at r2 (raw file):

    printf("Name: %s.\n", name);
    printf("Status message: %s.\n", STATUS_MESSAGE);
    printf("Number of friends: %lu.\n", tox_self_get_friend_list_size(tox));

%zu


other/save-generator.c, line 133 at r2 (raw file):

    }

    int num_friends = atoi(argv[2]);

This isn't used.


other/save-generator.c, line 138 at r2 (raw file):

        uint8_t *address = hex_string_to_bin(argv[i]);
        Tox_Err_Friend_Add friend_err;
        uint32_t friend_num = tox_friend_add(tox, address, (const uint8_t *)REQUEST_MESSAGE, strlen(REQUEST_MESSAGE),

Remove dead assignment.

Copy link
Copy Markdown
Member Author

@e0ff e0ff left a comment

Choose a reason for hiding this comment

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

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


other/save-generator.c, line 50 at r2 (raw file):

Previously, iphydf wrote…

Error numbers are signed ints (enum values), so %d.

Done.


other/save-generator.c, line 89 at r2 (raw file):

Previously, iphydf wrote…

%zu

Done.


other/save-generator.c, line 133 at r2 (raw file):

Previously, iphydf wrote…

This isn't used.

Done.


other/save-generator.c, line 138 at r2 (raw file):

Previously, iphydf wrote…

Remove dead assignment.

Done.

Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r1, 1 of 4 files at r3.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @endoffile78)


auto_tests/data/save.tox, line 0 at r3 (raw file):
This file should be moved to a different PR where we actually use it.


other/save-generator.c, line 50 at r2 (raw file):

Previously, endoffile78 (Endoffile) wrote…

Done.

Not really.

Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

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


other/fun/save-generator.c, line 137 at r3 (raw file):

        if (friend_err != TOX_ERR_FRIEND_ADD_OK) {
            printf("Failed to add friend number %u. Error number: %d\n", i - 1, friend_err);

i is int, so %d, or make i unsigned and keep %u.

Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

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


other/fun/save-generator.c, line 3 at r3 (raw file):

#include <stdio.h>
#include <string.h>
#include <stdint.h>

Also sort these includes.


other/fun/save-generator.c, line 7 at r3 (raw file):

#include "../../toxcore/tox.h"
#include "../../testing/misc_tools.h"

Sort includes.

Copy link
Copy Markdown
Member Author

@e0ff e0ff left a comment

Choose a reason for hiding this comment

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

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


auto_tests/data/save.tox, line at r3 (raw file):

Previously, iphydf wrote…

This file should be moved to a different PR where we actually use it.

Added a test that uses it


other/fun/save-generator.c, line 3 at r3 (raw file):

Previously, iphydf wrote…

Also sort these includes.

Done.


other/fun/save-generator.c, line 7 at r3 (raw file):

Previously, iphydf wrote…

Sort includes.

Done.


other/fun/save-generator.c, line 137 at r3 (raw file):

Previously, iphydf wrote…

i is int, so %d, or make i unsigned and keep %u.

Done.

@e0ff e0ff changed the title Add save file generator and generate a save file Add save file generator, compability test, and generate a savefile Aug 15, 2018
Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

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


auto_tests/save_compatibility_test.c, line 74 at r4 (raw file):

    struct Tox_Options options;

    memset(&options, 0, sizeof(struct Tox_Options));

Use regular struct zero-initialisation instead of memset.


auto_tests/save_compatibility_test.c, line 94 at r4 (raw file):

    uint8_t name[name_size];
    tox_self_get_name(tox, name);
    ck_assert_msg(strncmp((char *)name, NAME, name_size) == 0, "Names do not match, expected %s got %s.", NAME, name);

const char *


auto_tests/save_compatibility_test.c, line 103 at r4 (raw file):

    tox_self_get_status_message(tox, status_message);
    ck_assert_msg(strncmp((char *)status_message, STATUS_MESSAGE, status_message_size) == 0,
                  "Status messages do not match, expeceted %s got %s.",

expected


auto_tests/save_compatibility_test.c, line 114 at r4 (raw file):

    size_t length = snprintf(nospam_str, sizeof(nospam_str), "%08X", nospam);
    nospam_str[length] = '\0';
    ck_assert_msg(strcmp(nospam_str, NOSPAM) == 0, "Nospam does not match, expected %s got %s.", NOSPAM, nospam_str);

Error messages here should start with lowercase and not have a "." at the end.


other/fun/save-generator.c, line 3 at r3 (raw file):

Previously, endoffile78 (Endoffile) wrote…

Done.

Not done.


other/fun/save-generator.c, line 112 at r4 (raw file):

    while (!connected) {
        tox_iterate(tox, nullptr);

Pass &connected to tox_iterate, so it ends up in userdata for the connection callback.

Copy link
Copy Markdown
Member Author

@e0ff e0ff left a comment

Choose a reason for hiding this comment

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

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


auto_tests/save_compatibility_test.c, line 74 at r4 (raw file):

Previously, iphydf wrote…

Use regular struct zero-initialisation instead of memset.

Done.


auto_tests/save_compatibility_test.c, line 94 at r4 (raw file):

Previously, iphydf wrote…

const char *

Done.


auto_tests/save_compatibility_test.c, line 103 at r4 (raw file):

Previously, iphydf wrote…

expected

Done.


auto_tests/save_compatibility_test.c, line 114 at r4 (raw file):

Previously, iphydf wrote…

Error messages here should start with lowercase and not have a "." at the end.

Done.


other/fun/save-generator.c, line 112 at r4 (raw file):

Previously, iphydf wrote…

Pass &connected to tox_iterate, so it ends up in userdata for the connection callback.

Done.

Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed 2 of 4 files at r3, 1 of 3 files at r4, 2 of 2 files at r5.
Reviewable status: 1 change requests, 0 of 1 approvals obtained

Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

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


auto_tests/save_compatibility_test.c, line 67 at r5 (raw file):

    *length = size;

    return data;

Add fclose here.

Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Can you add the generator tool to other/fun/BUILD.bazel?

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


auto_tests/save_compatibility_test.c, line 98 at r5 (raw file):

                  STATUS_MESSAGE_SIZE, status_message_size);

    uint8_t status_message[status_message_size];

You'll need to use the VLA macro here, or preferably avoid VLAs altogether. Rather malloc and free, or allocate the maximum size (from tox.h).

Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

:lgtm_cancel: still more things to do.

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

Copy link
Copy Markdown
Member Author

@e0ff e0ff left a comment

Choose a reason for hiding this comment

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

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


auto_tests/save_compatibility_test.c, line 67 at r5 (raw file):

Previously, iphydf wrote…

Add fclose here.

Done.


auto_tests/save_compatibility_test.c, line 98 at r5 (raw file):

Previously, iphydf wrote…

You'll need to use the VLA macro here, or preferably avoid VLAs altogether. Rather malloc and free, or allocate the maximum size (from tox.h).

Done.

@e0ff e0ff force-pushed the save-generator branch 2 times, most recently from 1c64bbe to 2b17ed4 Compare August 16, 2018 21:59
Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

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


CMakeLists.txt, line 442 at r6 (raw file):

auto_test(typing)
auto_test(version)
auto_test(save_compatibility)

Sort these.

@iphydf iphydf changed the title Add save file generator, compability test, and generate a savefile Add save file generator, compatibility test, and generate a savefile Aug 18, 2018
Copy link
Copy Markdown
Member Author

@e0ff e0ff left a comment

Choose a reason for hiding this comment

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

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


CMakeLists.txt, line 442 at r6 (raw file):

Previously, iphydf wrote…

Sort these.

Done.

Copy link
Copy Markdown
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @iphydf and @endoffile78)


other/fun/BUILD.bazel, line 34 at r7 (raw file):

    deps = [
        "//c-toxcore/testing:misc_tools",
        "//c-toxcore/toxcore"

Add "," at end of line.


other/fun/BUILD.bazel, line 35 at r7 (raw file):

        "//c-toxcore/testing:misc_tools",
        "//c-toxcore/toxcore"
    ]

Here too.

Try running buildifier on this file, it'll fix these kinds of style issues.

@iphydf
Copy link
Copy Markdown
Member

iphydf commented Aug 18, 2018

After fixing the last style issues, rebase on master and I'll merge it.

@iphydf iphydf merged commit 30960dc into TokTok:master Aug 19, 2018
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.7 Aug 30, 2018
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.

4 participants