lib/dload: enforce usage of TCP keepalives
 

On Sun, Jan 22, 2012 at 7:31 PM, Dave Reisner <d@falconindy.com> wrote:
> This is particularly important in the case of FTP control connections,
> which may be closed by rogue NAT/firewall devices detecting idle
> connections on larger transfers which may take 5-10+ minutes.
>
> Signed-off-by: Dave Reisner <dreisner@archlinux.org>
> ---
> This is basically on the advice of Daniel Stenberg, who mentions that it's a
> common problem that he's aware of. I have no idea if this is actually the fix
> we're looking for and I'm still unable to reproduce, but it seems plausible
> solution.
>
> Also, fuck FTP.
>
> *configure.ac * * * *| * *7 +++--
> *lib/libalpm/dload.c | * 59 ++++++++++++++++++++++++++++++++++++++++++++++++++ +
> *2 files changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 6941054..c8ba03f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -174,9 +174,10 @@ AM_CONDITIONAL([HAVE_LIBGPGME], [test "x$with_gpgme" = "xyes"])
>
> *# Checks for header files.
> *AC_CHECK_HEADERS([fcntl.h float.h glob.h libintl.h limits.h locale.h
> - * * * * * * * * *mntent.h stddef.h string.h sys/ioctl.h sys/mount.h
> - * * * * * * * * *sys/param.h sys/statvfs.h sys/time.h sys/types.h
> - * * * * * * * * *sys/ucred.h syslog.h termios.h wchar.h])
> + * * * * * * * * *mntent.h netinet/in.h netinet/tcp.h stddef.h string.h
> + * * * * * * * * *sys/ioctl.h sys/mount.h sys/param.h sys/statvfs.h
> + * * * * * * * * *sys/time.h sys/types.h sys/ucred.h syslog.h termios.h
> + * * * * * * * * *wchar.h])
>
> *# Checks for typedefs, structures, and compiler characteristics.
> *AC_C_INLINE
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index a3980d6..790080c 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -23,11 +23,19 @@
> *#include <errno.h>
> *#include <string.h>
> *#include <unistd.h>
> +#include <sys/socket.h> /* setsockopt, SO_KEEPALIVE */
> *#include <sys/time.h>
> *#include <sys/types.h>
> *#include <sys/stat.h>
> *#include <signal.h>
>
> +#ifdef _HAVE_NETINET_IN_H
> +#include <netinet/in.h> /* IPPROTO_TCP */
> +#endif
> +#ifdef _HAVE_NETINET_TCP_H
> +#include <netinet/tcp.h> /* TCP_KEEPINTVL, TCP_KEEPIDLE */
> +#endif
> +
> *#ifdef HAVE_LIBCURL
> *#include <curl/curl.h>
> *#endif
> @@ -215,6 +223,53 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user)
> * * * *return realsize;
> *}
>
> +static int curl_sockopt_cb(void *userdata, curl_socket_t curlfd,
> + * * * * * * * curlsocktype purpose)
> +{
> + * * * alpm_handle_t *handle = userdata;
> +
> + * * * int onoff = 1;
> +
> +#ifdef TCP_KEEPIDLE
> + * * * int keepidle = TCP_KEEPIDLE;
> +#endif
> +#ifdef TCP_KEEPINTVL
> + * * * int keepintvl = TCP_KEEPINTVL;
> +#endif
> +
> + * * * switch(purpose) {
> + * * * * * * * case CURLSOCKTYPE_IPCXN:
> + * * * * * * * * * * * if(setsockopt(curlfd, SOL_SOCKET, SO_KEEPALIVE, (void *)&onoff,
> + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * sizeof(onoff)) < 0) {
> + * * * * * * * * * * * * * * * /* don't abort operation, just log to debug */
> + * * * * * * * * * * * * * * * _alpm_log(handle, ALPM_LOG_DEBUG, "Failed to set SO_KEEPALIVE on fd %d
",
> + * * * * * * * * * * * * * * * * * * * * * * * curlfd);
This makes sense...

> + * * * * * * * * * * * }
> + * * * * * * * * * * * else {
> +#ifdef TCP_KEEPIDLE
> + * * * * * * * * * * * * * * * if(setsockopt(curlfd, IPPROTO_TCP, TCP_KEEPIDLE, (void *)&keepidle,
> + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * sizeof(keepidle)) < 0) {
> + * * * * * * * * * * * * * * * * * * * /* don't abort operation, just log to debug */
> + * * * * * * * * * * * * * * * * * * * _alpm_log(handle, ALPM_LOG_DEBUG, "Failed to set TCP_KEEPIDLE on fd %d
",
> + * * * * * * * * * * * * * * * * * * * * * * * * * * * curlfd);
> + * * * * * * * * * * * * * * * }
> +#endif
> +#ifdef TCP_KEEPINTVL
> + * * * * * * * * * * * * * * * if(setsockopt(curlfd, IPPROTO_TCP, TCP_KEEPINTVL, (void *)&keepintvl,
> + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * sizeof(keepintvl)) < 0) {
> + * * * * * * * * * * * * * * * * * * * /* don't abort operation, just log to debug */
> + * * * * * * * * * * * * * * * * * * * _alpm_log(handle, ALPM_LOG_DEBUG, "Failed to set TCP_KEEPINTVL on fd %d
",
> + * * * * * * * * * * * * * * * * * * * * * * * * * * * curlfd);
> + * * * * * * * * * * * * * * * }
> +#endif
These, however, seem a bit suspect.

1) Linux-specific (although you guarded it fine, this is quite ugly).
2) Looks like the default keepidle value is 7200 secs; aka 2 hours.
This is far too long, but do we need to be messing with keepintvl?
That defaults to 75 seconds which seems relatively sane.
3) Why are you using TCP_KEEPIDLE and TCP_KEEPINTVL for both the
option_name AND option_value? Passing a logical value rather than the
arbitrary constant values (4 and 5) would seem to make more sense in
any case. Probably something like 60 seconds for both would work.

> + * * * * * * * * * * * }
> + * * * * * * * default:
> + * * * * * * * * * * * break;
> + * * * }
> +
> + * * * return 0;
> +}
> +
> *static void curl_set_handle_opts(struct dload_payload *payload,
> * * * * * * * *CURL *curl, char *error_buffer)
> *{
> @@ -239,6 +294,8 @@ static void curl_set_handle_opts(struct dload_payload *payload,
> * * * *curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, parse_headers);
> * * * *curl_easy_setopt(curl, CURLOPT_WRITEHEADER, (void *)payload);
> * * * *curl_easy_setopt(curl, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
> + * * * curl_easy_setopt(curl, CURLOPT_SOCKOPTFUNCTION, curl_sockopt_cb);
> + * * * curl_easy_setopt(curl, CURLOPT_SOCKOPTDATA, (void *)handle);
>
> * * * *_alpm_log(handle, ALPM_LOG_DEBUG, "url: %s
", payload->fileurl);
>
> @@ -388,6 +445,8 @@ static int curl_download_internal(struct dload_payload *payload,
>
> * * * */* perform transfer */
> * * * *payload->curlerr = curl_easy_perform(curl);
> + * * * _alpm_log(handle, ALPM_LOG_DEBUG, "curl returned error %d from transfer
",
> + * * * * * * * * * * * payload->curlerr);
>
> * * * */* disconnect relationships from the curl handle for things that might go out
> * * * * * of scope, but could still be touched on connection teardown. *This really
> --
> 1.7.8.4
>
>