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) >
