On Wed, Sep 21, 2016 at 10:29:59PM +0000, Eric Wong wrote:
> Jeff King <peff@xxxxxxxx> wrote:
> > On Wed, Sep 21, 2016 at 09:46:23PM +0000, Eric Wong wrote:
> >
> > > -----------8<-----------
> > > Subject: [PATCH] http: check curl_multi_remove_handle error code
> > >
> > > This should help detect bugs in future changes. While we're at
> > > it, fix a (probably innocuous) bug in our http_cleanup function
> > > for users of older curl.
> > >
> > > curl_multi_remove_handle was not idempotent until curl 7.33.0
> > > with commit 84f3b3dd448399f9548468676e1bd1475dba8de5
> > > ("curl_multi_remove_handle: allow multiple removes"),
> > > so we track the "curlm" membership of the curl easy handle
> > > ourselves with a new "in_multi" flag.
> >
> > Does curl provide a meaningful error here? I'm just wondering if we
> > could simply let curl handle this, and just ignore the error that comes
> > from older versions. We're basically just replicating curl's own state
> > data here.
>
> curl before 7.33.0 returned CURLM_BAD_EASY_HANDLE. This error
> code also happens if we pass a bad/corrupt easy handle;
> so it could be hiding an error on our end.
Hmm, yeah. So we would want to make it conditional on the version
anyway, so that we detected such bugs on more modern systems. That still
doesn't seem unreasonable to me, but your patch is also not very big, so
it's probably fine (the major risk with yours is simply that our state
and curl's state get out of sync).
-Peff