[Qemu-devel] [PATCH v3 0/5] Do not set SO_REUSEADDR on Windows

[Qemu-devel] [PATCH v3 0/5] Do not set SO_REUSEADDR on Windows

[Sebastian Ottlik]

This patchset disabels all use of SO_REUSEADDR on Windows. On Windows systems
the default behaviour is equivalent to SO_REUSEADDR on other operating
systems. SO_REUSEADDR can still be set but results in undesired behaviour
instead. It may even lead to situations were system behaviour is
unspecified. More information on this can be found at:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx

I originally encountered this issue when accidentally launching two QEMU
instances with identical GDB ports at the same time. In which case QEMU won't
fail as one might expect.

v3 Changes:
- Fixed coding style issues.
  According to checkpatch.pl patch #4 still introduces style errors as tabs are
  used instead of space for some indentation. I keept the tabs to stay
  consistent with the sourrounding code, as tabs seem to be used consitently in
  parts (all?) of the slirp code.

- Changed patch #3 to keep SO_REUSEADDR for multicast sockets on windows and
  added an explainatory comment.

- Rebased to current master (94c2b6aff43cdfcfdfb552773a6b6b973a72ef0b).

v2 Changes:

- Introduce a function with os specific implementation instead of using #ifdef
  I named it socket_set_fast_reuse instead of the suggested qemu_set_reuseaddr
  so the name better reflects what the function actually does.

 gdbstub.c              |    6 ++----
 include/qemu/sockets.h |    1 +
 net/socket.c           |   18 ++++++++++--------
 slirp/misc.c           |    3 +--
 slirp/socket.c         |    4 +---
 slirp/tcp_subr.c       |    6 ++----
 slirp/udp.c            |    4 ++--
 util/oslib-posix.c     |   14 ++++++++++++++
 util/oslib-win32.c     |   10 ++++++++++
 util/qemu-sockets.c    |    6 +++---
 10 files changed, 46 insertions(+), 26 deletions(-)

util: add socket_set_fast_reuse function which will
gdbstub: call socket_set_fast_reuse instead of
net: call socket_set_fast_reuse instead of setting
slirp: call socket_set_fast_reuse instead of setting
util: call socket_set_fast_reuse instead of setting

[Qemu-devel] [PATCH v3 1/5] util: add socket_set_fast_reuse function which will replace setting SO_REUSEADDR

[Sebastian Ottlik]

If a socket is closed it remains in TIME_WAIT state for some time. On operating
systems using BSD sockets the endpoint of the socket may not be reused while in
this state unless SO_REUSEADDR was set on the socket. On windows on the other
hand the default behaviour is to allow reuse (i.e. identical to SO_REUSEADDR on
other operating systems) and setting SO_REUSEADDR on a socket allows it to be
bound to a endpoint even if the endpoint is already used by another socket
independently of the other sockets state. This can even result in undefined
behaviour.

Many sockets used by QEMU should not block the use of their endpoint after being
closed while they are still in TIME_WAIT state. Currently QEMU sets SO_REUSEADDR
for such sockets, which can lead to problems on Windows. This patch introduces
the function socket_set_fast_reuse that should be used instead of setting
SO_REUSEADDR and does the right thing on all operating systems.

Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
---
 include/qemu/sockets.h |    1 +
 util/oslib-posix.c     |   14 ++++++++++++++
 util/oslib-win32.c     |   10 ++++++++++
 3 files changed, 25 insertions(+)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index c5174d7..45588d7 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -39,6 +39,7 @@ int socket_set_cork(int fd, int v);
 int socket_set_nodelay(int fd);
 void qemu_set_block(int fd);
 void qemu_set_nonblock(int fd);
+int socket_set_fast_reuse(int fd);
 int send_all(int fd, const void *buf, int len1);
 int recv_all(int fd, void *buf, int len1, bool single_read);
 
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 3dc8b1b..c01c847 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -159,6 +159,20 @@ void qemu_set_nonblock(int fd)
     fcntl(fd, F_SETFL, f | O_NONBLOCK);
 }
 
+int socket_set_fast_reuse(int fd)
+{
+    int val = 1, ret;
+
+    ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
+                     (const char *)&val, sizeof(val));
+
+    if (ret < 0) {
+        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
+    }
+
+    return ret;
+}
+
 void qemu_set_cloexec(int fd)
 {
     int f;
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 961fbf5..dd81f55 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -127,6 +127,16 @@ void qemu_set_nonblock(int fd)
     qemu_fd_register(fd);
 }
 
+int socket_set_fast_reuse(int fd)
+{
+    /* Enabling the reuse of an endpoint that was used by a socket still in
+     * TIME_WAIT state is usually performed by setting SO_REUSEADDR. On Windows
+     * fast reuse is the default and SO_REUSEADDR does strange things. So we
+     * don't have to do anything here. More info can be found at:
+     * http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx */
+    return 0;
+}
+
 int inet_aton(const char *cp, struct in_addr *ia)
 {
     uint32_t addr = inet_addr(cp);
-- 
1.7.9.5

[Qemu-devel] [PATCH v3 2/5] gdbstub: call socket_set_fast_reuse instead of setting SO_REUSEADDR

[Sebastian Ottlik]

SO_REUSEADDR should be avoided on Windows but is desired on other operating
systems. So instead of setting it we call socket_set_fast_reuse that will result
in the appropriate behaviour on all operating systems.

Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
---
 gdbstub.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 2b7f22b..0e5a3f5 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1553,7 +1553,7 @@ static void gdb_accept(void)
 static int gdbserver_open(int port)
 {
     struct sockaddr_in sockaddr;
-    int fd, val, ret;
+    int fd, ret;
 
     fd = socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
@@ -1564,9 +1564,7 @@ static int gdbserver_open(int port)
     fcntl(fd, F_SETFD, FD_CLOEXEC);
 #endif
 
-    /* allow fast reuse */
-    val = 1;
-    qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
+    socket_set_fast_reuse(fd);
 
     sockaddr.sin_family = AF_INET;
     sockaddr.sin_port = htons(port);
-- 
1.7.9.5

[Qemu-devel] [PATCH v3 3/5] net: call socket_set_fast_reuse instead of setting SO_REUSEADDR

[Sebastian Ottlik]

SO_REUSEADDR should be avoided on Windows but is desired on other operating
systems. So instead of setting it we call socket_set_fast_reuse that will result
in the appropriate behaviour on all operating systems.

An exception to this rule are multicast sockets where it is sensible to have
multiple sockets listen on the same ip and port an we should set SO_REUSEADDR on
windows.

Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
---
 net/socket.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index e61309d..56218ce 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -262,6 +262,11 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
         return -1;
     }
 
+    /* Allow multiple sockets to bind the same multicast ip and port by setting
+     * SO_REUSEADDR. This is the only situation where SO_REUSEADDR should be set
+     * on windows. Use socket_set_fast_reuse otherwise as it sets SO_REUSEADDR
+     * only on posix systems.
+     */
     val = 1;
     ret = qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
     if (ret < 0) {
@@ -510,7 +515,7 @@ static int net_socket_listen_init(NetClientState *peer,
     NetClientState *nc;
     NetSocketState *s;
     struct sockaddr_in saddr;
-    int fd, val, ret;
+    int fd, ret;
 
     if (parse_host_port(&saddr, host_str) < 0)
         return -1;
@@ -523,8 +528,7 @@ static int net_socket_listen_init(NetClientState *peer,
     qemu_set_nonblock(fd);
 
     /* allow fast reuse */
-    val = 1;
-    qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
+    socket_set_fast_reuse(fd);
 
     ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
     if (ret < 0) {
@@ -645,7 +649,7 @@ static int net_socket_udp_init(NetClientState *peer,
                                  const char *lhost)
 {
     NetSocketState *s;
-    int fd, val, ret;
+    int fd, ret;
     struct sockaddr_in laddr, raddr;
 
     if (parse_host_port(&laddr, lhost) < 0) {
@@ -661,11 +665,9 @@ static int net_socket_udp_init(NetClientState *peer,
         perror("socket(PF_INET, SOCK_DGRAM)");
         return -1;
     }
-    val = 1;
-    ret = qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
-                          &val, sizeof(val));
+
+    ret = socket_set_fast_reuse(fd);
     if (ret < 0) {
-        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
         closesocket(fd);
         return -1;
     }
-- 
1.7.9.5

[Qemu-devel] [PATCH v3 4/5] slirp: call socket_set_fast_reuse instead of setting SO_REUSEADDR

[Sebastian Ottlik]

SO_REUSEADDR should be avoided on Windows but is desired on other operating
systems. So instead of setting it we call socket_set_fast_reuse that will result
in the appropriate behaviour on all operating systems.

Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
---
 slirp/misc.c     |    3 +--
 slirp/socket.c   |    4 +---
 slirp/tcp_subr.c |    6 ++----
 slirp/udp.c      |    4 ++--
 4 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/slirp/misc.c b/slirp/misc.c
index c0d4899..6c1636f 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -212,8 +212,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
                     so->s = accept(s, (struct sockaddr *)&addr, &addrlen);
                 } while (so->s < 0 && errno == EINTR);
                 closesocket(s);
-                opt = 1;
-                qemu_setsockopt(so->s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(int));
+                socket_set_fast_reuse(so->s);
                 opt = 1;
                 qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
 		qemu_set_nonblock(so->s);
diff --git a/slirp/socket.c b/slirp/socket.c
index 25d60e7..37ac5cf 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -627,9 +627,7 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 	addr.sin_port = hport;
 
 	if (((s = qemu_socket(AF_INET,SOCK_STREAM,0)) < 0) ||
-#ifndef _WIN32
-	    (qemu_setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(int)) < 0) ||
-#endif
+	    (socket_set_fast_reuse(s) < 0) ||
 	    (bind(s,(struct sockaddr *)&addr, sizeof(addr)) < 0) ||
 	    (listen(s,1) < 0)) {
 		int tmperrno = errno; /* Don't clobber the real reason we failed */
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 043f28f..7571c5a 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -337,8 +337,7 @@ int tcp_fconnect(struct socket *so)
     struct sockaddr_in addr;
 
     qemu_set_nonblock(s);
-    opt = 1;
-    qemu_setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
+    socket_set_fast_reuse(s);
     opt = 1;
     qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(opt));
 
@@ -426,8 +425,7 @@ void tcp_connect(struct socket *inso)
         return;
     }
     qemu_set_nonblock(s);
-    opt = 1;
-    qemu_setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(int));
+    socket_set_fast_reuse(s);
     opt = 1;
     qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
     socket_set_nodelay(s);
diff --git a/slirp/udp.c b/slirp/udp.c
index b105f87..8cc6cb6 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -354,7 +354,7 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 {
 	struct sockaddr_in addr;
 	struct socket *so;
-	socklen_t addrlen = sizeof(struct sockaddr_in), opt = 1;
+	socklen_t addrlen = sizeof(struct sockaddr_in);
 
 	so = socreate(slirp);
 	if (!so) {
@@ -372,7 +372,7 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 		udp_detach(so);
 		return NULL;
 	}
-	qemu_setsockopt(so->s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(int));
+	socket_set_fast_reuse(so->s);
 
 	getsockname(so->s,(struct sockaddr *)&addr,&addrlen);
 	so->so_fport = addr.sin_port;
-- 
1.7.9.5

[Qemu-devel] [PATCH v3 5/5] util: call socket_set_fast_reuse instead of setting SO_REUSEADDR

[Sebastian Ottlik]

SO_REUSEADDR should be avoided on Windows but is desired on other operating
systems. So instead of setting it we call socket_set_fast_reuse that will result
in the appropriate behaviour on all operating systems.

Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
---
 util/qemu-sockets.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 095716e..6b97dc1 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -155,7 +155,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
             continue;
         }
 
-        qemu_setsockopt(slisten, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
+        socket_set_fast_reuse(slisten);
 #ifdef IPV6_V6ONLY
         if (e->ai_family == PF_INET6) {
             /* listen on both ipv4 and ipv6 */
@@ -274,7 +274,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
         error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
         return -1;
     }
-    qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
+    socket_set_fast_reuse(sock);
     if (connect_state != NULL) {
         qemu_set_nonblock(sock);
     }
@@ -455,7 +455,7 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
         error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
         goto err;
     }
-    qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
+    socket_set_fast_reuse(sock);
 
     /* bind socket */
     if (bind(sock, local->ai_addr, local->ai_addrlen) < 0) {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v3 3/5] net: call socket_set_fast_reuse instead of setting SO_REUSEADDR

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

On 09/10/2013 07:26 AM, Sebastian Ottlik wrote:
> SO_REUSEADDR should be avoided on Windows but is desired on other operating
> systems. So instead of setting it we call socket_set_fast_reuse that will result
> in the appropriate behaviour on all operating systems.
> 
> An exception to this rule are multicast sockets where it is sensible to have
> multiple sockets listen on the same ip and port an we should set SO_REUSEADDR on

s/an/and/

> windows.
> 
> Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
> ---
>  net/socket.c |   18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v3 1/5] util: add socket_set_fast_reuse function which will replace setting SO_REUSEADDR

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

On 09/10/2013 07:26 AM, Sebastian Ottlik wrote:
> If a socket is closed it remains in TIME_WAIT state for some time. On operating
> systems using BSD sockets the endpoint of the socket may not be reused while in
> this state unless SO_REUSEADDR was set on the socket. On windows on the other
> hand the default behaviour is to allow reuse (i.e. identical to SO_REUSEADDR on
> other operating systems) and setting SO_REUSEADDR on a socket allows it to be
> bound to a endpoint even if the endpoint is already used by another socket
> independently of the other sockets state. This can even result in undefined
> behaviour.
> 
> Many sockets used by QEMU should not block the use of their endpoint after being
> closed while they are still in TIME_WAIT state. Currently QEMU sets SO_REUSEADDR
> for such sockets, which can lead to problems on Windows. This patch introduces
> the function socket_set_fast_reuse that should be used instead of setting
> SO_REUSEADDR and does the right thing on all operating systems.
> 
> Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
> ---

> +int socket_set_fast_reuse(int fd)
> +{
> +    int val = 1, ret;
> +
> +    ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
> +                     (const char *)&val, sizeof(val));
> +
> +    if (ret < 0) {
> +        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
> +    }

This would be the first use of perror in this file; I'm not sure if that
is the right function, or if there is a better thing to be using (in
fact, returning -1 and letting the client decide whether to issue a
warning may even be better).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v3 4/5] slirp: call socket_set_fast_reuse instead of setting SO_REUSEADDR

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

