Use Winsock definition for errno values rather than the POSIX ones #13

cgutman · 2019-09-28T20:43:28Z

Modern Windows 10 SDKs have POSIX-compatible errno definitions in errno.h. These definitions match POSIX values, but Winsock uses different values. As a result, libnatpmp doesn't properly recognize WSAEWOULDBLOCK or WSAECONNREFUSED errors when built against the Windows 10 SDK. This prevents the NAT-PMP request retry logic from working properly.

The source we are currently using is from 2015. The upstream repo has received a small number of bug fixes and improvements since then. Including one that fixes an issue for Windows users: miniupnp/libnatpmp#13. The source we are currently using is the most recent "official" release, however I don't think it's worth waiting for a new one. The maintainer was prompted to do so in Oct 2020, then again in Jan of this year, and no release has eventuated. Given libnatpmp is a new inclusion into our repository, I think we should be using this newer source. This also cleans up a few warnings we currently see in depends builds: ```bash Extracting libnatpmp... /home/ubuntu/bitcoin/depends/sources/libnatpmp-20150609.tar.gz: OK Preprocessing libnatpmp... Configuring libnatpmp... Building libnatpmp... make[1]: Entering directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87' x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR -c -o natpmp.o natpmp.c x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR -c -o getgateway.o getgateway.c natpmp.c:42: warning: "EWOULDBLOCK" redefined 42 | #define EWOULDBLOCK WSAEWOULDBLOCK | In file included from natpmp.c:38: /usr/share/mingw-w64/include/errno.h:166: note: this is the location of the previous definition 166 | #define EWOULDBLOCK 140 | natpmp.c:43: warning: "ECONNREFUSED" redefined 43 | #define ECONNREFUSED WSAECONNREFUSED | In file included from natpmp.c:38: /usr/share/mingw-w64/include/errno.h:110: note: this is the location of the previous definition 110 | #define ECONNREFUSED 107 | natpmp.c:271:5: warning: ‘readnatpmpresponseorretry’ redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] 271 | int readnatpmpresponseorretry(natpmp_t * p, natpmpresp_t * response) | ^~~~~~~~~~~~~~~~~~~~~~~~~ ar crs libnatpmp.a natpmp.o getgateway.o make[1]: Leaving directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87' Staging libnatpmp... Postprocessing libnatpmp... Caching libnatpmp... ```

7af2502 build: compile libnatpmp with -DNATPMP_STATICLIB on Windows (fanquake) ee35745 build: use newer source for libnatpmp (fanquake) Pull request description: The source we are currently using is from 2015. The upstream repo has received a small number of bug fixes and improvements since then. Including one that fixes an issue for Windows users: miniupnp/libnatpmp#13. The source we are currently using is the most recent "official" release, however I don't think it's worth waiting for a new one. The maintainer was prompted to do so in Oct 2020, then again in Jan of this year, and no release has eventuated. Given libnatpmp is a new inclusion into our repository, I think we should be using this newer source. This also cleans up a few warnings we currently see in Windows depends builds: ```bash Extracting libnatpmp... /home/ubuntu/bitcoin/depends/sources/libnatpmp-20150609.tar.gz: OK Preprocessing libnatpmp... Configuring libnatpmp... Building libnatpmp... make[1]: Entering directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87' x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR -c -o natpmp.o natpmp.c x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR -c -o getgateway.o getgateway.c natpmp.c:42: warning: "EWOULDBLOCK" redefined 42 | #define EWOULDBLOCK WSAEWOULDBLOCK | In file included from natpmp.c:38: /usr/share/mingw-w64/include/errno.h:166: note: this is the location of the previous definition 166 | #define EWOULDBLOCK 140 | natpmp.c:43: warning: "ECONNREFUSED" redefined 43 | #define ECONNREFUSED WSAECONNREFUSED | In file included from natpmp.c:38: /usr/share/mingw-w64/include/errno.h:110: note: this is the location of the previous definition 110 | #define ECONNREFUSED 107 | natpmp.c:271:5: warning: ‘readnatpmpresponseorretry’ redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] 271 | int readnatpmpresponseorretry(natpmp_t * p, natpmpresp_t * response) | ^~~~~~~~~~~~~~~~~~~~~~~~~ ar crs libnatpmp.a natpmp.o getgateway.o make[1]: Leaving directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87' Staging libnatpmp... Postprocessing libnatpmp... Caching libnatpmp... ``` ACKs for top commit: hebasto: ACK 7af2502 Tree-SHA512: 6939014ea986149a5bfdd42b516d563a65ae643516e234579d3f28e7c2f877b0270cc4305ae7c7cb131d6d946a6e0aedc84b4cc880a412612a878a333398b9d7

