| Message ID | 20240508124453.600871-3-toon@iotcl.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bundle-uri: show progress when downloading from bundle URIs | expand |
On Wed, May 8, 2024 at 8:45 AM Toon Claes <toon@iotcl.com> wrote: > Add an option `progress` to `struct http_get_options` to allow the > caller to enable download progress using the progress.c API. > > Signed-off-by: Toon Claes <toon@iotcl.com> > --- > diff --git a/http.c b/http.c > @@ -2061,6 +2081,13 @@ static int http_request(const char *url, > + if (options && options->progress) { > + progress = start_progress(_("Downloading via HTTP"), 0); > + > + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 0L); > + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, progress); > + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, &http_progress_callback); > + } > @@ -2079,6 +2106,11 @@ static int http_request(const char *url, > + if (progress) { > + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); > + stop_progress(&progress); > + } The changes thus far in the series all seem very straightforward. Nicely done. Can you explain to this reviewer why you only reset CURLOPT_XFERINFODATA here, but not CURLOPT_NOPROGRESS and CURLOPT_XFERINFOFUNCTION?
On Wed, May 08, 2024 at 02:44:51PM +0200, Toon Claes wrote: > @@ -1457,6 +1458,9 @@ struct active_request_slot *get_active_slot(void) > curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); > curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); > curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL); > + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 1L); > + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); > + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, NULL); These last two CURLOPTs appeared in 7.32.0, but our INSTALL doc claims to support back to 7.21.3. Before that you're supposed to use PROGRESSFUNCTION instead, which has a slightly different signature. I think you could support both, though it would also be OK to just disable this extra progress for antique curl. It might also be reasonable to just bump to 7.32.0 as our minimum. The last bump was recent via c28ee09503 (INSTALL: bump libcurl version to 7.21.3, 2024-04-02), and the version picked there was arbitrary-ish (it was something we had happened to depend on accidentally). 7.32.0 is itself almost 11 years old now. -Peff
Jeff King <peff@peff.net> writes: > On Wed, May 08, 2024 at 02:44:51PM +0200, Toon Claes wrote: > >> @@ -1457,6 +1458,9 @@ struct active_request_slot *get_active_slot(void) >> curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); >> curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); >> curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL); >> + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 1L); >> + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); >> + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, NULL); > > These last two CURLOPTs appeared in 7.32.0, but our INSTALL doc claims > to support back to 7.21.3. Before that you're supposed to use > PROGRESSFUNCTION instead, which has a slightly different signature. I > think you could support both, though it would also be OK to just disable > this extra progress for antique curl. > > It might also be reasonable to just bump to 7.32.0 as our minimum. The > last bump was recent via c28ee09503 (INSTALL: bump libcurl version to > 7.21.3, 2024-04-02), and the version picked there was arbitrary-ish (it > was something we had happened to depend on accidentally). 7.32.0 is > itself almost 11 years old now. The last bump was 7.19.5 (May 2009, 14.9 years) to 7.21.3 (Dec 2010, 13.3 years). As 10 is a nice round number, we may even be able to pick randomly a slightly newer one, say, 7.35.0 (Mar 2014, 10.0 years). It is in a sense an inferiour way to pick the minimum dependency than the choice of 7.32.0, which is backed by "we use this and that, which appeared in that version", of course. But being able to update mechanically without thinking is tempting, and as long as the horizon is sufficiently long, such an approach would not have a huge downside. Thanks.
On Wed, May 08, 2024 at 02:44:51PM +0200, Toon Claes wrote: > @@ -2017,6 +2021,21 @@ static void http_opt_request_remainder(CURL *curl, off_t pos) > #define HTTP_REQUEST_STRBUF 0 > #define HTTP_REQUEST_FILE 1 > > +static int http_progress_callback(void *clientp, curl_off_t dltotal, > + curl_off_t dlnow, curl_off_t ultotal, > + curl_off_t ulnow) > +{ > + struct progress *progress = clientp; > + > + if (progress) { > + progress_set_total(progress, dltotal); > + display_progress(progress, dlnow); > + display_throughput(progress, dlnow); > + } > + > + return 0; > +} A very long time ago I implemented something similar (for a proto-bundle-uri feature), and I found out the hard way that both curl and our progress code may rely on SIGALRM (curl may use it to put a timeout on blocking calls that don't support select, like DNS resolution). Making things even more confusing, it's platform dependent, since on modern platforms it spawns a separate resolving thread to avoid blocking the main one. So it seems to work OK for me on Linux without anything further, but we may want this to be extra careful: -- >8 -- Date: Thu, 10 Nov 2011 01:29:55 -0500 Subject: [PATCH] http: turn off curl signals Curl sets and clears the handler for SIGALRM, which makes it incompatible with git's progress code. However, we can ask curl not to do this. Signed-off-by: Jeff King <peff@peff.net> --- http.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http.c b/http.c index 752c879c1f..e7986e1353 100644 --- a/http.c +++ b/http.c @@ -1230,6 +1230,8 @@ static CURL *get_curl_handle(void) set_curl_keepalive(result); + curl_easy_setopt(result, CURLOPT_NOSIGNAL, 1); + return result; } -- 8< -- In the other part of the thread I suggested the possibility of having remote-https shuttle machine-readable progress back to the caller. That would also solve this issue, because the actual display_progress() calls would happen in the parent. -Peff
On Thu, May 09, 2024 at 09:51:51AM -0700, Junio C Hamano wrote: > > It might also be reasonable to just bump to 7.32.0 as our minimum. The > > last bump was recent via c28ee09503 (INSTALL: bump libcurl version to > > 7.21.3, 2024-04-02), and the version picked there was arbitrary-ish (it > > was something we had happened to depend on accidentally). 7.32.0 is > > itself almost 11 years old now. > > The last bump was 7.19.5 (May 2009, 14.9 years) to 7.21.3 (Dec 2010, > 13.3 years). As 10 is a nice round number, we may even be able to > pick randomly a slightly newer one, say, 7.35.0 (Mar 2014, 10.0 > years). > > It is in a sense an inferiour way to pick the minimum dependency > than the choice of 7.32.0, which is backed by "we use this and that, > which appeared in that version", of course. I am OK with just using round numbers, too. The biggest thing for a time-oriented scheme is really what made the cutoff for long-term distro releases. With a 10-year support term, we are often looking at 11 or so years to have made it into a release. OTOH, if somebody using a 10-year-old distro is forced to use a slightly older version of Git to match, IMHO that is not the end of the world. Bumping to 7.35.0 would also let us simplify some curl-compat code, which is always my real goal. ;) -Peff
diff --git a/http.c b/http.c index 3d80bd6116..15c5d53712 100644 --- a/http.c +++ b/http.c @@ -10,6 +10,7 @@ #include "credential.h" #include "version.h" #include "pkt-line.h" +#include "progress.h" #include "gettext.h" #include "trace.h" #include "transport.h" @@ -1457,6 +1458,9 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL); + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, NULL); /* * Default following to off unless "ALWAYS" is configured; this gives @@ -2017,6 +2021,21 @@ static void http_opt_request_remainder(CURL *curl, off_t pos) #define HTTP_REQUEST_STRBUF 0 #define HTTP_REQUEST_FILE 1 +static int http_progress_callback(void *clientp, curl_off_t dltotal, + curl_off_t dlnow, curl_off_t ultotal, + curl_off_t ulnow) +{ + struct progress *progress = clientp; + + if (progress) { + progress_set_total(progress, dltotal); + display_progress(progress, dlnow); + display_throughput(progress, dlnow); + } + + return 0; +} + static int http_request(const char *url, void *result, int target, const struct http_get_options *options) @@ -2025,6 +2044,7 @@ static int http_request(const char *url, struct slot_results results; struct curl_slist *headers = http_copy_default_headers(); struct strbuf buf = STRBUF_INIT; + struct progress *progress = NULL; const char *accept_language; int ret; @@ -2061,6 +2081,13 @@ static int http_request(const char *url, if (options && options->initial_request && http_follow_config == HTTP_FOLLOW_INITIAL) curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1); + if (options && options->progress) { + progress = start_progress(_("Downloading via HTTP"), 0); + + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 0L); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, progress); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, &http_progress_callback); + } headers = curl_slist_append(headers, buf.buf); @@ -2079,6 +2106,11 @@ static int http_request(const char *url, ret = run_one_slot(slot, &results); + if (progress) { + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); + stop_progress(&progress); + } + if (options && options->content_type) { struct strbuf raw = STRBUF_INIT; curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, &raw); diff --git a/http.h b/http.h index 3af19a8bf5..37ecddec17 100644 --- a/http.h +++ b/http.h @@ -146,6 +146,11 @@ struct http_get_options { * request has completed. */ struct string_list *extra_headers; + + /* + * If not zero, display the progress. + */ + int progress; }; /* Return values for http_get_*() */
Add an option `progress` to `struct http_get_options` to allow the caller to enable download progress using the progress.c API. Signed-off-by: Toon Claes <toon@iotcl.com> --- http.c | 32 ++++++++++++++++++++++++++++++++ http.h | 5 +++++ 2 files changed, 37 insertions(+)