block/curl: should we be checking curl_easy_setopt() for errors?

block/curl: should we be checking curl_easy_setopt() for errors?

[Peter Maydell]

Coverity complains (CID 1460331, 1459482, 1459336, 1458895)
that we call curl_easy_setopt(), which can return an error value,
but we never check the return value.

Is it correct? Looking at the libcurl documentation, the function
does return an error status, and there's nothing that says it's
ok to ignore (e.g. that it's guaranteed that the library will
safely accumulate any errors and return them when you make the
subsequent curl_easy_perform() call). On the other hand, neither
the libcurl manpage example nor the handful of example programs
at https://curl.se/libcurl/c/example.html ever seem to check the
return value from curl_easy_setopt()...

thanks
-- PMM

Re: block/curl: should we be checking curl_easy_setopt() for errors?

[Daniel P. Berrangé]

On Mon, Aug 30, 2021 at 04:34:56PM +0100, Peter Maydell wrote:
> Coverity complains (CID 1460331, 1459482, 1459336, 1458895)
> that we call curl_easy_setopt(), which can return an error value,
> but we never check the return value.
> 
> Is it correct? Looking at the libcurl documentation, the function
> does return an error status, and there's nothing that says it's
> ok to ignore (e.g. that it's guaranteed that the library will
> safely accumulate any errors and return them when you make the
> subsequent curl_easy_perform() call). On the other hand, neither
> the libcurl manpage example nor the handful of example programs
> at https://curl.se/libcurl/c/example.html ever seem to check the
> return value from curl_easy_setopt()...

Options that accept a string can return CURLE_OUT_OF_MEMORY from
curl_easy_setopt.Most other failures seem to be reporting caller
errors such as forgotten arguments, or too long strings.

Does look like we ought to check return status though for at
least some of the options, and if you check it for some then
coverity will also complain if you don't check it for all.


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

Re: block/curl: should we be checking curl_easy_setopt() for errors?

[Peter Maydell]

On Tue, 31 Aug 2021 at 09:44, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Aug 30, 2021 at 04:34:56PM +0100, Peter Maydell wrote:
> > Coverity complains (CID 1460331, 1459482, 1459336, 1458895)
> > that we call curl_easy_setopt(), which can return an error value,
> > but we never check the return value.
> >
> > Is it correct? Looking at the libcurl documentation, the function
> > does return an error status, and there's nothing that says it's
> > ok to ignore (e.g. that it's guaranteed that the library will
> > safely accumulate any errors and return them when you make the
> > subsequent curl_easy_perform() call). On the other hand, neither
> > the libcurl manpage example nor the handful of example programs
> > at https://curl.se/libcurl/c/example.html ever seem to check the
> > return value from curl_easy_setopt()...
>
> Options that accept a string can return CURLE_OUT_OF_MEMORY from
> curl_easy_setopt.Most other failures seem to be reporting caller
> errors such as forgotten arguments, or too long strings.
>
> Does look like we ought to check return status though for at
> least some of the options, and if you check it for some then
> coverity will also complain if you don't check it for all.

Coverity complains about them all regardless -- I think that the scan
servers have been updated with models for some of these common
software libraries, and it now "knows" that the function has
an error-return that must be checked.

-- PMM