compat: document code in compat.h #25493

fanquake commented at 4:46 PM on June 28, 2022: member

Move compat.h into compat/, and document what is in there.

in src/compat/compat.h:56 in e872992e89 outdated

  54 |  #define WSAEINPROGRESS      EINPROGRESS
  55 |  #define WSAEADDRINUSE       EADDRINUSE
  56 | -#define WSAENOTSOCK         EBADF
  57 |  #define INVALID_SOCKET      (SOCKET)(~0)
  58 |  #define SOCKET_ERROR        -1
  59 |  #else

fanquake commented at 4:55 PM on June 28, 2022:

@vasild could you explain what the following block

#ifndef WSAEAGAIN
#ifdef EAGAIN
#define WSAEAGAIN EAGAIN
#else
#define WSAEAGAIN WSAEWOULDBLOCK
#endif
#endif

is accounting for (given you added it)? I'd have thought everything inside this #else block could be replaced by #define WSAEAGAIN WSAEWOULDBLOCK, as WSAEAGAIN would never be defined?

vasild commented at 7:13 AM on June 29, 2022:

The purpose of that is to ensure that WSAEAGAIN is defined on Windows - if it is already defined, don't do anything. If it is not defined but EAGAIN is defined then define WSAEAGAIN to EAGAIN, otherwise define WSAEAGAIN to WSAEWOULDBLOCK which is assumed to be present.

fanquake commented at 7:29 AM on June 29, 2022:

Right, but when would that be the case? I find the code confusing because I'm pretty sure it's trying to account for situations that don't exist. Can you point to a toolchain / system where we'd be compiling Bitcoin Core, for Windows, where WSAEAGAIN would be defined? (so we can document it)?

to WSAEWOULDBLOCK which is assumed to be present.

If you're going to assume WSAEWOULDBLOCK is present, I think we can assume WSAEAGAIN wont be, and again, just simplify to #define WSAEAGAIN WSAEWOULDBLOCK.

vasild commented at 7:54 AM on June 29, 2022:

Can you point to a toolchain...

No, I just coded it defensively so that it works in all scenarios, including future platforms that may define WSAEAGAIN or aux headers from dependencies - e.g. leveldb, sqlite may define it even if Windows itself does not.

But ok, if you feel like simplifying it, then I am fine with that. Worst that can happen is that it will fail to compile with an error like "WSAEAGAIN is redefined" - no subtle behavior at runtime. So, it would become:

#ifdef EAGAIN
#define WSAEAGAIN EAGAIN
#else
#define WSAEAGAIN WSAEWOULDBLOCK
#endif

fanquake commented at 10:04 AM on June 29, 2022:

Worst that can happen is that it will fail to compile with an error like "WSAEAGAIN is redefined"

I think that would be much better behaviour, and make it obvious if such a situation where to exist, rather than silently accomodating dependencies / toolchains changing underneath us with some #ifdef soup.

fanquake force-pushed on Jun 28, 2022

DrahtBot added the label Refactoring on Jun 28, 2022

jamesob commented at 7:19 PM on June 28, 2022: member

Concept ACK

DrahtBot commented at 12:34 AM on June 29, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/random.cpp:13 in ae2db1c476 outdated

   9 | @@ -10,7 +10,7 @@
  10 |  #include <crypto/sha512.h>
  11 |  #include <support/cleanse.h>
  12 |  #ifdef WIN32
  13 | -#include <compat.h> // for Windows API
  14 | +#include <compat/compat.h> // for Windows API

vasild commented at 7:27 AM on June 29, 2022:

Maybe the comment // for Windows API is too narrow and should be removed? (here and elsewhere)

laanwj commented at 9:43 AM on June 29, 2022:

Agree. If it was specifically about windows compatibility, we could name it like wincompat.h instead of having comments like this. But I think the file is less specific?

fanquake commented at 10:05 AM on June 29, 2022:

Will drop this comment.

in src/compat/compat.h:89 in ae2db1c476 outdated

  87 | @@ -77,6 +88,8 @@ typedef int32_t ssize_t;
  88 |  #endif
  89 |  #endif
  90 |  
  91 | +// The type of the option value passed to getsockopt & setsockopt
  92 | +// differs between Windows and non-Windows.
  93 |  #ifndef WIN32
  94 |  typedef void* sockopt_arg_type;

vasild commented at 7:38 AM on June 29, 2022:

I think sockopt_arg_type can be removed altogether, but I did not try on Windows.

Sock::SetSockOpt() is taking void*, thus any pointer is ok to be passed. Then Sock::SetSockOpt() casts explicitly the void* to char* when calling setsockopt() - this is already working in the current code.

This works on FreeBSD:

diff --git i/src/compat.h w/src/compat.h
index 0a44b98b4e..180fb31275 100644
--- i/src/compat.h
+++ w/src/compat.h
@@ -74,18 +74,12 @@ typedef int64_t ssize_t;
 #else
 typedef int32_t ssize_t;
 #endif
 #endif
 #endif
 
