Dear John, I already send a new patch with V2. Please see that one!
> On December 10, 2015 at 10:26 PM John Snow <js...@redhat.com> wrote: > > > > > On 12/08/2015 03:49 PM, Boris Schrijver wrote: > > See inline! Thanks for your response! > > > > -- > > > > Met vriendelijke groet / Kind regards, > > > > Boris Schrijver > > > > PCextreme B.V. > > > > http://www.pcextreme.nl/contact > > Tel direct: +31 (0) 118 700 215 > > > >> On December 8, 2015 at 8:40 PM John Snow <js...@redhat.com> wrote: > >> > >> > >> > >> > >> On 12/07/2015 04:23 PM, Boris Schrijver wrote: > >>> Hi all, > >>> > >> > >> Hi! > >> > >>> I was testing out the "qemu-img info/convert" options in combination with > >>> "http/https" when I stumbled upon this issue. When "qemu-img info/convert" > >>> tries > >>> to collect the file info it will first try to fetch the Content-Size of > >>> the > >>> remote file. It does a HEAD request and after a GET request for the > >>> correct > >>> range. > >>> > >>> The HEAD request is an issue. Because when you've got a pre-signed url, > >>> for > >>> example from S3, which INCLUDES the REQUEST METHOD in it's signature, > >>> you'll > >>> get > >>> a 403 Forbidden. > >>> > >>> It's is therefore better to use only the GET request method, and discard > >>> the > >>> body at the first call. > >>> > >> > >> How big is the body? Won't this introduce a really large overhead? > > > > The body is "worst-case" the size of the Ethernet v2 frame, around 1500 > > bytes. > > > >> > >>> Please review! I'll be ready for answers! > >>> > >> > >> Please use the git format-patch format for sending patch emails; see > >> http://qemu-project.org/Contribute/SubmitAPatch#Use_git_format-patch -- > >> and remember to include a Signed-off-by line. > >> > > > > Ok, will do! > > > >>> [PATCH] qemu-img / curl: When fetching Content-Size use GET instead of > >>> HEAD. > >>> > >>> A server can respond different to both methods, or can block one of the > >>> two. > >>> --- > >>> block/curl.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/block/curl.c b/block/curl.c > >>> index 8994182..2e74c32 100644 > >>> --- a/block/curl.c > >>> +++ b/block/curl.c > >>> @@ -594,11 +594,11 @@ static int curl_open(BlockDriverState *bs, QDict > >>> *options, > >>> int flags, > >>> // Get file size > >>> > >>> s->accept_range = false; > >>> - curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1); > >>> + curl_easy_setopt(state->curl, CURLOPT_HTTPGET, 1); > >>> curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, > >>> curl_header_cb); > >>> curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s); > >>> - if (curl_easy_perform(state->curl)) > >>> + if (curl_easy_perform(state->curl) != 23) > >> > >> We go from making sure there were no errors to enforcing that we *do* > >> get CURLE_WRITE_ERROR? Can you explain why this change doesn't break > >> error handling scenarios for all other cases? > >> > > > > We're enforcing the CURLE_WRITE_ERROR here. We receive data, but don't want > > to > > save it anywhere -> We only want the header. CURLE_WRITE_ERROR implicitly > > means > > the connection is successful, because we received a response body! Any other > > error will not be CURLE_WRITE_ERROR and thus fail. > > > > Please run the following command: curl -v -X GET -I http://qemu.org/ > > It will at the last line read: > > > > * Excess found in a non pipelined read: excess = 279 url = / (zero-length > > body) > > > > That is the body we're discarding. > > > > Libcurl basically doesn't provide a nice way to handle this. That's why I've > > implemented in this fashion. > > > > > > Hm... I suppose this works, though it leaves a slightly bad taste in my > mouth. Can you replace 23 by a definition (CURLE_WRITE_ERROR?) and > include a little blurb about why this quirk works? > > Please send the follow-up patch as a new thread, with a "v2" tag so > others (particularly Jeff Cody) can see it -- he might have a different > opinion here. > > Thanks! > --js > > >>> goto out; > >>> curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d); > >>> if (d) > >>> > > > > [PATCH] > > > > commit ec8d3ef01eaca9264d97e9ad757fe536e0dc037b > > Author: Boris Schrijver <bo...@pcextreme.nl> > > Date: Mon Dec 7 22:01:59 2015 +0100 > > > > qemu-img / curl: When fetching Content-Size use GET instead of HEAD. > > > > A server can respond different to both methods, or can block one of the > > two. > > > > Signed-off-by: Boris Schrijver <bo...@pcextreme.nl> > > > > diff --git a/block/curl.c b/block/curl.c > > index 8994182..2e74c32 100644 > > --- a/block/curl.c > > +++ b/block/curl.c > > @@ -594,11 +594,11 @@ static int curl_open(BlockDriverState *bs, QDict > > *options, > > int flags, > > // Get file size > > > > s->accept_range = false; > > - curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1); > > + curl_easy_setopt(state->curl, CURLOPT_HTTPGET, 1); > > curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, > > curl_header_cb); > > curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s); > > - if (curl_easy_perform(state->curl)) > > + if (curl_easy_perform(state->curl) != 23) > > goto out; > > curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d); > > if (d) > >