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

build: fix IoctlSocket FIONBIO check #7375

wants to merge 1 commit into from


Copy link

@Tachi107 Tachi107 commented Jul 10, 2021

HAVE_IOCTLSOCKET_CAMEL_FIONBIO should get defined if IoctlSocket with FIONBIO is available, but the current check uses the (lowercase) ioctlsocket function, resulting in the same check that is done for HAVE_IOCTLSOCKET_FIONBIO.

But... Does this make sense? This doesn't really check that ioctlsocket supports the FIONBIO mode, but it just makes sure that the code compiles; so in reality this just checks if FIONBIO is defined, since the ioctlsocket check is already made when setting HAVE_IOCTLSOCKET. But maybe I'm missing something, I'm not really familiar with the Win32 API (or Amiga, since this seems the only system that has a camelcase IoctlSocket function).

Copy link

@jay jay commented Jul 13, 2021

This doesn't really check that ioctlsocket supports the FIONBIO mode

It looks like it checks by calling IoctlSocket with a socket of 0 in FIONBIO so it does check something... beats me if it actually works or not. Anyone that can target amiga want to check?

@jay jay added the build label Jul 13, 2021
bagder approved these changes Jul 16, 2021
Copy link

@bagder bagder left a comment

Seems correct based on the code. I cannot verify that it actually does right on such a target.

Copy link
Contributor Author

@Tachi107 Tachi107 commented Jul 16, 2021

Me neither :/

@jay jay closed this in af1ee13 Jul 16, 2021
Copy link

@jay jay commented Jul 16, 2021

Looks right enough. Thanks

@Tachi107 Tachi107 deleted the Tachi107:ioctlsocket-camel-fionbio-fix branch Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants