[Spice-devel] [spice-server 1/3] reds-stream: Introduce reds_stream_set_no_delay() helper

[Frediano Ziglio]

> 
> The code to enable/disable on a TCP socket is duplicated in multiple
> places in the code base, this commit replaces this duplicated code with
> a helper in RedsStream.
> ---
> 
> This patch has already been sent a few times with various variations around
> bool/gboolean/true/TRUE. I'd like to stick with things as they are in this
> patch now as this is consistent with the rest of the reds-stream.c code.
> 

I would prefer to have a patch to fix the incoherences in reds-stream.c
before.

But beside this I think these function would be better into a new file
dealing with lower level sockets instead of RedsStream.
Setting these features on the stream make the function less reusable.
For instance the keep alive setting could be even moved to spice-common
and reused in the client code.
And for instance you can set no blocking on socket before creating
the RedsStream.

>  server/common-graphics-channel.c | 13 ++-----------
>  server/red-channel-client.c      | 17 ++---------------
>  server/reds-stream.c             | 16 ++++++++++++++++
>  server/reds-stream.h             |  1 +
>  server/reds.c                    |  8 +-------
>  server/sound.c                   |  8 +-------
>  6 files changed, 23 insertions(+), 40 deletions(-)
> 
> diff --git a/server/common-graphics-channel.c
> b/server/common-graphics-channel.c
> index 394a68e..4731939 100644
> --- a/server/common-graphics-channel.c
> +++ b/server/common-graphics-channel.c
> @@ -19,9 +19,6 @@
>  #endif
>  
>  #include <fcntl.h>
> -#include <sys/socket.h>
> -#include <netinet/in.h>
> -#include <netinet/tcp.h>
>  
>  #include "common-graphics-channel.h"
>  #include "dcc.h"
> @@ -125,24 +122,18 @@ int common_channel_config_socket(RedChannelClient *rcc)
>      RedClient *client = red_channel_client_get_client(rcc);
>      MainChannelClient *mcc = red_client_get_main(client);
>      RedsStream *stream = red_channel_client_get_stream(rcc);
> -    int delay_val;
>      gboolean is_low_bandwidth;
>  
>      // TODO - this should be dynamic, not one time at channel creation
>      is_low_bandwidth = main_channel_client_is_low_bandwidth(mcc);
> -    delay_val = is_low_bandwidth ? 0 : 1;
>      /* FIXME: Using Nagle's Algorithm can lead to apparent delays, depending
>       * on the delayed ack timeout on the other side.
>       * Instead of using Nagle's, we need to implement message buffering on
>       * the application level.
>       * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
>       */
> -    if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, &delay_val,
> -                   sizeof(delay_val)) == -1) {
> -        if (errno != ENOTSUP) {
> -            spice_warning("setsockopt failed, %s", strerror(errno));
> -        }
> -    }
> +    reds_stream_set_no_delay(stream, is_low_bandwidth);
> +
>      // TODO: move wide/narrow ack setting to red_channel.
>      red_channel_client_ack_set_client_window(rcc,
>          is_low_bandwidth ?
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 807a61f..393ed0c 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -570,13 +570,7 @@ static void
> red_channel_client_send_ping(RedChannelClient *rcc)
>          }  else {
>              rcc->priv->latency_monitor.tcp_nodelay = delay_val;
>              if (!delay_val) {
> -                delay_val = 1;
> -                if (setsockopt(rcc->priv->stream->socket, IPPROTO_TCP,
> TCP_NODELAY, &delay_val,
> -                               sizeof(delay_val)) == -1) {
> -                   if (errno != ENOTSUP) {
> -                        spice_warning("setsockopt failed, %s",
> strerror(errno));
> -                    }
> -                }
> +                reds_stream_set_no_delay(rcc->priv->stream, TRUE);
>              }
>          }
>      }
> @@ -1373,14 +1367,7 @@ static void
> red_channel_client_handle_pong(RedChannelClient *rcc, SpiceMsgPing *
>  
>      /* set TCP_NODELAY=0, in case we reverted it for the test*/
>      if (!rcc->priv->latency_monitor.tcp_nodelay) {
> -        int delay_val = 0;
> -
> -        if (setsockopt(rcc->priv->stream->socket, IPPROTO_TCP, TCP_NODELAY,
> &delay_val,
> -                       sizeof(delay_val)) == -1) {
> -            if (errno != ENOTSUP) {
> -                spice_warning("setsockopt failed, %s", strerror(errno));
> -            }
> -        }
> +        reds_stream_set_no_delay(rcc->priv->stream, FALSE);
>      }
>  
>      /*
> diff --git a/server/reds-stream.c b/server/reds-stream.c
> index 910385b..8faa174 100644
> --- a/server/reds-stream.c
> +++ b/server/reds-stream.c
> @@ -21,6 +21,7 @@
>  
>  #include <errno.h>
>  #include <netdb.h>
> +#include <netinet/tcp.h>
>  #include <unistd.h>
>  #include <sys/socket.h>
>  #include <fcntl.h>
> @@ -256,6 +257,21 @@ int reds_stream_is_plain_unix(const RedsStream *s)
>  
>  }
>  
> +bool reds_stream_set_no_delay(RedsStream *stream, bool no_delay)
> +{
> +    int delay_val = no_delay;
> +
> +    if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
> +                   &delay_val, sizeof(delay_val)) != 0) {
> +        if (errno != ENOTSUP && errno != ENOPROTOOPT) {
> +            spice_printerr("setsockopt failed, %s", strerror(errno));

why not spice_warning? One of the usage was using spice_warning
and would remove a direct print on stderr.

> +            return FALSE;
> +        }
> +    }
> +
> +    return TRUE;
> +}
> +
>  int reds_stream_send_msgfd(RedsStream *stream, int fd)
>  {
>      struct msghdr msgh = { 0, };
> diff --git a/server/reds-stream.h b/server/reds-stream.h
> index 3a4aa25..568ec49 100644
> --- a/server/reds-stream.h
> +++ b/server/reds-stream.h
> @@ -73,6 +73,7 @@ RedsStreamSslStatus reds_stream_ssl_accept(RedsStream
> *stream);
>  int reds_stream_enable_ssl(RedsStream *stream, SSL_CTX *ctx);
>  int reds_stream_get_family(const RedsStream *stream);
>  int reds_stream_is_plain_unix(const RedsStream *stream);
> +bool reds_stream_set_no_delay(RedsStream *stream, bool no_delay);
>  int reds_stream_send_msgfd(RedsStream *stream, int fd);
>  
>  typedef enum {
> diff --git a/server/reds.c b/server/reds.c
> index 2ad231e..f1c3ef9 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2406,7 +2406,6 @@ static bool reds_init_keepalive(int socket)
>  static RedLinkInfo *reds_init_client_connection(RedsState *reds, int socket)
>  {
>      RedLinkInfo *link;
> -    int delay_val = 1;
>      int flags;
>  
>      if ((flags = fcntl(socket, F_GETFL)) == -1) {
> @@ -2419,17 +2418,12 @@ static RedLinkInfo
> *reds_init_client_connection(RedsState *reds, int socket)
>          goto error;
>      }
>  
> -    if (setsockopt(socket, IPPROTO_TCP, TCP_NODELAY, &delay_val,
> sizeof(delay_val)) == -1) {
> -        if (errno != ENOTSUP) {
> -            spice_warning("setsockopt failed, %s", strerror(errno));
> -        }
> -    }
> -
>      reds_init_keepalive(socket);
>  
>      link = spice_new0(RedLinkInfo, 1);
>      link->reds = reds;
>      link->stream = reds_stream_new(reds, socket);
> +    reds_stream_set_no_delay(link->stream, TRUE);
>  
>      /* gather info + send event */
>  
> diff --git a/server/sound.c b/server/sound.c
> index 118f439..30c20e0 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -734,7 +734,6 @@ static void record_channel_send_item(RedChannelClient
> *rcc, G_GNUC_UNUSED RedPip
>  
>  static int snd_channel_config_socket(RedChannelClient *rcc)
>  {
> -    int delay_val;
>  #ifdef SO_PRIORITY
>      int priority;
>  #endif
> @@ -760,12 +759,7 @@ static int snd_channel_config_socket(RedChannelClient
> *rcc)
>          }
>      }
>  
> -    delay_val = main_channel_client_is_low_bandwidth(mcc) ? 0 : 1;
> -    if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, &delay_val,
> sizeof(delay_val)) == -1) {
> -        if (errno != ENOTSUP) {
> -            spice_printerr("setsockopt failed, %s", strerror(errno));
> -        }
> -    }
> +    reds_stream_set_no_delay(stream,
> main_channel_client_is_low_bandwidth(mcc));
>  
>      return TRUE;
>  }

Frediano