[pacman-dev] [PATCH v2] Parametrise the different ways in which the payload is reset

[Dave Reisner]

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 <mysatyre at 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