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

Reply via email to