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

[Morgan Adamiec]

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.

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