On Mon, Oct 14, 2013 at 05:27:39AM +0000, Eric Wong wrote: > Daniel Stenberg <daniel@xxxxxxx> wrote: > > On Sat, 12 Oct 2013, Eric Wong wrote: > > > > >This is a follow up to commit > > >e47a8583a20256851e7fc882233e3bd5bf33dc6e (enable SO_KEEPALIVE for > > >connected TCP sockets). > > > > Just keep in mind that TCP keep-alive is enabled in awkwardly many > > different ways on different systems and this patch only supports one > > of them. Feel free to take inspiration from libcurl's source code > > for doing this. See: > > > > https://github.com/bagder/curl/blob/master/lib/connect.c#L108 > > Thanks. I think the Linux-specific TCP_KEEP* knobs are overkill for git. > (since this is mainly for non-interactive users, I went at least a day > before realizing the process was stuck on my machine). > I cannot comment on the knobs for other OSes. I don't think we should get into having a big compatibility layer that just reproduces what is in curl. But is there any reason not to use CURLOPT_TCP_KEEPALIVE when it is available, falling back to CURLOPT_SOCKOPTFUNCTION, and then finally to nothing? That lets people on modern curl benefit from curl's more portable code, without punishing people on older versions. I.e., something like: --- diff --git a/http.c b/http.c index 5834c9b..e221efb 100644 --- a/http.c +++ b/http.c @@ -254,36 +254,54 @@ static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type) if (!cert_auth.password) { cert_auth.protocol = xstrdup("cert"); cert_auth.username = xstrdup(""); cert_auth.path = xstrdup(ssl_cert); credential_fill(&cert_auth); } return 1; } -/* curl 7.25.0 has CURLOPT_TCP_KEEPALIVE, too, but we support older curl */ +#if LIBCURL_VERSION_NUM >= 0x071900 +static void set_curl_keepalive(CURL *c) +{ + curl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1); +} + +#elif LIBCURL_VERSION_NUM >= 0x071000 static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type) { int ka = 1; int rc; socklen_t len = (socklen_t)sizeof(ka); if (type != CURLSOCKTYPE_IPCXN) return 0; rc = setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, (void *)&ka, len); if (rc < 0) warning("unable to set SO_KEEPALIVE on socket %s", strerror(errno)); return 0; /* CURL_SOCKOPT_OK only exists since curl 7.21.5 */ } +static void set_curl_keepalive(CURL *c) +{ + curl_easy_setopt(c, CURLOPT_SOCKOPTFUNCTION, sockopt_callback); +} + +#else +static void set_curl_keepalive(CURL *c) +{ + /* not supported on older curl versions */ +} +#endif + static CURL *get_curl_handle(void) { CURL *result = curl_easy_init(); if (!curl_ssl_verify) { curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0); curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 0); } else { /* Verify authenticity of the peer's certificate */ @@ -344,21 +362,19 @@ static CURL *get_curl_handle(void) if (curl_ssl_try) curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY); #endif if (curl_http_proxy) { curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy); curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY); } -#if LIBCURL_VERSION_NUM >= 0x071000 - curl_easy_setopt(result, CURLOPT_SOCKOPTFUNCTION, sockopt_callback); -#endif + set_curl_keepalive(result); return result; } static void set_from_env(const char **var, const char *envname) { const char *val = getenv(envname); if (val) *var = val; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html