From 28bb0a59f84c11c42117a7009646508256ed4475 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 18 Dec 2017 09:53:08 +0100 Subject: [PATCH 1/7] io: fix QIONetListener memory leak The sources array does not escape out of qio_net_listener_wait_client, so we have to free it. Reported by Coverity. Signed-off-by: Paolo Bonzini Signed-off-by: Daniel P. Berrange --- io/net-listener.c | 1 + 1 file changed, 1 insertion(+) diff --git a/io/net-listener.c b/io/net-listener.c index 77a4e2831c..de38dfae99 100644 --- a/io/net-listener.c +++ b/io/net-listener.c @@ -234,6 +234,7 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener) for (i = 0; i < listener->nsioc; i++) { g_source_unref(sources[i]); } + g_free(sources); g_main_loop_unref(loop); g_main_context_unref(ctxt); From a46ded1de5cf0edd6c780e071ddafb92601070b5 Mon Sep 17 00:00:00 2001 From: Edgar Kaziakhmedov Date: Wed, 10 Jan 2018 18:39:24 +0300 Subject: [PATCH 2/7] io/channel-websock: handle continuous reads without any data According to the current implementation of websocket protocol in QEMU, qio_channel_websock_handshake_io tries to read handshake from the channel to start communication over socket. But this approach doesn't cover scenario when socket was closed while handshaking. Therefore, if G_IO_IN is caught and qio_channel_read returns zero, error has to be set and connection has to be done. Such behaviour causes 100% CPU load in main QEMU loop, because main loop poll continues to receive and handle G_IO_IN events from websocket. Step to reproduce 100% CPU load: 1) start qemu with the simplest configuration $ qemu -vnc [::1]:1,websocket=7500 2) open any vnc listener (which doesn't follow websocket protocol) $ vncviewer :7500 3) kill listener 4) qemu main thread eats 100% CPU Signed-off-by: Edgar Kaziakhmedov Signed-off-by: Daniel P. Berrange --- io/channel-websock.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/io/channel-websock.c b/io/channel-websock.c index 7fd6bb68ba..ec48a305f0 100644 --- a/io/channel-websock.c +++ b/io/channel-websock.c @@ -499,9 +499,12 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, error_setg(errp, "End of headers not found in first 4096 bytes"); return 1; - } else { - return 0; + } else if (ret == 0) { + error_setg(errp, + "End of headers not found before connection closed"); + return -1; } + return 0; } *handshake_end = '\0'; From 902f6e14fc68743ce24efb7d87dc3f8464a78bf3 Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Wed, 1 Nov 2017 14:25:24 +0000 Subject: [PATCH 3/7] io: Fix QIOChannelFile when creating and opening read-write The code wrongly passes the mode to open() only if O_WRONLY is set. Instead, the mode should be passed when O_CREAT is set (or O_TMPFILE on Linux). Fix this by always passing the mode since open() will correctly ignore the mode if it is not needed. Add a testcase which exercises this bug and also change the existing testcase to check that the mode of the created file is correct. Signed-off-by: Ross Lagerwall Signed-off-by: Daniel P. Berrange --- include/io/channel-file.h | 2 +- io/channel-file.c | 6 +----- tests/test-io-channel-file.c | 29 +++++++++++++++++++++++++---- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/include/io/channel-file.h b/include/io/channel-file.h index 79245f1183..ebfe54ec70 100644 --- a/include/io/channel-file.h +++ b/include/io/channel-file.h @@ -73,7 +73,7 @@ qio_channel_file_new_fd(int fd); * qio_channel_file_new_path: * @path: the file path * @flags: the open flags (O_RDONLY|O_WRONLY|O_RDWR, etc) - * @mode: the file creation mode if O_WRONLY is set in @flags + * @mode: the file creation mode if O_CREAT is set in @flags * @errp: pointer to initialized error object * * Create a new IO channel object for a file represented diff --git a/io/channel-file.c b/io/channel-file.c index b383273201..16bf7ed270 100644 --- a/io/channel-file.c +++ b/io/channel-file.c @@ -50,11 +50,7 @@ qio_channel_file_new_path(const char *path, ioc = QIO_CHANNEL_FILE(object_new(TYPE_QIO_CHANNEL_FILE)); - if (flags & O_WRONLY) { - ioc->fd = open(path, flags, mode); - } else { - ioc->fd = open(path, flags); - } + ioc->fd = open(path, flags, mode); if (ioc->fd < 0) { object_unref(OBJECT(ioc)); error_setg_errno(errp, errno, diff --git a/tests/test-io-channel-file.c b/tests/test-io-channel-file.c index 6bfede6bb7..2e94f638f2 100644 --- a/tests/test-io-channel-file.c +++ b/tests/test-io-channel-file.c @@ -24,16 +24,21 @@ #include "io-channel-helpers.h" #include "qapi/error.h" -static void test_io_channel_file(void) +#define TEST_FILE "tests/test-io-channel-file.txt" +#define TEST_MASK 0600 + +static void test_io_channel_file_helper(int flags) { QIOChannel *src, *dst; QIOChannelTest *test; + struct stat st; + mode_t mask; + int ret; -#define TEST_FILE "tests/test-io-channel-file.txt" unlink(TEST_FILE); src = QIO_CHANNEL(qio_channel_file_new_path( TEST_FILE, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0600, + flags, TEST_MASK, &error_abort)); dst = QIO_CHANNEL(qio_channel_file_new_path( TEST_FILE, @@ -45,18 +50,33 @@ static void test_io_channel_file(void) qio_channel_test_run_reader(test, dst); qio_channel_test_validate(test); + /* Check that the requested mode took effect. */ + mask = umask(0); + umask(mask); + ret = stat(TEST_FILE, &st); + g_assert_cmpint(ret, >, -1); + g_assert_cmpuint(TEST_MASK & ~mask, ==, st.st_mode & 0777); + unlink(TEST_FILE); object_unref(OBJECT(src)); object_unref(OBJECT(dst)); } +static void test_io_channel_file(void) +{ + test_io_channel_file_helper(O_WRONLY | O_CREAT | O_TRUNC | O_BINARY); +} + +static void test_io_channel_file_rdwr(void) +{ + test_io_channel_file_helper(O_RDWR | O_CREAT | O_TRUNC | O_BINARY); +} static void test_io_channel_fd(void) { QIOChannel *ioc; int fd = -1; -#define TEST_FILE "tests/test-io-channel-file.txt" fd = open(TEST_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600); g_assert_cmpint(fd, >, -1); @@ -114,6 +134,7 @@ int main(int argc, char **argv) g_test_init(&argc, &argv, NULL); g_test_add_func("/io/channel/file", test_io_channel_file); + g_test_add_func("/io/channel/file/rdwr", test_io_channel_file_rdwr); g_test_add_func("/io/channel/file/fd", test_io_channel_fd); #ifndef _WIN32 g_test_add_func("/io/channel/pipe/sync", test_io_channel_pipe_sync); From a2565df12c59362c061084a0c853dace410cac26 Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Wed, 1 Nov 2017 14:25:25 +0000 Subject: [PATCH 4/7] io: Don't call close multiple times in QIOChannelFile If the file descriptor underlying QIOChannelFile is closed in the io_close() method, don't close it again in the finalize() method since the file descriptor number may have been reused in the meantime. Signed-off-by: Ross Lagerwall Signed-off-by: Daniel P. Berrange --- io/channel-file.c | 1 + 1 file changed, 1 insertion(+) diff --git a/io/channel-file.c b/io/channel-file.c index 16bf7ed270..1f2f710bf9 100644 --- a/io/channel-file.c +++ b/io/channel-file.c @@ -178,6 +178,7 @@ static int qio_channel_file_close(QIOChannel *ioc, "Unable to close file"); return -1; } + fioc->fd = -1; return 0; } From b8f244b13ca3c754c34c0ab1c2b0e7241b54318a Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Wed, 1 Nov 2017 14:25:26 +0000 Subject: [PATCH 5/7] io: Add /dev/fdset/ support to QIOChannelFile Add /dev/fdset/ support to QIOChannelFile by calling qemu_open() instead of open() and qemu_close() instead of close(). There is a subtle semantic change since qemu_open() automatically sets O_CLOEXEC, but this doesn't affect any of the users of the function. Signed-off-by: Ross Lagerwall Signed-off-by: Daniel P. Berrange --- io/channel-file.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/io/channel-file.c b/io/channel-file.c index 1f2f710bf9..db948abc3e 100644 --- a/io/channel-file.c +++ b/io/channel-file.c @@ -50,7 +50,7 @@ qio_channel_file_new_path(const char *path, ioc = QIO_CHANNEL_FILE(object_new(TYPE_QIO_CHANNEL_FILE)); - ioc->fd = open(path, flags, mode); + ioc->fd = qemu_open(path, flags, mode); if (ioc->fd < 0) { object_unref(OBJECT(ioc)); error_setg_errno(errp, errno, @@ -74,7 +74,7 @@ static void qio_channel_file_finalize(Object *obj) { QIOChannelFile *ioc = QIO_CHANNEL_FILE(obj); if (ioc->fd != -1) { - close(ioc->fd); + qemu_close(ioc->fd); ioc->fd = -1; } } @@ -173,7 +173,7 @@ static int qio_channel_file_close(QIOChannel *ioc, { QIOChannelFile *fioc = QIO_CHANNEL_FILE(ioc); - if (close(fioc->fd) < 0) { + if (qemu_close(fioc->fd) < 0) { error_setg_errno(errp, errno, "Unable to close file"); return -1; From fe823b6f87b2ebedd692ca480ceb9693439d816e Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 14 Feb 2018 17:09:35 +0100 Subject: [PATCH 6/7] io/channel-command: Do not kill the child process after closing the pipe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We are currently facing some migration failure on s390x when running certain avocado-vt tests, e.g. when running the test type_specific.io-github-autotest-qemu.migrate.with_reboot.exec.gzip_exec. This test is using 'migrate -d "exec:nc localhost 5200"' for the migration. The problem is detected at the receiving side, where the migration stream apparently ends too early. However, the cause for the problem is at the sending side: After writing the migration stream into the pipe to netcat, the source QEMU calls qio_channel_command_close() which closes the pipe and immediately (!) kills the child process afterwards (via the function qio_channel_command_abort()). So if the sending netcat did not read the final bytes from the pipe yet, or if it did not manage to send out all its buffers yet, it is killed before the whole migration stream is passed to the destination side. QEMU can not know how much time is required by the child process to send over all migration data, so we should not kill it, neither directly nor after a delay. Let's simply wait for the child process to exit gracefully instead (this was also the behaviour of pclose() that was used in "exec:" migration before the QIOChannel rework). Signed-off-by: Thomas Huth Signed-off-by: Daniel P. Berrangé --- io/channel-command.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/io/channel-command.c b/io/channel-command.c index 319c5ed50c..3e7eb17eff 100644 --- a/io/channel-command.c +++ b/io/channel-command.c @@ -301,6 +301,9 @@ static int qio_channel_command_close(QIOChannel *ioc, { QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc); int rv = 0; +#ifndef WIN32 + pid_t wp; +#endif /* We close FDs before killing, because that * gives a better chance of clean shutdown @@ -315,11 +318,18 @@ static int qio_channel_command_close(QIOChannel *ioc, rv = -1; } cioc->writefd = cioc->readfd = -1; + #ifndef WIN32 - if (qio_channel_command_abort(cioc, errp) < 0) { + do { + wp = waitpid(cioc->pid, NULL, 0); + } while (wp == (pid_t)-1 && errno == EINTR); + if (wp == (pid_t)-1) { + error_setg_errno(errp, errno, "Failed to wait for pid %llu", + (unsigned long long)cioc->pid); return -1; } #endif + if (rv < 0) { error_setg_errno(errp, errno, "%s", "Unable to close command"); From 6809df1df036840d41a0cc9ca77cc6a0214fb1b5 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Fri, 19 Jan 2018 00:52:27 -0700 Subject: [PATCH 7/7] allow to build with older sed sed's -E option may not be supported by older distros. As there's no point using sed here at all, use just shell mechanisms to establish the variable values, starting from the stem instead of the full target. Signed-off-by: Jan Beulich Signed-off-by: Daniel P. Berrange --- Makefile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Makefile b/Makefile index b5a6d602b2..90e05ac409 100644 --- a/Makefile +++ b/Makefile @@ -256,8 +256,7 @@ GENERATED_FILES += $(KEYCODEMAP_FILES) ui/input-keymap-%.c: $(KEYCODEMAP_GEN) $(KEYCODEMAP_CSV) $(SRC_PATH)/ui/Makefile.objs $(call quiet-command,\ - src=$$(echo $@ | sed -E -e "s,^ui/input-keymap-(.+)-to-(.+)\.c$$,\1,") && \ - dst=$$(echo $@ | sed -E -e "s,^ui/input-keymap-(.+)-to-(.+)\.c$$,\2,") && \ + stem=$* && src=$${stem%-to-*} dst=$${stem#*-to-} && \ test -e $(KEYCODEMAP_GEN) && \ $(PYTHON) $(KEYCODEMAP_GEN) \ --lang glib2 \