Re: [PATCH v2] Add unix domain socket support to HTTP transport

[Leslie Cheng <leslie.cheng5@gmail.com>]

On 2024-02-23 12:37 a.m., Junio C Hamano wrote:
> How about following that convention, perhaps like:
>
>     In some corporate environments, the proxy server listens to a
>     local unix domain socket for requests, instead of listening to a
>     network port.  Even though we have http.proxy (and more
>     destination specific http.<url>.proxy) configuration variables
>     to specify the network address/port of a proxy, that would not
>     help if your proxy does not listen to the network.
>
>     Introduce an `http.unixSocket` (and `http.<url>.unixSocket`)
>     configuration variables that specify the path to a unix domain
>     socket for such a proxy.  Recent versions of libcURL library
>     added CURLOPT_UNIX_SOCKET_PATH to support "curl --unix-socket
>     <path>"---use the same mechanism to implement it.

This is excellent, thanks for the guidance (and all the other
suggestions prior)! I'll update in the next patch.


> Unlike NO_UNIX_SOCKETS, GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH is
> entirely internal to your implementation and not surfaced to neither
> the end-users or the binary packagers.  Because of that, I suspect
> that any description that has to use that name probably falls on the
> other side of "too much implementation details" to be useful to help
> future developers..

That's reasonable, I figured it would fit as a cover letter detail but I
agree it's not relevant as a commit message that lives in the history of
the project. I'll also update this in the next patch.


> Talking about precedence between this and http.proxy is good thing,
> but one very important piece of information is missing.  What value
> does it take?
>
> 	The absolute path of a unix-domain socket to pass the HTTP
> 	traffic over, instead of using the network.
>
> or something, perhaps?

I like that wording, I'll update in the next patch.


> It might make the code easier to follow if you did:
>
> 	#if !defined(NO_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
> 	#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH)
>         #define USE_CURLOPT_UNIX_SOCKET_PATH
> 	#endif
> 	#endif
>
> The points are
>
>  (1) the users can decline to use CURLOPT_UNIX_SOCKET_PATH while
>      still using unix domain socket for other purposes, and
>
>  (2) you do not have to care if you HAVE it or not most of time;
>      what matters more often is if the user told you to USE it.
>
> Hmm?

Do you think this functionality is worth adding another macro to
conditionally include it in the build? It felt out-of-the-way enough
that we could just use the same `NO_UNIX_SOCKETS` macro to control for
environments that don't support unix domain sockets.


>> +#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) &&
!defined(NO_UNIX_SOCKETS)
>> +static const char *curl_unix_socket_path;
>> +#endif
>
> The guard here would become "#ifdef USE_CURLOPT_UNIX_SOCKET_PATH" if
> we wanted this to be conditional, but I think it is easier to make
> the variable unconditionally available; see below.

Agreed in general, I was looking to other patterns for conditional
variables in file, e.g.
https://github.com/git/git/blob/3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0/http.c#L66-L68

But, revisiting, this looks like an exception rather than the norm.


> In general, it is inadvisable to issue a warning in the codepath
> that parses configuration variables, as the values we read may not
> be necessarily used.  We could instead accept the given path into a
> variable unconditionally, and complain only before it gets used,
> near the call to curl_easy_setopt().

Similar to above, I followed what was already done for certain
configuration variables (e.g.
https://github.com/git/git/blob/3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0/http.c#L485-L501),
but I agree with your feedback since this would result in constant warnings.


To summarize, I'll do the following in the next patch:
 - change the wording of the commit message
 - move the conditional variables and parsing to a check at
`curl_easy_setopt()` time

I'm still undecided on whether I should introduce another macro
specifically for this functionality, and I'd like to hear your thoughts
on why it might make sense.