Libguestfs - Jan 2020 - Re: [PATCH] Fix lossy conversion of Content-Length

On Wed, Jan 08, 2020 at 07:19:56AM -0600, Eric Blake
wrote:
> On 1/7/20 4:13 AM, Adrian Ambrożewicz wrote:
> >Actual variable holding content length is int64_t, but it was assigned
> >by explicit cast to size_t. On 32-bit systems it's a lossy
conversion,
> >so it was replaced by casting to int64_t instead.
> >
> >Signed-off-by: Adrian Ambrożewicz
<adrian.ambrozewicz@linux.intel.com>
> >---
> >  plugins/curl/curl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c
> >index 031bd32..fe1330e 100644
> >--- a/plugins/curl/curl.c
> >+++ b/plugins/curl/curl.c
> >@@ -389,7 +389,7 @@ curl_open (int readonly)
> >      goto err;
> >    }
> >
> >-  h->exportsize = (size_t) d;
> >+  h->exportsize = (int64_t) d;
> 
> Why is a cast needed at all?
> This is C, an implicit conversion
> works just as well.
Present since the initial commit, but not present in the qemu
block/curl.c driver which is where most of this code was derived from,
so I suspect I simply added it "because reasons".

But even more to the point, why on earth does the
CURLINFO_CONTENT_LENGTH_DOWNLOAD API return a double!  Well it seems
as if they replaced that API with:

https://curl.haxx.se/libcurl/c/CURLINFO_CONTENT_LENGTH_DOWNLOAD_T.html

which is what we really ought to use instead.  This was added in Curl
7.55.0, released 2 and a half years ago, so I think we ought to use
that instead.  Adrian could you write a patch to change over to that
API?

Rich.
> >    nbdkit_debug ("content length: %" PRIi64,
h->exportsize);
> >
> >    if (strncasecmp (url, "http://", strlen
("http://")) == 0 ||
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

W dniu 1/9/2020 o 14:07, Richard W.M. Jones pisze:
> On Wed, Jan 08, 2020 at 07:19:56AM -0600, Eric Blake wrote:
>> On 1/7/20 4:13 AM, Adrian Ambrożewicz wrote:
>>> Actual variable holding content length is int64_t, but it was
assigned
>>> by explicit cast to size_t. On 32-bit systems it's a lossy
conversion,
>>> so it was replaced by casting to int64_t instead.
>>>
>>> Signed-off-by: Adrian Ambrożewicz
<adrian.ambrozewicz@linux.intel.com>
>>> ---
>>>   plugins/curl/curl.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c
>>> index 031bd32..fe1330e 100644
>>> --- a/plugins/curl/curl.c
>>> +++ b/plugins/curl/curl.c
>>> @@ -389,7 +389,7 @@ curl_open (int readonly)
>>>       goto err;
>>>     }
>>>
>>> -  h->exportsize = (size_t) d;
>>> +  h->exportsize = (int64_t) d;
>>
>> Why is a cast needed at all?
>> This is C, an implicit conversion
>> works just as well.
> 
> Present since the initial commit, but not present in the qemu
> block/curl.c driver which is where most of this code was derived from,
> so I suspect I simply added it "because reasons".
> 
> But even more to the point, why on earth does the
> CURLINFO_CONTENT_LENGTH_DOWNLOAD API return a double!  Well it seems
> as if they replaced that API with:
> 
> https://curl.haxx.se/libcurl/c/CURLINFO_CONTENT_LENGTH_DOWNLOAD_T.html
> 
> which is what we really ought to use instead.  This was added in Curl
> 7.55.0, released 2 and a half years ago, so I think we ought to use
> that instead.  Adrian could you write a patch to change over to that
> API?
> 
> Rich.
I see that you've already ACK-ed such change by Pino so I assume my 
patch is OK? Or should I remove casting altogether?
> 
>>>     nbdkit_debug ("content length: %" PRIi64,
h->exportsize);
>>>
>>>     if (strncasecmp (url, "http://", strlen
("http://")) == 0 ||
>>
>> -- 
>> Eric Blake, Principal Software Engineer
>> Red Hat, Inc.           +1-919-301-3226
>> Virtualization:  qemu.org | libvirt.org
>>
>> _______________________________________________
>> Libguestfs mailing list
>> Libguestfs@redhat.com
>> https://www.redhat.com/mailman/listinfo/libguestfs
>

On Thursday, 9 January 2020 17:41:58 CET Adrian Ambrożewicz
wrote:
> W dniu 1/9/2020 o 14:07, Richard W.M. Jones pisze:
> > On Wed, Jan 08, 2020 at 07:19:56AM -0600, Eric Blake wrote:
> >> On 1/7/20 4:13 AM, Adrian Ambrożewicz wrote:
> >>> Actual variable holding content length is int64_t, but it was
assigned
> >>> by explicit cast to size_t. On 32-bit systems it's a lossy
conversion,
> >>> so it was replaced by casting to int64_t instead.
> >>>
> >>> Signed-off-by: Adrian Ambrożewicz
<adrian.ambrozewicz@linux.intel.com>
> >>> ---
> >>>   plugins/curl/curl.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c
> >>> index 031bd32..fe1330e 100644
> >>> --- a/plugins/curl/curl.c
> >>> +++ b/plugins/curl/curl.c
> >>> @@ -389,7 +389,7 @@ curl_open (int readonly)
> >>>       goto err;
> >>>     }
> >>>
> >>> -  h->exportsize = (size_t) d;
> >>> +  h->exportsize = (int64_t) d;
> >>
> >> Why is a cast needed at all?
> >> This is C, an implicit conversion
> >> works just as well.
> > 
> > Present since the initial commit, but not present in the qemu
> > block/curl.c driver which is where most of this code was derived from,
> > so I suspect I simply added it "because reasons".
> > 
> > But even more to the point, why on earth does the
> > CURLINFO_CONTENT_LENGTH_DOWNLOAD API return a double!  Well it seems
> > as if they replaced that API with:
> > 
> > https://curl.haxx.se/libcurl/c/CURLINFO_CONTENT_LENGTH_DOWNLOAD_T.html
> > 
> > which is what we really ought to use instead.  This was added in Curl
> > 7.55.0, released 2 and a half years ago, so I think we ought to use
> > that instead.  Adrian could you write a patch to change over to that
> > API?
> > 
> > Rich.
> I see that you've already ACK-ed such change by Pino so I assume my 
> patch is OK? Or should I remove casting altogether?
My patch used the better API with curl >= 7.55.0. In older versions the
issue still exists, and removing the cast should work.

Can you please provide a v2 of your patch with that change?

Thanks,
-- 
Pino Toscano