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

Uri Lublin uril at redhat.com
Sun Mar 12 09:50:12 UTC 2017


On 03/10/2017 10:59 AM, Christophe Fergeau wrote:
> 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.
>
> Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> ---
>  server/Makefile.am               |  2 ++
>  server/common-graphics-channel.c | 13 ++---------
>  server/net-utils.c               | 47 ++++++++++++++++++++++++++++++++++++++++
>  server/net-utils.h               | 23 ++++++++++++++++++++
>  server/red-channel-client.c      | 17 ++-------------
>  server/reds-stream.c             |  7 ++++++
>  server/reds-stream.h             |  1 +
>  server/reds.c                    |  8 +------
>  server/sound.c                   |  8 +------
>  9 files changed, 86 insertions(+), 40 deletions(-)
>  create mode 100644 server/net-utils.c
>  create mode 100644 server/net-utils.h
>
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 49c0822..a771911 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -166,6 +166,8 @@ libserver_la_SOURCES =				\
>  	image-encoders.c					\
>  	image-encoders.h					\
>  	glib-compat.h				\
> +	net-utils.c				\
> +	net-utils.h				\
>  	$(spice_built_sources)		\
>  	$(NULL)
>
> diff --git a/server/common-graphics-channel.c b/server/common-graphics-channel.c
> index 91374fc..8507711 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 @@ bool common_channel_client_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;

Hi Christophe,

if  is_low_bandwidth then delay_val = 0
if !is_low_bandwidth then delay_val = 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);

So, this should be !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/net-utils.c b/server/net-utils.c
> new file mode 100644
> index 0000000..e2c4853
> --- /dev/null
> +++ b/server/net-utils.c
> @@ -0,0 +1,47 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2009, 2013 Red Hat, Inc.
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +*/
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdbool.h>
> +#include <string.h>
> +#include <netinet/ip.h>
> +#include <netinet/tcp.h>
> +#include <sys/socket.h>
> +
> +#include <common/log.h>
> +
> +#include "net-utils.h"
> +
> +bool red_socket_set_no_delay(int fd, bool no_delay)
> +{
> +    int delay_val = no_delay;

nitpick, consider renaming to 'val' (delay/no_delay)
(I know the removed code in this patch is using delay_val).

> +
> +    if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY,
> +                   &delay_val, sizeof(delay_val)) != 0) {
> +        if (errno != ENOTSUP && errno != ENOPROTOOPT) {
> +            spice_printerr("setsockopt failed, %s", strerror(errno));
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> diff --git a/server/net-utils.h b/server/net-utils.h
> new file mode 100644
> index 0000000..d49ebc4
> --- /dev/null
> +++ b/server/net-utils.h
> @@ -0,0 +1,23 @@
> +/*
> +   Copyright (C) 2009-2017 Red Hat, Inc.
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#ifndef _H_RED_NET_UTILS
> +#define _H_RED_NET_UTILS
> +
> +bool red_socket_set_no_delay(int fd, bool no_delay);
> +
> +#endif
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index a86833d..9fca8c1 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -571,13 +571,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);
>              }
>          }
>      }
> @@ -1385,14 +1379,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 a813a8b..f9cdcf0 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>
> @@ -32,6 +33,7 @@
>  #include <common/log.h>
>
>  #include "main-dispatcher.h"
> +#include "net-utils.h"
>  #include "red-common.h"
>  #include "reds-stream.h"
>  #include "reds.h"
> @@ -259,6 +261,11 @@ bool reds_stream_is_plain_unix(const RedsStream *s)
>
>  }
>
> +bool reds_stream_set_no_delay(RedsStream *stream, bool no_delay)
> +{
> +    return red_socket_set_no_delay(stream->socket, no_delay);
> +}
> +
>  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 d5bd682..37ba87c 100644
> --- a/server/reds-stream.h
> +++ b/server/reds-stream.h
> @@ -72,6 +72,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);
>  bool 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 a165a9b..e3e2655 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2405,7 +2405,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) {
> @@ -2418,17 +2417,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 9a80bb7..388baea 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 bool snd_channel_client_config_socket(RedChannelClient *rcc)
>  {
> -    int delay_val;
>  #ifdef SO_PRIORITY
>      int priority;
>  #endif
> @@ -760,12 +759,7 @@ static bool snd_channel_client_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));

Also here, it should be !main_channel_client_is_low_bandwidth(mcc)

Regards,
     Uri.

>
>      return TRUE;
>  }
>



More information about the Spice-devel mailing list