[pacman-dev] [PATCH] libalpm: download rates becoming negative

[Andrew Gregory]

On 04/19/21 at 10:26am, Morgan Adamiec wrote:
> 
> 
> On 19/04/2021 05:53, Andrew Gregory wrote:
> > On 04/18/21 at 11:31pm, morganamilo wrote:
> >> When a download fails on one mirror a new download is started on the
> >> next mirror. This new download will have the initial size of whatever
> >> has been downloaded so far, as well as the ammount downloaded reset to
> >> 0.
> >>
> >> To account for this, when a download changes mirror, save how much has
> >> been downloaded so far and add that to dlcb calls.
> >> ---
> >>  lib/libalpm/dload.c | 14 ++++++++++++--
> >>  lib/libalpm/dload.h |  1 +
> >>  2 files changed, 13 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> >> index a4c42f8d..27a7748a 100644
> >> --- a/lib/libalpm/dload.c
> >> +++ b/lib/libalpm/dload.c
> >> @@ -207,8 +207,8 @@ static int dload_progress_cb(void *file, curl_off_t dltotal, curl_off_t dlnow,
> >>  
> >>  	/* do NOT include initial_size since it wasn't part of the package's
> >>  	 * download_size (nor included in the total download size callback) */
> >> -	cb_data.total = dltotal;
> >> -	cb_data.downloaded = dlnow;
> >> +	cb_data.total = dltotal + payload->resetprogress;
> >> +	cb_data.downloaded = dlnow + payload->resetprogress;
> >>  	payload->handle->dlcb(payload->remote_name, ALPM_DOWNLOAD_PROGRESS, &cb_data);
> >>  	payload->prevprogress = current_size;
> >>  
> >> @@ -440,6 +440,16 @@ static int curl_retry_next_server(CURLM *curlm, CURL *curl, struct dload_payload
> >>  		fseek(payload->localf, 0, SEEK_SET);
> >>  	}
> >>  
> >> +
> >> +	/* reset progress for next server */
> >> +	payload->resetprogress += payload->prevprogress - payload->initial_size;
> >> +	payload->unlink_on_fail = 0;
> > 
> > Without looking at the rest of the patch, how does this line not just
> > straight up break unlink_on_fail functionality for any download that
> > falls back to another server?
> > 
> 
> Honestly don't really know. I was using
> e83e868a77865d42a33076605f9a90a165f7c93a as a base and it was done there.
> 
> I guess the point is that curl_check_finished_download() will set
> unlink_on_fail = 1 in most situations, so we reset it to 0 on retry
> knowing it may then be set to 1 again on the next check_finished.
> 
> Though I guess this breaks tempfile downloads which should always be
> unlink_on_fail = 1.
> 
> 
> Also looking at the code above:
> 
> if(payload->unlink_on_fail) {
> 	/* 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);
> }
> 
> I don't see why we'd want to wipe the file instead of trying to resume.

Because the file may not be the same after we fallback to a different
server.

> In fact I don't see the point of this variable at all. Why would we ever
> not want to unlink on fail?

So that it can be resumed later.