The source we are currently using is from 2015. The upstream repo has received a small number of bug fixes and improvements since then. Including one that fixes an issue for Windows users: miniupnp/libnatpmp#13. The source we are currently using is the most recent "official" release, however I don't think it's worth waiting for a new one. The maintainer was prompted to do so in Oct 2020, then again in Jan of this year, and no release has eventuated. Given libnatpmp is a new inclusion into our repository, I think we should be using this newer source. This also cleans up a few warnings we currently see in depends builds: ```bash Extracting libnatpmp... /home/ubuntu/bitcoin/depends/sources/libnatpmp-20150609.tar.gz: OK Preprocessing libnatpmp... Configuring libnatpmp... Building libnatpmp... make[1]: Entering directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87' x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR -c -o natpmp.o natpmp.c x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR -c -o getgateway.o getgateway.c natpmp.c:42: warning: "EWOULDBLOCK" redefined 42 | #define EWOULDBLOCK WSAEWOULDBLOCK | In file included from natpmp.c:38: /usr/share/mingw-w64/include/errno.h:166: note: this is the location of the previous definition 166 | #define EWOULDBLOCK 140 | natpmp.c:43: warning: "ECONNREFUSED" redefined 43 | #define ECONNREFUSED WSAECONNREFUSED | In file included from natpmp.c:38: /usr/share/mingw-w64/include/errno.h:110: note: this is the location of the previous definition 110 | #define ECONNREFUSED 107 | natpmp.c:271:5: warning: ‘readnatpmpresponseorretry’ redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] 271 | int readnatpmpresponseorretry(natpmp_t * p, natpmpresp_t * response) | ^~~~~~~~~~~~~~~~~~~~~~~~~ ar crs libnatpmp.a natpmp.o getgateway.o make[1]: Leaving directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87' Staging libnatpmp... Postprocessing libnatpmp... Caching libnatpmp... ```

Summary: bitcoin/bitcoin@a8d9f27 (main commit) > net: Add libnatpmp support bitcoin/bitcoin@ae749d1 > doc: Add libnatpmp stuff Note: I excluded the changes in tor.md, as the documentation added in that file is not yet applicable until core#18077 is fully backported. I suggest we should backport this tohether with the release notes. --- [[bitcoin/bitcoin#21320 | core#21320]]: > build: fix libnatpmp macos cross compile > > Currently, our cross-compile of libnatpmp for macOS doesn't work at all. > The wrong archiver is used, which produces an archive the linker doesn't like. > > Fix this by using the right `ar` (we do the same for upnp). > > While we're at it, we fixe the build so that we are using our c/ppflags. > This means building with `-O2` rather than `-Os`. ---- [[bitcoin/bitcoin#21209 | core#21209]]: > build: compile libnatpmp with -DNATPMP_STATICLIB on Windows > > This fixes linking issues and mirrors what we do with miniupnpc. > build: use newer source for libnatpmp > > The source we are currently using is from 2015. The upstream repo has > received a small number of bug fixes and improvements since then. > Including one that fixes an issue for Windows users: > miniupnp/libnatpmp#13. > > The source we are currently using is the most recent "official" release, > however I don't think it's worth waiting for a new one. The maintainer > was prompted to do so in Oct 2020, then again in Jan of this year, and > no release has eventuated. Given libnatpmp is a new inclusion into our > repository, I think we should be using this newer source. > > This also cleans up a few warnings we currently see in depends builds ---- [[bitcoin/bitcoin#25917 | core#25917]]: > depends: libnatpmp 07004b97cf691774efebe70404cf22201e4d330d > > This pulls in two changes I've upstreamed: > Support for pkg-config: miniupnp/libnatpmp#19 > Suppressing a deprecation warning: miniupnp/libnatpmp#28 ---- This is a backport of [[bitcoin/bitcoin#18077 | core#18077]] [7 & 12/13], [[bitcoin/bitcoin#21320 | core#21320]], [[bitcoin/bitcoin#21209 | core#21209]] and [[bitcoin/bitcoin#25917 | core#25917]] Depends on D12094 Test Plan: ``` cmake .. -GNinja ninja ``` `@bot build-win64 build-osx` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12075

Use Winsock definition of EWOULDBLOCK rather than the POSIX one

2022f21

miniupnp merged commit cad5c36 into miniupnp:master Sep 24, 2020

fanquake mentioned this pull request Feb 17, 2021

build: use newer source for libnatpmp bitcoin/bitcoin#21209

Merged

Repository owner deleted a comment May 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Winsock definition for errno values rather than the POSIX ones #13

Use Winsock definition for errno values rather than the POSIX ones #13

cgutman commented Sep 28, 2019

Use Winsock definition for errno values rather than the POSIX ones #13

Use Winsock definition for errno values rather than the POSIX ones #13

Conversation

cgutman commented Sep 28, 2019