On 03/24/2010 04:30 PM, Daniel Stenberg wrote:
> On Wed, 24 Mar 2010, Ben Greear wrote:
>
>> So, here it is the patch against latest git repo. I'm off for more
>> caffeine right now!
>
> Thanks! Although I have some quirks and considerations regarding it:
>
> You introduced two warnings:
>
> tftp.c:1213: warning: conversion to 'long int' from 'curl_off_t' may
> alter its value
> tftp.c:1224: warning: conversion to 'long int' from 'curl_off_t' may
> alter its value

Hrm, I even checked that, but on a 64-bit system. I'll test it out
on 32-bit as well next time.

> ... and if you check the patch again, you reverted my recent fixes that
> took away exactly those problems. sleep_time() returns a long (and it
> has a range check towards the end to prevent overflows), while you
> introduce Curl_sleep_time() instead that again returns a curl_off_t

I used curl_off_t since that is the input type, but I can
change it to long (32-bits should be enough).
I'll explicitly cast away any warnings.

> Also, when you bring that function to the rest of the library, can I
> please ask that you add some comments to the code that explains it? It
> uses a lot of magic constants and I don't follow the logic behind all of
> them. Like why does it bother with the buffer size, does that really
> make any noticable difference?

I'll add some comments.

As for block size, the code is trying to determine how long to wait before
trying the next read/write. If block sizes are large, then it should wait
longer because it expects to read/write more at that time.

This allows someone to set a small block size to have very fine granularity
speed control, for instance.

> Taking that sleep function to the "global" scope in transfer.c, we get
> your function added to the existing speed checks. What good does it
> actually bring to the code outside tftp.c? As far as I know the rate
> limiting already works for non-TFTP protocol so what is this fix for?
> The sub-second waits until the speed is checked again?

Existing code does mostly work for rate limitation, but it always sleeps
for one sec when it needs to slow down. So, for higher-speed limits,
you'll see a fast burst of traffic as it sends as fast as it can until
it's running too fast, and then a full second of sleep.

My aim was to decrease both the length of any bursts as well as
the length of any sleeps, giving an over-all smoother flow when
viewed on sub-second intervals.

Thanks,
Ben

-- 
Ben Greear <greearb_at_candelatech.com>
Candela Technologies Inc  http://www.candelatech.com
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html