[PATCH v2 0/1] MR3403: server: Revert "server: Avoid using SOL_IPX to detect whether IPX is supported."
This reverts commit 41cc117b3f37ab4b9b4ac8a815cd2a496d38fb4b. This commit broke musl support. With the changes introduced, the following code evaluates to the #else branch without checking for SO_DEFAULT_HEADERS resulting in an error. server/sock.c +1888 ``` \#ifdef HAS_IPX int ipx_type = protocol - WS_NSPROTO_IPX; \#ifdef SOL_IPX setsockopt( sockfd, SOL_IPX, IPX_TYPE, &ipx_type, sizeof(ipx_type) ); \#else struct ipx val; /* Should we retrieve val using a getsockopt call and then * set the modified one? */ val.ipx_pt = ipx_type; setsockopt( sockfd, 0, SO_DEFAULT_HEADERS, &val, sizeof(val) ); \#endif \#endif ``` I propose reverting the commit, but we could probably alter the macro in the code section provided to match the changes relevant github issue: https://github.com/void-linux/void-packages/pull/45202 Signed-off-by: Fotios Valasiadis <fvalasiad(a)gmail.com> -- v2: server: Check for SO_DEFAULT_HEADERS before using it. https://gitlab.winehq.org/wine/wine/-/merge_requests/3403
From: Fotios Valasiadis <fvalasiad(a)gmail.com> Not doing so after commit 41cc117b broke musl libc compatibility. Tested the patch in my void 64bit musl install and it compiled properly. Signed-off-by: Fotios Valasiadis <fvalasiad(a)gmail.com> --- server/sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/sock.c b/server/sock.c index 550fe61e477..591876aab33 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1880,7 +1880,7 @@ static int init_socket( struct sock *sock, int family, int type, int protocol ) #ifdef SOL_IPX setsockopt( sockfd, SOL_IPX, IPX_TYPE, &ipx_type, sizeof(ipx_type) ); -#else +#elif defined(SO_DEFAULT_HEADERS) struct ipx val; /* Should we retrieve val using a getsockopt call and then * set the modified one? */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3403
On Tue Jul 25 13:38:36 2023 +0000, Jinoh Kang wrote:
Sounds great. That'll also make `server/sock.c` consistent with `dlls/ntdll/unix/socket.c` in terms of `SO_DEFAULT_HEADERS` usage. @iamahuman done! Let me know for further changes requested.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3403#note_40162
This merge request was approved by Jinoh Kang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3403
On Tue Jul 25 14:02:22 2023 +0000, Fotios Valasiadis wrote:
@iamahuman done! Let me know for further changes requested. Looks good to me.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3403#note_40185
Except that if we can't set the IPX type we shouldn't pretend to support IPX. So musl exposes IPX header files but not SOL_IPX or SO_DEFAULT_HEADERS (for some reason), and bionic doesn't have header files but still exposes SOL_IPX and SO_DEFAULT_HEADERS. I really wish that if these libraries are going to remove support for IPX they'd do so completely, rather than in halves. Perhaps we should do something like v1 of 2965, and check for a different constant? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3403#note_40205
Except that if we can't set the IPX type we shouldn't pretend to support IPX.
If we're really going to advertise IPX support only if the system supports, then we'll have to stick to runtime probe, since `SOL_IPX` may in fact not be supported by the kernel even if IPX definitions exists in the headers. In fact, *any* address family may or may not be supported by the underlying kernel if the support is compiled out.
So musl exposes IPX header files but not SOL_IPX or SO_DEFAULT_HEADERS (for some reason), and bionic doesn't have header files but still exposes SOL_IPX and SO_DEFAULT_HEADERS. I really wish that if these libraries are going to remove support for IPX they'd do so completely, rather than in halves.
FYI, no Linux header files seem to declare `SO_DEFAULT_HEADERS`; I couldn't find it in Linux kernel, glibc, bionic, or musl. It looks like a BSD thing.
Perhaps we should do something like v1 of 2965, and check for a different constant?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3403#note_40517
I've submitted an alternative approach: https://gitlab.winehq.org/wine/wine/-/merge_requests/3428. Does this work? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3403#note_40530
On Thu Jul 27 16:21:32 2023 +0000, Jinoh Kang wrote:
I've submitted an alternative approach: https://gitlab.winehq.org/wine/wine/-/merge_requests/3428. Does this work? Tested it on musl and it compiled as expected.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3403#note_40545
This merge request was closed by Fotios Valasiadis. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3403
Since !3428 has been merged, providing an alternative fix, I am closing this pull request. Pleasure working with ya! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3403#note_42792
participants (4)
-
Fotios Valasiadis -
Fotios Valasiadis (＠fvalasiad)
-
Jinoh Kang (＠iamahuman)
-
Zebediah Figura (＠zfigura)