On 09/10/2013 07:26 AM, Sebastian Ottlik wrote:
> SO_REUSEADDR should be avoided on Windows but is desired on other operating
> systems. So instead of setting it we call socket_set_fast_reuse that will result
> in the appropriate behaviour on all operating systems.
> 
> Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
> ---
>  slirp/misc.c     |    3 +--
>  slirp/socket.c   |    4 +---
>  slirp/tcp_subr.c |    6 ++----
>  slirp/udp.c      |    4 ++--
>  4 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/slirp/misc.c b/slirp/misc.c
> index c0d4899..6c1636f 100644
> --- a/slirp/misc.c
> +++ b/slirp/misc.c
> @@ -212,8 +212,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>                      so->s = accept(s, (struct sockaddr *)&addr, &addrlen);
>                  } while (so->s < 0 && errno == EINTR);
>                  closesocket(s);
> -                opt = 1;
> -                qemu_setsockopt(so->s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(int));
> +                socket_set_fast_reuse(so->s);

Here's a case where the old code didn't care about failure, but the code
in 1/5 results in a message via perror().  If that is intentional, it's
best to mention it in the commit message.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v3 1/5] util: add socket_set_fast_reuse function which will replace setting SO_REUSEADDR

[Sebastian Ottlik]

On 10.09.2013 17:56, Eric Blake wrote:
> On 09/10/2013 07:26 AM, Sebastian Ottlik wrote:
>> If a socket is closed it remains in TIME_WAIT state for some time. On operating
>> systems using BSD sockets the endpoint of the socket may not be reused while in
>> this state unless SO_REUSEADDR was set on the socket. On windows on the other
>> hand the default behaviour is to allow reuse (i.e. identical to SO_REUSEADDR on
>> other operating systems) and setting SO_REUSEADDR on a socket allows it to be
>> bound to a endpoint even if the endpoint is already used by another socket
>> independently of the other sockets state. This can even result in undefined
>> behaviour.
>>
>> Many sockets used by QEMU should not block the use of their endpoint after being
>> closed while they are still in TIME_WAIT state. Currently QEMU sets SO_REUSEADDR
>> for such sockets, which can lead to problems on Windows. This patch introduces
>> the function socket_set_fast_reuse that should be used instead of setting
>> SO_REUSEADDR and does the right thing on all operating systems.
>>
>> Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
>> ---
>> +int socket_set_fast_reuse(int fd)
>> +{
>> +    int val = 1, ret;
>> +
>> +    ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
>> +                     (const char *)&val, sizeof(val));
>> +
>> +    if (ret < 0) {
>> +        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
>> +    }
> This would be the first use of perror in this file; I'm not sure if that
> is the right function, or if there is a better thing to be using (in
> fact, returning -1 and letting the client decide whether to issue a
> warning may even be better).
>
When I started writing the patch I was going to return the error and lat 
the client handle the issue. But the code in net/socket.c then becomes:

     ret = socket_set_fast_reuse(fd);
     if (ret < 0) {
         perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
         closesocket(fd);
         return -1;
     }