-#ifndef WIN32
-typedef void* sockopt_arg_type;
-#else
-typedef char* sockopt_arg_type;
-#endif
-
 #ifdef WIN32
 // Export main() and ensure working ASLR when using mingw-w64.
 // Exporting a symbol will prevent the linker from stripping
 // the .reloc section from the binary, which is a requirement
 // for ASLR. While release builds are not affected, anyone
 // building with a binutils < 2.36 is subject to this ld bug.
diff --git i/src/net.cpp w/src/net.cpp
index 7f4e571c8d..4a3fa29390 100644
--- i/src/net.cpp
+++ w/src/net.cpp
@@ -2297,22 +2297,22 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError,
         LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original);
         return false;
     }
 
     // Allow binding if the port is still in TIME_WAIT state after
     // the program was closed and restarted.
-    if (sock->SetSockOpt(SOL_SOCKET, SO_REUSEADDR, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) {
+    if (sock->SetSockOpt(SOL_SOCKET, SO_REUSEADDR, &nOne, sizeof(int)) == SOCKET_ERROR) {
         strError = strprintf(Untranslated("Error setting SO_REUSEADDR on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError()));
         LogPrintf("%s\n", strError.original);
     }
 
     // some systems don't have IPV6_V6ONLY but are always v6only; others do have the option
     // and enable it by default or not. Try to enable it, if possible.
     if (addrBind.IsIPv6()) {
 #ifdef IPV6_V6ONLY
-        if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_V6ONLY, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) {
+        if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_V6ONLY, &nOne, sizeof(int)) == SOCKET_ERROR) {
             strError = strprintf(Untranslated("Error setting IPV6_V6ONLY on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError()));
             LogPrintf("%s\n", strError.original);
         }
 #endif
 #ifdef WIN32
         int nProtLevel = PROTECTION_LEVEL_UNRESTRICTED;
diff --git i/src/netbase.cpp w/src/netbase.cpp
index 030f462ed9..22eda88399 100644
--- i/src/netbase.cpp
+++ w/src/netbase.cpp
@@ -582,13 +582,13 @@ bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nT
             // Even if the wait was successful, the connect might not
             // have been successful. The reason for this failure is hidden away
             // in the SO_ERROR for the socket in modern systems. We read it into
             // sockerr here.
             int sockerr;
             socklen_t sockerr_len = sizeof(sockerr);
-            if (sock.GetSockOpt(SOL_SOCKET, SO_ERROR, (sockopt_arg_type)&sockerr, &sockerr_len) ==
+            if (sock.GetSockOpt(SOL_SOCKET, SO_ERROR, &sockerr, &sockerr_len) ==
                 SOCKET_ERROR) {
                 LogPrintf("getsockopt() for %s failed: %s\n", addrConnect.ToString(), NetworkErrorString(WSAGetLastError()));
                 return false;
             }
             if (sockerr != 0) {
                 LogConnectFailure(manual_connection,
diff --git i/src/test/sock_tests.cpp w/src/test/sock_tests.cpp
index 01a402833d..f10cca8c17 100644
--- i/src/test/sock_tests.cpp
+++ w/src/test/sock_tests.cpp
@@ -21,13 +21,13 @@ static bool SocketIsClosed(const SOCKET& s)
 {
     // Notice that if another thread is running and creates its own socket after `s` has been
     // closed, it may be assigned the same file descriptor number. In this case, our test will
     // wrongly pretend that the socket is not closed.
     int type;
     socklen_t len = sizeof(type);
-    return getsockopt(s, SOL_SOCKET, SO_TYPE, (sockopt_arg_type)&type, &len) == SOCKET_ERROR;
+    return getsockopt(s, SOL_SOCKET, SO_TYPE, &areupayingattention, &len) == SOCKET_ERROR;
 }
 
 static SOCKET CreateSocket()
 {
     const SOCKET s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
     BOOST_REQUIRE(s != static_cast<SOCKET>(SOCKET_ERROR));

fanquake commented at 10:06 AM on June 29, 2022:

areupayingattention 👀

vasild approved

vasild commented at 7:47 AM on June 29, 2022: contributor

ACK ae2db1c4764e2ab0e027c9526ba51c5330bb8b1e

fanquake force-pushed on Jun 29, 2022

vasild approved

vasild commented at 11:13 AM on June 29, 2022: contributor

ACK 5cb0589c2159c50d3f5d2f7b22287446a54a1070

fanquake commented at 11:34 AM on June 29, 2022: member

We will need to update the minisketch subtree, to accomodate the compat.h renaming, as the minisketch header, in the subtree, actually includes compat.h, when building for MSVC.

fanquake force-pushed on Jun 29, 2022

fanquake commented at 1:48 PM on June 29, 2022: member

For now I've rebased this on top of a minisketch subtree update, and a cherry-pick of https://github.com/sipa/minisketch/pull/66; that way we'll get a run against the MSVC CI. I've also added a commit to use the SSIZE_T change in our code.

Once that PR is merged, I'll open a PR to update the subtree in our repo. Once that is merged, I'll rebase this.

achow101 referenced this in commit 5bc10b39ab on Jun 29, 2022

sidhujag referenced this in commit 89439cd26e on Jun 29, 2022

fanquake force-pushed on Jun 30, 2022

fanquake marked this as ready for review on Jun 30, 2022

fanquake requested review from vasild on Jul 4, 2022

vasild approved

vasild commented at 9:37 AM on July 5, 2022: contributor

ACK 18ce5f990d7a2ae5815d09c87be5db7f5d86c1e7

in src/compat/compat.h:15 in 18ce5f990d outdated

  12 |  #include <config/bitcoin-config.h>
  13 |  #endif
  14 |  
  15 | +// Windows defines FD_SETSIZE to 64, see _fd_types.h (mingw-w64),
  16 | +// which is too small for our usage. We redefine it to be 1024, to
  17 | +// match glibc, see typesizes.h.

luke-jr commented at 11:30 PM on July 5, 2022:

Some systems don't allow redefining FD_SETSIZE (notably including glibc), so IMO good to point out that Windows specifically allows it.

// Windows defines FD_SETSIZE to 64 (see _fd_types.h in mingw-w64),
// which is too small for our usage, but allows us to redefine it safely.
// We redefine it to be 1024, to match glibc, see typesizes.h.

fanquake commented at 10:13 AM on July 6, 2022:

Taken the suggestion.

fanquake force-pushed on Jul 6, 2022

vasild approved

vasild commented at 10:22 AM on July 6, 2022: contributor

ACK bdbdfe074b7c54268ec813f5092ce722d6cc5899

DrahtBot added the label Needs rebase on Jul 7, 2022

fanquake force-pushed on Jul 13, 2022

vasild approved

vasild commented at 10:20 AM on July 13, 2022: contributor

ACK 122c242cae6e459ab58f2ef0ef53e2f8db6fa363

DrahtBot removed the label Needs rebase on Jul 13, 2022

fanquake requested review from laanwj on Jul 13, 2022

fanquake requested review from jamesob on Jul 19, 2022

fanquake requested review from hebasto on Jul 19, 2022

hebasto approved

hebasto commented at 8:54 AM on July 20, 2022: member

ACK 122c242cae6e459ab58f2ef0ef53e2f8db6fa363

The S_IRUSR and S_IWUSR macro are used in src/wallet/bdb.cpp which requires to #include <compat/compat.h>. Maybe #include <sys/stat.h> could be moved from src/wallet/bdb.cpp into compat/compat.h as well.

refactor: move compat.h into compat/ cc7b2fdd70

compat: document FD_SETSIZE redefinition for WIN32 7c3df5e548

compat: remove unused WSA* definitions b63ddb7e6d

compat: extract and document MAX_PATH 203e682d22

fanquake force-pushed on Jul 20, 2022

fanquake commented at 9:47 AM on July 20, 2022: member

Maybe #include <sys/stat.h> could be moved from src/wallet/bdb.cpp into compat/compat.h as well.

Rebased and incorporated this change.

vasild commented at 11:21 AM on July 20, 2022: contributor

I would expect non-standard headers to be included in compat.h (inside some #ifdef magic) and then other source files to include compat.h. This implies that a non-standard header is included only in compat.h. sys/stat.h is included in 5 files (in httpserver.cpp without #ifdef :eyes:). Should all of those be replaced with #include <compat.h>?

compat.h is included in 27 files. Now all of them (indirectly) include sys/stat.h.

The commit message of 3a865ea692cc46a68816dbd84f7254f4390a4f42 compat: document S_I* defines when building for Windows needs to be extended because now the commit does more than mentioned.

compat: document S_I* defines when building for Windows fb6db6fb0e

compat: document sockopt_arg_type definition 3f1d2fb035

compat: document error-code mapping

See:
https://docs.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2

3be7ee750f

compat: document redefining ssize_t when using MSVC

See:
https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types#ssize_t
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html

f7dc99244c

fanquake force-pushed on Jul 20, 2022

fanquake commented at 12:11 PM on July 20, 2022: member

Ok. I've just reverted Hebastos suggestion for now. If someone wants to go an massage the headers later (with IWYU), they can. However doing that is unrelated to adding documentation.

vasild approved

vasild commented at 12:28 PM on July 20, 2022: contributor

ACK f7dc99244c8e78dbd0196f612690efcc449c37dc

hebasto approved

hebasto commented at 1:37 PM on July 20, 2022: member

re-ACK f7dc99244c8e78dbd0196f612690efcc449c37dc

MarcoFalke merged this on Jul 20, 2022

MarcoFalke closed this on Jul 20, 2022

fanquake deleted the branch on Jul 20, 2022

sidhujag referenced this in commit 224670ee5f on Jul 20, 2022

bitcoin locked this on Jul 20, 2023