{"thread":{"id":"41408","subject":"[PATCH] remote-curl: Include curl_errorstr on SSL setup failures","startedAt":"2016-02-14T01:39:34Z","lastAt":"2016-02-14T18:04:24Z","messageCount":3,"participants":["Shawn Pearce","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"278090","messageId":"1455413974-25147-1-git-send-email-spearce@spearce.org","threadId":"41408","inReplyTo":null,"subject":"[PATCH] remote-curl: Include curl_errorstr on SSL setup failures","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2016-02-14T01:39:34Z","receivedAt":"2016-02-14T01:39:34Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"For curl error 35 (CURLE_SSL_CONNECT_ERROR) users need the\nadditional text stored in CURLOPT_ERRORBUFFER to debug why\nthe connection did not start. This is curl_errorstr inside\nof http.c, so include that in the message if it is non-empty.\n\nSometimes HTTP response codes aren't yet available, such as\nwhen the SSL setup fails. Don't include HTTP 0 in the message.\n\nSigned-off-by: Shawn Pearce <spearce@spearce.org>\n---\n remote-curl.c | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex c704857..f611432 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -439,8 +439,20 @@ static int run_slot(struct active_request_slot *slot,\n \terr = run_one_slot(slot, results);\n \n \tif (err != HTTP_OK && err != HTTP_REAUTH) {\n-\t\terror(\"RPC failed; result=%d, HTTP code = %ld\",\n-\t\t      results->curl_result, results->http_code);\n+\t\tstruct strbuf msg = STRBUF_INIT;\n+\t\tif (results->http_code && results->http_code != 200)\n+\t\t\tstrbuf_addf(&msg, \"HTTP %ld\", results->http_code);\n+\t\tif (results->curl_result != CURLE_OK) {\n+\t\t\tif (msg.len)\n+\t\t\t\tstrbuf_addch(&msg, ' ');\n+\t\t\tstrbuf_addf(&msg, \"curl %d\", results->curl_result);\n+\t\t\tif (curl_errorstr[0]) {\n+\t\t\t\tstrbuf_addch(&msg, ' ');\n+\t\t\t\tstrbuf_addstr(&msg, curl_errorstr);\n+\t\t\t}\n+\t\t}\n+\t\terror(\"RPC failed; %s\", msg.buf);\n+\t\tstrbuf_release(&msg);\n \t}\n \n \treturn err;\n-- \n2.7.0\n"},{"id":"278115","messageId":"20160214165048.GA10219@sigill.intra.peff.net","threadId":"41408","inReplyTo":"1455413974-25147-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] remote-curl: Include curl_errorstr on SSL setup failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-14T16:50:48Z","receivedAt":"2016-02-14T16:50:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2016 at 05:39:34PM -0800, Shawn Pearce wrote:\n\n> For curl error 35 (CURLE_SSL_CONNECT_ERROR) users need the\n> additional text stored in CURLOPT_ERRORBUFFER to debug why\n> the connection did not start. This is curl_errorstr inside\n> of http.c, so include that in the message if it is non-empty.\n\nI think this is an all-around improvement. GitHub sometimes get support\nrequests for result=18 on a git-push, which is curl's code for \"I dunno,\nthe remote end hung up\". Having a human-readable message may make things\nless confusing.\n\nUnfortunately I cannot seem to create the problem at will to confirm\nthat it kicks in in this case[1], but it seems like it should just based on\nreading your patch.\n\n-Peff\n\n[1] I tried inserting \"exit(0)\" in various places of receive-pack, and\n    it seems make the protocol deadlock. Yikes.\n"},{"id":"278118","messageId":"CAJo=hJsks9h-PDxrTYWOh0xEz150-q3gjyGNz9p3qhdqqQUM2A@mail.gmail.com","threadId":"41408","inReplyTo":"20160214165048.GA10219@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: Include curl_errorstr on SSL setup failures","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2016-02-14T18:04:24Z","receivedAt":"2016-02-14T18:04:24Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sun, Feb 14, 2016 at 8:50 AM, Jeff King <peff@peff.net> wrote:\n> On Sat, Feb 13, 2016 at 05:39:34PM -0800, Shawn Pearce wrote:\n>\n>> For curl error 35 (CURLE_SSL_CONNECT_ERROR) users need the\n>> additional text stored in CURLOPT_ERRORBUFFER to debug why\n>> the connection did not start. This is curl_errorstr inside\n>> of http.c, so include that in the message if it is non-empty.\n>\n> I think this is an all-around improvement. GitHub sometimes get support\n> requests for result=18 on a git-push, which is curl's code for \"I dunno,\n> the remote end hung up\". Having a human-readable message may make things\n> less confusing.\n\nI have been trying to recreate the error 35 scenario, but after almost\na day of continuous attempts with a suspected broken Git I thus far\nhave not been able to reproduce it. *sigh*\n\nWhat made me pick this up was I got another recent report of a Debian\nsystem having trouble connecting to $DAY_JOB's HTTPS server, which\nreminded me of [2]. I was going to revive that patch, but instead\ndecided to expand on the error reporting.\n\n[2] http://article.gmane.org/gmane.comp.version-control.git/206770\n\n> Unfortunately I cannot seem to create the problem at will to confirm\n> that it kicks in in this case[1], but it seems like it should just based on\n> reading your patch.\n>\n> -Peff\n>\n> [1] I tried inserting \"exit(0)\" in various places of receive-pack, and\n>     it seems make the protocol deadlock. Yikes.\n\nThis actually doesn't surprise me. *sigh*\n\nWith all the pipes and libcurl in there, we must somewhere be ignoring\nthe fact that the HTTP server closed the connection.\n"}]}