MinGW: port POSIX socket compatibility layer #2046

kinkie · 2025-03-29T12:35:06Z

winsock's socket-related functions are very similar but not
identical to the POSIX standard. Make the mswindows
compatibility layer available to MinGW, and modernize it.

Implement portable wrappers around socket-related calls, named
x[function], and use them in all callsites.

Error highlighting the issue:

TcpAcceptor.cc: In member function
  'bool Comm::TcpAcceptor::acceptInto(Comm::ConnectionPointer&)':
TcpAcceptor.cc:352:17: error:
  comparison of unsigned expression in '< 0'
  is always false [-Werror=type-limits]
  352 |     if (rawSock < 0) {

rousskov · 2025-03-31T20:47:40Z

more socket-related functions will need to have this treatment. I wonder whether it's best to have each have their own header and implementation or to group them all under a single xsocket.* file

We should group sys/socket.h wrappers (i.e. listen(2), accept(2), connect(2), shutdown(2), getsockopt(2), etc.), especially if they have similar implementations.

A more difficult question is whether introduction of xfoo() warrants conversion from legacy POSIX foo() to modern C++ API: If every foo() caller is a C++ caller and that caller has to be updated anyway, then it could be the right time to start doing proper memory management and error handling (instead of writing or preserving awkward and duplicated code). That requires a lot more/serious work though. It may also require to get rid of the remaining legacy C callers.

compat/xaccept.cc

@rousskov

okay, pending @rousskov review issues and final call on whether to use compat/xsocket.* filenames.

Windows' accept(2) has the same signature as POSIX', but returns a SOCKET (under the hood, a int *). This confuses error handling code in squid, which uses POSIX return values to detect error values. Implement xaccept() to wrap around it, taking the implementation from mswindows.h and using it on MinGW as well. Error highlighting the issue: ``` TcpAcceptor.cc: In member function 'bool Comm::TcpAcceptor::acceptInto(Comm::ConnectionPointer&)': TcpAcceptor.cc:352:17: error: comparison of unsigned expression in '< 0' is always false [-Werror=type-limits] 352 | if (rawSock < 0) { ```

Co-authored-by: Amos Jeffries <[email protected]>

compat/socket.cc

kinkie · 2025-05-21T08:52:38Z

@yadij I think this got to a point where I would feel confident about checkpointing and continuing in a new PR

compat/wserrno.cc

Co-authored-by: Alex Rousskov <[email protected]>

compat/socket.h

rousskov · 2025-05-22T13:16:47Z

compat/unistd.cc

@@ -23,18 +23,34 @@
 #define _S_IWRITE 0x0080
 #endif

+/// true if handle is a socket, false if a file or error


Where feasible, we only document one of the mutually exclusive boolean result values to avoid ambiguities, conflicts, etc.

Suggested change

/// true if handle is a socket, false if a file or error

/// whether a given handle is a valid socket

rousskov · 2025-05-22T13:18:34Z

compat/unistd.cc

+isSocket(intptr_t handle)
+{
+    if (!isValidSocketHandle(handle)) {
+        // errno is already set by _get_osfhandle()


If this function guarantees errno setting on errors, then it is missing a SetErrnoFromWsaError() below, right? If it does not guarantee, then this comment is misleading.

Suggested change

// errno is already set by _get_osfhandle()

If you mean calling SetErrnoFromWsaError(); before the return false; statement .... then yes that is missing.

If you mean calling SetErrnoFromWsaError(); before the return false; statement

Yes, I do (and there are two return false statement below, but it can be argued that only the second one is relevant here). However, the If parts of my change request are very important -- I do not know whether this function should guarantee/set errno, and the correct way to address my change request depends on whether that guarantee exists.

In the ideal world, isFoo() test functions should not alter global state and, hence, should not promise to set errno. I hope we can agree on (and approach) that ideal and, hence, should not promise that isSocket() sets errno, but that remains to be seen.

From _get_osfhandle() documentation:

returns INVALID_HANDLE_VALUE (-1). It also sets errno to EBADF, indicating an invalid file handle.

There is no documentation about what a call to WSAGetLastError() will produce. So IMHO we should not call our mapping function and clobber that EBADF value with an unknown and possibly unrelated error value.

There is no documentation about what a call to WSAGetLastError() will produce

IMO, existing WSAGetLastError() documentation strongly implies that WSAGetLastError() does not change errno. However, WSAGetLastError() behavior is secondary to what this change request is about.

I suggest to adopt an "isFoo() test functions should not alter global state" principle and adjust isSocket() code accordingly (i.e. apply my suggestion as is, without adding a SetErrnoFromWsaError() call).

Due to factors outside of our control (e.g., globals-changing non-Squid functions that isFoo() has to call), we cannot fully comply with that principle in some cases, but we can usually make our own code compliant.

I agree on not adding the SetErrnoFromWsaError() call. At least for the OSF API calls. If for no reason other than they are not WSA API calls.

FWIW, IMO that handling of "is" functions/methods is basic good practice we should not need to have a policy about. That calls into question whether these functions should be "is functions" at all. Maybe they are better as "requireX" instead.

I agree on not adding the SetErrnoFromWsaError() call. At least for the OSF API calls. If for no reason other than they are not WSA API calls.

FWIW, IMO that handling of "is" functions/methods is basic good practice we should not need to have a policy about.

Glad we agree regarding the important bits. The difference between an implicit "basic good practice" (that everybody applies) and an explicit principle (that everybody honors) is a secondary detail. Current PR code does not follow that practice or honors that principle, prompting this change request.

That calls into question whether these functions should be "is functions" at all. Maybe they are better as "requireX" instead.

If callers need a boolean function that tests whether condition Foo is true, then isFoo() is better than requireFoo().

If callers need a void function that enforces condition Bar, then requireBar() is better than isBar().

If callers need a boolean function that helps the caller enforce condition Baz (i.e. a function that contains partial enforcement code in addition to condition testing code), then neither isBaz() nor requireBaz() work well.

AFAICT, all existing isSocket() callers belong to the first category and do not enforce an "x is a socket" condition, so isSocket() name sounds correct to me.

compat/unistd.cc

compat/wserrno.cc

compat/socket.cc

Co-authored-by: Alex Rousskov <[email protected]>

compat/wserrno.cc

rousskov · 2025-06-30T16:05:30Z

compat/wserrno.cc

+        { WSAEISCONN, EISCONN },
+
+        { WSAEWOULDBLOCK, EWOULDBLOCK },
+        // Windows does not have a equivalent of EAGAIN


To match the other two similar comments (and fix grammar):

Suggested change

// Windows does not have a equivalent of EAGAIN

// no Windows error code maps to EAGAIN

rousskov · 2025-06-30T16:10:09Z

compat/wserrno.cc

+        // no Windows error code maps to ENFILE
+
+        { WSAECONNRESET, ECONNRESET }
+    };


I recommend sorting {WSAX, EY} entries alphabetically so that it is easier to find a particular error and easier to decide where to place new entries. I would sort by the second/EY field (so that comments can participate in the same sort rather than moved to the beginning or end), but sorting by the first/WSAX field is better than no sorting at all.

I do not insist on this, but the current grouping looks odd. I am aware of this grouping origins.

rousskov · 2025-06-30T16:22:34Z

compat/socket.cc

+    const auto result = ::getsockopt(handle, level, option_name, static_cast<char *>(option_value), &ol);
+    if (result == SOCKET_ERROR)
+        SetErrnoFromWsaError();
+    return result;


Unresolving: xrecvfrom() is still broken.

rousskov · 2025-06-30T16:25:47Z

compat/socket.cc

+        // errno is already set by _get_osfhandle()
+        return SOCKET_ERROR;
+    }
+    auto fl = static_cast<int>(*fromlen);


WIndows and Linux documentation says that fromlen may be nil. Please do not dereference without checking.

While fixing this, please rename fromlen to fromLen or similar to use camelCase and make this variable more readable.

rousskov · 2025-06-30T16:32:31Z

compat/socket.h

+inline ssize_t
+xsendto(int socketFd, const void * buf, size_t len, int flags, const struct sockaddr * to, socklen_t l)
+{
+    return sendto(socketFd, static_cast<const char *>(buf), static_cast<int>(len), flags, to, l);


These two problematic casts are Windows-specific and do not belong to this non-WIndows code, right? At least some of the existing callers do not perform these cast. Please check all non-Windows wrappers for similar unwanted casts.

This comment was marked as resolved.

Sign in to view

rousskov self-requested a review March 29, 2025 14:24

This comment was marked as resolved.

Sign in to view

yadij added the S-waiting-for-author author action is expected (and usually required) label Mar 30, 2025

rousskov requested changes Mar 31, 2025

View reviewed changes

compat/xaccept.cc Outdated Show resolved Hide resolved

compat/xaccept.cc Outdated Show resolved Hide resolved

compat/xaccept.cc Outdated Show resolved Hide resolved

kinkie added 7 commits April 17, 2025 08:22

fix return type on POSIX

8aec939

Source format

4c6995f

More source formatting

c85fe33

implement xbind and xaccept

009650d

xconnect() and xgethostbyname()

01efb92

port setsockopt

ed2c975

kinkie force-pushed the update-mingw-16 branch from 084949b to ed2c975 Compare April 18, 2025 18:54

Source maintenance

7359d54

kinkie changed the title ~~MinGW: implement xaccept()~~ MinGW: port POSIX socket compatibility layer Apr 18, 2025

This comment was marked as resolved.

Sign in to view

kinkie force-pushed the update-mingw-16 branch from d96076c to 32ca454 Compare April 19, 2025 17:28

Fix files missing headers

a6dd98b

kinkie force-pushed the update-mingw-16 branch from 32ca454 to a6dd98b Compare April 19, 2025 17:48

kinkie and others added 8 commits April 19, 2025 17:55

do not include windows headers in socket.cc

b4b7bcb

Implement code review suggestion

21dd497

Accept proposal for SOCKET_ERROR handling

f78d5cb

Co-authored-by: Amos Jeffries <[email protected]>

make xclose more readable

5aec29e

increase xconnect readibility

ce61630

AAA for xgethostbyname

737edb3

improve xsetsockopt readability

60c88de

formatting

7942f2a

yadij requested changes Apr 20, 2025

View reviewed changes

kinkie requested a review from yadij May 21, 2025 08:46

This comment was marked as resolved.

Sign in to view

rousskov reviewed May 21, 2025

View reviewed changes

compat/wserrno.cc Outdated Show resolved Hide resolved

rousskov reviewed May 21, 2025

View reviewed changes

compat/wserrno.cc Outdated Show resolved Hide resolved

kinkie and others added 11 commits May 21, 2025 22:29

Use AAA and static_cast in compat/socket.cc

9793e3b

Co-authored-by: Alex Rousskov <[email protected]>

apply aaa to all type casts in socket.cc

8d5e371

actually update reeturned size in getters in compat/socket.cc

ab90183

do not explicitly refer to the global namespace in compat/socket.cc

e19acba

assert that size paramers do not overflow int

8269bdb

set errno in case of _open_osfhandle failure

3064f89

do not use global namespace explicitly in all touched compat/*.cc files

88db730

AAA for the result of _get_osfhandle

caa72c4

do not try to be smarter than the compiler

d232acd

remove unnecessary wserrno.cc comments

f3c61ff

use isSocket instead of getsockopt

be0dc37

rousskov requested changes May 22, 2025

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label May 22, 2025

yadij and others added 3 commits June 15, 2025 22:49

Apply suggestions from code review

89e9e8e

Co-authored-by: Alex Rousskov <[email protected]>

Apply suggestions from code review

3b3a5e1

Co-authored-by: Alex Rousskov <[email protected]>

Apply suggestions from code review

dab2fbf

Co-authored-by: Alex Rousskov <[email protected]>

yadij removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Jun 15, 2025

yadij and others added 7 commits June 15, 2025 23:37

Merge branch 'master' into update-mingw-16

bfca24e

Move xrecv

1211842

Remove double definition of xrecv on non-windows

fab8d08

reword comment on EAGAIN

e5541d3

Always check _get_osfhandle return value

8217cac

removed duplicate xrecv implementation

01f17ba

rename local variables in isSocket

5c1aba1

rousskov requested changes Jun 30, 2025

View reviewed changes

	/// true if handle is a socket, false if a file or error
	/// whether a given handle is a valid socket

	// Windows does not have a equivalent of EAGAIN
	// no Windows error code maps to EAGAIN

MinGW: port POSIX socket compatibility layer #2046

Are you sure you want to change the base?

MinGW: port POSIX socket compatibility layer #2046

Uh oh!

Conversation

kinkie commented Mar 29, 2025 • edited by yadij Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

rousskov commented Mar 31, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kinkie commented May 21, 2025

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadij Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kinkie commented Mar 29, 2025 •

edited by yadij

Loading

rousskov May 23, 2025 •

edited

Loading

yadij Jun 17, 2025 •

edited

Loading