Networking bug fixes and cleanup #1462
Conversation
|
I will merge this in 3 days if there are no objections. |
| @@ -1020,7 +1022,13 @@ SOCKET NET_IP6Socket( const char *net_interface, int port, struct sockaddr_in6 * | |||
| // Print the name in brackets if there is a colon: | |||
| Log::Notice( "Opening IP6 socket: %s%s%s:%s", brackets ? "[" : "", net_interface ? net_interface : "[::]", brackets ? "]" : "", port ? va( "%i", port ) : "*" ); | |||
|
|
|||
| if ( ( newsocket = socket( PF_INET6, SOCK_DGRAM, IPPROTO_UDP ) ) == INVALID_SOCKET ) | |||
| #ifdef _WIN32 | |||
| newsocket = WSASocketW( PF_INET6, SOCK_DGRAM, IPPROTO_UDP, nullptr, 0, WSA_FLAG_NO_HANDLE_INHERIT ); | |||
There was a problem hiding this comment.
Why is this better than socket?
There was a problem hiding this comment.
It's the only way to pass the no-inherit flag to avoid the handle leaking into child processes.
| @@ -935,7 +935,13 @@ SOCKET NET_IPSocket( const char *net_interface, int port, struct sockaddr_in *bi | |||
|
|
|||
| Log::Notice( "Opening IP socket: %s:%s", net_interface ? net_interface : "0.0.0.0", port ? va( "%i", port ) : "*" ); | |||
|
|
|||
| if ( ( newsocket = socket( PF_INET, SOCK_DGRAM, IPPROTO_UDP ) ) == INVALID_SOCKET ) | |||
| #ifdef _WIN32 | |||
| newsocket = WSASocketW( PF_INET, SOCK_DGRAM, IPPROTO_UDP, nullptr, 0, WSA_FLAG_NO_HANDLE_INHERIT ); | |||
There was a problem hiding this comment.
Should we use SO_REUSEPORT/SO_REUSEADDR instead? That should be consistent across all OSs (well SO_REUSEPORT on Linux and SO_REUSEADDR on others)...
There was a problem hiding this comment.
But we don't want more than one server to bind to the same address.
| SV_NET_Config(); | ||
| Net::ShutDownDNS(); | ||
| NET_Config( true ); |
There was a problem hiding this comment.
It's not really doing something different. The diff may be confusing because the boolean argument on the left side is networkingEnabled and on the right side it is serverMode (the responsibility of deciding serverMode was put on the caller).
| @@ -756,7 +756,7 @@ void SV_Shutdown( const char *finalmsg ) | |||
|
|
|||
| Cvar_Set( "sv_running", "0" ); | |||
| #ifndef BUILD_SERVER | |||
| NET_Config( true ); | |||
| NET_EnableNetworking( false ); | |||
There was a problem hiding this comment.
This probably deserves a comment? Why was this changed?
There was a problem hiding this comment.
It's not really doing something different. The diff may be confusing because the boolean argument on the left side is networkingEnabled and on the right side it is serverMode (the responsibility of deciding serverMode was put on the caller).
The ifndef stuff is indeed quite confusing, so I added a comment. Thanks for the suggestion.
It was producing errors about the port being already in use, and thus switching to port 27961 instead of 27960. This was caused by the socket being inherited by NaCl processes.
NET_Config(bool enableNetworking) was a function to either open the network sockets if the arg is true, or close them if it is false. It checked the modified flag of a bunch of net_ cvars, with the idea that if it is requested to enable networking while it was already enabled, to not do anything unless the configuration is changed. But actually this situation can never happen, so we can drop the modify checks and always reinitialize. NET_Config( true ) was only done in 4 places, none of which could possibly omit the network (re-)initialization: - NET_Init - only called once on startup - NET_Restart - the point of /net_restart is to force reinitialization - SV_Startup - this is only done in clients and starting the server is a state change for clients so restart can't be skipped - SV_Shutdown - this is only done in clients and stopping the server is a state change for clients so restart can't be skipped. Also split NET_Config into two functions NET_EnableNetworking and NET_DisableNetworking for better readability.
Note: nothing release-critical here