Hi Daniel,

I just wanted to check in and see if you'd had a chance to look at
these patches and make sure they were acceptable.

Thanks,
-Brandon

On Fri, Aug 29, 2014 at 2:48 PM, Brandon Casey <drafnel_at_gmail.com> wrote:
> Historically the default "unknown" value for progress.size_dl and
> progress.size_ul has been zero, since these values are initialized
> implicitly by the calloc that allocates the curl handle that these
> variables are a part of. Users of curl that install progress
> callbacks may expect these values to always be >= 0.
>
> Currently it is possible for progress.size_dl and progress.size_ul
> to by set to a value of -1, if Curl_pgrsSetDownloadSize() or
> Curl_pgrsSetUploadSize() are passed a "size" of -1 (which a few
> places currently do, and a following patch will add more). So
> lets update Curl_pgrsSetDownloadSize() and Curl_pgrsSetUploadSize()
> so they make sure that these variables always contain a value that
> is >= 0.
>
> Update test579.
>
> Signed-off-by: Brandon Casey <drafnel_at_gmail.com>
> ---
>
> On Thu, Aug 28, 2014 at 3:12 PM, Daniel Stenberg <daniel_at_haxx.se> wrote:
>> On Wed, 27 Aug 2014, Brandon Casey wrote:
>>
>>> Instead of passing in 0 to reset the download size and the upload size,
>>> let's pass in -1. This will make Curl_pgrsSetDownloadSize() and
>>> Curl_pgrsSetUploadSize() clear their respective "KNOWN" bits in
>>> progress.flags and will allow curl_easy_getinfo() to return -1 appropriately
>>> when CURLINFO_CONTENT_LENGTH_DOWNLOAD or CURLINFO_CONTENT_LENGTH_UPLOAD is
>>> requested.
>>
>>
>> Thanks what an awesome research and fix!
>
> Thanks.
>
>> I only have one little remark here: by also storing -1 in the struct fields
>> progress.size_dl and progress.size_ul this change alters what the progress
>> function callbacks send in their 2nd and 4th arguments - and that is
>> probably an ABI breakage users won't like.
>>
>> How about changing Curl_pgrsSetDownloadSize() and Curl_pgrsSetUploadSize()
>> to simply see the value as negative, clear the known flag but then store the
>> value as 0?
>
> That sounds sane. I was tempted to say that maybe progress callbacks
> would want to be passed -1 to indicate that the value was unknown and
> that this would be in-line with the CURLINFO_CONTENT_LENGTH_DOWNLOAD
> et al behavior, but the progress callbacks have never worked like this
> (I think), since the base curl handle is initialized using calloc and
> the progress.size_[du]l variables get initialized to zero implicitly
> by this. So the progress callbacks have always been passed a value
> that is >= 0 regardless of whether the underlying values are known.
> So I think it makes sense to ensure that these values are always >= 0.
>
> -Brandon
>
> lib/progress.c | 18 ++++++++++++------
> tests/data/test579 | 11 +++++------
> 2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/lib/progress.c b/lib/progress.c
> index e6a8d82..3963e67 100644
> --- a/lib/progress.c
> +++ b/lib/progress.c
> @@ -234,20 +234,26 @@ void Curl_pgrsSetUploadCounter(struct SessionHandle *data, curl_off_t size)
>
> void Curl_pgrsSetDownloadSize(struct SessionHandle *data, curl_off_t size)
> {
> - data->progress.size_dl = size;
> - if(size >= 0)
> + if(size >= 0) {
> + data->progress.size_dl = size;
> data->progress.flags |= PGRS_DL_SIZE_KNOWN;
> - else
> + }
> + else {
> + data->progress.size_dl = 0;
> data->progress.flags &= ~PGRS_DL_SIZE_KNOWN;
> + }
> }
>
> void Curl_pgrsSetUploadSize(struct SessionHandle *data, curl_off_t size)
> {
> - data->progress.size_ul = size;
> - if(size >= 0)
> + if(size >= 0) {
> + data->progress.size_ul = size;
> data->progress.flags |= PGRS_UL_SIZE_KNOWN;
> - else
> + }
> + else {
> + data->progress.size_ul = 0;
> data->progress.flags &= ~PGRS_UL_SIZE_KNOWN;
> + }
> }
>
> /*
> diff --git a/tests/data/test579 b/tests/data/test579
> index adbb3dd..e352e3d 100644
> --- a/tests/data/test579
> +++ b/tests/data/test579
> @@ -77,12 +77,11 @@ http://%HOSTIP:%HTTPPORT/579 log/ip579
> <verify>
> <file name="log/ip579">
> Progress callback called with UL 0 out of 0
> -Progress callback called with UL 0 out of -1
> -Progress callback called with UL 8 out of -1
> -Progress callback called with UL 16 out of -1
> -Progress callback called with UL 26 out of -1
> -Progress callback called with UL 61 out of -1
> -Progress callback called with UL 66 out of -1
> +Progress callback called with UL 8 out of 0
> +Progress callback called with UL 16 out of 0
> +Progress callback called with UL 26 out of 0
> +Progress callback called with UL 61 out of 0
> +Progress callback called with UL 66 out of 0
> </file>
> </verify>
> </testcase>
> --
> 2.0.0.rc2
>
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2014-09-04