Remove some static variables

[zugz]

This partially implements #405, reworking ip_ntoa(), ID2String() and cmp_dht_entry() to avoid the use of the global static variables 'addresstext', 'IDString', 'cmp_public_key', and moving 'lastdump' into the Messenger struct.

---

This change is 

[iphydf]

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

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

#define IDSTRING_LEN (CRYPTO_PUBLIC_KEY_SIZE * 2 + 1)
/* buf should be of length at least IDSTRING_LEN */
static char *ID2String(const uint8_t *pk, char *buf, int length)

Use size_t for lengths.

---

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

#define IDSTRING_LEN (CRYPTO_PUBLIC_KEY_SIZE * 2 + 1)
/* buf should be of length at least IDSTRING_LEN */
static char *ID2String(const uint8_t *pk, char *buf, int length)

Perhaps rename this function. All other functions are snake-case. How about id_to_string?

---

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

{
    if (length < IDSTRING_LEN) {
        snprintf(buf, length, "Bad buf length");

Perhaps you can make it print up to the provided length and use "..." at the end if it doesn't fit.

---

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

    uint32_t i;

    for (i = 0; i < CRYPTO_PUBLIC_KEY_SIZE; i++) {

We're allowed to use C99 for loops now. You can move the declaration together with the initialisation in here.

---

toxcore/network.c, line 881 at r1 (raw file):

 *   returns buf
 */
const char *ip_ntoa(const IP *ip, char *buf, size_t length)

You could have left it at "addresstext". buf is so generic.. do you prefer buf?

---

Comments from Reviewable

[iphydf]

Reviewed 5 of 6 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

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

    uint32_t i;
    Cmp_data cmp_list[length];
    for (i = 0; i < length; i++) {

Use for-init-decl.

---

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

    for (i = 0; i < length; i++) {
        cmp_list[i].entry = list[i];
        cmp_list[i].base_public_key = comp_public_key;

Nit: reverse the initialisations, put them in order of declaration.

---

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

    }

    qsort(cmp_list, length, sizeof(Cmp_data), cmp_dht_entry);

After this, the cmp_list is sorted, but nothing happened to the original list.

---

Comments from Reviewable

[zugz]

Review status: 4 of 7 files reviewed at latest revision, 8 unresolved discussions.

---

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

Previously, iphydf wrote…

Use for-init-decl.

Done.

---

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

Previously, iphydf wrote…

Nit: reverse the initialisations, put them in order of declaration.

Done.

---

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

Previously, iphydf wrote…

After this, the cmp_list is sorted, but nothing happened to the original list.

Oops... done.

---

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

Previously, iphydf wrote…

Use size_t for lengths.

Done.

---

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

Previously, iphydf wrote…

Perhaps rename this function. All other functions are snake-case. How about id_to_string?

Done.

---

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

Previously, iphydf wrote…

Perhaps you can make it print up to the provided length and use "..." at the end if it doesn't fit.

We could, but is it worth complicating the code for? Seems neater to just consider a too short buffer as an error, here and in ip_ntoa.

---

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

Previously, iphydf wrote…

We're allowed to use C99 for loops now. You can move the declaration together with the initialisation in here.

Done.

---

toxcore/network.c, line 881 at r1 (raw file):

Previously, iphydf wrote…

You could have left it at "addresstext". buf is so generic.. do you prefer buf?

made it 'ip_str'

---

Comments from Reviewable

[iphydf]

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

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

Previously, zugz (zugz) wrote…

We could, but is it worth complicating the code for? Seems neater to just consider a too short buffer as an error, here and in ip_ntoa.

Ok, no need.

---

Comments from Reviewable

[zugz]

For the remaining statics listed in #405, with the exception of at_startup_ran, I've made sure they're used in a thread-safe way rather than getting rid of them. I'm not sure that this is the best approach, particularly in the case of broadcast_* - please review.

[iphydf]

Reviewed 4 of 5 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

toxcore/LAN_discovery.c, line 137 at r5 (raw file):

    }

    /* for thread-safety, we copy these to the static variables broadcast_* only at the end */

I wonder: what effect on thread-safety does this have?

---

toxcore/network.c, line 250 at r5 (raw file):

     /* Check if time has decreased because of 32 bit wrap from GetTickCount(), while avoiding false positives from race
      * conditions when multiple threads call this function at once */
    if (time + 0x10000 < last_monotime) {

Why 0x10000? "64K ought to be enough for anyone"?

Monotonic time stuff will also move to libev at some point, so perhaps we don't need to worry about this now.

---

toxcore/util.c, line 43 at r5 (raw file):

static uint64_t unix_base_time_value;

/* note that this is thread-safe despite the use of static variables */

I'm not sure what "thread-safe" means here. You can have a data race here, and you have code from a different Tox instance altering state used by another Tox instance. That could be surprising. No need to change this now, though, because we'll hopefully move away from our homebrew event loop and use a proper one (like libev).

---

Comments from Reviewable

[zugz]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

toxcore/LAN_discovery.c, line 137 at r5 (raw file):

Previously, iphydf wrote…

I wonder: what effect on thread-safety does this have?

Previously, the increments to broadcast_count meant that if multiple threads entered this function simultaneously, there could be problems (broadcast_count could be incremented beyond the number of interfaces). Now, the statics are set only once the determination of available interfaces has been performed; even if for some strange reason different threads got different results, and even if the setting of the variables got interleaved, only valid interfaces will be set to be broadcast to. It does require thinking through to verify that this is the case, however, so a solution which got rid of these statics would be preferable. Putting them in DHT would work, but seems like a pollution of DHT.

---

toxcore/network.c, line 250 at r5 (raw file):

Previously, iphydf wrote…

Why 0x10000? "64K ought to be enough for anyone"?

Monotonic time stuff will also move to libev at some point, so perhaps we don't need to worry about this now.

Yes, anything sufficiently far from 0 and 0xffffffff would do. Moving to libev sounds good, but perhaps it's worth keeping this change in for now anyway, as false positives are quite possible with the previous approach, which would lead to oddness (namely, the time suddenly increasing by a couple of months).

---

toxcore/util.c, line 43 at r5 (raw file):

Previously, iphydf wrote…

I'm not sure what "thread-safe" means here. You can have a data race here, and you have code from a different Tox instance altering state used by another Tox instance. That could be surprising. No need to change this now, though, because we'll hopefully move away from our homebrew event loop and use a proper one (like libev).

Right, perhaps "thread-safe" is overstating it. Semantically thread-safe, in that such races don't actually matter - it's just a matter of which thread gets to set the time, which doesn't affect what the time is set to.

---

Comments from Reviewable

[iphydf]

---

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

toxcore/LAN_discovery.c, line 137 at r5 (raw file):

Previously, zugz (zugz) wrote…

Previously, the increments to broadcast_count meant that if multiple threads entered this function simultaneously, there could be problems (broadcast_count could be incremented beyond the number of interfaces). Now, the statics are set only once the determination of available interfaces has been performed; even if for some strange reason different threads got different results, and even if the setting of the variables got interleaved, only valid interfaces will be set to be broadcast to. It does require thinking through to verify that this is the case, however, so a solution which got rid of these statics would be preferable. Putting them in DHT would work, but seems like a pollution of DHT.

Ok, sounds good. Perhaps you can add this explanation to the comment. We'll get rid of the global at some other time.

---

toxcore/network.c, line 250 at r5 (raw file):

Previously, zugz (zugz) wrote…

Yes, anything sufficiently far from 0 and 0xffffffff would do. Moving to libev sounds good, but perhaps it's worth keeping this change in for now anyway, as false positives are quite possible with the previous approach, which would lead to oddness (namely, the time suddenly increasing by a couple of months).

Sounds good.

---

Comments from Reviewable

[robinlinden]

---

Reviewed 3 of 5 files at r4, 1 of 3 files at r5, 1 of 1 files at r6, 8 of 8 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[JFreegman]

For future reference, this pull request should have been split up into a few smaller requests. There's too much going on here, making it needlessly difficult to review.

---

Reviewed 1 of 5 files at r4, 8 of 8 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable