Re: [PATCH v2 2/2] block/curl.c: Check error return from curl_easy_setopt()

[Peter Maydell]

On Thu, 24 Feb 2022 at 14:11, Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 22.02.22 16:23, 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.
> >
> > Some of these options are documented as always succeeding (e.g.
> > CURLOPT_VERBOSE) but others have documented failure cases (e.g.
> > CURLOPT_URL).  For consistency we check every call, even the ones
> > that theoretically cannot fail.
> >
> > Fixes: Coverity CID 1459336, 1459482, 1460331
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Changes v1->v2:
> >   * set the error string in the failure path for the
> >     direct setopt calls in curl_open()
> >   * fix the failure path in curl_setup_preadv() by putting
> >     the curl_easy_setopt() call in the same if() condition
> >     as the existing curl_multi_add_handle()
> > ---
> >   block/curl.c | 92 +++++++++++++++++++++++++++++++++-------------------
> >   1 file changed, 58 insertions(+), 34 deletions(-)
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>

For the record, I had a late thought that maybe we were setting
some optional-for-us options that were only added in later versions
of libcurl and accidentally relying on not checking the error code.
But it turns out this isn't the case: most of the options we set
are always supported, and the exceptions are:
 NOSIGNAL -- 7.10 onward
 PRIVATE -- 7.10.3 onward
 USERNAME, PASSWORD, PROXYUSERNAME, PROXYPASSWORD -- 7.19.1 onward,
  but we only set these if the user asked for them, so failing
  would be the right thing anyway
 PROTOCOLS, REDIR_PROTOCOLS -- 7.19.4 onward, guarded by a
  compile-time LIBCURL_VERSION_NUM check

And in any case our meson.build insists on at least 7.29.
(That means we could drop the LIBCURL_VERSION_NUM guards, I guess.)

-- PMM