* [Qemu-devel] [PATCH v3 0/2] Unit test+fix for problem with QEMU handling of multiple bind()s to the same port @ 2017-06-14 16:53 Knut Omang 2017-06-14 16:53 ` [Qemu-devel] [PATCH v3 1/2] tests: Add test-listen - a stress test for QEMU socket listen Knut Omang 2017-06-14 16:53 ` [Qemu-devel] [PATCH v3 2/2] sockets: Handle race condition between binds to the same port Knut Omang 0 siblings, 2 replies; 8+ messages in thread From: Knut Omang @ 2017-06-14 16:53 UTC (permalink / raw) To: Daniel P . Berrange, Gerd Hoffmann, Paolo Bonzini; +Cc: qemu-devel, Knut Omang This series contains: * a unit test that exposes a race condition which causes QEMU to fail to find a port even when there is plenty of available ports. * a refactor of the qemu-sockets inet_listen_saddr() function to better handle this situation. Changes from v2: * Non-trivial rebase + further abstraction on top of 7ad9af343c7f1c70c8015c7c519c312d8c5f9fa1 'tests: add functional test validating ipv4/ipv6 address flag handling' Changes from v1: * Fix potential uninitialized variable only detected by optimize. * Improve unexpected error detection in test-listen to give more details about why the test fails unexpectedly. * Fix some line length style issues. Thanks, Knut Knut Omang (2): tests: Add test-listen - a stress test for QEMU socket listen sockets: Handle race condition between binds to the same port tests/Makefile.include | 2 +- tests/test-listen.c | 141 +++++++++++++++++++++++++++++++++++++- util/qemu-sockets.c | 159 ++++++++++++++++++++++++++++-------------- 3 files changed, 250 insertions(+), 52 deletions(-) create mode 100644 tests/test-listen.c base-commit: 7ad9af343c7f1c70c8015c7c519c312d8c5f9fa1 -- git-series 0.9.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v3 1/2] tests: Add test-listen - a stress test for QEMU socket listen 2017-06-14 16:53 [Qemu-devel] [PATCH v3 0/2] Unit test+fix for problem with QEMU handling of multiple bind()s to the same port Knut Omang @ 2017-06-14 16:53 ` Knut Omang 2017-06-16 14:41 ` Daniel P. Berrange 2017-06-14 16:53 ` [Qemu-devel] [PATCH v3 2/2] sockets: Handle race condition between binds to the same port Knut Omang 1 sibling, 1 reply; 8+ messages in thread From: Knut Omang @ 2017-06-14 16:53 UTC (permalink / raw) To: Daniel P . Berrange, Gerd Hoffmann, Paolo Bonzini; +Cc: qemu-devel, Knut Omang There's a potential race condition between multiple bind()'s attempting to bind to the same port, which occasionally allows more than one bind to succeed against the same port. When a subsequent listen() call is made with the same socket only one will succeed. The current QEMU code does however not take this situation into account and the listen will cause the code to break out and fail even when there are actually available ports to use. This test exposes two subtests: /socket/listen-serial /socket/listen-compete The "compete" subtest creates a number of threads and have them all trying to bind to the same port with a large enough offset input to allow all threads to get it's own port. The "serial" subtest just does the same, except in series in a single thread. The serial version passes, probably in most versions of QEMU. The parallel version exposes the problem in a relatively reliable way, eg. it fails a majority of times, but not with a 100% rate, occasional passes can be seen. Nevertheless this is quite good given that the bug was tricky to reproduce and has been left undetected for a while. The problem seems to be present in all versions of QEMU. The original failure scenario occurred with VNC port allocation in a traditional Xen based build, in different code but with similar functionality. Reported-by: Bhavesh Davda <bhavesh.davda@oracle.com> Signed-off-by: Knut Omang <knut.omang@oracle.com> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com> Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com> --- tests/Makefile.include | 2 +- tests/test-listen.c | 141 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 143 insertions(+) create mode 100644 tests/test-listen.c diff --git a/tests/Makefile.include b/tests/Makefile.include index 7180fe4..22bb97e 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -127,6 +127,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF) gcov-files-check-bufferiszero-y = util/bufferiszero.c check-unit-y += tests/test-uuid$(EXESUF) check-unit-y += tests/ptimer-test$(EXESUF) +#check-unit-y += tests/test-listen$(EXESUF) gcov-files-ptimer-test-y = hw/core/ptimer.c check-unit-y += tests/test-qapi-util$(EXESUF) gcov-files-test-qapi-util-y = qapi/qapi-util.c @@ -764,6 +765,7 @@ tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y) tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y) tests/numa-test$(EXESUF): tests/numa-test.o +tests/test-listen$(EXESUF): tests/test-listen.o $(test-util-obj-y) tests/migration/stress$(EXESUF): tests/migration/stress.o $(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@") diff --git a/tests/test-listen.c b/tests/test-listen.c new file mode 100644 index 0000000..45fe9a8 --- /dev/null +++ b/tests/test-listen.c @@ -0,0 +1,141 @@ +/* + * Test parallel port listen configuration with + * dynamic port allocation + */ + +#include "qemu/osdep.h" +#include "libqtest.h" +#include "qemu-common.h" +#include "qemu/thread.h" +#include "qemu/sockets.h" +#include "qapi/error.h" + +#define NAME_LEN 1024 +#define PORT_LEN 16 + +struct thr_info { + QemuThread thread; + int to_port; + int got_port; + int eno; + int fd; + const char *errstr; +}; + +static char hostname[NAME_LEN + 1]; +static char port[PORT_LEN + 1]; + +static void *listener_thread(void *arg) +{ + struct thr_info *thr = (struct thr_info *)arg; + SocketAddress addr = { + .type = SOCKET_ADDRESS_TYPE_INET, + .u = { + .inet = { + .host = hostname, + .port = port, + .ipv4 = true, + .has_to = true, + .to = thr->to_port, + }, + }, + }; + Error *err = NULL; + int fd; + + fd = socket_listen(&addr, &err); + if (fd < 0) { + thr->eno = errno; + thr->errstr = error_get_pretty(err); + } else { + struct sockaddr_in a; + socklen_t a_len = sizeof(a); + g_assert_cmpint(getsockname(fd, (struct sockaddr *)&a, &a_len), ==, 0); + thr->got_port = ntohs(a.sin_port); + thr->fd = fd; + } + return arg; +} + + +static void listen_compete_nthr(bool threaded, int nthreads, + int start_port, int max_offset) +{ + int i; + int failed_listens = 0; + size_t alloc_sz = sizeof(struct thr_info) * nthreads; + struct thr_info *thr = g_malloc(alloc_sz); + int used[max_offset + 1]; + memset(used, 0, sizeof(used)); + g_assert_nonnull(thr); + g_assert_cmpint(gethostname(hostname, NAME_LEN), == , 0); + snprintf(port, PORT_LEN, "%d", start_port); + memset(thr, 0, alloc_sz); + + for (i = 0; i < nthreads; i++) { + thr[i].to_port = start_port + max_offset; + if (threaded) { + qemu_thread_create(&thr[i].thread, "listener", + listener_thread, &thr[i], + QEMU_THREAD_JOINABLE); + } else { + listener_thread(&thr[i]); + } + } + + if (threaded) { + for (i = 0; i < nthreads; i++) { + qemu_thread_join(&thr[i].thread); + } + } + for (i = 0; i < nthreads; i++) { + if (thr[i].got_port) { + closesocket(thr[i].fd); + } + } + + for (i = 0; i < nthreads; i++) { + if (thr[i].eno != 0) { + const char *m; + printf("** Failed to assign a port to thread %d (errno = %d)\n", + i, thr[i].eno); + /* This is what we are interested in capturing - + * catch and report details if something unexpected happens: + */ + m = strstr(thr[i].errstr, "Failed to listen on socket"); + if (m != NULL) { + g_assert_cmpstr(thr[i].errstr, ==, + "Failed to listen on socket: Address already in use"); + } + failed_listens++; + } else { + int assigned_port = thr[i].got_port; + g_assert_cmpint(assigned_port, <= , thr[i].to_port); + g_assert_cmpint(used[assigned_port - start_port], == , 0); + } + } + g_assert_cmpint(failed_listens, ==, 0); + free(thr); +} + + +static void listen_compete(void) +{ + listen_compete_nthr(true, 200, 5920, 300); +} + +static void listen_serial(void) +{ + listen_compete_nthr(false, 200, 6300, 300); +} + + +int main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + + g_test_add_func("/socket/listen-serial", listen_serial); + g_test_add_func("/socket/listen-compete", listen_compete); + + return g_test_run(); +} -- git-series 0.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] tests: Add test-listen - a stress test for QEMU socket listen 2017-06-14 16:53 ` [Qemu-devel] [PATCH v3 1/2] tests: Add test-listen - a stress test for QEMU socket listen Knut Omang @ 2017-06-16 14:41 ` Daniel P. Berrange 2017-06-20 20:58 ` Knut Omang 0 siblings, 1 reply; 8+ messages in thread From: Daniel P. Berrange @ 2017-06-16 14:41 UTC (permalink / raw) To: Knut Omang; +Cc: Gerd Hoffmann, Paolo Bonzini, qemu-devel On Wed, Jun 14, 2017 at 06:53:51PM +0200, Knut Omang wrote: > There's a potential race condition between multiple bind()'s > attempting to bind to the same port, which occasionally > allows more than one bind to succeed against the same port. > > When a subsequent listen() call is made with the same socket > only one will succeed. > > The current QEMU code does however not take this situation into account > and the listen will cause the code to break out and fail even > when there are actually available ports to use. > > This test exposes two subtests: > > /socket/listen-serial > /socket/listen-compete > > The "compete" subtest creates a number of threads and have them all trying to bind > to the same port with a large enough offset input to > allow all threads to get it's own port. > The "serial" subtest just does the same, except in series in a > single thread. > > The serial version passes, probably in most versions of QEMU. > > The parallel version exposes the problem in a relatively reliable way, > eg. it fails a majority of times, but not with a 100% rate, occasional > passes can be seen. Nevertheless this is quite good given that > the bug was tricky to reproduce and has been left undetected for > a while. > > The problem seems to be present in all versions of QEMU. > > The original failure scenario occurred with VNC port allocation > in a traditional Xen based build, in different code > but with similar functionality. > > Reported-by: Bhavesh Davda <bhavesh.davda@oracle.com> > Signed-off-by: Knut Omang <knut.omang@oracle.com> > Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> > Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com> > Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com> > --- > tests/Makefile.include | 2 +- > tests/test-listen.c | 141 ++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 143 insertions(+) > create mode 100644 tests/test-listen.c > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 7180fe4..22bb97e 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -127,6 +127,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF) > gcov-files-check-bufferiszero-y = util/bufferiszero.c > check-unit-y += tests/test-uuid$(EXESUF) > check-unit-y += tests/ptimer-test$(EXESUF) > +#check-unit-y += tests/test-listen$(EXESUF) Did you really mean to leave this commented out ? > diff --git a/tests/test-listen.c b/tests/test-listen.c > new file mode 100644 > index 0000000..45fe9a8 > --- /dev/null > +++ b/tests/test-listen.c > @@ -0,0 +1,141 @@ > +/* > + * Test parallel port listen configuration with > + * dynamic port allocation > + */ Should stick a standard license header on this new file > + > +#include "qemu/osdep.h" > +#include "libqtest.h" > +#include "qemu-common.h" > +#include "qemu/thread.h" > +#include "qemu/sockets.h" > +#include "qapi/error.h" > + > +#define NAME_LEN 1024 > +#define PORT_LEN 16 > + > +struct thr_info { > + QemuThread thread; > + int to_port; > + int got_port; > + int eno; > + int fd; > + const char *errstr; > +}; > + > +static char hostname[NAME_LEN + 1]; > +static char port[PORT_LEN + 1]; > + > +static void *listener_thread(void *arg) > +{ > + struct thr_info *thr = (struct thr_info *)arg; > + SocketAddress addr = { > + .type = SOCKET_ADDRESS_TYPE_INET, > + .u = { > + .inet = { > + .host = hostname, > + .port = port, > + .ipv4 = true, .ipv4 is ignored unless you set .has_ipv4 too. I'd inclined to allow ipv6 too though, since that's normal usage scenario out of the box. Or repeat the test multiple times, with ipv4 only, ipv6 only and ipv4/ipv6 automatic. > + .has_to = true, > + .to = thr->to_port, > + }, > + }, > + }; > + Error *err = NULL; > + int fd; > + > + fd = socket_listen(&addr, &err); > + if (fd < 0) { > + thr->eno = errno; > + thr->errstr = error_get_pretty(err); > + } else { > + struct sockaddr_in a; > + socklen_t a_len = sizeof(a); > + g_assert_cmpint(getsockname(fd, (struct sockaddr *)&a, &a_len), ==, 0); > + thr->got_port = ntohs(a.sin_port); > + thr->fd = fd; > + } > + return arg; > +} > + > + > +static void listen_compete_nthr(bool threaded, int nthreads, > + int start_port, int max_offset) > +{ > + int i; > + int failed_listens = 0; > + size_t alloc_sz = sizeof(struct thr_info) * nthreads; > + struct thr_info *thr = g_malloc(alloc_sz); These two lines are reinventing g_new - just do struct thr_info *thr = g_new0(struct thr_info, nthreads); > + int used[max_offset + 1]; > + memset(used, 0, sizeof(used)); > + g_assert_nonnull(thr); It is already guaranteed non-null by g_new > + g_assert_cmpint(gethostname(hostname, NAME_LEN), == , 0); > + snprintf(port, PORT_LEN, "%d", start_port); Just use "localhost" instead of the public hostname Also, storing data in global variables is going to fail if the test harness runs both tests in parallel. Better to pass the required data into the thread main method via the thr_info struct. > + memset(thr, 0, alloc_sz); g_new0 will set it to zeros > + > + for (i = 0; i < nthreads; i++) { > + thr[i].to_port = start_port + max_offset; > + if (threaded) { > + qemu_thread_create(&thr[i].thread, "listener", > + listener_thread, &thr[i], > + QEMU_THREAD_JOINABLE); > + } else { > + listener_thread(&thr[i]); > + } > + } > + > + if (threaded) { > + for (i = 0; i < nthreads; i++) { > + qemu_thread_join(&thr[i].thread); > + } > + } > + for (i = 0; i < nthreads; i++) { > + if (thr[i].got_port) { > + closesocket(thr[i].fd); > + } > + } > + > + for (i = 0; i < nthreads; i++) { > + if (thr[i].eno != 0) { > + const char *m; > + printf("** Failed to assign a port to thread %d (errno = %d)\n", > + i, thr[i].eno); I'd suggest g_printerr() instead so it gets to stderr without buffering > + /* This is what we are interested in capturing - > + * catch and report details if something unexpected happens: > + */ > + m = strstr(thr[i].errstr, "Failed to listen on socket"); > + if (m != NULL) { > + g_assert_cmpstr(thr[i].errstr, ==, > + "Failed to listen on socket: Address already in use"); > + } > + failed_listens++; > + } else { > + int assigned_port = thr[i].got_port; > + g_assert_cmpint(assigned_port, <= , thr[i].to_port); > + g_assert_cmpint(used[assigned_port - start_port], == , 0); > + } > + } > + g_assert_cmpint(failed_listens, ==, 0); > + free(thr); Needs to be g_free, not free > +} > + > + > +static void listen_compete(void) > +{ > + listen_compete_nthr(true, 200, 5920, 300); > +} > + > +static void listen_serial(void) > +{ > + listen_compete_nthr(false, 200, 6300, 300); > +} > + > + > +int main(int argc, char **argv) > +{ > + g_test_init(&argc, &argv, NULL); > + > + g_test_add_func("/socket/listen-serial", listen_serial); > + g_test_add_func("/socket/listen-compete", listen_compete); Not all our CI systems have network access. You'll want todo a check for access first, and exit if not available. See check_protocol_support() in test-io-channel-socket.c for example. > + > + return g_test_run(); > +} > -- > git-series 0.9.1 Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] tests: Add test-listen - a stress test for QEMU socket listen 2017-06-16 14:41 ` Daniel P. Berrange @ 2017-06-20 20:58 ` Knut Omang 2017-06-20 22:08 ` Daniel P. Berrange 0 siblings, 1 reply; 8+ messages in thread From: Knut Omang @ 2017-06-20 20:58 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: Gerd Hoffmann, Paolo Bonzini, qemu-devel On Fri, 2017-06-16 at 15:41 +0100, Daniel P. Berrange wrote: > On Wed, Jun 14, 2017 at 06:53:51PM +0200, Knut Omang wrote: > > There's a potential race condition between multiple bind()'s > > attempting to bind to the same port, which occasionally > > allows more than one bind to succeed against the same port. > > > > When a subsequent listen() call is made with the same socket > > only one will succeed. > > > > The current QEMU code does however not take this situation into account > > and the listen will cause the code to break out and fail even > > when there are actually available ports to use. > > > > This test exposes two subtests: > > > > /socket/listen-serial > > /socket/listen-compete > > > > The "compete" subtest creates a number of threads and have them all trying to bind > > to the same port with a large enough offset input to > > allow all threads to get it's own port. > > The "serial" subtest just does the same, except in series in a > > single thread. > > > > The serial version passes, probably in most versions of QEMU. > > > > The parallel version exposes the problem in a relatively reliable way, > > eg. it fails a majority of times, but not with a 100% rate, occasional > > passes can be seen. Nevertheless this is quite good given that > > the bug was tricky to reproduce and has been left undetected for > > a while. > > > > The problem seems to be present in all versions of QEMU. > > > > The original failure scenario occurred with VNC port allocation > > in a traditional Xen based build, in different code > > but with similar functionality. > > > > Reported-by: Bhavesh Davda <bhavesh.davda@oracle.com> > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> > > Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com> > > Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com> > > --- > > tests/Makefile.include | 2 +- > > tests/test-listen.c | 141 ++++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 143 insertions(+) > > create mode 100644 tests/test-listen.c > > > > diff --git a/tests/Makefile.include b/tests/Makefile.include > > index 7180fe4..22bb97e 100644 > > --- a/tests/Makefile.include > > +++ b/tests/Makefile.include > > @@ -127,6 +127,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF) > > gcov-files-check-bufferiszero-y = util/bufferiszero.c > > check-unit-y += tests/test-uuid$(EXESUF) > > check-unit-y += tests/ptimer-test$(EXESUF) > > +#check-unit-y += tests/test-listen$(EXESUF) > > Did you really mean to leave this commented out ? Yes, it is enabled by the next commit with the fix - otherwise it would fail 'make check'. Just wanted to make it convenient for you reproduce the issue (I appreciate that myself.. :-) ) > > diff --git a/tests/test-listen.c b/tests/test-listen.c > > new file mode 100644 > > index 0000000..45fe9a8 > > --- /dev/null > > +++ b/tests/test-listen.c > > @@ -0,0 +1,141 @@ > > +/* > > + * Test parallel port listen configuration with > > + * dynamic port allocation > > + */ > > Should stick a standard license header on this new file Oh - just forgot it, will add, thanks.. > > > + > > +#include "qemu/osdep.h" > > +#include "libqtest.h" > > +#include "qemu-common.h" > > +#include "qemu/thread.h" > > +#include "qemu/sockets.h" > > +#include "qapi/error.h" > > + > > +#define NAME_LEN 1024 > > +#define PORT_LEN 16 > > + > > +struct thr_info { > > + QemuThread thread; > > + int to_port; > > + int got_port; > > + int eno; > > + int fd; > > + const char *errstr; > > +}; > > + > > +static char hostname[NAME_LEN + 1]; > > +static char port[PORT_LEN + 1]; > > + > > +static void *listener_thread(void *arg) > > +{ > > + struct thr_info *thr = (struct thr_info *)arg; > > + SocketAddress addr = { > > + .type = SOCKET_ADDRESS_TYPE_INET, > > + .u = { > > + .inet = { > > + .host = hostname, > > + .port = port, > > + .ipv4 = true, > > .ipv4 is ignored unless you set .has_ipv4 too. I see.. > I'd inclined to allow ipv6 too though, since that's normal > usage scenario out of the box. > > Or repeat the test multiple times, with ipv4 only, ipv6 > only and ipv4/ipv6 automatic. Good idea, will look at making separate subtests. > > > + .has_to = true, > > + .to = thr->to_port, > > + }, > > + }, > > + }; > > + Error *err = NULL; > > + int fd; > > + > > + fd = socket_listen(&addr, &err); > > + if (fd < 0) { > > + thr->eno = errno; > > + thr->errstr = error_get_pretty(err); > > + } else { > > + struct sockaddr_in a; > > + socklen_t a_len = sizeof(a); > > + g_assert_cmpint(getsockname(fd, (struct sockaddr *)&a, &a_len), ==, 0); > > + thr->got_port = ntohs(a.sin_port); > > + thr->fd = fd; > > + } > > + return arg; > > +} > > + > > + > > +static void listen_compete_nthr(bool threaded, int nthreads, > > + int start_port, int max_offset) > > +{ > > + int i; > > + int failed_listens = 0; > > + size_t alloc_sz = sizeof(struct thr_info) * nthreads; > > + struct thr_info *thr = g_malloc(alloc_sz); > > These two lines are reinventing g_new - just do Will do! > struct thr_info *thr = g_new0(struct thr_info, nthreads); > > > + int used[max_offset + 1]; > > + memset(used, 0, sizeof(used)); > > + g_assert_nonnull(thr); > > It is already guaranteed non-null by g_new > > > + g_assert_cmpint(gethostname(hostname, NAME_LEN), == , 0); > > + snprintf(port, PORT_LEN, "%d", start_port); > > Just use "localhost" instead of the public hostname Will do, > Also, storing data in global variables is going to fail if > the test harness runs both tests in parallel. Better to > pass the required data into the thread main method via > the thr_info struct. Ok - will fix. > > > + memset(thr, 0, alloc_sz); > > g_new0 will set it to zeros Thanks, will fix. > > > + > > + for (i = 0; i < nthreads; i++) { > > + thr[i].to_port = start_port + max_offset; > > + if (threaded) { > > + qemu_thread_create(&thr[i].thread, "listener", > > + listener_thread, &thr[i], > > + QEMU_THREAD_JOINABLE); > > + } else { > > + listener_thread(&thr[i]); > > + } > > + } > > + > > + if (threaded) { > > + for (i = 0; i < nthreads; i++) { > > + qemu_thread_join(&thr[i].thread); > > + } > > + } > > + for (i = 0; i < nthreads; i++) { > > + if (thr[i].got_port) { > > + closesocket(thr[i].fd); > > + } > > + } > > + > > + for (i = 0; i < nthreads; i++) { > > + if (thr[i].eno != 0) { > > + const char *m; > > + printf("** Failed to assign a port to thread %d (errno = %d)\n", > > + i, thr[i].eno); > > I'd suggest g_printerr() instead so it gets to stderr without buffering Will do > > > + /* This is what we are interested in capturing - > > + * catch and report details if something unexpected happens: > > + */ > > + m = strstr(thr[i].errstr, "Failed to listen on socket"); > > + if (m != NULL) { > > + g_assert_cmpstr(thr[i].errstr, ==, > > + "Failed to listen on socket: Address already in use"); > > + } > > + failed_listens++; > > + } else { > > + int assigned_port = thr[i].got_port; > > + g_assert_cmpint(assigned_port, <= , thr[i].to_port); > > + g_assert_cmpint(used[assigned_port - start_port], == , 0); > > + } > > + } > > + g_assert_cmpint(failed_listens, ==, 0); > > + free(thr); > > Needs to be g_free, not free Oh - yes, will fix. > > > +} > > + > > + > > +static void listen_compete(void) > > +{ > > + listen_compete_nthr(true, 200, 5920, 300); > > +} > > + > > +static void listen_serial(void) > > +{ > > + listen_compete_nthr(false, 200, 6300, 300); > > +} > > + > > + > > +int main(int argc, char **argv) > > +{ > > + g_test_init(&argc, &argv, NULL); > > + > > + g_test_add_func("/socket/listen-serial", listen_serial); > > + g_test_add_func("/socket/listen-compete", listen_compete); > > Not all our CI systems have network access. You'll want todo a check for > access first, and exit if not available. See check_protocol_support() in > test-io-channel-socket.c for example. Will do - maybe these functions could be made part of the test framework to avoid duplication? > > > + > > + return g_test_run(); > > +} > > -- > > git-series 0.9.1 > > Regards, > Daniel Thanks, Knut ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] tests: Add test-listen - a stress test for QEMU socket listen 2017-06-20 20:58 ` Knut Omang @ 2017-06-20 22:08 ` Daniel P. Berrange 0 siblings, 0 replies; 8+ messages in thread From: Daniel P. Berrange @ 2017-06-20 22:08 UTC (permalink / raw) To: Knut Omang; +Cc: Gerd Hoffmann, Paolo Bonzini, qemu-devel On Tue, Jun 20, 2017 at 10:58:43PM +0200, Knut Omang wrote: > On Fri, 2017-06-16 at 15:41 +0100, Daniel P. Berrange wrote: > > On Wed, Jun 14, 2017 at 06:53:51PM +0200, Knut Omang wrote: > > > +static void listen_compete(void) > > > +{ > > > + listen_compete_nthr(true, 200, 5920, 300); > > > +} > > > + > > > +static void listen_serial(void) > > > +{ > > > + listen_compete_nthr(false, 200, 6300, 300); > > > +} > > > + > > > + > > > +int main(int argc, char **argv) > > > +{ > > > + g_test_init(&argc, &argv, NULL); > > > + > > > + g_test_add_func("/socket/listen-serial", listen_serial); > > > + g_test_add_func("/socket/listen-compete", listen_compete); > > > > Not all our CI systems have network access. You'll want todo a check for > > access first, and exit if not available. See check_protocol_support() in > > test-io-channel-socket.c for example. > > Will do > - maybe these functions could be made part of the test framework to avoid > duplication? Sure, pulling the code out into a common helper file is a fine idea. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v3 2/2] sockets: Handle race condition between binds to the same port 2017-06-14 16:53 [Qemu-devel] [PATCH v3 0/2] Unit test+fix for problem with QEMU handling of multiple bind()s to the same port Knut Omang 2017-06-14 16:53 ` [Qemu-devel] [PATCH v3 1/2] tests: Add test-listen - a stress test for QEMU socket listen Knut Omang @ 2017-06-14 16:53 ` Knut Omang 2017-06-16 14:45 ` Daniel P. Berrange 1 sibling, 1 reply; 8+ messages in thread From: Knut Omang @ 2017-06-14 16:53 UTC (permalink / raw) To: Daniel P . Berrange, Gerd Hoffmann, Paolo Bonzini; +Cc: qemu-devel, Knut Omang If an offset of ports is specified to the inet_listen_saddr function(), and two or more processes tries to bind from these ports at the same time, occasionally more than one process may be able to bind to the same port. The condition is detected by listen() but too late to avoid a failure. This function is called by socket_listen() and used by all socket listening code in QEMU, so all cases where any form of dynamic port selection is used should be subject to this issue. Add code to close and re-establish the socket when this condition is observed, hiding the race condition from the user. This has been developed and tested by means of the test-listen unit test in the previous commit. Enable the test for make check now that it passes. Signed-off-by: Knut Omang <knut.omang@oracle.com> Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com> --- tests/Makefile.include | 2 +- util/qemu-sockets.c | 159 ++++++++++++++++++++++++++++-------------- 2 files changed, 108 insertions(+), 53 deletions(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index 22bb97e..c38f94e 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -127,7 +127,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF) gcov-files-check-bufferiszero-y = util/bufferiszero.c check-unit-y += tests/test-uuid$(EXESUF) check-unit-y += tests/ptimer-test$(EXESUF) -#check-unit-y += tests/test-listen$(EXESUF) +check-unit-y += tests/test-listen$(EXESUF) gcov-files-ptimer-test-y = hw/core/ptimer.c check-unit-y += tests/test-qapi-util$(EXESUF) gcov-files-test-qapi-util-y = qapi/qapi-util.c diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 852773d..7b118b4 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -149,6 +149,94 @@ int inet_ai_family_from_address(InetSocketAddress *addr, return PF_UNSPEC; } +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) +{ + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); + if (slisten < 0) { + if (!e->ai_next) { + error_setg_errno(errp, errno, "Failed to create socket"); + } + return -1; + } + + socket_set_fast_reuse(slisten); + return slisten; +} + +static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e) +{ +#ifndef IPV6_V6ONLY + return bind(socket, e->ai_addr, e->ai_addrlen); +#else + /* + * Deals with first & last cases in matrix in comment + * for inet_ai_family_from_address(). + */ + int v6only = + ((!saddr->has_ipv4 && !saddr->has_ipv6) || + (saddr->has_ipv4 && saddr->ipv4 && + saddr->has_ipv6 && saddr->ipv6)) ? 0 : 1; + int stat; + + rebind: + if (e->ai_family == PF_INET6) { + qemu_setsockopt(socket, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, + sizeof(v6only)); + } + + stat = bind(socket, e->ai_addr, e->ai_addrlen); + if (!stat) { + return 0; + } + + /* If we got EADDRINUSE from an IPv6 bind & v6only is unset, + * it could be that the IPv4 port is already claimed, so retry + * with v6only set + */ + if (e->ai_family == PF_INET6 && errno == EADDRINUSE && !v6only) { + v6only = 1; + goto rebind; + } + return stat; +#endif +} + +static int try_bind_listen(int *socket, InetSocketAddress *saddr, + struct addrinfo *e, int port, Error **errp) +{ + int s = *socket; + int ret; + + inet_setport(e, port); + ret = try_bind(s, saddr, e); + if (ret) { + if (errno != EADDRINUSE) { + error_setg_errno(errp, errno, "Failed to bind socket"); + } + return errno; + } + if (listen(s, 1) == 0) { + return 0; + } + if (errno == EADDRINUSE) { + /* We got to bind the socket to a port but someone else managed + * to bind to the same port and beat us to listen on it! + * Recreate the socket and return EADDRINUSE to preserve the + * expected state by the caller: + */ + closesocket(s); + s = create_fast_reuse_socket(e, errp); + if (s < 0) { + return errno; + } + *socket = s; + errno = EADDRINUSE; + return errno; + } + error_setg_errno(errp, errno, "Failed to listen on socket"); + return errno; +} + static int inet_listen_saddr(InetSocketAddress *saddr, int port_offset, bool update_addr, @@ -158,7 +246,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr, char port[33]; char uaddr[INET6_ADDRSTRLEN+1]; char uport[33]; - int slisten, rc, port_min, port_max, p; + int rc, port_min, port_max, p; + int slisten = 0; + int saved_errno = 0; Error *err = NULL; memset(&ai,0, sizeof(ai)); @@ -210,75 +300,40 @@ static int inet_listen_saddr(InetSocketAddress *saddr, return -1; } - /* create socket + bind */ + /* create socket + bind/listen */ for (e = res; e != NULL; e = e->ai_next) { getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, uaddr,INET6_ADDRSTRLEN,uport,32, NI_NUMERICHOST | NI_NUMERICSERV); - slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); + + slisten = create_fast_reuse_socket(e, &err); if (slisten < 0) { - if (!e->ai_next) { - error_setg_errno(errp, errno, "Failed to create socket"); - } continue; } - socket_set_fast_reuse(slisten); - port_min = inet_getport(e); port_max = saddr->has_to ? saddr->to + port_offset : port_min; for (p = port_min; p <= port_max; p++) { -#ifdef IPV6_V6ONLY - /* - * Deals with first & last cases in matrix in comment - * for inet_ai_family_from_address(). - */ - int v6only = - ((!saddr->has_ipv4 && !saddr->has_ipv6) || - (saddr->has_ipv4 && saddr->ipv4 && - saddr->has_ipv6 && saddr->ipv6)) ? 0 : 1; -#endif - inet_setport(e, p); -#ifdef IPV6_V6ONLY - rebind: - if (e->ai_family == PF_INET6) { - qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, - sizeof(v6only)); - } -#endif - if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { - goto listen; - } - -#ifdef IPV6_V6ONLY - /* If we got EADDRINUSE from an IPv6 bind & V6ONLY is unset, - * it could be that the IPv4 port is already claimed, so retry - * with V6ONLY set - */ - if (e->ai_family == PF_INET6 && errno == EADDRINUSE && !v6only) { - v6only = 1; - goto rebind; - } -#endif - - if (p == port_max) { - if (!e->ai_next) { - error_setg_errno(errp, errno, "Failed to bind socket"); - } + int eno = try_bind_listen(&slisten, saddr, e, p, &err); + if (!eno) { + goto listen_ok; + } else if (eno != EADDRINUSE) { + goto listen_failed; } } + } + error_setg_errno(errp, errno, "Failed to find available port"); + +listen_failed: + saved_errno = errno; + if (slisten >= 0) { closesocket(slisten); } freeaddrinfo(res); + errno = saved_errno; return -1; -listen: - if (listen(slisten,1) != 0) { - error_setg_errno(errp, errno, "Failed to listen on socket"); - closesocket(slisten); - freeaddrinfo(res); - return -1; - } +listen_ok: if (update_addr) { g_free(saddr->host); saddr->host = g_strdup(uaddr); -- git-series 0.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] sockets: Handle race condition between binds to the same port 2017-06-14 16:53 ` [Qemu-devel] [PATCH v3 2/2] sockets: Handle race condition between binds to the same port Knut Omang @ 2017-06-16 14:45 ` Daniel P. Berrange 2017-06-20 20:59 ` Knut Omang 0 siblings, 1 reply; 8+ messages in thread From: Daniel P. Berrange @ 2017-06-16 14:45 UTC (permalink / raw) To: Knut Omang; +Cc: Gerd Hoffmann, Paolo Bonzini, qemu-devel On Wed, Jun 14, 2017 at 06:53:52PM +0200, Knut Omang wrote: > If an offset of ports is specified to the inet_listen_saddr function(), > and two or more processes tries to bind from these ports at the same time, > occasionally more than one process may be able to bind to the same > port. The condition is detected by listen() but too late to avoid a failure. > > This function is called by socket_listen() and used > by all socket listening code in QEMU, so all cases where any form of dynamic > port selection is used should be subject to this issue. > > Add code to close and re-establish the socket when this > condition is observed, hiding the race condition from the user. > > This has been developed and tested by means of the > test-listen unit test in the previous commit. > Enable the test for make check now that it passes. > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com> > Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> > Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com> > --- > tests/Makefile.include | 2 +- > util/qemu-sockets.c | 159 ++++++++++++++++++++++++++++-------------- > 2 files changed, 108 insertions(+), 53 deletions(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 22bb97e..c38f94e 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -127,7 +127,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF) > gcov-files-check-bufferiszero-y = util/bufferiszero.c > check-unit-y += tests/test-uuid$(EXESUF) > check-unit-y += tests/ptimer-test$(EXESUF) > -#check-unit-y += tests/test-listen$(EXESUF) > +check-unit-y += tests/test-listen$(EXESUF) > gcov-files-ptimer-test-y = hw/core/ptimer.c > check-unit-y += tests/test-qapi-util$(EXESUF) > gcov-files-test-qapi-util-y = qapi/qapi-util.c > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index 852773d..7b118b4 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -149,6 +149,94 @@ int inet_ai_family_from_address(InetSocketAddress *addr, > return PF_UNSPEC; > } > > +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) > +{ > + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > + if (slisten < 0) { > + if (!e->ai_next) { > + error_setg_errno(errp, errno, "Failed to create socket"); > + } > + return -1; > + } > + > + socket_set_fast_reuse(slisten); > + return slisten; > +} > + > +static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e) > +{ > +#ifndef IPV6_V6ONLY > + return bind(socket, e->ai_addr, e->ai_addrlen); > +#else > + /* > + * Deals with first & last cases in matrix in comment > + * for inet_ai_family_from_address(). > + */ > + int v6only = > + ((!saddr->has_ipv4 && !saddr->has_ipv6) || > + (saddr->has_ipv4 && saddr->ipv4 && > + saddr->has_ipv6 && saddr->ipv6)) ? 0 : 1; > + int stat; > + > + rebind: > + if (e->ai_family == PF_INET6) { > + qemu_setsockopt(socket, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, > + sizeof(v6only)); > + } > + > + stat = bind(socket, e->ai_addr, e->ai_addrlen); > + if (!stat) { > + return 0; > + } > + > + /* If we got EADDRINUSE from an IPv6 bind & v6only is unset, > + * it could be that the IPv4 port is already claimed, so retry > + * with v6only set > + */ > + if (e->ai_family == PF_INET6 && errno == EADDRINUSE && !v6only) { > + v6only = 1; > + goto rebind; > + } > + return stat; > +#endif > +} > + > +static int try_bind_listen(int *socket, InetSocketAddress *saddr, > + struct addrinfo *e, int port, Error **errp) > +{ > + int s = *socket; > + int ret; > + > + inet_setport(e, port); > + ret = try_bind(s, saddr, e); > + if (ret) { > + if (errno != EADDRINUSE) { > + error_setg_errno(errp, errno, "Failed to bind socket"); > + } > + return errno; > + } > + if (listen(s, 1) == 0) { > + return 0; > + } > + if (errno == EADDRINUSE) { > + /* We got to bind the socket to a port but someone else managed > + * to bind to the same port and beat us to listen on it! > + * Recreate the socket and return EADDRINUSE to preserve the > + * expected state by the caller: > + */ > + closesocket(s); > + s = create_fast_reuse_socket(e, errp); > + if (s < 0) { > + return errno; > + } > + *socket = s; > + errno = EADDRINUSE; > + return errno; > + } > + error_setg_errno(errp, errno, "Failed to listen on socket"); > + return errno; > +} > + > static int inet_listen_saddr(InetSocketAddress *saddr, > int port_offset, > bool update_addr, > @@ -158,7 +246,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > char port[33]; > char uaddr[INET6_ADDRSTRLEN+1]; > char uport[33]; > - int slisten, rc, port_min, port_max, p; > + int rc, port_min, port_max, p; > + int slisten = 0; > + int saved_errno = 0; > Error *err = NULL; > > memset(&ai,0, sizeof(ai)); > @@ -210,75 +300,40 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > return -1; > } > > - /* create socket + bind */ > + /* create socket + bind/listen */ > for (e = res; e != NULL; e = e->ai_next) { > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > uaddr,INET6_ADDRSTRLEN,uport,32, > NI_NUMERICHOST | NI_NUMERICSERV); > - slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > + > + slisten = create_fast_reuse_socket(e, &err); > if (slisten < 0) { > - if (!e->ai_next) { > - error_setg_errno(errp, errno, "Failed to create socket"); > - } > continue; > } > > - socket_set_fast_reuse(slisten); > - > port_min = inet_getport(e); > port_max = saddr->has_to ? saddr->to + port_offset : port_min; > for (p = port_min; p <= port_max; p++) { > -#ifdef IPV6_V6ONLY > - /* > - * Deals with first & last cases in matrix in comment > - * for inet_ai_family_from_address(). > - */ > - int v6only = > - ((!saddr->has_ipv4 && !saddr->has_ipv6) || > - (saddr->has_ipv4 && saddr->ipv4 && > - saddr->has_ipv6 && saddr->ipv6)) ? 0 : 1; > -#endif > - inet_setport(e, p); > -#ifdef IPV6_V6ONLY > - rebind: > - if (e->ai_family == PF_INET6) { > - qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, > - sizeof(v6only)); > - } > -#endif > - if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { > - goto listen; > - } > - > -#ifdef IPV6_V6ONLY > - /* If we got EADDRINUSE from an IPv6 bind & V6ONLY is unset, > - * it could be that the IPv4 port is already claimed, so retry > - * with V6ONLY set > - */ > - if (e->ai_family == PF_INET6 && errno == EADDRINUSE && !v6only) { > - v6only = 1; > - goto rebind; > - } > -#endif > - > - if (p == port_max) { > - if (!e->ai_next) { > - error_setg_errno(errp, errno, "Failed to bind socket"); > - } > + int eno = try_bind_listen(&slisten, saddr, e, p, &err); > + if (!eno) { > + goto listen_ok; > + } else if (eno != EADDRINUSE) { > + goto listen_failed; > } > } > + } > + error_setg_errno(errp, errno, "Failed to find available port"); > + > +listen_failed: > + saved_errno = errno; > + if (slisten >= 0) { > closesocket(slisten); > } > freeaddrinfo(res); > + errno = saved_errno; > return -1; > > -listen: > - if (listen(slisten,1) != 0) { > - error_setg_errno(errp, errno, "Failed to listen on socket"); > - closesocket(slisten); > - freeaddrinfo(res); > - return -1; > - } > +listen_ok: > if (update_addr) { > g_free(saddr->host); > saddr->host = g_strdup(uaddr); I find this patch rather hard to review for correctness, because it has mixed up a huge amount of code movement / refactoring, with the bug fix. Can you split this up into 2 (or possibly more) patches, so we have 1 (or more) no-functional-change refactoring steps, and then the bug fix on its own. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] sockets: Handle race condition between binds to the same port 2017-06-16 14:45 ` Daniel P. Berrange @ 2017-06-20 20:59 ` Knut Omang 0 siblings, 0 replies; 8+ messages in thread From: Knut Omang @ 2017-06-20 20:59 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: Gerd Hoffmann, Paolo Bonzini, qemu-devel On Fri, 2017-06-16 at 15:45 +0100, Daniel P. Berrange wrote: > On Wed, Jun 14, 2017 at 06:53:52PM +0200, Knut Omang wrote: > > If an offset of ports is specified to the inet_listen_saddr function(), > > and two or more processes tries to bind from these ports at the same time, > > occasionally more than one process may be able to bind to the same > > port. The condition is detected by listen() but too late to avoid a failure. > > > > This function is called by socket_listen() and used > > by all socket listening code in QEMU, so all cases where any form of dynamic > > port selection is used should be subject to this issue. > > > > Add code to close and re-establish the socket when this > > condition is observed, hiding the race condition from the user. > > > > This has been developed and tested by means of the > > test-listen unit test in the previous commit. > > Enable the test for make check now that it passes. > > > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com> > > Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> > > Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com> > > --- > > tests/Makefile.include | 2 +- > > util/qemu-sockets.c | 159 ++++++++++++++++++++++++++++-------------- > > 2 files changed, 108 insertions(+), 53 deletions(-) > > > > diff --git a/tests/Makefile.include b/tests/Makefile.include > > index 22bb97e..c38f94e 100644 > > --- a/tests/Makefile.include > > +++ b/tests/Makefile.include > > @@ -127,7 +127,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF) > > gcov-files-check-bufferiszero-y = util/bufferiszero.c > > check-unit-y += tests/test-uuid$(EXESUF) > > check-unit-y += tests/ptimer-test$(EXESUF) > > -#check-unit-y += tests/test-listen$(EXESUF) > > +check-unit-y += tests/test-listen$(EXESUF) > > gcov-files-ptimer-test-y = hw/core/ptimer.c > > check-unit-y += tests/test-qapi-util$(EXESUF) > > gcov-files-test-qapi-util-y = qapi/qapi-util.c > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > > index 852773d..7b118b4 100644 > > --- a/util/qemu-sockets.c > > +++ b/util/qemu-sockets.c > > @@ -149,6 +149,94 @@ int inet_ai_family_from_address(InetSocketAddress *addr, > > return PF_UNSPEC; > > } > > > > +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) > > +{ > > + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > > + if (slisten < 0) { > > + if (!e->ai_next) { > > + error_setg_errno(errp, errno, "Failed to create socket"); > > + } > > + return -1; > > + } > > + > > + socket_set_fast_reuse(slisten); > > + return slisten; > > +} > > + > > +static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e) > > +{ > > +#ifndef IPV6_V6ONLY > > + return bind(socket, e->ai_addr, e->ai_addrlen); > > +#else > > + /* > > + * Deals with first & last cases in matrix in comment > > + * for inet_ai_family_from_address(). > > + */ > > + int v6only = > > + ((!saddr->has_ipv4 && !saddr->has_ipv6) || > > + (saddr->has_ipv4 && saddr->ipv4 && > > + saddr->has_ipv6 && saddr->ipv6)) ? 0 : 1; > > + int stat; > > + > > + rebind: > > + if (e->ai_family == PF_INET6) { > > + qemu_setsockopt(socket, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, > > + sizeof(v6only)); > > + } > > + > > + stat = bind(socket, e->ai_addr, e->ai_addrlen); > > + if (!stat) { > > + return 0; > > + } > > + > > + /* If we got EADDRINUSE from an IPv6 bind & v6only is unset, > > + * it could be that the IPv4 port is already claimed, so retry > > + * with v6only set > > + */ > > + if (e->ai_family == PF_INET6 && errno == EADDRINUSE && !v6only) { > > + v6only = 1; > > + goto rebind; > > + } > > + return stat; > > +#endif > > +} > > + > > +static int try_bind_listen(int *socket, InetSocketAddress *saddr, > > + struct addrinfo *e, int port, Error **errp) > > +{ > > + int s = *socket; > > + int ret; > > + > > + inet_setport(e, port); > > + ret = try_bind(s, saddr, e); > > + if (ret) { > > + if (errno != EADDRINUSE) { > > + error_setg_errno(errp, errno, "Failed to bind socket"); > > + } > > + return errno; > > + } > > + if (listen(s, 1) == 0) { > > + return 0; > > + } > > + if (errno == EADDRINUSE) { > > + /* We got to bind the socket to a port but someone else managed > > + * to bind to the same port and beat us to listen on it! > > + * Recreate the socket and return EADDRINUSE to preserve the > > + * expected state by the caller: > > + */ > > + closesocket(s); > > + s = create_fast_reuse_socket(e, errp); > > + if (s < 0) { > > + return errno; > > + } > > + *socket = s; > > + errno = EADDRINUSE; > > + return errno; > > + } > > + error_setg_errno(errp, errno, "Failed to listen on socket"); > > + return errno; > > +} > > + > > static int inet_listen_saddr(InetSocketAddress *saddr, > > int port_offset, > > bool update_addr, > > @@ -158,7 +246,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > > char port[33]; > > char uaddr[INET6_ADDRSTRLEN+1]; > > char uport[33]; > > - int slisten, rc, port_min, port_max, p; > > + int rc, port_min, port_max, p; > > + int slisten = 0; > > + int saved_errno = 0; > > Error *err = NULL; > > > > memset(&ai,0, sizeof(ai)); > > @@ -210,75 +300,40 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > > return -1; > > } > > > > - /* create socket + bind */ > > + /* create socket + bind/listen */ > > for (e = res; e != NULL; e = e->ai_next) { > > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > > uaddr,INET6_ADDRSTRLEN,uport,32, > > NI_NUMERICHOST | NI_NUMERICSERV); > > - slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > > + > > + slisten = create_fast_reuse_socket(e, &err); > > if (slisten < 0) { > > - if (!e->ai_next) { > > - error_setg_errno(errp, errno, "Failed to create socket"); > > - } > > continue; > > } > > > > - socket_set_fast_reuse(slisten); > > - > > port_min = inet_getport(e); > > port_max = saddr->has_to ? saddr->to + port_offset : port_min; > > for (p = port_min; p <= port_max; p++) { > > -#ifdef IPV6_V6ONLY > > - /* > > - * Deals with first & last cases in matrix in comment > > - * for inet_ai_family_from_address(). > > - */ > > - int v6only = > > - ((!saddr->has_ipv4 && !saddr->has_ipv6) || > > - (saddr->has_ipv4 && saddr->ipv4 && > > - saddr->has_ipv6 && saddr->ipv6)) ? 0 : 1; > > -#endif > > - inet_setport(e, p); > > -#ifdef IPV6_V6ONLY > > - rebind: > > - if (e->ai_family == PF_INET6) { > > - qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, > > - sizeof(v6only)); > > - } > > -#endif > > - if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { > > - goto listen; > > - } > > - > > -#ifdef IPV6_V6ONLY > > - /* If we got EADDRINUSE from an IPv6 bind & V6ONLY is unset, > > - * it could be that the IPv4 port is already claimed, so retry > > - * with V6ONLY set > > - */ > > - if (e->ai_family == PF_INET6 && errno == EADDRINUSE && !v6only) { > > - v6only = 1; > > - goto rebind; > > - } > > -#endif > > - > > - if (p == port_max) { > > - if (!e->ai_next) { > > - error_setg_errno(errp, errno, "Failed to bind socket"); > > - } > > + int eno = try_bind_listen(&slisten, saddr, e, p, &err); > > + if (!eno) { > > + goto listen_ok; > > + } else if (eno != EADDRINUSE) { > > + goto listen_failed; > > } > > } > > + } > > + error_setg_errno(errp, errno, "Failed to find available port"); > > + > > +listen_failed: > > + saved_errno = errno; > > + if (slisten >= 0) { > > closesocket(slisten); > > } > > freeaddrinfo(res); > > + errno = saved_errno; > > return -1; > > > > -listen: > > - if (listen(slisten,1) != 0) { > > - error_setg_errno(errp, errno, "Failed to listen on socket"); > > - closesocket(slisten); > > - freeaddrinfo(res); > > - return -1; > > - } > > +listen_ok: > > if (update_addr) { > > g_free(saddr->host); > > saddr->host = g_strdup(uaddr); > > I find this patch rather hard to review for correctness, because it has > mixed up a huge amount of code movement / refactoring, with the bug > fix. Can you split this up into 2 (or possibly more) patches, so we have > 1 (or more) no-functional-change refactoring steps, and then the bug fix > on its own. Ok, I'll see what I can do - I realize the patch became a great deal more complicated by the refactoring from v2 to v3 to accomodate the IPv6 changes. Thanks, Knut > > Regards, > Daniel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-20 22:08 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-06-14 16:53 [Qemu-devel] [PATCH v3 0/2] Unit test+fix for problem with QEMU handling of multiple bind()s to the same port Knut Omang 2017-06-14 16:53 ` [Qemu-devel] [PATCH v3 1/2] tests: Add test-listen - a stress test for QEMU socket listen Knut Omang 2017-06-16 14:41 ` Daniel P. Berrange 2017-06-20 20:58 ` Knut Omang 2017-06-20 22:08 ` Daniel P. Berrange 2017-06-14 16:53 ` [Qemu-devel] [PATCH v3 2/2] sockets: Handle race condition between binds to the same port Knut Omang 2017-06-16 14:45 ` Daniel P. Berrange 2017-06-20 20:59 ` Knut Omang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).