[Spice-devel] [spice-server v2 3/6] net: Introduce red_socket_set_keepalive() helper

[Frediano Ziglio]

> 
> This allows to move some low-level code out of reds.c
> 
> Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> ---
>  server/net-utils.c | 25 +++++++++++++++++++++++++
>  server/net-utils.h |  1 +
>  server/reds.c      | 27 ++-------------------------
>  3 files changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/server/net-utils.c b/server/net-utils.c
> index ce409be..995b0d4 100644
> --- a/server/net-utils.c
> +++ b/server/net-utils.c
> @@ -31,6 +31,31 @@
>  
>  #include "net-utils.h"
>  
> +bool red_socket_set_keepalive(int fd, bool enable, int timeout)
> +{
> +    int keepalive = !!enable;
> +
> +    if (setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, &keepalive,
> sizeof(keepalive)) == -1) {
> +        if (errno != ENOTSUP) {
> +            spice_printerr("setsockopt for keepalive failed, %s",
> strerror(errno));
> +            return false;
> +        }
> +    }
> +
> +    if (!enable) {
> +        return true;
> +    }
> +
> +    if (setsockopt(fd, SOL_TCP, TCP_KEEPIDLE, &timeout, sizeof(timeout)) ==
> -1) {

About this value. NetBSD, Windows and Mac use milliseconds.
If I would to write a portable wrapper I would go for milliseconds
even if honestly I don't feel the need for such precision.

> +        if (errno != ENOTSUP) {
> +            spice_printerr("setsockopt for keepalive timeout failed, %s",
> strerror(errno));
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  bool red_socket_set_no_delay(int fd, bool no_delay)
>  {
>      int delay_val = no_delay;
> diff --git a/server/net-utils.h b/server/net-utils.h
> index aed6956..d06932d 100644
> --- a/server/net-utils.h
> +++ b/server/net-utils.h
> @@ -18,6 +18,7 @@
>  #ifndef _H_RED_NET_UTILS
>  #define _H_RED_NET_UTILS
>  
> +bool red_socket_set_keepalive(int fd, bool enable, int timeout);

Why not adding some documentation comment too?
(this and other patches)

>  bool red_socket_set_no_delay(int fd, bool no_delay);
>  bool red_socket_set_non_blocking(int fd, bool non_blocking);
>  
> diff --git a/server/reds.c b/server/reds.c
> index 12c797b..024ff63 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -73,6 +73,7 @@
>  #include "main-channel-client.h"
>  #include "red-client.h"
>  #include "glib-compat.h"
> +#include "net-utils.h"
>  
>  #define REDS_MAX_STAT_NODES 100
>  
> @@ -2377,39 +2378,15 @@ static void reds_handle_ssl_accept(int fd, int event,
> void *data)
>  
>  #define KEEPALIVE_TIMEOUT (10*60)
>  
> -static bool reds_init_keepalive(int socket)
> -{
> -    int keepalive = 1;
> -    int keepalive_timeout = KEEPALIVE_TIMEOUT;
> -
> -    if (setsockopt(socket, SOL_SOCKET, SO_KEEPALIVE, &keepalive,
> sizeof(keepalive)) == -1) {
> -        if (errno != ENOTSUP) {
> -            spice_printerr("setsockopt for keepalive failed, %s",
> strerror(errno));
> -            return false;
> -        }
> -    }
> -
> -    if (setsockopt(socket, SOL_TCP, TCP_KEEPIDLE,
> -                   &keepalive_timeout, sizeof(keepalive_timeout)) == -1) {
> -        if (errno != ENOTSUP) {
> -            spice_printerr("setsockopt for keepalive timeout failed, %s",
> strerror(errno));
> -            return false;
> -        }
> -    }
> -
> -    return true;
> -}
> -
>  static RedLinkInfo *reds_init_client_connection(RedsState *reds, int socket)
>  {
>      RedLinkInfo *link;
>  
> -    reds_init_keepalive(socket);
> -
>      if (!red_socket_set_non_blocking(socket, TRUE))
>      {
>         goto error;
>      }
> +    red_socket_set_keepalive(socket, TRUE, KEEPALIVE_TIMEOUT);
>  

Here, like in a previous order you revert the setting of these
2 flags (blocking and keep alive).
If the end result is the same order I would always keep the same
order.
I assume you did some split of patches and didn't notice.

>      link = spice_new0(RedLinkInfo, 1);
>      link->reds = reds;

Frediano