[Spice-devel] [spice-server v4 2/6] reds-stream: Introduce reds_stream_set_no_delay() helper
Frediano Ziglio
fziglio at redhat.com
Fri Mar 31 09:46:08 UTC 2017
>
> 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 | 54
> ++++++++++++++++++++++++++++++++++++++++
> server/net-utils.h | 25 +++++++++++++++++++
> server/red-channel-client.c | 17 ++-----------
> server/reds-stream.c | 14 +++++++++++
> server/reds-stream.h | 1 +
> server/reds.c | 8 +++---
> server/sound.c | 8 +-----
> 9 files changed, 104 insertions(+), 38 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 74a1302..ef8d31f 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -125,6 +125,8 @@ libserver_la_SOURCES = \
> memslot.h \
> migration-protocol.h \
> mjpeg-encoder.c \
> + net-utils.c \
> + net-utils.h \
> pixmap-cache.c \
> pixmap-cache.h \
> red-channel.c \
> diff --git a/server/common-graphics-channel.c
> b/server/common-graphics-channel.c
> index 91374fc..e8c18a5 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;
> /* 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/net-utils.c b/server/net-utils.c
> new file mode 100644
> index 0000000..10f447b
> --- /dev/null
> +++ b/server/net-utils.c
> @@ -0,0 +1,54 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> + 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/>.
> +*/
> +#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"
> +
> +/**
> + * red_socket_set_no_delay:
> + * @fd: a socket file descriptor
> + * @no_delay: whether to enable TCP_NODELAY on @fd
> + *
> + * Returns: #true if the operation succeeded, #false otherwise.
> + */
> +bool red_socket_set_no_delay(int fd, bool no_delay)
> +{
> + int optval = no_delay;
> +
> + if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY,
> + &optval, sizeof(optval)) != 0) {
> + if (errno != ENOTSUP && errno != ENOPROTOOPT) {
> + spice_warning("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..9f4932e
> --- /dev/null
> +++ b/server/net-utils.h
> @@ -0,0 +1,25 @@
> +/*
> + 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 RED_NET_UTILS_H_
> +#define RED_NET_UTILS_H_
> +
> +#include <stdbool.h>
> +
> +bool red_socket_set_no_delay(int fd, bool no_delay);
> +
> +#endif
The guard is slightly different from the style you defined
yesterday but just picky.
> 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 77f9424..dbe6962 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>
This should not be necessary.
> @@ -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,18 @@ bool reds_stream_is_plain_unix(const RedsStream *s)
>
> }
>
> +/**
> + * reds_stream_set_no_delay:
> + * @stream: a #RedsStream
> + * @no_delay: whether to enable TCP_NODELAY on @@stream
> + *
> + * Returns: #true if the operation succeeded, #false otherwise.
> + */
> +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 c730daa..653a045 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -75,6 +75,7 @@
> #include "main-channel-client.h"
> #include "red-client.h"
> #include "glib-compat.h"
> +#include "net-utils.h"
>
> #define REDS_MAX_STAT_NODES 100
>
> @@ -2405,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) {
> @@ -2418,10 +2418,8 @@ 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));
> - }
> + if (!red_socket_set_no_delay(socket, TRUE)) {
> + goto error;
> }
>
> reds_init_keepalive(socket);
> diff --git a/server/sound.c b/server/sound.c
> index 9a80bb7..75bd0e7 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));
>
> return TRUE;
> }
Frediano
More information about the Spice-devel
mailing list