Richard W.M. Jones
2020-Jan-09 13:07 UTC
Re: [Libguestfs] [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
Adrian Ambrożewicz
2020-Jan-09 16:41 UTC
Re: [Libguestfs] [PATCH] Fix lossy conversion of Content-Length
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 >
Pino Toscano
2020-Jan-09 17:40 UTC
Re: [Libguestfs] [PATCH] Fix lossy conversion of Content-Length
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