Re: [PATCH 2/2] block/curl: add support for S3 presigned URLs

[Markus Armbruster <armbru@redhat.com>]

Antoine Damhet <adamhet@scaleway.com> writes:

> On Tue, Feb 17, 2026 at 09:53:11AM +0100, Markus Armbruster wrote:
>> Antoine Damhet <adamhet@scaleway.com> writes:
>> 
>> > S3 presigned URLs are signed for a specific HTTP method (typically GET
>> > for our use cases). The curl block driver currently issues a HEAD
>> > request to discover the backend features and the file size, which fails
>> > with 403.
>> >
>> > Add a 'force-range' option that skips the HEAD request and instead
>> > issues a minimal GET request (querying 1 byte from the server) to
>> > extract the file size from the 'Content-Range' response header. To
>> > achieve this the 'curl_header_cb' is redesigned to generically parse
>> > HTTP headers.
>> >
>> > $ $QEMU -drive driver=http,\
>> >              'url=https://s3.example.com/some.img?X-Amz-Security-Token=XXX',
>> >              force-range=true
>> >
>> > Enabling the 'force-range' option without the backend supporting it is
>> > undefined behavior and untested
>> 
>> "Undefined behavior" suggests it could do anything, even destroy data.
>> I hope that's not the case.  What is the case?
>> 
>> What is "the backend"?  The web server specified with @url?
>
> Undefined behavior was probably too strong of a wording. I have done way
> more tests and have a clearer picture of what happens:
>
> The web server for @url will respond with `HTTP 200` and try to send the
> whole file. Since we specified `CURLOPT_NOBODY` to the libcurl it stops
> reading the socket after the headers and justs shuts it down. The
> `force-range` mode is transparent for the user even if it can wastes a
> few TCP packets.
>
> I'll rewrite the commit message to reflect the actual behavior of the
> option in the v2.

Thanks!

>> >                                 but the libcurl should ignore the body
>> > and stop reading after the HTTP headers then we would fail with the
>> > expected `Server does not support 'range' (byte ranges).` error.
>> >
>> > Signed-off-by: Antoine Damhet <adamhet@scaleway.com>
>> > ---
>> 
>> [...]
>> 
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index b82af7425614..ff018c2d6bfb 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -4582,12 +4582,17 @@
>> >  # @cookie-secret: ID of a QCryptoSecret object providing the cookie
>> >  #     data in a secure way.  See @cookie for the format.  (since 2.10)
>> >  #
>> > +# @force-range: Don't issue a HEAD HTTP request to discover if the
>> > +#     backend supports range requests and rely only on GET requests.
>> > +#     This is especially useful for S3 presigned URLs.  (since 11.0)

Missing: Defaults to false.

>> Unlike the commit message, this doesn't mention the need for "the
>> backend" (whatever that may be) supporting it.
>
> Will rephrase "the  backend" with "the http server". Should I document
> the behavior of the http server missing the range requests here or is
> the current description sufficient ?

What do users need to know here?  I think it's when and why to use
@force-range.  Drawbacks of using it if there are any.

>> > +#
>> >  # Since: 2.9
>> >  ##
>> >  { 'struct': 'BlockdevOptionsCurlHttp',
>> >    'base': 'BlockdevOptionsCurlBase',
>> >    'data': { '*cookie': 'str',
>> > -            '*cookie-secret': 'str'} }
>> > +            '*cookie-secret': 'str',
>> > +            '*force-range': 'bool'} }
>> >  
>> >  ##
>> >  # @BlockdevOptionsCurlHttps:
>> > @@ -4605,13 +4610,18 @@
>> >  # @cookie-secret: ID of a QCryptoSecret object providing the cookie
>> >  #     data in a secure way.  See @cookie for the format.  (since 2.10)
>> >  #
>> > +# @force-range: Don't issue a HEAD HTTP request to discover if the
>> > +#     backend supports range requests and rely only on GET requests.
>> > +#     This is especially useful for S3 presigned URLs.  (since 11.0)
>> > +#
>> >  # Since: 2.9
>> >  ##
>> 
>> @force-range is is duplicated between BlockdevOptionsCurlHttp and
>> BlockdevOptionsCurlHttps.  @cookie and @cookie-secret is already
>> duplicated before the patch.  Time to factor out a common base type?
>
> This would be only on the QAPI ? looking something like:
>
> ```
>  { 'struct': 'BlockdevOptionsCurlHttps',
> -  'base': 'BlockdevOptionsCurlBase',
> -  'data': { '*cookie': 'str',
> -            '*sslverify': 'bool',
> -            '*cookie-secret': 'str',
> -            '*force-range': 'bool'} }
> +  'base': 'BlockdevOptionsCurlHttp',
> +  'data': { '*sslverify': 'bool' } }
> ```

Looks good to me.

> ? Would you rather see this in a separate commit or is the same patch OK
> ?

I'd prefer a separate commit.