[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-img / curl: When fetching Con
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-img / curl: When fetching Content-Size use GET instead of HEAD. |
Date: |
Thu, 10 Dec 2015 16:26:07 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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)
>