Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: [Network] Error handling on Windows broken #9116

Merged
merged 6 commits into from Apr 27, 2021

Conversation

@rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Apr 27, 2021

Motivation / Problem

Testing #9112 on Windows I found out it did not work. Looking at the specs I found out that errno is not useful for errors from Winsock; that's where GET_LAST_ERROR() was supposed to be used. However, strerror is not useful for errors from Winsock either and that has no abstraction, so most error messages on Windows from the network would be useless.

Description

Use GET_LAST_ERROR(), although renamed to NetworkGetLastError() to make it clearer it is only to be used for network errors, where errno was used in the network code.
Introduce NetworkGetLastErrorString which is strerror(NetworkGetLastError()) for everything but Windows; note that errno on OS/2 is also not used for socket errors, so also for OS/2 GET_LAST_ERROR() should have been used.
For Windows introduce NetworkGetLastErrorString that returns an error string using Windows' FormatMessage.

To prevent issues in the future, safeguards were added to not use strerror or errno in the networking code. "In the networking code" is everything that includes network/core/os_abstraction.h. That should generally be safe as, in theory, nothing outside the network could should touch that (with proper encapsulation ofcourse). However, it was leaking out through some header files, e.g. fios.h so some small changes were made to prevent leaking os_abstraction.h.

Limitations

Cannot use strerror/errno anymore when including os_abstraction.h (and safeguards.h), though if you need to there is probably some issue with encapsulation and the place that (indirectly) includes that knows way too much of the internal state of the network code.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
src/network/core/tcp_http.cpp Outdated Show resolved Hide resolved
src/network/core/os_abstraction.h Outdated Show resolved Hide resolved
@rubidium42 rubidium42 force-pushed the winsock-uses-no-errno branch 2 times, most recently from 5e4f1c1 to 51ba093 Apr 27, 2021
src/network/core/udp.cpp Outdated Show resolved Hide resolved
src/network/core/tcp_listen.h Outdated Show resolved Hide resolved
src/network/core/os_abstraction.h Outdated Show resolved Hide resolved
@rubidium42 rubidium42 force-pushed the winsock-uses-no-errno branch from 51ba093 to 52a6fb9 Apr 27, 2021
@LordAro LordAro merged commit 4880ec2 into OpenTTD:master Apr 27, 2021
12 checks passed
@rubidium42 rubidium42 deleted the winsock-uses-no-errno branch Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants