Improve network error reporting on Windows

[nurupo]

Windows doesn't report network errors though errno, it has its own facilities.

Not sure if the new and kill are the correct words here, toxcore seems to use them for alloc and free, so I'm just following the pattern.

This should fix the Unexpected error reading from socket: 0, No error error on Windows, which someone on IRC supposedly got 60000 times in 10 seconds. The issue was in

int fail_or_len = recvfrom(sock, (char *) data, MAX_UDP_PACKET_SIZE, 0, (struct sockaddr *)&addr, &addrlen);
if (fail_or_len < 0 && errno != EWOULDBLOCK) {
    LOGGER_ERROR(log, "Unexpected error reading from socket: %u, %s", errno, strerror(errno));
}

Because Windows doesn't use errno for error reporting, Windows was reporting EWOULDBLOCK through WSAGetLastError() instead of errno, resulting in the logger being called when it shouldn't had been.

---

This change is 

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[nurupo]

Don't merge it yet, can't get logs out of Travis, it seems to be having some issue

There was an error while trying to fetch the log.

[nurupo]

Removed the commit that enabled all Travis-CI jobs. Here is the the build.

Alright, now we get actual error codes in the logs rather than 0 Sucess, which might help us debug the failures. For example, onion_test has Failed to bind socket: 10022, (null) IP: [] port_from: 34567 port_to: 34667, 10022 means "invalid argument" or "socket in invalid state".

The reported Unexpected error reading from socket: 0, Success errors have disappeared, so that's fixed. Getting 60000 of those errors in 10 seconds sounds CPU intense, we might want include this fix into 0.2.2. Not sure which log level is turned on by default though, it might be that the person set a specific log level to get those and it's not a default?

Btw, you might notice that the logger on Travis CI Windows builds doesn't print text explanations of errors, it prints code and (null), as in Failed to activate local multicast membership. (10045, (null)). That's because most (all?) of Windows Sockets error codes are missing the corresponding text explanations in Wine, even in the most recent development version of Wine, so the net_new_strerror() returns NULL, which logger prints as (null). It should print the actual error messages on a real Windows system, since its dlls have texts for all error codes -- it's just Wine missing those.

[nurupo]

Feel free to merge if it passes the review, I think I'm done with the PR.

[robinlinden]

Reviewed 6 of 6 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

toxcore/network.h, line 407 at r2 (raw file):

int bind_to_port(Socket sock, int family, uint16_t port);

/* Get networking error code.

Get the last networking error code?

---

toxcore/network.h, line 409 at r2 (raw file):

/* Get networking error code.
 *
 * Similar to Unix's errno, but cross-patform, as not all platforms use errno

"cross-platform"

---

toxcore/network.h, line 413 at r2 (raw file):

 *
 * Note that different platforms may return different codes for the same error,
 * so you are likely shouldn't be checking the value returned by this function

"you likely shouldn't"

---

toxcore/network.h, line 414 at r2 (raw file):

 * Note that different platforms may return different codes for the same error,
 * so you are likely shouldn't be checking the value returned by this function
 * unless you know what you are doing, you are likely just want to use it in

"you likely just"

---

toxcore/network.h, line 415 at r2 (raw file):

 * so you are likely shouldn't be checking the value returned by this function
 * unless you know what you are doing, you are likely just want to use it in
 * the combination with net_new_strerror() to print the error.

Remove leading "the"

---

toxcore/network.h, line 425 at r2 (raw file):

 * return NULL on failure.
 * return pointer to a NULL-terminated string describing the error code on
 * success. The returned string must be freed using net_new_strerror()

"must be freed using net_kill_strerror." (Remove "function" and fix function name.)

---

toxcore/network.h, line 430 at r2 (raw file):

char *net_new_strerror(int error);

/* Frees the string returned by the net_new_strerror().

"returned by net_new_strerror()"

---

Comments from Reviewable

[nurupo]

Should be fixed now.

[nurupo]

So, how does the PR look, @robinlinden?

[robinlinden]

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

---

Comments from Reviewable

[robinlinden]

---

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

---

Comments from Reviewable

[nurupo]

Btw, I did

int error = net_error();

in one cases but

int neterror = net_error();

in other because there already was error variable defined, so I simply couldn't use that name there.

[iphydf]

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

---

toxcore/network.h, line 421 at r3 (raw file):

int net_error(void);

/* Get a text explanation for the error code from net_error().

Add a note that the string returned should not be mutated, despite the type saying it's allowed. Alternatively, strdup+free.

---

Comments from Reviewable

[nurupo]

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

---

toxcore/network.h, line 421 at r3 (raw file):

Previously, iphydf wrote…

Add a note that the string returned should not be mutated, despite the type saying it's allowed. Alternatively, strdup+free.

Nice catch. You are right, while on Windows a new error string would be allocated for every call, so it can be modified, on Unix-like it would return a pointer to a static string, which shouldn't be modified.

Your change suggestions are kind of weird though. Is there something wrong with changing the return type to const char *?

---

Comments from Reviewable

[iphydf]

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

---

toxcore/network.h, line 421 at r3 (raw file):

Previously, nurupo wrote…

Nice catch. You are right, while on Windows a new error string would be allocated for every call, so it can be modified, on Unix-like it would return a pointer to a static string, which shouldn't be modified.

Your change suggestions are kind of weird though. Is there something wrong with changing the return type to const char *?

The problem with making it const char * is that it requires a cast-to-non-const in the free function. To avoid that, you would wrap the result in a struct, return an allocated pointer to that, and make an accessor to get the actual value. I.e.:

// network.h
typedef struct Net_Error Net_Error;

Net_Error *net_error_new(void);
void net_error_kill(Net_Error *err);

int net_error_code(const Net_Error *err);
const char *net_error_message(const Net_Error *err);

// network.c
struct Net_Error {
  const int code;
  char *const message;
};

// net_error_new allocates Net_Error and calls strerror or the windows function to populate message
// net_error_message returns the message (no allocation or other calls here)

---

Comments from Reviewable

[nurupo]

toxcore/network.h, line 421 at r3 (raw file):

Previously, iphydf wrote…

The problem with making it const char * is that it requires a cast-to-non-const in the free function. To avoid that, you would wrap the result in a struct, return an allocated pointer to that, and make an accessor to get the actual value. I.e.:

// network.h
typedef struct Net_Error Net_Error;

Net_Error *net_error_new(void);
void net_error_kill(Net_Error *err);

int net_error_code(const Net_Error *err);
const char *net_error_message(const Net_Error *err);

// network.c
struct Net_Error {
  const int code;
  char *const message;
};

// net_error_new allocates Net_Error and calls strerror or the windows function to populate message
// net_error_message returns the message (no allocation or other calls here)

What about now?

---

Comments from Reviewable

[iphydf]

---

Review status: 1 of 7 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[robinlinden]

Reviewed 5 of 6 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable