Remove unnecessary casts when calling socket operations #33378

pull pinheadmz wants to merge 1 commits into bitcoin:master from pinheadmz:sockopt-cast changing 5 files +8 −16
  1. pinheadmz commented at 4:55 pm on September 12, 2025: member

    During review of #32747 several casting operations were questioned in existing code that had been copied or moved. That lead me to find a few other similar casts in the codebase.

    It turns out that since the Sock class wraps syscalls with its own internal casting (see #24357 and #20788 written in 2020-2022) we no longer need to cast the arguments when calling these functions. The original argument-casts are old and were cleaned up a bit in #12855 written in 2018.

    The casting is only needed for windows compatibility, where those syscalls require a data argument to be of type char* specifically:

    https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-getsockopt

    0int getsockopt(
    1  [in]      SOCKET s,
    2  [in]      int    level,
    3  [in]      int    optname,
    4  [out]     char   *optval,
    5  [in, out] int    *optlen
    6);
    

    but on POSIX the argument is void*:

    https://www.man7.org/linux/man-pages/man2/getsockopt.2.html

    0       int getsockopt(socklen *restrict optlen;
    1                      int sockfd, int level, int optname,
    2                      void optval[_Nullable restrict *optlen],
    3                      socklen_t *restrict optlen);
    
  2. DrahtBot commented at 4:55 pm on September 12, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33378.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator, vasild, Raimo33, davidgumberg, achow101

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

    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.

  3. fanquake commented at 9:16 am on September 13, 2025: member

    https://github.com/bitcoin/bitcoin/actions/runs/17680889551/job/50254583517?pr=33378#step:12:357:

    0 D:\a\bitcoin\bitcoin\src\test\sock_tests.cpp(37): Entering test case "constructor_and_destructor"
    12025-09-12T17:23:00.992709Z [unknown] [D:\a\bitcoin\bitcoin\src\test\util\random.cpp:48] [void __cdecl SeedRandomStateForTest(enum SeedRand)] Setting random seed for current tests to RANDOM_CTX_SEED=936d3154550e893420e302712f2c7f1031a35cebf124c36896c0cee3e5c2ece2
    22025-09-12T17:23:00.994377Z [test] [D:\a\bitcoin\bitcoin\src\init\common.cpp:153] [void __cdecl init::LogPackageVersion(void)] Bitcoin Core version v30.99.0-5ee8bacfaf42 (release build)
    32025-09-12T17:23:00.995687Z [test] [D:\a\bitcoin\bitcoin\src\kernel\context.cpp:20] [auto __cdecl kernel::Context::{ctor}::<lambda_1>::operator ()(void) const] Using the 'standard' SHA256 implementation
    4D:/a/bitcoin/bitcoin/src/test/sock_tests.cpp(42): error: in "sock_tests/constructor_and_destructor": check !SocketIsClosed(s) has failed
    5D:\a\bitcoin\bitcoin\src\test\sock_tests.cpp(37): Leaving test case "constructor_and_destructor"; testing time: 4943us
    
  4. pinheadmz force-pushed on Sep 13, 2025
  5. pinheadmz force-pushed on Sep 13, 2025
  6. pinheadmz marked this as ready for review on Sep 15, 2025
  7. pinheadmz requested review from hodlinator on Sep 15, 2025
  8. pinheadmz requested review from vasild on Sep 15, 2025
  9. in src/httpserver.cpp:404 in da23e2e2e1 outdated
    400@@ -401,7 +401,7 @@ static bool HTTPBindAddresses(struct evhttp* http)
    401             // Set the no-delay option (disable Nagle's algorithm) on the TCP socket.
    402             evutil_socket_t fd = evhttp_bound_socket_get_fd(bind_handle);
    403             int one = 1;
    404-            if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (sockopt_arg_type)&one, sizeof(one)) == SOCKET_ERROR) {
    405+            if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char*)&one, sizeof(one)) == SOCKET_ERROR) {
    


    hodlinator commented at 7:18 pm on September 15, 2025:

    Agree that we can get rid of the typedef that is so rarely used.

    Should probably use another kind of cast here and in sock_tests.cpp? https://github.com/bitcoin/bitcoin/blob/da23e2e2e104fc9d8e210c3989f2ae4945c072fa/doc/developer-notes.md?plain=1#L60-L63


    pinheadmz commented at 10:26 am on September 16, 2025:
    right thanks!
  10. net: remove unnecessary casts in socket operations
    These methods in the Sock class wrap corresponding syscalls,
    accepting void* arguments and casting to char* internally, which is
    needed for Windows support and ignored on other platforms because
    the syscall itself accepts void*:
    
    Send()
    Recv()
    GetSockOpt()
    SetSockOpt()
    67f632b6de
  11. pinheadmz force-pushed on Sep 16, 2025
  12. hodlinator approved
  13. hodlinator commented at 11:40 am on September 16, 2025: contributor

    ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5

    (PR description still ends with “Opening as a draft for now to test CI on supported platforms.”)

  14. pinheadmz commented at 12:51 pm on September 16, 2025: member
    Thanks for the edit @fanquake 🥰
  15. vasild approved
  16. vasild commented at 1:03 pm on September 16, 2025: contributor
    ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5
  17. Raimo33 commented at 0:02 am on September 18, 2025: none
    ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5
  18. davidgumberg approved
  19. davidgumberg commented at 1:06 am on September 18, 2025: contributor

    ACK https://github.com/bitcoin/bitcoin/commit/67f632b6deb8b4aa190c458b71d2bc8c793626d5

    As described in the commit messages, the Sock::(G|S)etSockOpt functions are already casting correctly, so there shouldn’t be casting when using those functions. The only other users of (g|s)etsockopt are the httpserver (which doesn’t use Sock):

    https://github.com/bitcoin/bitcoin/blob/67f632b6deb8b4aa190c458b71d2bc8c793626d5/src/httpserver.cpp#L404

    and the socket unit tests:

    https://github.com/bitcoin/bitcoin/blob/67f632b6deb8b4aa190c458b71d2bc8c793626d5/src/test/sock_tests.cpp#L20-L28

    SocketIsClosed() can’t use Sock::GetSockOpt() since it’s used to check the status of a SOCKET after e.g. the Sock object has been destructed:

    https://github.com/bitcoin/bitcoin/blob/67f632b6deb8b4aa190c458b71d2bc8c793626d5/src/test/sock_tests.cpp#L37-L45

  20. achow101 commented at 8:43 pm on September 18, 2025: member
    ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5
  21. achow101 merged this on Sep 18, 2025
  22. achow101 closed this on Sep 18, 2025


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-10-10 21:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me