[pacman-dev] [PATCH 0/2] Minor tweaks to download code
Just a couple fixes for the download code. I'm not entirely convinced that the values hard coded in patch 2/2 are appropriate, so I'll say that its up for discussion. I know that we have folks who insist on using dial-up, so we need to be aware of this as well. Based on the source of curl, it seems to be a comparison of the average speed over the life of the transfer against the low speed limit, so this would actually be elongated on larger transfers. Regardless, this should address bug reports such as FS#23690 and FS#15369. dave
If a connection drops below 10kb/s for 10s, curl will kill the transfer and we'll report failure. Signed-off-by: Dave Reisner <d@falconindy.com> --- lib/libalpm/dload.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index d024c73..3d90130 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -194,6 +194,8 @@ static int curl_download_internal(const char *url, const char *localpath, curl_easy_setopt(handle->curl, CURLOPT_FOLLOWLOCATION, 1L); curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)&dlfile); + curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 10240L); + curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); useragent = getenv("HTTP_USER_AGENT"); if(useragent != NULL) { -- 1.7.5.2
On Wed, Jun 1, 2011 at 2:24 PM, Dave Reisner <d@falconindy.com> wrote:
If a connection drops below 10kb/s for 10s, curl will kill the transfer and we'll report failure.
Yeah, the sad catch here is even 56K modems would max out at 56,600 bps == 7,000 B/sec == 6.84 KB/sec, which is below the 10K cutoff. From the manpage which uses these opts directly: If a download is slower than speed-limit bytes per second during a speed-time period, the download gets aborted. If speed-time is used, the default speed-limit will be 1 unless set with -Y. So it seems like it is only looked at over the limit time, not the entire length of the download as you mentioned elsewhere. The code in lib/speedcheck.c seems to confirm this. So I guess I'd advocate for a value below what a 28.8 K modem would give, just so you don't frustrate someone trying to download a small package over dial-up more than necessary. Something like 1200 would put it even below the 14.4K mark, and yet with the limit of 10 seconds would probably still do a good job here.
Signed-off-by: Dave Reisner <d@falconindy.com> --- lib/libalpm/dload.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index d024c73..3d90130 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -194,6 +194,8 @@ static int curl_download_internal(const char *url, const char *localpath, curl_easy_setopt(handle->curl, CURLOPT_FOLLOWLOCATION, 1L); curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)&dlfile); + curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 10240L); + curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L);
useragent = getenv("HTTP_USER_AGENT"); if(useragent != NULL) { -- 1.7.5.2
Callers to curl_download_internal now tell us if its okay to continue a transfer, so obey this instead of using a heuristic. Signed-off-by: Dave Reisner <d@falconindy.com> --- lib/libalpm/dload.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 3d90130..c79e5d2 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -202,10 +202,8 @@ static int curl_download_internal(const char *url, const char *localpath, curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); } - /* TODO: no assuming here. the calling function should tell us what's kosher */ - if(!force && stat(destfile, &st) == 0) { - /* assume its a sync, so we're starting from scratch. but, only download - * our local is out of date. */ + if(!force && stat(destfile, &st) == 0 && !allow_resume) { + /* start from scratch, but only download our local is out of date. */ curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); } else if(stat(tempfile, &st) == 0 && allow_resume) { -- 1.7.5.2
On Wed, Jun 1, 2011 at 2:24 PM, Dave Reisner <d@falconindy.com> wrote:
Callers to curl_download_internal now tell us if its okay to continue a transfer, so obey this instead of using a heuristic.
Signed-off-by: Dave Reisner <d@falconindy.com> --- lib/libalpm/dload.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 3d90130..c79e5d2 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -202,10 +202,8 @@ static int curl_download_internal(const char *url, const char *localpath, curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); }
- /* TODO: no assuming here. the calling function should tell us what's kosher */ - if(!force && stat(destfile, &st) == 0) { - /* assume its a sync, so we're starting from scratch. but, only download - * our local is out of date. */ + if(!force && stat(destfile, &st) == 0 && !allow_resume) { + /* start from scratch, but only download our local is out of date. */ "only download if our" ? Missing a word here. Otherwise patch looks good, although I'd check both of the simple things before doing the stat. !force && !allow_resume && stat()
curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); } else if(stat(tempfile, &st) == 0 && allow_resume) { -- 1.7.5.2
participants (2)
-
Dan McGee
-
Dave Reisner