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

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

[Boris Schrijver]

Hi all,

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.

Please review! I'll be ready for answers!

[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)
         goto out;
     curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
     if (d)
-- 
2.1.4

-- 

Met vriendelijke groet / Kind regards,

Boris Schrijver

PCextreme B.V.

http://www.pcextreme.nl/contact
Tel direct: +31 (0) 118 700 215

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

[John Snow]

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?

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

> [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?

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

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

[Michael Tokarev]

08.12.2015 00:23, Boris Schrijver wrote:
[]
> It's is therefore better to use only the GET request method, and discard the
> body at the first call.

Nooooooo!  Please NOOOO!

Fetching a large file once might be too long already.
Fetching it twice is twice as long.  Oh well!..

Thanks,

/mjt

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

[Boris Schrijver]

Hi!

To clarify: The body of the response is the maximum size defined by MTU network
policies, so by default around ~1500 bytes. After that is received, the header
is parsed and the connection is dropped!

So no whole file transfers!!! Please test and see for your self!

-- 

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:56 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> 
> 08.12.2015 00:23, Boris Schrijver wrote:
> []
> > It's is therefore better to use only the GET request method, and discard the
> > body at the first call.
> 
> Nooooooo!  Please NOOOO!
> 
> Fetching a large file once might be too long already.
> Fetching it twice is twice as long.  Oh well!..
> 
> Thanks,
> 
> /mjt

Re: [Qemu-devel] [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 <jsnow@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.


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

[PATCH]

commit ec8d3ef01eaca9264d97e9ad757fe536e0dc037b
Author: Boris Schrijver <boris@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 <boris@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)

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

[Boris Schrijver]

Dear all,

Thanks for your time so-far.

I send a mail to the libcurl development list and got a helpful reaction [1]! I
changed my patch accordingly. We now don't have to check for a CURLE_WRITE_ERROR
anymore and curl_easy_perform() will return CURLE_OK (0) on success.

Please review!

commit c5588e94f5f0e66636b16a4ee26f0dbcf48b44c9
Author: Boris Schrijver <boris@pcextreme.nl>
Date:   Wed Dec 9 23:32:36 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 <boris@pcextreme.nl>

diff --git a/block/curl.c b/block/curl.c
index 8994182..1677d0c 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -598,6 +598,7 @@ static int curl_open(BlockDriverState *bs, QDict *options,
int flags,
     curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
                      curl_header_cb);
     curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
+    curl_easy_setopt(state->curl, CURLOPT_CUSTOMREQUEST, "GET");
     if (curl_easy_perform(state->curl))
         goto out;
     curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);



[1] http://curl.haxx.se/mail/lib-2015-12/0041.html

-- 

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:56 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> 
> 08.12.2015 00:23, Boris Schrijver wrote:
> []
> > It's is therefore better to use only the GET request method, and discard the
> > body at the first call.
> 
> Nooooooo!  Please NOOOO!
> 
> Fetching a large file once might be too long already.
> Fetching it twice is twice as long.  Oh well!..
> 
> Thanks,
> 
> /mjt

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

[John Snow]

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 <jsnow@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 <boris@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 <boris@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)
> 

Re: [Qemu-devel] [Qemu-block] [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 <jsnow@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 <jsnow@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 <boris@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 <boris@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)
> >

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

[John Snow]

On 12/09/2015 05:37 PM, Boris Schrijver wrote:
> Dear all,
> 
> Thanks for your time so-far.
> 
> I send a mail to the libcurl development list and got a helpful reaction [1]! I
> changed my patch accordingly. We now don't have to check for a CURLE_WRITE_ERROR
> anymore and curl_easy_perform() will return CURLE_OK (0) on success.
> 
> Please review!
> 

I apologize in advance for being pedantic, but if the patch is in the
wrong format, maintainers may not even notice the patches due to their
filtering systems. To make sure the right people see it, please follow
the advice below.

