fs: fully initialize _OVERLAPPED for win32 #26090

1. 
   fanquake commented at 11:24 am on September 14, 2022: member

   0fs.cpp: In member function ‘bool fsbridge::FileLock::TryLock()’:
   1fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::InternalHigh’ [-Werror=missing-field-initializers]
   2  129 |     _OVERLAPPED overlapped = {0};
   3      |                                ^
   4fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::<anonymous>’ [-Werror=missing-field-initializers]
   5fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::hEvent’ [-Werror=missing-field-initializers]
   

   Came up in #25972. That PR is now rebased on this change.

   Closes: #26006

2. fs: fully initialize _OVERLAPPED for win32 02c9e56468
3. fanquake added the label Windows on Sep 14, 2022
4. fanquake requested review from sipsorcery on Sep 14, 2022
5. 
   MarcoFalke commented at 11:31 am on September 14, 2022: member

   This is a refactor?
6. hebasto approved
7. 
   hebasto commented at 7:53 pm on September 14, 2022: member

   ACK 02c9e564687af6ae2b0b6589108d502963f879cb, tested on Linux x86_64:

   • on the master branch (13fd9ee5c2d747e9f74d3fd8e33a4f0c9eaa769c):

    0$ make -j $(nproc) -C depends HOST=x86_64-w64-mingw32
    1$ ./autogen.sh
    2$ ./configure -q CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site CXXFLAGS="-Wno-return-type -Wmissing-field-initializers"
    3$ make clean
    4$ make -j $(nproc) > /dev/null 
    5fs.cpp: In member function ‘bool fsbridge::FileLock::TryLock()’:
    6fs.cpp:129:32: warning: missing initializer for member ‘_OVERLAPPED::InternalHigh’ [-Wmissing-field-initializers]
    7  129 |     _OVERLAPPED overlapped = {0};
    8      |                                ^
    9fs.cpp:129:32: warning: missing initializer for member ‘_OVERLAPPED::<anonymous>’ [-Wmissing-field-initializers]
   10fs.cpp:129:32: warning: missing initializer for member ‘_OVERLAPPED::hEvent’ [-Wmissing-field-initializers]
   11leveldb/util/env_windows.cc: In member function ‘virtual leveldb::Status leveldb::{anonymous}::WindowsRandomAccessFile::Read(uint64_t, size_t, leveldb::Slice*, char*) const’:
   12leveldb/util/env_windows.cc:197:31: warning: missing initializer for member ‘_OVERLAPPED::InternalHigh’ [-Wmissing-field-initializers]
   13  197 |     OVERLAPPED overlapped = {0};
   14      |                               ^
   15leveldb/util/env_windows.cc:197:31: warning: missing initializer for member ‘_OVERLAPPED::<anonymous>’ [-Wmissing-field-initializers]
   16leveldb/util/env_windows.cc:197:31: warning: missing initializer for member ‘_OVERLAPPED::hEvent’ [-Wmissing-field-initializers]
   

   • with this PR (02c9e564687af6ae2b0b6589108d502963f879cb):

    0$ make -j $(nproc) -C depends HOST=x86_64-w64-mingw32
    1$ ./autogen.sh
    2$ ./configure -q CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site CXXFLAGS="-Wno-return-type -Wmissing-field-initializers"
    3$ make clean
    4$ make -j $(nproc) > /dev/null 
    5leveldb/util/env_windows.cc: In member function ‘virtual leveldb::Status leveldb::{anonymous}::WindowsRandomAccessFile::Read(uint64_t, size_t, leveldb::Slice*, char*) const’:
    6leveldb/util/env_windows.cc:197:31: warning: missing initializer for member ‘_OVERLAPPED::InternalHigh’ [-Wmissing-field-initializers]
    7  197 |     OVERLAPPED overlapped = {0};
    8      |                               ^
    9leveldb/util/env_windows.cc:197:31: warning: missing initializer for member ‘_OVERLAPPED::<anonymous>’ [-Wmissing-field-initializers]
   10leveldb/util/env_windows.cc:197:31: warning: missing initializer for member ‘_OVERLAPPED::hEvent’ [-Wmissing-field-initializers]
   

   Maybe submit a similar change to leveldb upstream as well?

8. sipsorcery approved
9. 
   sipsorcery commented at 8:26 pm on September 14, 2022: member

   tACK 02c9e564687af6ae2b0b6589108d502963f879cb.
10. MarcoFalke added the label Refactoring on Sep 15, 2022
11. 
    fanquake commented at 9:24 am on September 15, 2022: member

    Maybe submit a similar change to leveldb upstream as well?

    I’ve submitted the same change upstream, https://github.com/google/leveldb/pull/1053, however given the “very limited maintenance” state of leveldb, I think the likelyhood of it being merged is low. We could pull a similar change into our subtree if we wanted.

12. fanquake merged this on Sep 15, 2022
13. fanquake closed this on Sep 15, 2022

    ---

14. fanquake deleted the branch on Sep 15, 2022
15. 
    fanquake commented at 2:38 pm on September 15, 2022: member

    Also opened a PR with the same change as sent upstream, in our subtree repo: https://github.com/bitcoin-core/leveldb-subtree/pull/34.
16. sidhujag referenced this in commit 1c746cca25 on Sep 15, 2022
17. 
    theuni commented at 7:42 pm on September 19, 2022: member

    Post-merge comment:

    From: https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-overlapped

    Any unused members of this structure should always be initialized to zero before the structure is used in a function call. Otherwise, the function may fail and return ERROR_INVALID_PARAMETER.

    According to that note at least, it’s possible that this was always failing before.

    This commit is obviously correct and a fix, we should still investigate whether or not this changes behavior in any meaningful way. Though I suspect we would’ve noticed, as LockDataDirectory in init.cpp effectively acts as a sanity check for this.

18. fanquake referenced this in commit 44a29758a0 on Oct 4, 2022
19. sidhujag referenced this in commit 73e27f2fe2 on Oct 4, 2022
20. bitcoin locked this on Sep 19, 2023

Contributors
fanquake MarcoFalke hebasto sipsorcery theuni

Labels
Refactoring Windows