axel: fix delay_time calculation - Githubissues

axel-download-accelerator / axel

Lightweight CLI download accelerator

GNU General Public License v2.0

2.87k stars 258 forks source link

axel: fix delay_time calculation #327

Closed wtywtykk closed 3 years ago

wtywtykk commented 3 years ago

In the original code delay_time is cleared when the speed is within spec. It should be cleared only when it's too small for another substraction.

wtywtykk commented 3 years ago

Also 64 bit variables should be used in calculation. A 5M download speed can make the 32 bit variable overflow. (1000 * 5,000,000 > 2,147,483,647 )

ismaell commented 3 years ago

Please document this in the commit message.

Also, it would be useful to cap axel->bytes_per_second.

wtywtykk commented 3 years ago

I noticed that size_t is also used for bytes_done, start_byte and size. On 32bit machines, this ia a problem. Is it necessary to change this to 64bit? Changing this seems to break the state file backward compatibility on 32bit machines.

ismaell commented 3 years ago

I noticed that size_t is also used for bytes_done, start_byte and size. On 32bit machines, this ia a problem. Is it necessary to change this to 64bit? Changing this seems to break the state file backward compatibility on 32bit machines.

Yes, the state file format has no forward compatibility method in place, so it will break with any changes.

The proper type for the conversion would be off_t; special care needs to be taken on 32 bits systems with glibc, at least it would require an autoconf test, and _POSIX_C_SOURCE should be defined globally.

See https://pubs.opengroup.org/onlinepubs/9699919799/.

ismaell commented 3 years ago

Do you mind condensing everything into a single patch and rebasing?

wtywtykk commented 3 years ago

Ok, I've made it a single patch and rebased to your latest commit.

For some variables, I had a hard time choosing between size_t and off_t. And I changed many size_t to off_t for consistency. If those changes are inapproprate or too aggressive, please let me know.

ismaell commented 3 years ago

On 18/Nov/2020 01:29, wtywtykk wrote:

Ok, I've made it a single patch and rebased to your latest commit.

For some variables, I had a hard time choosing between size_t and off_t. And I changed many size_t to off_t for consistency. If those changes are inapproprate or too aggressive, please let me know.

Thanks. I'll try to review it on the weekend.

ismaell commented 3 years ago

LGTM. I'll merge with some changes to the commit message if you don't mind.

ismaell commented 3 years ago

Thinking about it, the algorithm still seems wrong to me, and some manual testing revealed issues right away:

it takes long to converge at the beginning
if it reaches an invalid value it aborts instead of trying to reset to something sensible

So there's further opportunities for improvement.

Great work.

ismaell commented 3 years ago

Merged.