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

From: Leslie Cheng <leslie.cheng5@gmail.com>
Date: Feb 23, 2024, 22:24 UTC
Message-ID: <4dfca119-3210-4574-998b-0bee53e61464@gmail.com>
In-Reply-To: <xmqq5xyfyyn0.fsf@gitster.g>

On 2024-02-23 12:37 a.m., Junio C Hamano wrote:

Show 14 quoted lines

> 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.

Show 6 quoted lines

> 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.

Show 8 quoted lines

> 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.

Show 17 quoted lines

> 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)

Show 6 quoted lines

>> +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.

Show 5 quoted lines

> 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.