> > 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@xxxxxxxxxx> > --- > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel