[pacman-dev] [PATCH v2] libalpm: fix download rates becoming negative
Allan McRae
allan at archlinux.org
Sun May 9 12:46:09 UTC 2021
On 3/5/21 1:41 pm, morganamilo wrote:
> When a download fails on one mirror a new download is started on the
> next mirror. This causes the ammount downloaded to reset, confusing the
> rate math and making it display a negative rate.
>
> This is further complicated by the fact that a download may be resumed
> from where it is or started over.
>
> To account for this we alert the frontend that the download was
> restarted. Pacman then starts the progress bar over.
>
> ---
>
> v2: stat tempfile to get initial_size
> ---
> lib/libalpm/alpm.h | 7 +++++++
> lib/libalpm/dload.c | 33 +++++++++++++++++++++++++--------
> src/pacman/callback.c | 30 ++++++++++++++++++++++++++++++
> 3 files changed, 62 insertions(+), 8 deletions(-)
>
> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 833df829..e9cad015 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -1169,6 +1169,8 @@ typedef enum _alpm_download_event_type_t {
> ALPM_DOWNLOAD_INIT,
> /** A download made progress */
> ALPM_DOWNLOAD_PROGRESS,
> + /** Download will be retried */
> + ALPM_DOWNLOAD_RETRY,
> /** A download completed */
> ALPM_DOWNLOAD_COMPLETED
> } alpm_download_event_type_t;
OK.
> @@ -1187,6 +1189,11 @@ typedef struct _alpm_download_event_progress_t {
> off_t total;
> } alpm_download_event_progress_t;
>
> +/** Context struct for when a download retries. */
> +typedef struct _alpm_download_event_retry_t {
> + /** If the download will resume or start over */
> + int resume;
> +} alpm_download_event_retry_t;
>
> /** Context struct for when a download completes. */
> typedef struct _alpm_download_event_completed_t {
OK.
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index a4c42f8d..5beea1d9 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -412,6 +412,7 @@ static int curl_retry_next_server(CURLM *curlm, CURL *curl, struct dload_payload
> {
> const char *server;
> size_t len;
> + struct stat st;
> alpm_handle_t *handle = payload->handle;
>
> while(payload->servers && should_skip_server(handle, payload->servers->data)) {
> @@ -431,15 +432,31 @@ static int curl_retry_next_server(CURLM *curlm, CURL *curl, struct dload_payload
> MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
>
> - if(payload->unlink_on_fail) {
> +
> + fflush(payload->localf);
> +
> + if(stat(payload->tempfile_name, &st) == 0 && payload->allow_resume) {
No point doing the stat unless we are resuming.
if(payload->allow_resume) {
if(stat(payload....)
> + /* a previous partial download exists, resume from end of file. */
> + payload->tempfile_openmode = "ab";
> + curl_easy_setopt(curl, CURLOPT_RESUME_FROM_LARGE, (curl_off_t)st.st_size);
> + _alpm_log(handle, ALPM_LOG_DEBUG,
> + "%s: tempfile found, attempting continuation from %jd bytes\n",
> + payload->remote_name, (intmax_t)st.st_size);
> + payload->initial_size = st.st_size;
> + } else {
> /* we keep the file for a new retry but remove its data if any */
> - fflush(payload->localf);
> if(ftruncate(fileno(payload->localf), 0)) {
> RET_ERR(handle, ALPM_ERR_SYSTEM, -1);
> }
> fseek(payload->localf, 0, SEEK_SET);
> }
OK.
>
> + if(handle->dlcb && !payload->signature) {
> + alpm_download_event_retry_t cb_data;
> + cb_data.resume = payload->allow_resume;
> + handle->dlcb(payload->remote_name, ALPM_DOWNLOAD_RETRY, &cb_data);
This was changed by Andrew's recent patch.
handle->dlcb(handle->dlcb_ctx, payload->remote_name, ...
> + }
> +
> /* Set curl with the new URL */
> curl_easy_setopt(curl, CURLOPT_URL, payload->fileurl);
>
> @@ -487,7 +504,6 @@ static int curl_check_finished_download(CURLM *curlm, CURLMsg *msg,
> _alpm_log(handle, ALPM_LOG_DEBUG, "%s: response code %ld\n",
> payload->remote_name, payload->respcode);
> if(payload->respcode >= 400) {
> - payload->unlink_on_fail = 1;
> if(!payload->errors_ok) {
> handle->pm_errno = ALPM_ERR_RETRIEVE;
> /* non-translated message is same as libcurl */
> @@ -502,6 +518,7 @@ static int curl_check_finished_download(CURLM *curlm, CURLMsg *msg,
> (*active_downloads_num)++;
> return 2;
> } else {
> + payload->unlink_on_fail = 1;
> goto cleanup;
> }
> }
Hrm... This looks OK. But possibly needed fixed in a separate patch?
> @@ -519,7 +536,6 @@ static int curl_check_finished_download(CURLM *curlm, CURLMsg *msg,
> }
> goto cleanup;
> case CURLE_COULDNT_RESOLVE_HOST:
> - payload->unlink_on_fail = 1;
> handle->pm_errno = ALPM_ERR_SERVER_BAD_URL;
> _alpm_log(handle, ALPM_LOG_ERROR,
> _("failed retrieving file '%s' from %s : %s\n"),
> @@ -529,13 +545,10 @@ static int curl_check_finished_download(CURLM *curlm, CURLMsg *msg,
> (*active_downloads_num)++;
> return 2;
> } else {
> + payload->unlink_on_fail = 1;
> goto cleanup;
> }
OK
> default:
> - /* delete zero length downloads */
> - if(fstat(fileno(payload->localf), &st) == 0 && st.st_size == 0) {
> - payload->unlink_on_fail = 1;
> - }
> if(!payload->errors_ok) {
> handle->pm_errno = ALPM_ERR_LIBCURL;
> _alpm_log(handle, ALPM_LOG_ERROR,
> @@ -551,6 +564,10 @@ static int curl_check_finished_download(CURLM *curlm, CURLMsg *msg,
> (*active_downloads_num)++;
> return 2;
> } else {
> + /* delete zero length downloads */
> + if(fstat(fileno(payload->localf), &st) == 0 && st.st_size == 0) {
> + payload->unlink_on_fail = 1;
> + }
> goto cleanup;
> }
> }
OK
> diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> index a28a79a9..19bcfe58 100644
> --- a/src/pacman/callback.c
> +++ b/src/pacman/callback.c
> @@ -992,6 +992,34 @@ static void dload_progress_event(const char *filename, alpm_download_event_progr
> fflush(stdout);
> }
>
> +/* download retried */
> +static void dload_retry_event(const char *filename, alpm_download_event_retry_t *data) {
> + if(!dload_progressbar_enabled()) {
> + return;
> + }
> +
> + int index;
> + struct pacman_progress_bar *bar;
> + bool ok = find_bar_for_filename(filename, &index, &bar);
> + assert(ok);
> +
> + if(!data->resume) {
> + if(total_enabled) {
This is always enabled now.
> + /* TODO if !resume then we delete the file. this may make the new total larger */
I don't think this is a TODO, but rather a note. Change this to a comment:
/* note total download does not reflect partial downloads that are
restarted */
> + totalbar->xfered -= bar->xfered;
> + }
> + }
> +
> + bar->xfered = 0;
> + bar->total_size = 0;
> + bar->init_time = get_time_ms();
> + bar->sync_time = 0;
> + bar->sync_xfered = 0;
> + bar->rate = 0.0;
> + bar->eta = 0.0;
OK.
> +}
> +
> +
> /* download completed */
> static void dload_complete_event(const char *filename, alpm_download_event_completed_t *data)
> {
> @@ -1075,6 +1103,8 @@ void cb_download(const char *filename, alpm_download_event_type_t event, void *d
> dload_init_event(filename, data);
> } else if(event == ALPM_DOWNLOAD_PROGRESS) {
> dload_progress_event(filename, data);
> + } else if(event == ALPM_DOWNLOAD_RETRY) {
> + dload_retry_event(filename, data);
> } else if(event == ALPM_DOWNLOAD_COMPLETED) {
> dload_complete_event(filename, data);
> } else {
>
OK
More information about the pacman-dev
mailing list