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

[Boris Schrijver]

Dear John,

I already send a new patch with V2. Please see that one!

> On December 10, 2015 at 10:26 PM John Snow <address@hidden> 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 <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.
> > 
> > 
> 
> 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 <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)
> >