Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

'vquic/curl_msh3.c' and '-DCURL_DISABLE_SOCKETPAIR' #12213

Closed
gvanem opened this issue Oct 27, 2023 · 5 comments
Closed

'vquic/curl_msh3.c' and '-DCURL_DISABLE_SOCKETPAIR' #12213

gvanem opened this issue Oct 27, 2023 · 5 comments
Labels
build HTTP/3 h3 or quic related

Comments

@gvanem
Copy link
Contributor

gvanem commented Oct 27, 2023 •
edited

I did this

Building libcurl with -DCURL_DISABLE_SOCKETPAIR and -DUSE_MSH3, gave me this link error:
curl_msh3.obj : error LNK2019: unresolved external symbol Curl_socketpair referenced in function cf_msh3_connect

Perhaps the code should be:

--- a/vquic/curl_msh3.c 2023-10-26 09:20:23
+++ b/vquic/curl_msh3.c 2023-10-27 09:48:17
@@ -863,13 +863,15 @@

   CF_DATA_SAVE(save, cf, data);

+#if !defined(CURL_DISABLE_SOCKETPAIR)
   if(ctx->sock[SP_LOCAL] == CURL_SOCKET_BAD) {
-    if(Curl_socketpair(AF_UNIX, SOCK_STREAM, 0, &ctx->sock[0]) < 0) {
+    if(wakeup_create(&ctx->sock[0]) < 0) {
       ctx->sock[SP_LOCAL] = CURL_SOCKET_BAD;
       ctx->sock[SP_REMOTE] = CURL_SOCKET_BAD;
       return CURLE_COULDNT_CONNECT;
     }
   }
+#endif

   *done = FALSE;

since I see no other direct call to Curl_socketpair() and (MSH3 is for non-Windows too).

Not sure, but other places this is wrapped inside a #ifdef ENABLE_WAKEUP.

I expected the following

The .DLL to link.

curl/libcurl version

8.5.0-DEV

operating system

Win-10.
@gvanem gvanem changed the title vquic/curl_msh3.c' and '-DCURL_DISABLE_SOCKETPAIR' 'vquic/curl_msh3.c' and '-DCURL_DISABLE_SOCKETPAIR' Oct 27, 2023
@bagder bagder added build HTTP/3 h3 or quic related labels Oct 27, 2023
@bagder
Copy link
Member

bagder commented Oct 27, 2023

First: disabling the IPC communication with msh3 seriously cripples how it works and frankly, I am not convinced allowing this is a good idea. If you want to use msh3, you want it used. It might make sense to rather create an error that says so.

A complete fix to make this code use the wakeup_-macros instead of hardcoded socketpair also needs to usewakeup_write, wakeup_read and wakeup_close.

@gvanem
Copy link
Contributor Author

gvanem commented Oct 27, 2023

I am not convinced allowing this is a good idea.

This combo? Fine by me, but where should the error be generated?

@bagder
Copy link
Member

bagder commented Oct 27, 2023

This combo? Fine by me, but where should the error be generated?

I think maybe in msh3.c, because I believe it is msh3's design that makes socketpair/pipe necessary.

@icing
Copy link
Contributor

icing commented Nov 2, 2023

Something like this maybe?

--- a/lib/vquic/curl_msh3.c
+++ b/lib/vquic/curl_msh3.c
@@ -46,6 +46,10 @@
 #include "curl_memory.h"
 #include "memdebug.h"

+#ifdef CURL_DISABLE_SOCKETPAIR
+#error "MSH3 cannot be build with CURL_DISABLE_SOCKETPAIR set"
+#endif
+
 #define H3_STREAM_WINDOW_SIZE (128 * 1024)
 #define H3_STREAM_CHUNK_SIZE   (16 * 1024)
 #define H3_STREAM_RECV_CHUNKS \

@bagder
Copy link
Member

bagder commented Nov 2, 2023

@icing yup!

@icing icing mentioned this issue Nov 2, 2023
Closed
@bagder bagder closed this as completed in 5d0b3ed Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build HTTP/3 h3 or quic related
Milestone
No milestone
Development

No branches or pull requests
3 participants
@icing @bagder @gvanem