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