Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
- From
Ramsay Jones <ramsay@ramsayjones.plus.com>
- Date
- Jan 15, 2023, 21:37 UTC
- Message-ID
- <8f175d26-3d84-3019-031d-e358390f2de4@ramsayjones.plus.com>
- In-Reply-To
- <Y8ReHbGWetJHQcI1@coredump.intra.peff.net>
On 15/01/2023 20:12, Jeff King wrote:
Show 5 quoted lines
> The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was > deprecated in curl 7.85.0, and using it generate compiler warnings as of > curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we > can't just do so unilaterally, as it was only introduced less than a > year ago in 7.85.0.
Yep, I hadn't quite finished my version of this patch yet, but you would probably not be shocked to learn that I had two separate sets of functions #ifdef-ed by curl version number! What you have here looks *much* better!
Show 53 quoted lines
> > Until that version becomes ubiquitous, we have to either disable the > deprecation warning or conditionally use the "STR" variant on newer > versions of libcurl. This patch switches to the new variant, which is > nice for two reasons: > > - we don't have to worry that silencing curl's deprecation warnings > might cause us to miss other more useful ones > > - we'd eventually want to move to the new variant anyway, so this gets > us set up (albeit with some extra ugly boilerplate for the > conditional) > > There are a lot of ways to split up the two cases. One way would be to > abstract the storage type (strbuf versus a long), how to append > (strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use, > and so on. But the resulting code looks pretty magical: > > GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT; > if (...http is allowed...) > GIT_CURL_PROTOCOL_APPEND(&allowed, "http", CURLOPT_HTTP); > > and you end up with more "#define GIT_CURL_PROTOCOL_TYPE" macros than > actual code. > > On the other end of the spectrum, we could just implement two separate > functions, one that handles a string list and one that handles bits. But > then we end up repeating our list of protocols (http, https, ftp, ftp). > > This patch takes the middle ground. The run-time code is always there to > handle both types, and we just choose which one to feed to curl. > > Signed-off-by: Jeff King <peff@peff.net> > --- > git-curl-compat.h | 8 ++++++++ > http.c | 41 ++++++++++++++++++++++++++++++++++------- > 2 files changed, 42 insertions(+), 7 deletions(-) > > diff --git a/git-curl-compat.h b/git-curl-compat.h > index 56a83b6bbd..fd96b3cdff 100644 > --- a/git-curl-compat.h > +++ b/git-curl-compat.h > @@ -126,4 +126,12 @@ > #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS > #endif > > +/** > + * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0, > + * released in August 2022. > + */ > +#if LIBCURL_VERSION_NUM >= 0x075500 > +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1 > +#endif
Ah, I haven't really grokked what this file is about ... but this looks simple enough. ;)
Show 57 quoted lines
> +
> #endif
> diff --git a/http.c b/http.c
> index ca0fe80ddb..e529ebc993 100644
> --- a/http.c
> +++ b/http.c
> @@ -764,18 +764,29 @@ void setup_curl_trace(CURL *handle)
> curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
> }
>
> -static long get_curl_allowed_protocols(int from_user)
> +static void proto_list_append(struct strbuf *list_str, const char *proto_str,
> + long *list_bits, long proto_bits)
> +{
> + *list_bits |= proto_bits;
> + if (list_str) {
> + if (list_str->len)
> + strbuf_addch(list_str, ',');
> + strbuf_addstr(list_str, proto_str);
> + }
> +}
> +
> +static long get_curl_allowed_protocols(int from_user, struct strbuf *list)
> {
> long allowed_protocols = 0;
>
> if (is_transport_allowed("http", from_user))
> - allowed_protocols |= CURLPROTO_HTTP;
> + proto_list_append(list, "http", &allowed_protocols, CURLPROTO_HTTP);
> if (is_transport_allowed("https", from_user))
> - allowed_protocols |= CURLPROTO_HTTPS;
> + proto_list_append(list, "https", &allowed_protocols, CURLPROTO_HTTPS);
> if (is_transport_allowed("ftp", from_user))
> - allowed_protocols |= CURLPROTO_FTP;
> + proto_list_append(list, "ftp", &allowed_protocols, CURLPROTO_FTP);
> if (is_transport_allowed("ftps", from_user))
> - allowed_protocols |= CURLPROTO_FTPS;
> + proto_list_append(list, "ftps", &allowed_protocols, CURLPROTO_FTPS);
>
> return allowed_protocols;
> }
> @@ -921,10 +932,26 @@ static CURL *get_curl_handle(void)
>
> curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
> curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
> +
> +#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
> + {
> + struct strbuf buf = STRBUF_INIT;
> +
> + get_curl_allowed_protocols(0, &buf);
> + curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);
> + strbuf_reset(&buf);
> +
> + get_curl_allowed_protocols(-1, &buf);
> + curl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);
> + strbuf_release(&buf);I used two static char arrays to accumulate the strings before passing them to curl. I was unsure of the lifetime/ownership semantics - I still haven't got around to looking them up!
Show 13 quoted lines
> + }
> +#else
> curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> - get_curl_allowed_protocols(0));
> + get_curl_allowed_protocols(0, NULL));
> curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> - get_curl_allowed_protocols(-1));
> + get_curl_allowed_protocols(-1, NULL));
> +#endif
> +
> if (getenv("GIT_CURL_VERBOSE"))
> http_trace_curl_no_data();
> setup_curl_trace(result);(another reason for not completing these patches - I don't know what the test coverage is like for these changes; are more tests required? dunno).
For what it's worth, this LGTM.
Thanks!
ATB, Ramsay Jones