Which looked unclean to me, as the code implies assumptions about the 
implementation of socket_set_fast_reuse. One could also call 
perror("socket_set_fast_reuse()") but this would break the convention in 
the surrounding code of passing for the function that failed to perror.

As both approaches were not so great, I moved the error message to 
socket_set_fast_reuse and accepted the side effect, that the other 
places output an error message if something goes wrong. I agree I should 
have been mentioned this change in the commit messages. Also it is 
unlikely the function will fail during normal use of QEMU.

Another approach would be to indeed let the client decide what to do 
with the error and use other error reporting facilities. But I am not 
sure what would be appropriate and how to handle errno in this case, 
which could provide some useful insights.

Re: [Qemu-devel] [PATCH v3 1/5] util: add socket_set_fast_reuse function which will replace setting SO_REUSEADDR

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

On 09/10/2013 10:23 AM, Sebastian Ottlik wrote:

>>> +    if (ret < 0) {
>>> +        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
>>> +    }
>> This would be the first use of perror in this file; I'm not sure if that
>> is the right function, or if there is a better thing to be using (in
>> fact, returning -1 and letting the client decide whether to issue a
>> warning may even be better).
>>
> When I started writing the patch I was going to return the error and lat
> the client handle the issue. But the code in net/socket.c then becomes:
> 
>     ret = socket_set_fast_reuse(fd);
>     if (ret < 0) {
>         perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
>         closesocket(fd);
>         return -1;
>     }
> 
> Which looked unclean to me, as the code implies assumptions about the
> implementation of socket_set_fast_reuse. One could also call
> perror("socket_set_fast_reuse()") but this would break the convention in
> the surrounding code of passing for the function that failed to perror.

