Re: [Qemu-block] [PATCH] qemu-img / curl: When fetching Content-Size use GET instead of HEAD.

[Boris Schrijver]

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 <address@hidden> 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.


> >          goto out;
> >      curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
> >      if (d)
> >

[PATCH]

commit ec8d3ef01eaca9264d97e9ad757fe536e0dc037b
Author: Boris Schrijver <address@hidden>
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 <address@hidden>

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)