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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

[adding qemu-devel; per http://wiki.qemu.org/Contribute/SubmitAPatch,
ALL patches must include qemu-devel, even if they are also copying other
sublists]

On 12/11/2015 01:40 AM, Boris Schrijver wrote:
> 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);
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[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] [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 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 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!