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

Replace ioctl with fcntl, fix ioctlsocket calls (64-bit fix) #356

Merged
merged 1 commit into from
May 27, 2024

Conversation

GravisZro
Copy link
Contributor

@GravisZro GravisZro commented May 16, 2024

These take two different types of arguments but worked in the past due to long being 32-bit.

Pull Request Type

  • GitHub Workflow changes
  • Documentation or Wiki changes
  • Build and Dependency changes
  • Runtime changes
    • Render changes
    • Audio changes
    • Input changes
    • Network changes
    • Other changes

Description

The ioctl argument for UNIX systems is int* while the ioctlsocket argument for Windows is unsigned long*. On 32-bit systems theses coincided so no problems but not on 64-bit. However, the behavior of ioctl calls is non-standard which is why code should use fnctl.

See also:

Checklist

  • I have tested my changes locally and verified that they work as intended.
  • I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
  • I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Additional Comments

@GravisZro GravisZro mentioned this pull request May 17, 2024
13 tasks
@GravisZro GravisZro changed the title Fix ioctl/ioctlsocket calls (64-bit fix) Replace ioctl with fcntl, fix ioctlsocket calls (64-bit fix) May 19, 2024
@JeodC JeodC requested a review from winterheart May 20, 2024 19:41
@winterheart
Copy link
Collaborator

I would rewrite socket initialization as

#ifdef WIN32

dedicated_listen_socket = socket(AF_INET, SOCK_STREAM, 0);

// ...

unsigned long arg = 1;
ioctlsocket(dedicated_listen_socket, FIONBIO, &arg);

#else // WIN32

#ifndef SOCK_NONBLOCK
#include <fcntl.h>
#define SOCK_NONBLOCK O_NONBLOCK
#endif

dedicated_listen_socket = socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0);

#endif

And you don't have to use additional fcntl(..., F_SETFL (fcntl(..., F_GETFL) )) calls on Linux / macOS platforms. There a plenty code duplication around socket initialization, so would be some helper functions would be useful.

@GravisZro GravisZro force-pushed the fix/ioctl branch 6 times, most recently from 817b4d9 to 212d114 Compare May 22, 2024 21:42
@GravisZro
Copy link
Contributor Author

@winterheart Not all of these are socket initializations. As such, I consolidated the code into a single function in networking.h. Including networking.h in other headers lead to conflicting duplicate code which is why there are now code deletions.

@@ -343,6 +346,10 @@ typedef struct {
extern BOOL DP_active;
extern BOOL TCP_active;
extern BOOL IPX_active;

// create a new non-blocking socket
int make_nonblocking(SOCKET sock);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be called something like nw_SocketSetNonblock() to match the rest of the functions defined here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was considered but it's used on other sockets as well where the nw_* scheme wouldn't be appropriate.

`ioctl` and `ioctlsocket` take two different types of arguments but worked in the past due to
long being 32-bit. However, ioctl functionality is non-standard and should not be used in
code written after **1997** in order to make sockets non-blocking. This functionality was
standardized as part of fcntl.

* Using the new function `make_nonblocking` to make socket nonblocking
@GravisZro
Copy link
Contributor Author

Would someone please merge this? It's been reviewed by two people already.

@bryanperris bryanperris merged commit cf02a76 into DescentDevelopers:main May 27, 2024
12 checks passed
@winterheart
Copy link
Collaborator

PR has regression on Linux, see #398.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants