Coverity points out that we aren't checking the return value
from curl_easy_setopt() for any of the calls to it we make
in block/curl.c.

Fixes: Coverity CID 1459336, 1459482, 1460331
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Big fat disclaimer: tested only with 'make check', which I suspect
may not be exercising this block backend. Hints on how to test
more thoroughly are welcome.

 block/curl.c | 90 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 58 insertions(+), 32 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 6a6cd729758..aaee1b17bef 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -458,38 +458,51 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state)
         if (!state->curl) {
             return -EIO;
         }
-        curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
-        curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
-                         (long) s->sslverify);
-        curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYHOST,
-                         s->sslverify ? 2L : 0L);
-        if (s->cookie) {
-            curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
+        if (curl_easy_setopt(state->curl, CURLOPT_URL, s->url) ||
+            curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
+                             (long) s->sslverify) ||
+            curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYHOST,
+                             s->sslverify ? 2L : 0L)) {
+            goto err;
+        }
+        if (s->cookie) {
+            if (curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie)) {
+                goto err;
+            }
+        }
+        if (curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout) ||
+            curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
+                             (void *)curl_read_cb) ||
+            curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state) ||
+            curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state) ||
+            curl_easy_setopt(state->curl, CURLOPT_AUTOREFERER, 1) ||
+            curl_easy_setopt(state->curl, CURLOPT_FOLLOWLOCATION, 1) ||
+            curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1) ||
+            curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg) ||
+            curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1)) {
+            goto err;
         }
-        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout);
-        curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
-                         (void *)curl_read_cb);
-        curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
-        curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state);
-        curl_easy_setopt(state->curl, CURLOPT_AUTOREFERER, 1);
-        curl_easy_setopt(state->curl, CURLOPT_FOLLOWLOCATION, 1);
-        curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1);
-        curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg);
-        curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1);
-
         if (s->username) {
-            curl_easy_setopt(state->curl, CURLOPT_USERNAME, s->username);
+            if (curl_easy_setopt(state->curl, CURLOPT_USERNAME, s->username)) {
+                goto err;
+            }
         }
         if (s->password) {
-            curl_easy_setopt(state->curl, CURLOPT_PASSWORD, s->password);
+            if (curl_easy_setopt(state->curl, CURLOPT_PASSWORD, s->password)) {
+                goto err;
+            }
         }
         if (s->proxyusername) {
-            curl_easy_setopt(state->curl,
-                             CURLOPT_PROXYUSERNAME, s->proxyusername);
+            if (curl_easy_setopt(state->curl,
+                                 CURLOPT_PROXYUSERNAME, s->proxyusername)) {
+                goto err;
+            }
         }
         if (s->proxypassword) {
-            curl_easy_setopt(state->curl,
-                             CURLOPT_PROXYPASSWORD, s->proxypassword);
+            if (curl_easy_setopt(state->curl,
+                                 CURLOPT_PROXYPASSWORD, s->proxypassword)) {
+                goto err;
+            }
         }
 
         /* Restrict supported protocols to avoid security issues in the more
@@ -499,18 +512,27 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state)
          * Restricting protocols is only supported from 7.19.4 upwards.
          */
 #if LIBCURL_VERSION_NUM >= 0x071304
-        curl_easy_setopt(state->curl, CURLOPT_PROTOCOLS, PROTOCOLS);
-        curl_easy_setopt(state->curl, CURLOPT_REDIR_PROTOCOLS, PROTOCOLS);
+        if (curl_easy_setopt(state->curl, CURLOPT_PROTOCOLS, PROTOCOLS) ||
+            curl_easy_setopt(state->curl, CURLOPT_REDIR_PROTOCOLS, PROTOCOLS)) {
+            goto err;
+        }
 #endif
 
 #ifdef DEBUG_VERBOSE
-        curl_easy_setopt(state->curl, CURLOPT_VERBOSE, 1);
+        if (curl_easy_setopt(state->curl, CURLOPT_VERBOSE, 1)) {
+            goto err;
+        }
 #endif
     }
 
     state->s = s;
 
     return 0;
+
+err:
+    curl_easy_cleanup(state->curl);
+    state->curl = NULL;
+    return -EIO;
 }
 
 /* Called with s->mutex held.  */
@@ -763,10 +785,11 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->accept_range = false;
-    curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
-    curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
-                     curl_header_cb);
-    curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
+    if (curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1) ||
+        curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, curl_header_cb) ||
+        curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s)) {
+        goto out;
+    }
     if (curl_easy_perform(state->curl))
         goto out;
     if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) {
@@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
 
     snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end);
     trace_curl_setup_preadv(acb->bytes, start, state->range);
-    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
+    if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) {
+        curl_clean_state(state);
+        goto out;
+    }
 
     if (curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
         state->acb[0] = NULL;
-- 
2.25.1

On Fri, Jan 28, 2022 at 04:55:35PM +0000, Peter Maydell wrote:
> Coverity points out that we aren't checking the return value
> from curl_easy_setopt() for any of the calls to it we make
> in block/curl.c.
> 
> Fixes: Coverity CID 1459336, 1459482, 1460331
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Big fat disclaimer: tested only with 'make check', which I suspect
> may not be exercising this block backend. Hints on how to test
> more thoroughly are welcome.

yeah afaik, qemu-iotests don't have support for this. As a very
basic smoke test do

$ qemu-img info https://cloud.debian.org/images/cloud/buster/daily/latest/debian-10-nocloud-amd64-daily.qcow2
image: https://cloud.debian.org/images/cloud/buster/daily/latest/debian-10-nocloud-amd64-daily.qcow2
file format: qcow2
virtual size: 2 GiB (2147483648 bytes)
disk size: unavailable
cluster_size: 65536
Format specific information:
    compat: 1.1
    compression type: zlib
    lazy refcounts: false
    refcount bits: 16
    corrupt: false
    extended l2: false

and/or

$ qemu-img info --image-opts driver=qcow2,file.driver=https,file.url=https://cloud.debian.org/images/cloud/buster/daily/latest/debian-10-nocloud-amd64-daily.qcow2
image: https://cloud.debian.org/images/cloud/buster/daily/latest/debian-10-nocloud-amd64-daily.qcow2
file format: qcow2
virtual size: 2 GiB (2147483648 bytes)
disk size: unavailable
cluster_size: 65536
Format specific information:
    compat: 1.1
    compression type: zlib
    lazy refcounts: false
    refcount bits: 16
    corrupt: false
    extended l2: false


> 
>  block/curl.c | 90 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 58 insertions(+), 32 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 6a6cd729758..aaee1b17bef 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -458,38 +458,51 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state)
>          if (!state->curl) {
>              return -EIO;
>          }
> -        curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
> -        curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
> -                         (long) s->sslverify);
> -        curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYHOST,
> -                         s->sslverify ? 2L : 0L);
> -        if (s->cookie) {
> -            curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
> +        if (curl_easy_setopt(state->curl, CURLOPT_URL, s->url) ||
> +            curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
> +                             (long) s->sslverify) ||
> +            curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYHOST,
> +                             s->sslverify ? 2L : 0L)) {
> +            goto err;
> +        }
> +        if (s->cookie) {
> +            if (curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie)) {
> +                goto err;
> +            }
> +        }
> +        if (curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout) ||
> +            curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
> +                             (void *)curl_read_cb) ||
> +            curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state) ||
> +            curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state) ||
> +            curl_easy_setopt(state->curl, CURLOPT_AUTOREFERER, 1) ||
> +            curl_easy_setopt(state->curl, CURLOPT_FOLLOWLOCATION, 1) ||
> +            curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1) ||
> +            curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg) ||
> +            curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1)) {
> +            goto err;
>          }
> -        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout);
> -        curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
> -                         (void *)curl_read_cb);
> -        curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
> -        curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state);
> -        curl_easy_setopt(state->curl, CURLOPT_AUTOREFERER, 1);
> -        curl_easy_setopt(state->curl, CURLOPT_FOLLOWLOCATION, 1);
> -        curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1);
> -        curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg);
> -        curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1);
> -
>          if (s->username) {
> -            curl_easy_setopt(state->curl, CURLOPT_USERNAME, s->username);
> +            if (curl_easy_setopt(state->curl, CURLOPT_USERNAME, s->username)) {
> +                goto err;
> +            }
>          }
>          if (s->password) {
> -            curl_easy_setopt(state->curl, CURLOPT_PASSWORD, s->password);
> +            if (curl_easy_setopt(state->curl, CURLOPT_PASSWORD, s->password)) {
> +                goto err;
> +            }
>          }
>          if (s->proxyusername) {
> -            curl_easy_setopt(state->curl,
> -                             CURLOPT_PROXYUSERNAME, s->proxyusername);
> +            if (curl_easy_setopt(state->curl,
> +                                 CURLOPT_PROXYUSERNAME, s->proxyusername)) {
> +                goto err;
> +            }
>          }
>          if (s->proxypassword) {
> -            curl_easy_setopt(state->curl,
> -                             CURLOPT_PROXYPASSWORD, s->proxypassword);
> +            if (curl_easy_setopt(state->curl,
> +                                 CURLOPT_PROXYPASSWORD, s->proxypassword)) {
> +                goto err;
> +            }
>          }
>  
>          /* Restrict supported protocols to avoid security issues in the more
> @@ -499,18 +512,27 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state)
>           * Restricting protocols is only supported from 7.19.4 upwards.
>           */
>  #if LIBCURL_VERSION_NUM >= 0x071304
> -        curl_easy_setopt(state->curl, CURLOPT_PROTOCOLS, PROTOCOLS);
> -        curl_easy_setopt(state->curl, CURLOPT_REDIR_PROTOCOLS, PROTOCOLS);
> +        if (curl_easy_setopt(state->curl, CURLOPT_PROTOCOLS, PROTOCOLS) ||
> +            curl_easy_setopt(state->curl, CURLOPT_REDIR_PROTOCOLS, PROTOCOLS)) {
> +            goto err;
> +        }
>  #endif
>  
>  #ifdef DEBUG_VERBOSE
> -        curl_easy_setopt(state->curl, CURLOPT_VERBOSE, 1);
> +        if (curl_easy_setopt(state->curl, CURLOPT_VERBOSE, 1)) {
> +            goto err;
> +        }
>  #endif
>      }
>  
>      state->s = s;
>  
>      return 0;
> +
> +err:
> +    curl_easy_cleanup(state->curl);
> +    state->curl = NULL;
> +    return -EIO;
>  }
>  
>  /* Called with s->mutex held.  */
> @@ -763,10 +785,11 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      s->accept_range = false;
> -    curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
> -    curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
> -                     curl_header_cb);
> -    curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
> +    if (curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1) ||
> +        curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, curl_header_cb) ||
> +        curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s)) {
> +        goto out;
> +    }
>      if (curl_easy_perform(state->curl))
>          goto out;
>      if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) {
> @@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
>  
>      snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end);
>      trace_curl_setup_preadv(acb->bytes, start, state->range);
> -    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
> +    if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) {
> +        curl_clean_state(state);
> +        goto out;
> +    }
>  
>      if (curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
>          state->acb[0] = NULL;
> -- 
> 2.25.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Fri, 28 Jan 2022 at 17:50, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Fri, Jan 28, 2022 at 04:55:35PM +0000, Peter Maydell wrote:
> > Coverity points out that we aren't checking the return value
> > from curl_easy_setopt() for any of the calls to it we make
> > in block/curl.c.
> >
> > Fixes: Coverity CID 1459336, 1459482, 1460331
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Big fat disclaimer: tested only with 'make check', which I suspect
> > may not be exercising this block backend. Hints on how to test
> > more thoroughly are welcome.
>
> yeah afaik, qemu-iotests don't have support for this. As a very
> basic smoke test do
> [command lines snipped]

Thanks; yes, those both work.

-- PMM

On 28.01.22 17:55, Peter Maydell wrote:
> Coverity points out that we aren't checking the return value
> from curl_easy_setopt() for any of the calls to it we make
> in block/curl.c.
>
> Fixes: Coverity CID 1459336, 1459482, 1460331
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Big fat disclaimer: tested only with 'make check', which I suspect
> may not be exercising this block backend. Hints on how to test
> more thoroughly are welcome.
>
>   block/curl.c | 90 +++++++++++++++++++++++++++++++++-------------------
>   1 file changed, 58 insertions(+), 32 deletions(-)

One problem I see in general is that most of the setopt functions are 
(indirectly) called from `curl_open()`, which is supposed to return an 
error message.  Its `out` label seems to expect some error description 
in `state->errmsg`.  The error handling here doesn’t set such a description.

Then again, there are enough existing error paths that don’t set this 
description either, so it isn’t quite this patch’s duty to fix that 
situation.  I guess it would be nice if we had a wrapper for 
`curl_easy_setopt()` with an `Error **` parameter, so we could easily 
generate error messages that describe key and value (and then 
`curl_init_state()` should have an `Error **` parameter, too).

But this patch doesn’t make anything worse than it already is, so that’d 
rather be an idea for future clean-up.

> diff --git a/block/curl.c b/block/curl.c
> index 6a6cd729758..aaee1b17bef 100644
> --- a/block/curl.c
> +++ b/block/curl.c

[...]

> @@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
>   
>       snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end);
>       trace_curl_setup_preadv(acb->bytes, start, state->range);
> -    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
> +    if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) {
> +        curl_clean_state(state);
> +        goto out;

I think we need to mark the request as failed by setting `acb->ret` to a 
negative value (and probably also clear `state->acb[0]` like the error 
path below does).

Hanna

> +    }
>   
>       if (curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
>           state->acb[0] = NULL;

On Tue, 1 Feb 2022 at 11:25, Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 28.01.22 17:55, Peter Maydell wrote:
> > Coverity points out that we aren't checking the return value
> > from curl_easy_setopt() for any of the calls to it we make
> > in block/curl.c.
> >
> > Fixes: Coverity CID 1459336, 1459482, 1460331
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Big fat disclaimer: tested only with 'make check', which I suspect
> > may not be exercising this block backend. Hints on how to test
> > more thoroughly are welcome.
> >
> >   block/curl.c | 90 +++++++++++++++++++++++++++++++++-------------------
> >   1 file changed, 58 insertions(+), 32 deletions(-)
>
> One problem I see in general is that most of the setopt functions are
> (indirectly) called from `curl_open()`, which is supposed to return an
> error message.  Its `out` label seems to expect some error description
> in `state->errmsg`.  The error handling here doesn’t set such a description.

Ah, yes, I hadn't noticed that -- it's a pre-existing bug, where
we do this:

    if (curl_init_state(s, state) < 0) {
        goto out;
    }

and curl_init_state() already has an error path (for when curl_easy_init()
fails) that can take that goto without setting state->errmsg...

> Then again, there are enough existing error paths that don’t set this
> description either, so it isn’t quite this patch’s duty to fix that
> situation.

...as you've already noticed :-)

> I guess it would be nice if we had a wrapper for
> `curl_easy_setopt()` with an `Error **` parameter, so we could easily
> generate error messages that describe key and value (and then
> `curl_init_state()` should have an `Error **` parameter, too).
>
> But this patch doesn’t make anything worse than it already is, so that’d
> rather be an idea for future clean-up.
>
> > diff --git a/block/curl.c b/block/curl.c
> > index 6a6cd729758..aaee1b17bef 100644
> > --- a/block/curl.c
> > +++ b/block/curl.c
>
> [...]
>
> > @@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
> >
> >       snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end);
> >       trace_curl_setup_preadv(acb->bytes, start, state->range);
> > -    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
> > +    if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) {
> > +        curl_clean_state(state);
> > +        goto out;
>
> I think we need to mark the request as failed by setting `acb->ret` to a
> negative value (and probably also clear `state->acb[0]` like the error
> path below does).

OK; or I could roll the two curl calls into the same if:

    if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range) ||
        curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
        /* existing error handling and goto-out code */
    }

thanks
-- PMM

On 21.02.22 20:45, Peter Maydell wrote:
> On Tue, 1 Feb 2022 at 11:25, Hanna Reitz <hreitz@redhat.com> wrote:
>> On 28.01.22 17:55, Peter Maydell wrote:
>>> Coverity points out that we aren't checking the return value
>>> from curl_easy_setopt() for any of the calls to it we make
>>> in block/curl.c.
>>>
>>> Fixes: Coverity CID 1459336, 1459482, 1460331
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> Big fat disclaimer: tested only with 'make check', which I suspect
>>> may not be exercising this block backend. Hints on how to test
>>> more thoroughly are welcome.
>>>
>>>    block/curl.c | 90 +++++++++++++++++++++++++++++++++-------------------
>>>    1 file changed, 58 insertions(+), 32 deletions(-)
>> One problem I see in general is that most of the setopt functions are
>> (indirectly) called from `curl_open()`, which is supposed to return an
>> error message.  Its `out` label seems to expect some error description
>> in `state->errmsg`.  The error handling here doesn’t set such a description.
> Ah, yes, I hadn't noticed that -- it's a pre-existing bug, where
> we do this:
>
>      if (curl_init_state(s, state) < 0) {
>          goto out;
>      }
>
> and curl_init_state() already has an error path (for when curl_easy_init()
> fails) that can take that goto without setting state->errmsg...
>
>> Then again, there are enough existing error paths that don’t set this
>> description either, so it isn’t quite this patch’s duty to fix that
>> situation.
> ...as you've already noticed :-)
>
>> I guess it would be nice if we had a wrapper for
>> `curl_easy_setopt()` with an `Error **` parameter, so we could easily
>> generate error messages that describe key and value (and then
>> `curl_init_state()` should have an `Error **` parameter, too).
>>
>> But this patch doesn’t make anything worse than it already is, so that’d
>> rather be an idea for future clean-up.
>>
>>> diff --git a/block/curl.c b/block/curl.c
>>> index 6a6cd729758..aaee1b17bef 100644
>>> --- a/block/curl.c
>>> +++ b/block/curl.c
>> [...]
>>
>>> @@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
>>>
>>>        snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end);
>>>        trace_curl_setup_preadv(acb->bytes, start, state->range);
>>> -    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
>>> +    if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) {
>>> +        curl_clean_state(state);
>>> +        goto out;
>> I think we need to mark the request as failed by setting `acb->ret` to a
>> negative value (and probably also clear `state->acb[0]` like the error
>> path below does).
> OK; or I could roll the two curl calls into the same if:
>
>      if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range) ||
>          curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
>          /* existing error handling and goto-out code */
>      }

Sure, that sounds perfectly good to me.

Hanna