On Fri, Oct 14, 2016 at 09:26:46PM +0200, mar77i wrote:
> In FS#43434, Downloads which fail and are restarted on a different server
> will resume and may display a negative download speed. The payload's progress
> in libalpm was not properly reset which ultimately caused terminal noise
> because the line width calculation assumes positive download speeds.
>
> This patch fixes the incomplete reset of the payload by mimicing what
> be_sync.c:alpm_db_update() does over in sync.c:download_single_file().
> dload.c:_alpm_dload_payload_reset() bundles and extends over the current
> behavior by updating initial_size and prevprogress for this case.
> This makes pacman reset the progress properly in the next invocation of the
> callback and display positive download speeds.
>
> Fixes FS#43434.
>
> Signed-off-by: Martin Kühne <mysat...@gmail.com>
> ---
> lib/libalpm/be_sync.c | 4 ++--
> lib/libalpm/dload.c | 15 +++++++++++----
> lib/libalpm/dload.h | 2 +-
> lib/libalpm/sync.c | 4 +---
> 4 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> index a836277..2425359 100644
> --- a/lib/libalpm/be_sync.c
> +++ b/lib/libalpm/be_sync.c
> @@ -244,7 +244,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
> payload.unlink_on_fail = 1;
>
> ret = _alpm_download(&payload, syncpath, NULL, &final_db_url);
> - _alpm_dload_payload_reset(&payload);
> + _alpm_dload_payload_reset(&payload, 0);
> updated = (updated || ret == 0);
>
> if(ret != -1 && updated && (level & ALPM_SIG_DATABASE)) {
> @@ -300,7 +300,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
> sig_ret = _alpm_download(&payload, syncpath, NULL,
> NULL);
> /* errors_ok suppresses error messages, but not the
> return code */
> sig_ret = payload.errors_ok ? 0 : sig_ret;
> - _alpm_dload_payload_reset(&payload);
> + _alpm_dload_payload_reset(&payload, 0);
This is kind of opaque at a glance. What does 0 mean? Potential
modifications:
1) Add an inline comment:
_alpm_dload_payload_reset(&payload, 0 /* do not allow resume */);
2) Separate this out into 2 wrappers:
_alpm_dload_payload_reset_for_retry(&payload);
_alpm_dload_payload_reset(&payload);
_alpm_dload_payload_reset_internal(&payload, 0|1);
I don't feel too strongly about this, though, since this sort of
opaqueness is evident elsewhere in the codebase.
> }
>
> if(ret != -1 && sig_ret != -1) {
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index dc57c92..55ff41f 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -697,7 +697,7 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle,
> const char *url)
>
> /* download the file */
> ret = _alpm_download(&payload, cachedir, &final_file,
> &final_pkg_url);
> - _alpm_dload_payload_reset(&payload);
> + _alpm_dload_payload_reset(&payload, 0);
> if(ret == -1) {
> _alpm_log(handle, ALPM_LOG_WARNING, _("failed to
> download %s\n"), url);
> free(final_file);
> @@ -738,7 +738,7 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle,
> const char *url)
> FREE(sig_final_file);
> }
> free(sig_filepath);
> - _alpm_dload_payload_reset(&payload);
> + _alpm_dload_payload_reset(&payload, 0);
> }
>
> /* we should be able to find the file the second time around */
> @@ -750,15 +750,22 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t
> *handle, const char *url)
> return filepath;
> }
>
> -void _alpm_dload_payload_reset(struct dload_payload *payload)
> +void _alpm_dload_payload_reset(struct dload_payload *payload, int
> allow_resume)
> {
> ASSERT(payload, return);
>
> + FREE(payload->fileurl);
> + if(allow_resume) {
> + payload->initial_size += payload->prevprogress;
> + payload->prevprogress = 0;
> + payload->unlink_on_fail = 0;
> + return;
> + }
> +
> FREE(payload->remote_name);
> FREE(payload->tempfile_name);
> FREE(payload->destfile_name);
> FREE(payload->content_disp_name);
> - FREE(payload->fileurl);
> memset(payload, '\0', sizeof(*payload));
> }
>
> diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> index 427c486..c9c94b8 100644
> --- a/lib/libalpm/dload.h
> +++ b/lib/libalpm/dload.h
> @@ -46,7 +46,7 @@ struct dload_payload {
> #endif
> };
>
> -void _alpm_dload_payload_reset(struct dload_payload *payload);
> +void _alpm_dload_payload_reset(struct dload_payload *payload, int
> allow_resume);
>
> int _alpm_download(struct dload_payload *payload, const char *localpath,
> char **final_file, const char **final_url);
> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> index 00b68d0..81900df 100644
> --- a/lib/libalpm/sync.c
> +++ b/lib/libalpm/sync.c
> @@ -946,9 +946,7 @@ static int download_single_file(alpm_handle_t *handle,
> struct dload_payload *pay
> EVENT(handle, &event);
> return 0;
> }
> -
> - FREE(payload->fileurl);
> - payload->unlink_on_fail = 0;
> + _alpm_dload_payload_reset(payload, 1);
> }
>
> event.type = ALPM_EVENT_PKGDOWNLOAD_FAILED;
> --
> 2.10.0
