[PATCH] remote-curl: Include curl_errorstr on SSL setup failures

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] remote-curl: Include curl_errorstr on SSL setup failures

Shawn Pearce
For curl error 35 (CURLE_SSL_CONNECT_ERROR) users need the
additional text stored in CURLOPT_ERRORBUFFER to debug why
the connection did not start. This is curl_errorstr inside
of http.c, so include that in the message if it is non-empty.

Sometimes HTTP response codes aren't yet available, such as
when the SSL setup fails. Don't include HTTP 0 in the message.

Signed-off-by: Shawn Pearce <[hidden email]>
---
 remote-curl.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index c704857..f611432 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -439,8 +439,20 @@ static int run_slot(struct active_request_slot *slot,
  err = run_one_slot(slot, results);
 
  if (err != HTTP_OK && err != HTTP_REAUTH) {
- error("RPC failed; result=%d, HTTP code = %ld",
-      results->curl_result, results->http_code);
+ struct strbuf msg = STRBUF_INIT;
+ if (results->http_code && results->http_code != 200)
+ strbuf_addf(&msg, "HTTP %ld", results->http_code);
+ if (results->curl_result != CURLE_OK) {
+ if (msg.len)
+ strbuf_addch(&msg, ' ');
+ strbuf_addf(&msg, "curl %d", results->curl_result);
+ if (curl_errorstr[0]) {
+ strbuf_addch(&msg, ' ');
+ strbuf_addstr(&msg, curl_errorstr);
+ }
+ }
+ error("RPC failed; %s", msg.buf);
+ strbuf_release(&msg);
  }
 
  return err;
--
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] remote-curl: Include curl_errorstr on SSL setup failures

Jeff King
On Sat, Feb 13, 2016 at 05:39:34PM -0800, Shawn Pearce wrote:

> For curl error 35 (CURLE_SSL_CONNECT_ERROR) users need the
> additional text stored in CURLOPT_ERRORBUFFER to debug why
> the connection did not start. This is curl_errorstr inside
> of http.c, so include that in the message if it is non-empty.

I think this is an all-around improvement. GitHub sometimes get support
requests for result=18 on a git-push, which is curl's code for "I dunno,
the remote end hung up". Having a human-readable message may make things
less confusing.

Unfortunately I cannot seem to create the problem at will to confirm
that it kicks in in this case[1], but it seems like it should just based on
reading your patch.

-Peff

[1] I tried inserting "exit(0)" in various places of receive-pack, and
    it seems make the protocol deadlock. Yikes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] remote-curl: Include curl_errorstr on SSL setup failures

Shawn Pearce
On Sun, Feb 14, 2016 at 8:50 AM, Jeff King <[hidden email]> wrote:

> On Sat, Feb 13, 2016 at 05:39:34PM -0800, Shawn Pearce wrote:
>
>> For curl error 35 (CURLE_SSL_CONNECT_ERROR) users need the
>> additional text stored in CURLOPT_ERRORBUFFER to debug why
>> the connection did not start. This is curl_errorstr inside
>> of http.c, so include that in the message if it is non-empty.
>
> I think this is an all-around improvement. GitHub sometimes get support
> requests for result=18 on a git-push, which is curl's code for "I dunno,
> the remote end hung up". Having a human-readable message may make things
> less confusing.

I have been trying to recreate the error 35 scenario, but after almost
a day of continuous attempts with a suspected broken Git I thus far
have not been able to reproduce it. *sigh*

What made me pick this up was I got another recent report of a Debian
system having trouble connecting to $DAY_JOB's HTTPS server, which
reminded me of [2]. I was going to revive that patch, but instead
decided to expand on the error reporting.

[2] http://article.gmane.org/gmane.comp.version-control.git/206770

> Unfortunately I cannot seem to create the problem at will to confirm
> that it kicks in in this case[1], but it seems like it should just based on
> reading your patch.
>
> -Peff
>
> [1] I tried inserting "exit(0)" in various places of receive-pack, and
>     it seems make the protocol deadlock. Yikes.

This actually doesn't surprise me. *sigh*

With all the pipes and libcurl in there, we must somewhere be ignoring
the fact that the HTTP server closed the connection.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html