This is unnecessary stuff before the patch, which is serving as your
cover letter. Cover letters should either be sent separately as a "0/n"
patch email, or for single patch series (like this one), meta-commentary
should be included below the commit message, underneath the first "---"
line.

You can provide a --cover-letter argument to "git format-patch" to have
it generate a template cover letter for you if this is convenient.

Some people generate a cover letter for even single patch series; it's a
matter of taste/preference for where you want to include your commentary
if you have any to send.

> commit c5588e94f5f0e66636b16a4ee26f0dbcf48b44c9
> Author: Boris Schrijver <boris@pcextreme.nl>
> Date:   Wed Dec 9 23:32:36 2015 +0100
> 

Wrong patch format; it looks like you've done "git show > qemu.patch".
The header should look like this:


>From c5588e94...xxx... Mon Sep 17 00:00:00 2001
From: Joe Developer <jdev@example.com>
Date: Tue, 8 Dec 2015 16:00:11 -0500
Subject: [PATCH v3] component: subject

Brief summary

Additional explanation

Signed-off-by: Joe Developer <jdev@example.com>
Reviewed-by: Chris Reviewer <creviewe@example.com>


---
V3: Improvements over V2:
 - blah
 - blah

V2:
 - blah

Please review, thank you!
---
 tests/example.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/tests/example.c b/tests/example.c
index 0888506..f4945dc 100644
--- a/tests/ahci-test.c

[etc etc etc.]

You can be sure it will be in a proper format if you:

git format-patch origin/master
git send-email --to=qemu-block@nongnu.org --cc=jcody@redhat.com
--cc=jsnow@redhat.com --subject-prefix="PATCH v3"
0001-qemu-img-curl-use-get.patch

You can edit the .patch file before sending it to include any changelog
notes below the "---" line to separate it from your commit message,
before the git diff sections.

>     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 <boris@pcextreme.nl>
> 
> diff --git a/block/curl.c b/block/curl.c
> index 8994182..1677d0c 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -598,6 +598,7 @@ static int curl_open(BlockDriverState *bs, QDict *options,
> int flags,
>      curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
>                       curl_header_cb);
>      curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
> +    curl_easy_setopt(state->curl, CURLOPT_CUSTOMREQUEST, "GET");

I see. so curl thinks it is doing a HEAD request, but we trick it into
asking for GET instead. and since we clean up the context right away,
this should be harmless.

Much cleaner than the first attempt. Looks OK to me. When you re-spin
your patch, you may add:

Reviewed-by: John Snow <jsnow@redhat.com>

to the commit message.

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

---

You don't need to include reply data in patch emails.

> 
> 
> [1] http://curl.haxx.se/mail/lib-2015-12/0041.html
> 
> -- 
> 
> 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:56 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
>>
>>
>> 08.12.2015 00:23, Boris Schrijver wrote:
>> []
>>> It's is therefore better to use only the GET request method, and discard the
>>> body at the first call.
>>
>> Nooooooo!  Please NOOOO!
>>
>> Fetching a large file once might be too long already.
>> Fetching it twice is twice as long.  Oh well!..
>>
>> Thanks,
>>
>> /mjt
> 

Thanks!

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

[Boris Schrijver]

A server can respond different to both methods, or can block one of the two.

Signed-off-by: Boris Schrijver <boris@pcextreme.nl>
Reviewed-by: John Snow <jsnow@redhat.com>
---
V2: Impovements over V1:
- Don't check for CURLE_WRITE_ERROR, on success CURLE_OK will be returned.
- Use CURLOPT_CUSTOMREQUEST instead of CURLOPT_HTTPGET.
---
 block/curl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/curl.c b/block/curl.c
index 8994182..1677d0c 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -598,6 +598,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
                      curl_header_cb);
     curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
+    curl_easy_setopt(state->curl, CURLOPT_CUSTOMREQUEST, "GET");
     if (curl_easy_perform(state->curl))
         goto out;
     curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
-- 
2.1.4