remove requirement on HAVE_SYS_UN_H for unix domain sockets on Windows #7737

mzr · 2021-09-17T12:57:38Z

As described in #5162 curl with option --unix-socket on Windows was failing with:

* sa_addr inet_ntop() failed with errno 10047: Address family not supported

It happened because in Curl_addr2string's switch statement, case with AF_UNIX was behind define(HAVE_SYS_UN_H) && defined(AF_UNIX). There is no sys/un.h on Windows. On Windows sockaddr_un is defined in afunix.h (not in sys/un.h as it's on unix-like OSs). After this change, curl with --unix-socket worked for me as expected.

I'm not sure if this is the proper way to fix this, so I'd welcome some feedback. Also, I'm counting on tests that should run on this PR to see if things are OK.

bagder

👍

bagder · 2021-09-17T15:49:54Z

It seems to not be quite true everywhere:

connect.c: In function 'Curl_addr2string':
connect.c:663:48: error: invalid use of undefined type 'struct sockaddr_un'
  663 |         msnprintf(addr, MAX_IPADR_LEN, "%s", su->sun_path);
      |                                                ^~

mzr · 2021-09-19T10:31:04Z

I'll take a look into that. I'm guessing there is a mismatch between different toolchains/compilers? I'm not very familiar with Windows world :/ Also I might have had wrong compilation options (or just missing something) when I was compiling it myself on my machine - I was just using this as is. However, I think this might be unlikely as HAVE_SYS_UN_H seems like it's required for unix sockets to work and doesn't seem to have much sense on Windows.

gvanem · 2021-09-19T11:39:01Z

I'm guessing there is a mismatch between different toolchains/compilers?

The MinGW I have (TDM-gcc 8.0.0) have AF_UNIX defined in it's winsock*.h. But no struct sockaddr_un anywhere.
So maybe this somewhere:

#if !defined(HAVE_SOCKADDR_UN)
struct sockaddr_un {
       sa_family_t sun_family;   
       char        sun_path [200];
     };
#endif

mzr · 2021-09-19T22:15:45Z

Hmm, I see there is something similar already in the code here. Introduced in
#7034

mzr · 2021-09-19T23:18:53Z

Ahh this doesn't seem to do the trick as well. I think it might work but only with cmake build.

mzr · 2021-09-20T00:07:51Z

I added a little change, so it should now work not only with cmake.

lib/connect.c

mzr · 2021-09-29T06:43:24Z

I changed it to something much simpler. It should work for all cases and builds where it previously worked and for cases where sockaddr_un is manually defined on Windows in curl_setup.h. Might not be perfect, but it's still an improvement.
Tests that are failing I believe are not related.

mzr · 2021-09-29T10:27:43Z

rebased

bagder · 2021-09-30T10:28:11Z

Thanks!

Closes curl#7737

Summary: Vendoring this patch: curl/curl#7737 to curl-sys rust crate. On windows the hg client is using curl that's bundled with sys-curl. I need this patch to have unix domain sockets working in hg client on windows. I had to manually vendor https://raw.githubusercontent.com/mzr/curl/57e7ec4dbe4dd2831de51f2644879387d2ea7b44/docs/INSTALL because reindeer didn't do it. IDK why. oss-eden-{darwin,linux,windows}-getdeps fail with: ``` FAILED: eden/scm/lib/backingstore/CMakeFiles/rust_backingstore.cargo eden/scm/lib/backingstore/debug/libbackingstore.a eden/scm/lib/backingstore/release/libbackingstore.a cd /data/sandcastle/temp/fbcode_builder_getdeps/shipit/eden/eden/scm/lib/backingstore && /data/sandcastle/temp/fbcode_builder_getdeps/installed/cmake-hQhVzQT-WzFKTeqXjLxo5lLi8IG4_MjX2-YRqptCUVs/bin/cmake -E remove -f /data/sandcastle/temp/fbcode_builder_getdeps/shipit/eden/eden/scm/lib/backingstore/Cargo.lock && /data/sandcastle/temp/fbcode_builder_getdeps/installed/cmake-hQhVzQT-WzFKTeqXjLxo5lLi8IG4_MjX2-YRqptCUVs/bin/cmake -E env CARGO_TARGET_DIR=/data/sandcastle/temp/fbcode_builder_getdeps/build/eden/eden/scm/lib/backingstore CARGO_HOME=/data/sandcastle/temp/fbcode_builder_getdeps/build/eden/_cargo_home cargo build --release -p backingstore --features fb Blocking waiting for file lock on package cache Blocking waiting for file lock on package cache error: failed to calculate checksum of: /data/sandcastle/boxes/eden-trunk-hg-fbcode-fbsource/third-party/rust/vendor/curl-sys-0.4.45+curl-7.78.0/curl/docs/INSTALL Caused by: failed to open file `/data/sandcastle/boxes/eden-trunk-hg-fbcode-fbsource/third-party/rust/vendor/curl-sys-0.4.45+curl-7.78.0/curl/docs/INSTALL` Caused by: No such file or directory (os error 2) ``` Not idea how to fix this. Seems related to the fact that reindeer didn't vendor docs/INSTALL. # EDIT: # It might been caused by some bug in hg. now it's fine # The failure in oss-eden-linux-getdeps looks unrelated (something with rocksdb) Reviewed By: krallin Differential Revision: D31370778 fbshipit-source-id: a1245f8cb6b58f5765e34c95dfd78325a8e6e457

Summary: Bumping versions because I need this patch: curl/curl#7737. It's now in the 0.4.41 version of curl-rust and 0.4.51 version of curl-sys. I previously added it here D31370778 (083a9ad), but it somehow got removed here D32340068 (3d32723). Reviewed By: jsgf Differential Revision: D32791487 fbshipit-source-id: 8d7efaa3713b5cfa65b81d85d8d5f68bc8b8435a

bagder approved these changes Sep 17, 2021

View reviewed changes

bagder added build Windows Windows-specific labels Sep 17, 2021

mzr force-pushed the windows-unix-socket branch from 73bec01 to 23e31db Compare September 19, 2021 23:08

mzr requested a review from bagder September 20, 2021 09:06

bagder requested changes Sep 21, 2021

View reviewed changes

lib/connect.c Outdated Show resolved Hide resolved

mzr force-pushed the windows-unix-socket branch from 757c00b to 429917b Compare September 28, 2021 23:14

get sysaddr_un fron sys/un.h or just define it in the case of windows

584b7e6

mzr force-pushed the windows-unix-socket branch from 429917b to 584b7e6 Compare September 29, 2021 10:26

mzr requested a review from bagder September 29, 2021 13:18

bagder approved these changes Sep 30, 2021

View reviewed changes

bagder closed this in 0fe9018 Sep 30, 2021

mzr added a commit to mzr/curl that referenced this pull request Oct 2, 2021

connnect: use sysaddr_un fron sys/un.h or custom-defined for windows

57e7ec4

Closes curl#7737

mzr deleted the windows-unix-socket branch October 2, 2021 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove requirement on HAVE_SYS_UN_H for unix domain sockets on Windows #7737

remove requirement on HAVE_SYS_UN_H for unix domain sockets on Windows #7737

mzr commented Sep 17, 2021

bagder left a comment

bagder commented Sep 17, 2021 •

edited

mzr commented Sep 19, 2021

gvanem commented Sep 19, 2021

mzr commented Sep 19, 2021 •

edited

mzr commented Sep 19, 2021

mzr commented Sep 20, 2021

mzr commented Sep 29, 2021

mzr commented Sep 29, 2021

bagder commented Sep 30, 2021

remove requirement on HAVE_SYS_UN_H for unix domain sockets on Windows #7737

remove requirement on HAVE_SYS_UN_H for unix domain sockets on Windows #7737

Conversation

mzr commented Sep 17, 2021

bagder left a comment

Choose a reason for hiding this comment

bagder commented Sep 17, 2021 • edited

mzr commented Sep 19, 2021

gvanem commented Sep 19, 2021

mzr commented Sep 19, 2021 • edited

mzr commented Sep 19, 2021

mzr commented Sep 20, 2021

mzr commented Sep 29, 2021

mzr commented Sep 29, 2021

bagder commented Sep 30, 2021

bagder commented Sep 17, 2021 •

edited

mzr commented Sep 19, 2021 •

edited