Maybe a compromise?  Add a 'bool silent' flag to socket_set_fast_reuse,
and only issue perror() if the flag is false.  Existing callers that
don't care about failure (if we get fast reuse, great; if not, no huge
loss) pass false, existing callers that did their own error reporting
pass true to take advantage of the perror() on failure, and then you
aren't changing semantics at call sites.

But I'm just making this observation from the side; you might want to
get an opinion from an actual maintainer of this area of code on which
approach is best.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v3 1/5] util: add socket_set_fast_reuse function which will replace setting SO_REUSEADDR

[Sebastian Ottlik]

On 10.09.2013 18:34, Eric Blake wrote:
> On 09/10/2013 10:23 AM, Sebastian Ottlik wrote:
>
>>>> +    if (ret < 0) {
>>>> +        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
>>>> +    }
>>> This would be the first use of perror in this file; I'm not sure if that
>>> is the right function, or if there is a better thing to be using (in
>>> fact, returning -1 and letting the client decide whether to issue a
>>> warning may even be better).
>>>
>> When I started writing the patch I was going to return the error and lat
>> the client handle the issue. But the code in net/socket.c then becomes:
>>
>>      ret = socket_set_fast_reuse(fd);
>>      if (ret < 0) {
>>          perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
>>          closesocket(fd);
>>          return -1;
>>      }
>>
>> Which looked unclean to me, as the code implies assumptions about the
>> implementation of socket_set_fast_reuse. One could also call
>> perror("socket_set_fast_reuse()") but this would break the convention in
>> the surrounding code of passing for the function that failed to perror.
> Maybe a compromise?  Add a 'bool silent' flag to socket_set_fast_reuse,
> and only issue perror() if the flag is false.  Existing callers that
> don't care about failure (if we get fast reuse, great; if not, no huge
> loss) pass false, existing callers that did their own error reporting
> pass true to take advantage of the perror() on failure, and then you
> aren't changing semantics at call sites.
>
> But I'm just making this observation from the side; you might want to
> get an opinion from an actual maintainer of this area of code on which
> approach is best.
>
This is probably the least intrusive approach, which is probably best 
without further maintainer input. I will wait and see if someone responds.