* [Qemu-devel] [PULL 01/18] io: store reference to thread information in the QIOTask struct
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 02/18] io: add qio_task_wait_thread to join with a background thread Marc-André Lureau
` (18 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé
From: Daniel P. Berrangé <berrange@redhat.com>
Currently the struct QIOTaskThreadData is only needed by the worker
thread, but a subsequent patch will need to access it from another
context.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-2-berrange@redhat.com>
[ Marc-André - applied Daniel fixup ]
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
io/task.c | 64 +++++++++++++++++++++++++++++--------------------------
1 file changed, 34 insertions(+), 30 deletions(-)
diff --git a/io/task.c b/io/task.c
index 2886a2c1bc..396866b10f 100644
--- a/io/task.c
+++ b/io/task.c
@@ -24,6 +24,14 @@
#include "qemu/thread.h"
#include "trace.h"
+struct QIOTaskThreadData {
+ QIOTaskWorker worker;
+ gpointer opaque;
+ GDestroyNotify destroy;
+ GMainContext *context;
+};
+
+
struct QIOTask {
Object *source;
QIOTaskFunc func;
@@ -32,6 +40,7 @@ struct QIOTask {
Error *err;
gpointer result;
GDestroyNotify destroyResult;
+ struct QIOTaskThreadData *thread;
};
@@ -57,6 +66,18 @@ QIOTask *qio_task_new(Object *source,
static void qio_task_free(QIOTask *task)
{
+ if (task->thread) {
+ if (task->thread->destroy) {
+ task->thread->destroy(task->thread->opaque);
+ }
+
+ if (task->thread->context) {
+ g_main_context_unref(task->thread->context);
+ }
+
+ g_free(task->thread);
+ }
+
if (task->destroy) {
task->destroy(task->opaque);
}
@@ -72,31 +93,12 @@ static void qio_task_free(QIOTask *task)
}
-struct QIOTaskThreadData {
- QIOTask *task;
- QIOTaskWorker worker;
- gpointer opaque;
- GDestroyNotify destroy;
- GMainContext *context;
-};
-
-
static gboolean qio_task_thread_result(gpointer opaque)
{
- struct QIOTaskThreadData *data = opaque;
+ QIOTask *task = opaque;
- trace_qio_task_thread_result(data->task);
- qio_task_complete(data->task);
-
- if (data->destroy) {
- data->destroy(data->opaque);
- }
-
- if (data->context) {
- g_main_context_unref(data->context);
- }
-
- g_free(data);
+ trace_qio_task_thread_result(task);
+ qio_task_complete(task);
return FALSE;
}
@@ -104,22 +106,23 @@ static gboolean qio_task_thread_result(gpointer opaque)
static gpointer qio_task_thread_worker(gpointer opaque)
{
- struct QIOTaskThreadData *data = opaque;
+ QIOTask *task = opaque;
GSource *idle;
- trace_qio_task_thread_run(data->task);
- data->worker(data->task, data->opaque);
+ trace_qio_task_thread_run(task);
+
+ task->thread->worker(task, task->thread->opaque);
/* We're running in the background thread, and must only
* ever report the task results in the main event loop
* thread. So we schedule an idle callback to report
* the worker results
*/
- trace_qio_task_thread_exit(data->task);
+ trace_qio_task_thread_exit(task);
idle = g_idle_source_new();
- g_source_set_callback(idle, qio_task_thread_result, data, NULL);
- g_source_attach(idle, data->context);
+ g_source_set_callback(idle, qio_task_thread_result, task, NULL);
+ g_source_attach(idle, task->thread->context);
return NULL;
}
@@ -138,17 +141,18 @@ void qio_task_run_in_thread(QIOTask *task,
g_main_context_ref(context);
}
- data->task = task;
data->worker = worker;
data->opaque = opaque;
data->destroy = destroy;
data->context = context;
+ task->thread = data;
+
trace_qio_task_thread_start(task, worker, opaque);
qemu_thread_create(&thread,
"io-task-worker",
qio_task_thread_worker,
- data,
+ task,
QEMU_THREAD_DETACHED);
}
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 02/18] io: add qio_task_wait_thread to join with a background thread
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 01/18] io: store reference to thread information in the QIOTask struct Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 03/18] chardev: fix validation of options for QMP created chardevs Marc-André Lureau
` (17 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé
From: Daniel P. Berrangé <berrange@redhat.com>
Add the ability for a caller to wait for completion of the
background thread to synchronously dispatch its result, without
needing to wait for the main loop to run the idle callback.
This method needs very careful usage to avoid a dangerous
race condition with the free'ing of the task. The completion
callback is normally invoked from an idle callback registered
with the main loop context. The qio_task_wait_thread method
must only be called if the completion callback has not yet
run. The only safe way to achieve this is to run the
qio_task_wait_thread method from the thread that executes
the main loop.
It is generally a bad idea to use this method since it will
block execution of the main loop, however, the design of
the character devices and its usage from vhostuser already
requires blocking execution.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-3-berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/io/task.h | 29 ++++++++++++++++++++++++++++-
io/task.c | 40 ++++++++++++++++++++++++++++++++++++----
io/trace-events | 2 ++
3 files changed, 66 insertions(+), 5 deletions(-)
diff --git a/include/io/task.h b/include/io/task.h
index 9e09b95b2e..57d8ba835e 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -232,7 +232,8 @@ QIOTask *qio_task_new(Object *source,
*
* Run a task in a background thread. When @worker
* returns it will call qio_task_complete() in
- * the event thread context that provided.
+ * the thread that is running the main loop associated
+ * with @context.
*/
void qio_task_run_in_thread(QIOTask *task,
QIOTaskWorker worker,
@@ -240,6 +241,32 @@ void qio_task_run_in_thread(QIOTask *task,
GDestroyNotify destroy,
GMainContext *context);
+
+/**
+ * qio_task_wait_thread:
+ * @task: the task struct
+ *
+ * Wait for completion of a task that was previously
+ * invoked using qio_task_run_in_thread. This MUST
+ * ONLY be invoked if the task has not already
+ * completed, since after the completion callback
+ * is invoked, @task will have been freed.
+ *
+ * To avoid racing with execution of the completion
+ * callback provided with qio_task_new, this method
+ * MUST ONLY be invoked from the thread that is
+ * running the main loop associated with @context
+ * parameter to qio_task_run_in_thread.
+ *
+ * When the thread has completed, the completion
+ * callback provided to qio_task_new will be invoked.
+ * When that callback returns @task will be freed,
+ * so @task must not be referenced after this
+ * method completes.
+ */
+void qio_task_wait_thread(QIOTask *task);
+
+
/**
* qio_task_complete:
* @task: the task struct
diff --git a/io/task.c b/io/task.c
index 396866b10f..18ddc4afd1 100644
--- a/io/task.c
+++ b/io/task.c
@@ -29,6 +29,7 @@ struct QIOTaskThreadData {
gpointer opaque;
GDestroyNotify destroy;
GMainContext *context;
+ GSource *completion;
};
@@ -40,6 +41,8 @@ struct QIOTask {
Error *err;
gpointer result;
GDestroyNotify destroyResult;
+ QemuMutex thread_lock;
+ QemuCond thread_cond;
struct QIOTaskThreadData *thread;
};
@@ -58,6 +61,8 @@ QIOTask *qio_task_new(Object *source,
task->func = func;
task->opaque = opaque;
task->destroy = destroy;
+ qemu_mutex_init(&task->thread_lock);
+ qemu_cond_init(&task->thread_cond);
trace_qio_task_new(task, source, func, opaque);
@@ -89,6 +94,9 @@ static void qio_task_free(QIOTask *task)
}
object_unref(task->source);
+ qemu_mutex_destroy(&task->thread_lock);
+ qemu_cond_destroy(&task->thread_cond);
+
g_free(task);
}
@@ -98,6 +106,7 @@ static gboolean qio_task_thread_result(gpointer opaque)
QIOTask *task = opaque;
trace_qio_task_thread_result(task);
+
qio_task_complete(task);
return FALSE;
@@ -107,7 +116,6 @@ static gboolean qio_task_thread_result(gpointer opaque)
static gpointer qio_task_thread_worker(gpointer opaque)
{
QIOTask *task = opaque;
- GSource *idle;
trace_qio_task_thread_run(task);
@@ -120,9 +128,17 @@ static gpointer qio_task_thread_worker(gpointer opaque)
*/
trace_qio_task_thread_exit(task);
- idle = g_idle_source_new();
- g_source_set_callback(idle, qio_task_thread_result, task, NULL);
- g_source_attach(idle, task->thread->context);
+ qemu_mutex_lock(&task->thread_lock);
+
+ task->thread->completion = g_idle_source_new();
+ g_source_set_callback(task->thread->completion,
+ qio_task_thread_result, task, NULL);
+ g_source_attach(task->thread->completion,
+ task->thread->context);
+ trace_qio_task_thread_source_attach(task, task->thread->completion);
+
+ qemu_cond_signal(&task->thread_cond);
+ qemu_mutex_unlock(&task->thread_lock);
return NULL;
}
@@ -157,6 +173,22 @@ void qio_task_run_in_thread(QIOTask *task,
}
+void qio_task_wait_thread(QIOTask *task)
+{
+ qemu_mutex_lock(&task->thread_lock);
+ g_assert(task->thread != NULL);
+ while (task->thread->completion == NULL) {
+ qemu_cond_wait(&task->thread_cond, &task->thread_lock);
+ }
+
+ trace_qio_task_thread_source_cancel(task, task->thread->completion);
+ g_source_destroy(task->thread->completion);
+ qemu_mutex_unlock(&task->thread_lock);
+
+ qio_task_thread_result(task);
+}
+
+
void qio_task_complete(QIOTask *task)
{
task->func(task, task->opaque);
diff --git a/io/trace-events b/io/trace-events
index f70bad7cbe..07a7bbec6a 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -7,6 +7,8 @@ qio_task_thread_start(void *task, void *worker, void *opaque) "Task thread start
qio_task_thread_run(void *task) "Task thread run task=%p"
qio_task_thread_exit(void *task) "Task thread exit task=%p"
qio_task_thread_result(void *task) "Task thread result task=%p"
+qio_task_thread_source_attach(void *task, void *source) "Task thread source attach task=%p source=%p"
+qio_task_thread_source_cancel(void *task, void *source) "Task thread source cancel task=%p source=%p"
# io/channel-socket.c
qio_channel_socket_new(void *ioc) "Socket new ioc=%p"
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 03/18] chardev: fix validation of options for QMP created chardevs
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 01/18] io: store reference to thread information in the QIOTask struct Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 02/18] io: add qio_task_wait_thread to join with a background thread Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 04/18] chardev: forbid 'reconnect' option with server sockets Marc-André Lureau
` (16 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
The TLS creds option is not valid with certain address types. The user
config was only checked for errors when parsing legacy QemuOpts, thus
the user could pass unsupported values via QMP.
Pull all code for validating options out into a new method
qmp_chardev_validate_socket, that is called from the main
qmp_chardev_open_socket method. This adds a missing check for rejecting
TLS creds with the vsock address type.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-4-berrange@redhat.com>
---
chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------
1 file changed, 66 insertions(+), 26 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index eaa8e8b68f..e85250b624 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
return false;
}
+
+static bool qmp_chardev_validate_socket(ChardevSocket *sock,
+ SocketAddress *addr,
+ Error **errp)
+{
+ /* Validate any options which have a dependency on address type */
+ switch (addr->type) {
+ case SOCKET_ADDRESS_TYPE_FD:
+ if (sock->has_reconnect) {
+ error_setg(errp,
+ "'reconnect' option is incompatible with "
+ "'fd' address type");
+ return false;
+ }
+ if (sock->has_tls_creds &&
+ !(sock->has_server && sock->server)) {
+ error_setg(errp,
+ "'tls_creds' option is incompatible with "
+ "'fd' address type as client");
+ return false;
+ }
+ break;
+
+ case SOCKET_ADDRESS_TYPE_UNIX:
+ if (sock->has_tls_creds) {
+ error_setg(errp,
+ "'tls_creds' option is incompatible with "
+ "'unix' address type");
+ return false;
+ }
+ break;
+
+ case SOCKET_ADDRESS_TYPE_INET:
+ break;
+
+ case SOCKET_ADDRESS_TYPE_VSOCK:
+ if (sock->has_tls_creds) {
+ error_setg(errp,
+ "'tls_creds' option is incompatible with "
+ "'vsock' address type");
+ return false;
+ }
+
+ default:
+ break;
+ }
+
+ /* Validate any options which have a dependancy on client vs server */
+ if (!(sock->has_server && sock->server)) {
+ if (sock->has_websocket && sock->websocket) {
+ error_setg(errp, "%s", "Websocket client is not implemented");
+ return false;
+ }
+ }
+
+ return true;
+}
+
+
static void qmp_chardev_open_socket(Chardev *chr,
ChardevBackend *backend,
bool *be_opened,
@@ -1004,11 +1063,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
QIOChannelSocket *sioc = NULL;
SocketAddress *addr;
- if (!is_listen && is_websock) {
- error_setg(errp, "%s", "Websocket client is not implemented");
- goto error;
- }
-
s->is_listen = is_listen;
s->is_telnet = is_telnet;
s->is_tn3270 = is_tn3270;
@@ -1049,10 +1103,10 @@ static void qmp_chardev_open_socket(Chardev *chr,
s->addr = addr = socket_address_flatten(sock->addr);
- if (sock->has_reconnect && addr->type == SOCKET_ADDRESS_TYPE_FD) {
- error_setg(errp, "'reconnect' option is incompatible with 'fd'");
+ if (!qmp_chardev_validate_socket(sock, addr, errp)) {
goto error;
}
+
qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
/* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */
if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) {
@@ -1140,27 +1194,12 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
return;
}
- backend->type = CHARDEV_BACKEND_KIND_SOCKET;
- if (path) {
- if (tls_creds) {
- error_setg(errp, "TLS can only be used over TCP socket");
- return;
- }
- } else if (host) {
- if (!port) {
- error_setg(errp, "chardev: socket: no port given");
- return;
- }
- } else if (fd) {
- /* We don't know what host to validate against when in client mode */
- if (tls_creds && !is_listen) {
- error_setg(errp, "TLS can not be used with pre-opened client FD");
- return;
- }
- } else {
- g_assert_not_reached();
+ if (host && !port) {
+ error_setg(errp, "chardev: socket: no port given");
+ return;
}
+ backend->type = CHARDEV_BACKEND_KIND_SOCKET;
sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
@@ -1178,6 +1217,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
sock->wait = is_waitconnect;
sock->has_reconnect = qemu_opt_find(opts, "reconnect");
sock->reconnect = reconnect;
+ sock->has_tls_creds = tls_creds;
sock->tls_creds = g_strdup(tls_creds);
addr = g_new0(SocketAddressLegacy, 1);
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 04/18] chardev: forbid 'reconnect' option with server sockets
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (2 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 03/18] chardev: fix validation of options for QMP created chardevs Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 05/18] chardev: forbid 'wait' option with client sockets Marc-André Lureau
` (15 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
The 'reconnect' option is used to give the sleep time, in seconds,
before a client socket attempts to re-establish a connection to the
server. It does not make sense to set this for server sockets, as they
will always accept a new client connection immediately after the
previous one went away.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-5-berrange@redhat.com>
---
chardev/char-socket.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index e85250b624..743b7b11cd 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1035,7 +1035,14 @@ static bool qmp_chardev_validate_socket(ChardevSocket *sock,
}
/* Validate any options which have a dependancy on client vs server */
- if (!(sock->has_server && sock->server)) {
+ if (!sock->has_server || sock->server) {
+ if (sock->has_reconnect) {
+ error_setg(errp,
+ "'reconnect' option is incompatible with "
+ "socket in server listen mode");
+ return false;
+ }
+ } else {
if (sock->has_websocket && sock->websocket) {
error_setg(errp, "%s", "Websocket client is not implemented");
return false;
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 05/18] chardev: forbid 'wait' option with client sockets
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (3 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 04/18] chardev: forbid 'reconnect' option with server sockets Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 06/18] chardev: remove many local variables in qemu_chr_parse_socket Marc-André Lureau
` (14 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini, Thomas Huth, Laurent Vivier
From: Daniel P. Berrangé <berrange@redhat.com>
The 'wait'/'nowait' parameter is used to tell server sockets whether to
block until a client is accepted during initialization. Client chardevs
have always silently ignored this option. Various tests were mistakenly
passing this option for their client chardevs.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-6-berrange@redhat.com>
---
chardev/char-socket.c | 12 +++++++++++-
tests/ivshmem-test.c | 2 +-
tests/libqtest.c | 4 ++--
tests/test-filter-redirector.c | 4 ++--
4 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 743b7b11cd..728342dc9f 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1047,6 +1047,12 @@ static bool qmp_chardev_validate_socket(ChardevSocket *sock,
error_setg(errp, "%s", "Websocket client is not implemented");
return false;
}
+ if (sock->has_wait) {
+ error_setg(errp, "%s",
+ "'wait' option is incompatible with "
+ "socket in client connect mode");
+ return false;
+ }
}
return true;
@@ -1220,7 +1226,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
sock->tn3270 = is_tn3270;
sock->has_websocket = true;
sock->websocket = is_websock;
- sock->has_wait = true;
+ /*
+ * We have different default to QMP for 'wait' when 'server'
+ * is set, hence we can't just check for existence of 'wait'
+ */
+ sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
sock->wait = is_waitconnect;
sock->has_reconnect = qemu_opt_find(opts, "reconnect");
sock->reconnect = reconnect;
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index 4911b69317..942ddc9192 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -295,7 +295,7 @@ static void setup_vm_with_server(IVState *s, int nvectors)
{
char *cmd;
- cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
+ cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s "
"-device ivshmem-doorbell,chardev=chr0,vectors=%d",
tmpserver, nvectors);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 6fb30855fa..c49b85482d 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -232,9 +232,9 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
qtest_add_abrt_handler(kill_qemu_hook_func, s);
command = g_strdup_printf("exec %s "
- "-qtest unix:%s,nowait "
+ "-qtest unix:%s "
"-qtest-log %s "
- "-chardev socket,path=%s,nowait,id=char0 "
+ "-chardev socket,path=%s,id=char0 "
"-mon chardev=char0,mode=control "
"-machine accel=qtest "
"-display none "
diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
index 9ca9feabf8..6dc21dd4fb 100644
--- a/tests/test-filter-redirector.c
+++ b/tests/test-filter-redirector.c
@@ -96,7 +96,7 @@ static void test_redirector_tx(void)
"-device %s,netdev=qtest-bn0,id=qtest-e0 "
"-chardev socket,id=redirector0,path=%s,server,nowait "
"-chardev socket,id=redirector1,path=%s,server,nowait "
- "-chardev socket,id=redirector2,path=%s,nowait "
+ "-chardev socket,id=redirector2,path=%s "
"-object filter-redirector,id=qtest-f0,netdev=qtest-bn0,"
"queue=tx,outdev=redirector0 "
"-object filter-redirector,id=qtest-f1,netdev=qtest-bn0,"
@@ -166,7 +166,7 @@ static void test_redirector_rx(void)
"-device %s,netdev=qtest-bn0,id=qtest-e0 "
"-chardev socket,id=redirector0,path=%s,server,nowait "
"-chardev socket,id=redirector1,path=%s,server,nowait "
- "-chardev socket,id=redirector2,path=%s,nowait "
+ "-chardev socket,id=redirector2,path=%s "
"-object filter-redirector,id=qtest-f0,netdev=qtest-bn0,"
"queue=rx,indev=redirector0 "
"-object filter-redirector,id=qtest-f1,netdev=qtest-bn0,"
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 06/18] chardev: remove many local variables in qemu_chr_parse_socket
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (4 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 05/18] chardev: forbid 'wait' option with client sockets Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 07/18] chardev: ensure qemu_chr_parse_compat reports missing driver error Marc-André Lureau
` (13 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
Now that all validation is separated off into a separate method,
we can directly populate the ChardevSocket struct from the
QemuOpts values, avoiding many local variables.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-7-berrange@redhat.com>
---
chardev/char-socket.c | 40 ++++++++++++++++++----------------------
1 file changed, 18 insertions(+), 22 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 728342dc9f..8a6e203da7 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1186,18 +1186,10 @@ error:
static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
Error **errp)
{
- bool is_listen = qemu_opt_get_bool(opts, "server", false);
- bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
- bool is_telnet = qemu_opt_get_bool(opts, "telnet", false);
- bool is_tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
- bool is_websock = qemu_opt_get_bool(opts, "websocket", false);
- bool do_nodelay = !qemu_opt_get_bool(opts, "delay", true);
- int64_t reconnect = qemu_opt_get_number(opts, "reconnect", 0);
const char *path = qemu_opt_get(opts, "path");
const char *host = qemu_opt_get(opts, "host");
const char *port = qemu_opt_get(opts, "port");
const char *fd = qemu_opt_get(opts, "fd");
- const char *tls_creds = qemu_opt_get(opts, "tls-creds");
SocketAddressLegacy *addr;
ChardevSocket *sock;
@@ -1216,26 +1208,30 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
- sock->has_nodelay = true;
- sock->nodelay = do_nodelay;
+ sock->has_nodelay = qemu_opt_get(opts, "delay");
+ sock->nodelay = !qemu_opt_get_bool(opts, "delay", true);
+ /*
+ * We have different default to QMP for 'server', hence
+ * we can't just check for existence of 'server'
+ */
sock->has_server = true;
- sock->server = is_listen;
- sock->has_telnet = true;
- sock->telnet = is_telnet;
- sock->has_tn3270 = true;
- sock->tn3270 = is_tn3270;
- sock->has_websocket = true;
- sock->websocket = is_websock;
+ sock->server = qemu_opt_get_bool(opts, "server", false);
+ sock->has_telnet = qemu_opt_get(opts, "telnet");
+ sock->telnet = qemu_opt_get_bool(opts, "telnet", false);
+ sock->has_tn3270 = qemu_opt_get(opts, "tn3270");
+ sock->tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
+ sock->has_websocket = qemu_opt_get(opts, "websocket");
+ sock->websocket = qemu_opt_get_bool(opts, "websocket", false);
/*
* We have different default to QMP for 'wait' when 'server'
* is set, hence we can't just check for existence of 'wait'
*/
- sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
- sock->wait = is_waitconnect;
+ sock->has_wait = qemu_opt_find(opts, "wait") || sock->server;
+ sock->wait = qemu_opt_get_bool(opts, "wait", true);
sock->has_reconnect = qemu_opt_find(opts, "reconnect");
- sock->reconnect = reconnect;
- sock->has_tls_creds = tls_creds;
- sock->tls_creds = g_strdup(tls_creds);
+ sock->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
+ sock->has_tls_creds = qemu_opt_get(opts, "tls-creds");
+ sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
addr = g_new0(SocketAddressLegacy, 1);
if (path) {
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 07/18] chardev: ensure qemu_chr_parse_compat reports missing driver error
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (5 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 06/18] chardev: remove many local variables in qemu_chr_parse_socket Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 08/18] chardev: remove unused 'sioc' variable & cleanup paths Marc-André Lureau
` (12 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
If no valid char driver was identified the qemu_chr_parse_compat method
was silent, leaving callers no clue what failed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-8-berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
chardev/char.c | 2 ++
tests/test-char.c | 3 ++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/chardev/char.c b/chardev/char.c
index ccba36bafb..b99f3692f7 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -490,6 +490,8 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
return opts;
}
+ error_report("'%s' is not a valid char driver", filename);
+
fail:
qemu_opts_del(opts);
return NULL;
diff --git a/tests/test-char.c b/tests/test-char.c
index 19c3efad72..89c43e4ada 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -856,9 +856,10 @@ static void char_null_test(void)
static void char_invalid_test(void)
{
Chardev *chr;
-
+ g_setenv("QTEST_SILENT_ERRORS", "1", 1);
chr = qemu_chr_new("label-invalid", "invalid");
g_assert_null(chr);
+ g_unsetenv("QTEST_SILENT_ERRORS");
}
static int chardev_change(void *opaque)
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 08/18] chardev: remove unused 'sioc' variable & cleanup paths
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (6 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 07/18] chardev: ensure qemu_chr_parse_compat reports missing driver error Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 09/18] chardev: split tcp_chr_wait_connected into two methods Marc-André Lureau
` (11 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
The 'sioc' variable in qmp_chardev_open_socket was unused since
commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24
Author: Peter Xu <peterx@redhat.com>
Date: Tue Mar 6 13:33:17 2018 +0800
chardev: use chardev's gcontext for async connect
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-9-berrange@redhat.com>
---
chardev/char-socket.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 8a6e203da7..8a5e5c2fe7 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1073,7 +1073,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
bool is_waitconnect = sock->has_wait ? sock->wait : false;
bool is_websock = sock->has_websocket ? sock->websocket : false;
int64_t reconnect = sock->has_reconnect ? sock->reconnect : 0;
- QIOChannelSocket *sioc = NULL;
SocketAddress *addr;
s->is_listen = is_listen;
@@ -1088,7 +1087,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
if (!creds) {
error_setg(errp, "No TLS credentials with id '%s'",
sock->tls_creds);
- goto error;
+ return;
}
s->tls_creds = (QCryptoTLSCreds *)
object_dynamic_cast(creds,
@@ -1096,20 +1095,20 @@ static void qmp_chardev_open_socket(Chardev *chr,
if (!s->tls_creds) {
error_setg(errp, "Object with id '%s' is not TLS credentials",
sock->tls_creds);
- goto error;
+ return;
}
object_ref(OBJECT(s->tls_creds));
if (is_listen) {
if (s->tls_creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
error_setg(errp, "%s",
"Expected TLS credentials for server endpoint");
- goto error;
+ return;
}
} else {
if (s->tls_creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) {
error_setg(errp, "%s",
"Expected TLS credentials for client endpoint");
- goto error;
+ return;
}
}
}
@@ -1117,7 +1116,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
s->addr = addr = socket_address_flatten(sock->addr);
if (!qmp_chardev_validate_socket(sock, addr, errp)) {
- goto error;
+ return;
}
qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
@@ -1153,7 +1152,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
object_unref(OBJECT(s->listener));
s->listener = NULL;
- goto error;
+ return;
}
qapi_free_SocketAddress(s->addr);
@@ -1171,16 +1170,9 @@ static void qmp_chardev_open_socket(Chardev *chr,
chr->gcontext);
}
} else if (qemu_chr_wait_connected(chr, errp) < 0) {
- goto error;
+ return;
}
}
-
- return;
-
-error:
- if (sioc) {
- object_unref(OBJECT(sioc));
- }
}
static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 09/18] chardev: split tcp_chr_wait_connected into two methods
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (7 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 08/18] chardev: remove unused 'sioc' variable & cleanup paths Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 10/18] chardev: split up qmp_chardev_open_socket connection code Marc-André Lureau
` (10 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
The tcp_chr_wait_connected method can deal with either server or client
chardevs, but some callers only care about one of these possibilities.
The tcp_chr_wait_connected method will also need some refactoring to
reliably deal with its primary goal of allowing a device frontend to
wait for an established connection, which will interfere with other
callers.
Split it into two methods, one responsible for server initiated
connections, the other responsible for client initiated connections.
In doing this split the tcp_char_connect_async() method is renamed
to become consistent with naming of the new methods.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-10-berrange@redhat.com>
---
chardev/char-socket.c | 59 +++++++++++++++++++++++++++----------------
1 file changed, 37 insertions(+), 22 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 8a5e5c2fe7..222adbbad3 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -886,30 +886,47 @@ static void tcp_chr_accept(QIONetListener *listener,
tcp_chr_new_client(chr, cioc);
}
-static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
+
+static int tcp_chr_connect_client_sync(Chardev *chr, Error **errp)
+{
+ SocketChardev *s = SOCKET_CHARDEV(chr);
+ QIOChannelSocket *sioc = qio_channel_socket_new();
+ tcp_chr_set_client_ioc_name(chr, sioc);
+ if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
+ object_unref(OBJECT(sioc));
+ return -1;
+ }
+ tcp_chr_new_client(chr, sioc);
+ object_unref(OBJECT(sioc));
+ return 0;
+}
+
+
+static void tcp_chr_accept_server_sync(Chardev *chr)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
QIOChannelSocket *sioc;
+ info_report("QEMU waiting for connection on: %s",
+ chr->filename);
+ sioc = qio_net_listener_wait_client(s->listener);
+ tcp_chr_set_client_ioc_name(chr, sioc);
+ tcp_chr_new_client(chr, sioc);
+ object_unref(OBJECT(sioc));
+}
+
+static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
+{
+ SocketChardev *s = SOCKET_CHARDEV(chr);
/* It can't wait on s->connected, since it is set asynchronously
* in TLS and telnet cases, only wait for an accepted socket */
while (!s->ioc) {
if (s->is_listen) {
- info_report("QEMU waiting for connection on: %s",
- chr->filename);
- sioc = qio_net_listener_wait_client(s->listener);
- tcp_chr_set_client_ioc_name(chr, sioc);
- tcp_chr_new_client(chr, sioc);
- object_unref(OBJECT(sioc));
+ tcp_chr_accept_server_sync(chr);
} else {
- sioc = qio_channel_socket_new();
- tcp_chr_set_client_ioc_name(chr, sioc);
- if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
- object_unref(OBJECT(sioc));
+ if (tcp_chr_connect_client_sync(chr, errp) < 0) {
return -1;
}
- tcp_chr_new_client(chr, sioc);
- object_unref(OBJECT(sioc));
}
}
@@ -958,7 +975,7 @@ cleanup:
object_unref(OBJECT(sioc));
}
-static void tcp_chr_connect_async(Chardev *chr)
+static void tcp_chr_connect_client_async(Chardev *chr)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
QIOChannelSocket *sioc;
@@ -982,7 +999,7 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
return false;
}
- tcp_chr_connect_async(chr);
+ tcp_chr_connect_client_async(chr);
return false;
}
@@ -1139,7 +1156,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
}
if (s->reconnect_time) {
- tcp_chr_connect_async(chr);
+ tcp_chr_connect_client_async(chr);
} else {
if (s->is_listen) {
char *name;
@@ -1159,17 +1176,15 @@ static void qmp_chardev_open_socket(Chardev *chr,
s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
update_disconnected_filename(s);
- if (is_waitconnect &&
- qemu_chr_wait_connected(chr, errp) < 0) {
- return;
- }
- if (!s->ioc) {
+ if (is_waitconnect) {
+ tcp_chr_accept_server_sync(chr);
+ } else {
qio_net_listener_set_client_func_full(s->listener,
tcp_chr_accept,
chr, NULL,
chr->gcontext);
}
- } else if (qemu_chr_wait_connected(chr, errp) < 0) {
+ } else if (tcp_chr_connect_client_sync(chr, errp) < 0) {
return;
}
}
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 10/18] chardev: split up qmp_chardev_open_socket connection code
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (8 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 09/18] chardev: split tcp_chr_wait_connected into two methods Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 11/18] chardev: use a state machine for socket connection state Marc-André Lureau
` (9 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
In qmp_chardev_open_socket the code for connecting client chardevs is
split across two conditionals far apart with some server chardev code in
the middle. Split up the method so that code for client connection setup
is separate from code for server connection setup.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-11-berrange@redhat.com>
---
chardev/char-socket.c | 96 +++++++++++++++++++++++++++----------------
1 file changed, 60 insertions(+), 36 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 222adbbad3..90dafef7d4 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1005,6 +1005,61 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
}
+static int qmp_chardev_open_socket_server(Chardev *chr,
+ bool is_telnet,
+ bool is_waitconnect,
+ Error **errp)
+{
+ SocketChardev *s = SOCKET_CHARDEV(chr);
+ char *name;
+ if (is_telnet) {
+ s->do_telnetopt = 1;
+ }
+ s->listener = qio_net_listener_new();
+
+ name = g_strdup_printf("chardev-tcp-listener-%s", chr->label);
+ qio_net_listener_set_name(s->listener, name);
+ g_free(name);
+
+ if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
+ object_unref(OBJECT(s->listener));
+ s->listener = NULL;
+ return -1;
+ }
+
+ qapi_free_SocketAddress(s->addr);
+ s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
+ update_disconnected_filename(s);
+
+ if (is_waitconnect) {
+ tcp_chr_accept_server_sync(chr);
+ } else {
+ qio_net_listener_set_client_func_full(s->listener,
+ tcp_chr_accept,
+ chr, NULL,
+ chr->gcontext);
+ }
+
+ return 0;
+}
+
+
+static int qmp_chardev_open_socket_client(Chardev *chr,
+ int64_t reconnect,
+ Error **errp)
+{
+ SocketChardev *s = SOCKET_CHARDEV(chr);
+
+ if (reconnect > 0) {
+ s->reconnect_time = reconnect;
+ tcp_chr_connect_client_async(chr);
+ return 0;
+ } else {
+ return tcp_chr_connect_client_sync(chr, errp);
+ }
+}
+
+
static bool qmp_chardev_validate_socket(ChardevSocket *sock,
SocketAddress *addr,
Error **errp)
@@ -1147,44 +1202,13 @@ static void qmp_chardev_open_socket(Chardev *chr,
update_disconnected_filename(s);
- if (is_listen) {
- if (is_telnet || is_tn3270) {
- s->do_telnetopt = 1;
+ if (s->is_listen) {
+ if (qmp_chardev_open_socket_server(chr, is_telnet || is_tn3270,
+ is_waitconnect, errp) < 0) {
+ return;
}
- } else if (reconnect > 0) {
- s->reconnect_time = reconnect;
- }
-
- if (s->reconnect_time) {
- tcp_chr_connect_client_async(chr);
} else {
- if (s->is_listen) {
- char *name;
- s->listener = qio_net_listener_new();
-
- name = g_strdup_printf("chardev-tcp-listener-%s", chr->label);
- qio_net_listener_set_name(s->listener, name);
- g_free(name);
-
- if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
- object_unref(OBJECT(s->listener));
- s->listener = NULL;
- return;
- }
-
- qapi_free_SocketAddress(s->addr);
- s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
- update_disconnected_filename(s);
-
- if (is_waitconnect) {
- tcp_chr_accept_server_sync(chr);
- } else {
- qio_net_listener_set_client_func_full(s->listener,
- tcp_chr_accept,
- chr, NULL,
- chr->gcontext);
- }
- } else if (tcp_chr_connect_client_sync(chr, errp) < 0) {
+ if (qmp_chardev_open_socket_client(chr, reconnect, errp) < 0) {
return;
}
}
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 11/18] chardev: use a state machine for socket connection state
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (9 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 10/18] chardev: split up qmp_chardev_open_socket connection code Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 12/18] chardev: honour the reconnect setting in tcp_chr_wait_connected Marc-André Lureau
` (8 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
The socket connection state is indicated via the 'bool connected' field
in the SocketChardev struct. This variable is somewhat misleading
though, as it is only set to true once the connection has completed all
required handshakes (eg for TLS, telnet or websockets). IOW there is a
period of time in which the socket is connected, but the "connected"
flag is still false.
The socket chardev really has three states that it can be in,
disconnected, connecting and connected and those should be tracked
explicitly.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-12-berrange@redhat.com>
---
chardev/char-socket.c | 63 +++++++++++++++++++++++++++++++++----------
1 file changed, 49 insertions(+), 14 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 90dafef7d4..d6de5d2305 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -46,6 +46,12 @@ typedef struct {
size_t buflen;
} TCPChardevTelnetInit;
+typedef enum {
+ TCP_CHARDEV_STATE_DISCONNECTED,
+ TCP_CHARDEV_STATE_CONNECTING,
+ TCP_CHARDEV_STATE_CONNECTED,
+} TCPChardevState;
+
typedef struct {
Chardev parent;
QIOChannel *ioc; /* Client I/O channel */
@@ -53,7 +59,7 @@ typedef struct {
QIONetListener *listener;
GSource *hup_source;
QCryptoTLSCreds *tls_creds;
- int connected;
+ TCPChardevState state;
int max_size;
int do_telnetopt;
int do_nodelay;
@@ -82,6 +88,21 @@ typedef struct {
static gboolean socket_reconnect_timeout(gpointer opaque);
static void tcp_chr_telnet_init(Chardev *chr);
+static void tcp_chr_change_state(SocketChardev *s, TCPChardevState state)
+{
+ switch (state) {
+ case TCP_CHARDEV_STATE_DISCONNECTED:
+ break;
+ case TCP_CHARDEV_STATE_CONNECTING:
+ assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED);
+ break;
+ case TCP_CHARDEV_STATE_CONNECTED:
+ assert(s->state == TCP_CHARDEV_STATE_CONNECTING);
+ break;
+ }
+ s->state = state;
+}
+
static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
{
if (s->reconnect_timer) {
@@ -96,7 +117,7 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
SocketChardev *s = SOCKET_CHARDEV(chr);
char *name;
- assert(s->connected == 0);
+ assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED);
name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
s->reconnect_timer = qemu_chr_timeout_add_ms(chr,
s->reconnect_time * 1000,
@@ -131,7 +152,7 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
- if (s->connected) {
+ if (s->state == TCP_CHARDEV_STATE_CONNECTED) {
int ret = io_channel_send_full(s->ioc, buf, len,
s->write_msgfds,
s->write_msgfds_num);
@@ -164,7 +185,7 @@ static int tcp_chr_read_poll(void *opaque)
{
Chardev *chr = CHARDEV(opaque);
SocketChardev *s = SOCKET_CHARDEV(opaque);
- if (!s->connected) {
+ if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
return 0;
}
s->max_size = qemu_chr_be_can_write(chr);
@@ -277,7 +298,7 @@ static int tcp_set_msgfds(Chardev *chr, int *fds, int num)
s->write_msgfds = NULL;
s->write_msgfds_num = 0;
- if (!s->connected ||
+ if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||
!qio_channel_has_feature(s->ioc,
QIO_CHANNEL_FEATURE_FD_PASS)) {
return -1;
@@ -389,7 +410,7 @@ static void tcp_chr_free_connection(Chardev *chr)
s->ioc = NULL;
g_free(chr->filename);
chr->filename = NULL;
- s->connected = 0;
+ tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
}
static const char *qemu_chr_socket_protocol(SocketChardev *s)
@@ -442,12 +463,12 @@ static void update_disconnected_filename(SocketChardev *s)
/* NB may be called even if tcp_chr_connect has not been
* reached, due to TLS or telnet initialization failure,
- * so can *not* assume s->connected == true
+ * so can *not* assume s->state == TCP_CHARDEV_STATE_CONNECTED
*/
static void tcp_chr_disconnect(Chardev *chr)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
- bool emit_close = s->connected;
+ bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
tcp_chr_free_connection(chr);
@@ -471,7 +492,8 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
uint8_t buf[CHR_READ_BUF_LEN];
int len, size;
- if (!s->connected || s->max_size <= 0) {
+ if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||
+ s->max_size <= 0) {
return TRUE;
}
len = sizeof(buf);
@@ -508,7 +530,7 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
SocketChardev *s = SOCKET_CHARDEV(chr);
int size;
- if (!s->connected) {
+ if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
return 0;
}
@@ -564,7 +586,7 @@ static void update_ioc_handlers(SocketChardev *s)
{
Chardev *chr = CHARDEV(s);
- if (!s->connected) {
+ if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
return;
}
@@ -589,7 +611,7 @@ static void tcp_chr_connect(void *opaque)
g_free(chr->filename);
chr->filename = qemu_chr_compute_filename(s);
- s->connected = 1;
+ tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTED);
update_ioc_handlers(s);
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
}
@@ -828,7 +850,7 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
- if (s->ioc != NULL) {
+ if (s->state != TCP_CHARDEV_STATE_CONNECTING) {
return -1;
}
@@ -865,11 +887,17 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
{
int ret;
QIOChannelSocket *sioc;
+ SocketChardev *s = SOCKET_CHARDEV(chr);
+
+ if (s->state != TCP_CHARDEV_STATE_DISCONNECTED) {
+ return -1;
+ }
sioc = qio_channel_socket_new_fd(fd, NULL);
if (!sioc) {
return -1;
}
+ tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
tcp_chr_set_client_ioc_name(chr, sioc);
ret = tcp_chr_new_client(chr, sioc);
object_unref(OBJECT(sioc));
@@ -881,7 +909,9 @@ static void tcp_chr_accept(QIONetListener *listener,
void *opaque)
{
Chardev *chr = CHARDEV(opaque);
+ SocketChardev *s = SOCKET_CHARDEV(chr);
+ tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
tcp_chr_set_client_ioc_name(chr, cioc);
tcp_chr_new_client(chr, cioc);
}
@@ -891,8 +921,10 @@ static int tcp_chr_connect_client_sync(Chardev *chr, Error **errp)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
QIOChannelSocket *sioc = qio_channel_socket_new();
+ tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
tcp_chr_set_client_ioc_name(chr, sioc);
if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
+ tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
object_unref(OBJECT(sioc));
return -1;
}
@@ -908,6 +940,7 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
QIOChannelSocket *sioc;
info_report("QEMU waiting for connection on: %s",
chr->filename);
+ tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
sioc = qio_net_listener_wait_client(s->listener);
tcp_chr_set_client_ioc_name(chr, sioc);
tcp_chr_new_client(chr, sioc);
@@ -963,6 +996,7 @@ static void qemu_chr_socket_connected(QIOTask *task, void *opaque)
Error *err = NULL;
if (qio_task_propagate_error(task, &err)) {
+ tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
check_report_connect_error(chr, err);
error_free(err);
goto cleanup;
@@ -980,6 +1014,7 @@ static void tcp_chr_connect_client_async(Chardev *chr)
SocketChardev *s = SOCKET_CHARDEV(chr);
QIOChannelSocket *sioc;
+ tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
sioc = qio_channel_socket_new();
tcp_chr_set_client_ioc_name(chr, sioc);
qio_channel_socket_connect_async(sioc, s->addr,
@@ -1307,7 +1342,7 @@ char_socket_get_connected(Object *obj, Error **errp)
{
SocketChardev *s = SOCKET_CHARDEV(obj);
- return s->connected;
+ return s->state == TCP_CHARDEV_STATE_CONNECTED;
}
static void char_socket_class_init(ObjectClass *oc, void *data)
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 12/18] chardev: honour the reconnect setting in tcp_chr_wait_connected
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (10 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 11/18] chardev: use a state machine for socket connection state Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 13/18] chardev: disallow TLS/telnet/websocket with tcp_chr_wait_connected Marc-André Lureau
` (7 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
If establishing a client connection fails, the tcp_chr_wait_connected
method should sleep for the reconnect timeout and then retry the
attempt. This ensures the callers don't immediately abort with an
error when the initial connection fails.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-13-berrange@redhat.com>
---
chardev/char-socket.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index d6de5d2305..7db20ff0a0 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -957,8 +957,15 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
if (s->is_listen) {
tcp_chr_accept_server_sync(chr);
} else {
- if (tcp_chr_connect_client_sync(chr, errp) < 0) {
- return -1;
+ Error *err = NULL;
+ if (tcp_chr_connect_client_sync(chr, &err) < 0) {
+ if (s->reconnect_time) {
+ error_free(err);
+ g_usleep(s->reconnect_time * 1000ULL * 1000ULL);
+ } else {
+ error_propagate(errp, err);
+ return -1;
+ }
}
}
}
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 13/18] chardev: disallow TLS/telnet/websocket with tcp_chr_wait_connected
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (11 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 12/18] chardev: honour the reconnect setting in tcp_chr_wait_connected Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 14/18] chardev: fix race with client connections in tcp_chr_wait_connected Marc-André Lureau
` (6 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
In the previous commit
commit 1dc8a6695c731abb7461c637b2512c3670d82be4
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date: Tue Aug 16 12:33:32 2016 +0400
char: fix waiting for TLS and telnet connection
the tcp_chr_wait_connected() method was changed to check for a non-NULL
's->ioc' as a sign that there is already a connection present, as
opposed to checking the "connected" flag to supposedly fix handling of
TLS/telnet connections.
The original code would repeatedly call tcp_chr_wait_connected creating
many connections as 'connected' would never become true. The changed
code would still repeatedly call tcp_chr_wait_connected busy waiting
because s->ioc is set but the chardev will never see CHR_EVENT_OPENED.
IOW, the code is still broken with TLS/telnet, but in a different way.
Checking for a non-NULL 's->ioc' does not mean that a CHR_EVENT_OPENED
will be ready for a TLS/telnet connection. These protocols (and the
websocket protocol) all require the main loop to be running in order
to complete the protocol handshake before emitting CHR_EVENT_OPENED.
The tcp_chr_wait_connected() method is only used during early startup
before a main loop is running, so TLS/telnet/websock connections can
never complete initialization.
Making this work would require changing tcp_chr_wait_connected to run
a main loop. This is quite complex since we must not allow GSource's
that other parts of QEMU have registered to run yet. The current callers
of tcp_chr_wait_connected do not require use of the TLS/telnet/websocket
protocols, so the simplest option is to just forbid this combination
completely for now.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-14-berrange@redhat.com>
---
chardev/char-socket.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 7db20ff0a0..86c1f502d6 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -951,8 +951,20 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
- /* It can't wait on s->connected, since it is set asynchronously
- * in TLS and telnet cases, only wait for an accepted socket */
+ const char *opts[] = { "telnet", "tn3270", "websock", "tls-creds" };
+ bool optset[] = { s->is_telnet, s->is_tn3270, s->is_websock, s->tls_creds };
+ size_t i;
+
+ QEMU_BUILD_BUG_ON(G_N_ELEMENTS(opts) != G_N_ELEMENTS(optset));
+ for (i = 0; i < G_N_ELEMENTS(opts); i++) {
+ if (optset[i]) {
+ error_setg(errp,
+ "'%s' option is incompatible with waiting for "
+ "connection completion", opts[i]);
+ return -1;
+ }
+ }
+
while (!s->ioc) {
if (s->is_listen) {
tcp_chr_accept_server_sync(chr);
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 14/18] chardev: fix race with client connections in tcp_chr_wait_connected
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (12 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 13/18] chardev: disallow TLS/telnet/websocket with tcp_chr_wait_connected Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 15/18] tests: expand coverage of socket chardev test Marc-André Lureau
` (5 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
When the 'reconnect' option is given for a client connection, the
qmp_chardev_open_socket_client method will run an asynchronous
connection attempt. The QIOChannel socket executes this is a single use
background thread, so the connection will succeed immediately (assuming
the server is listening). The chardev, however, won't get the result
from this background thread until the main loop starts running and
processes idle callbacks.
Thus when tcp_chr_wait_connected is run s->ioc will be NULL, but the
state will be TCP_CHARDEV_STATE_CONNECTING, and there may already
be an established connection that will be associated with the chardev
by the pending idle callback. tcp_chr_wait_connected doesn't check the
state, only s->ioc, so attempts to establish another connection
synchronously.
If the server allows multiple connections this is unhelpful but not a
fatal problem as the duplicate connection will get ignored by the
tcp_chr_new_client method when it sees the state is already connected.
If the server only supports a single connection, however, the
tcp_chr_wait_connected method will hang forever because the server will
not accept its synchronous connection attempt until the first connection
is closed.
To deal with this tcp_chr_wait_connected needs to synchronize with the
completion of the background connection task. To do this it needs to
create the QIOTask directly and use the qio_task_wait_thread method.
This will cancel the pending idle callback and directly dispatch the
task completion callback, allowing the connection to be associated
with the chardev. If the background connection failed, it can still
attempt a new synchronous connection.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-15-berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
chardev/char-socket.c | 87 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 83 insertions(+), 4 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 86c1f502d6..7c3ee25945 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -80,6 +80,8 @@ typedef struct {
GSource *reconnect_timer;
int64_t reconnect_time;
bool connect_err_reported;
+
+ QIOTask *connect_task;
} SocketChardev;
#define SOCKET_CHARDEV(obj) \
@@ -965,7 +967,54 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
}
}
- while (!s->ioc) {
+ /*
+ * We expect states to be as follows:
+ *
+ * - server
+ * - wait -> CONNECTED
+ * - nowait -> DISCONNECTED
+ * - client
+ * - reconnect == 0 -> CONNECTED
+ * - reconnect != 0 -> CONNECTING
+ *
+ */
+ if (s->state == TCP_CHARDEV_STATE_CONNECTING) {
+ if (!s->connect_task) {
+ error_setg(errp,
+ "Unexpected 'connecting' state without connect task "
+ "while waiting for connection completion");
+ return -1;
+ }
+ /*
+ * tcp_chr_wait_connected should only ever be run from the
+ * main loop thread associated with chr->gcontext, otherwise
+ * qio_task_wait_thread has a dangerous race condition with
+ * free'ing of the s->connect_task object.
+ *
+ * Acquiring the main context doesn't 100% prove we're in
+ * the main loop thread, but it does at least guarantee
+ * that the main loop won't be executed by another thread
+ * avoiding the race condition with the task idle callback.
+ */
+ g_main_context_acquire(chr->gcontext);
+ qio_task_wait_thread(s->connect_task);
+ g_main_context_release(chr->gcontext);
+
+ /*
+ * The completion callback (qemu_chr_socket_connected) for
+ * s->connect_task should have set this to NULL by the time
+ * qio_task_wait_thread has returned.
+ */
+ assert(!s->connect_task);
+
+ /*
+ * NB we are *not* guaranteed to have "s->state == ..CONNECTED"
+ * at this point as this first connect may be failed, so
+ * allow the next loop to run regardless.
+ */
+ }
+
+ while (s->state != TCP_CHARDEV_STATE_CONNECTED) {
if (s->is_listen) {
tcp_chr_accept_server_sync(chr);
} else {
@@ -1014,6 +1063,8 @@ static void qemu_chr_socket_connected(QIOTask *task, void *opaque)
SocketChardev *s = SOCKET_CHARDEV(chr);
Error *err = NULL;
+ s->connect_task = NULL;
+
if (qio_task_propagate_error(task, &err)) {
tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
check_report_connect_error(chr, err);
@@ -1028,6 +1079,20 @@ cleanup:
object_unref(OBJECT(sioc));
}
+
+static void tcp_chr_connect_client_task(QIOTask *task,
+ gpointer opaque)
+{
+ QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
+ SocketAddress *addr = opaque;
+ Error *err = NULL;
+
+ qio_channel_socket_connect_sync(ioc, addr, &err);
+
+ qio_task_set_error(task, err);
+}
+
+
static void tcp_chr_connect_client_async(Chardev *chr)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -1036,9 +1101,23 @@ static void tcp_chr_connect_client_async(Chardev *chr)
tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
sioc = qio_channel_socket_new();
tcp_chr_set_client_ioc_name(chr, sioc);
- qio_channel_socket_connect_async(sioc, s->addr,
- qemu_chr_socket_connected,
- chr, NULL, chr->gcontext);
+ /*
+ * Normally code would use the qio_channel_socket_connect_async
+ * method which uses a QIOTask + qio_task_set_error internally
+ * to avoid blocking. The tcp_chr_wait_connected method, however,
+ * needs a way to synchronize with completion of the background
+ * connect task which can't be done with the QIOChannelSocket
+ * async APIs. Thus we must use QIOTask directly to implement
+ * the non-blocking concept locally.
+ */
+ s->connect_task = qio_task_new(OBJECT(sioc),
+ qemu_chr_socket_connected,
+ chr, NULL);
+ qio_task_run_in_thread(s->connect_task,
+ tcp_chr_connect_client_task,
+ s->addr,
+ NULL,
+ chr->gcontext);
}
static gboolean socket_reconnect_timeout(gpointer opaque)
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 15/18] tests: expand coverage of socket chardev test
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (13 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 14/18] chardev: fix race with client connections in tcp_chr_wait_connected Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 16/18] chardev: ensure termios is fully initialized Marc-André Lureau
` (4 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé
From: Daniel P. Berrangé <berrange@redhat.com>
The current socket chardev tests try to exercise the chardev socket
driver in both server and client mode at the same time. The chardev API
is not very well designed to handle both ends of the connection being in
the same process so this approach makes the test case quite unpleasant
to deal with.
This splits the tests into distinct cases, one to test server socket
chardevs and one to test client socket chardevs. In each case the peer
is run in a background thread using the simpler QIOChannelSocket APIs.
The main test case code can now be written in a way that mirrors the
typical usage from within QEMU.
In doing this refactoring it is possible to greatly expand the test
coverage for the socket chardevs to test all combinations except for a
server operating in blocking wait mode.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-16-berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
tests/test-char.c | 640 ++++++++++++++++++++++++++++++++++------------
1 file changed, 475 insertions(+), 165 deletions(-)
diff --git a/tests/test-char.c b/tests/test-char.c
index 89c43e4ada..8b478bb67d 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -11,6 +11,9 @@
#include "qapi/qapi-commands-char.h"
#include "qapi/qmp/qdict.h"
#include "qom/qom-qobject.h"
+#include "io/channel-socket.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qapi-visit-sockets.h"
static bool quit;
@@ -257,168 +260,6 @@ static void char_mux_test(void)
qemu_chr_fe_deinit(&chr_be2, true);
}
-typedef struct SocketIdleData {
- GMainLoop *loop;
- Chardev *chr;
- bool conn_expected;
- CharBackend *be;
- CharBackend *client_be;
-} SocketIdleData;
-
-static gboolean char_socket_test_idle(gpointer user_data)
-{
- SocketIdleData *data = user_data;
-
- if (object_property_get_bool(OBJECT(data->chr), "connected", NULL)
- == data->conn_expected) {
- quit = true;
- return FALSE;
- }
-
- return TRUE;
-}
-
-static void socket_read(void *opaque, const uint8_t *buf, int size)
-{
- SocketIdleData *data = opaque;
-
- g_assert_cmpint(size, ==, 1);
- g_assert_cmpint(*buf, ==, 'Z');
-
- size = qemu_chr_fe_write(data->be, (const uint8_t *)"hello", 5);
- g_assert_cmpint(size, ==, 5);
-}
-
-static int socket_can_read(void *opaque)
-{
- return 10;
-}
-
-static void socket_read_hello(void *opaque, const uint8_t *buf, int size)
-{
- g_assert_cmpint(size, ==, 5);
- g_assert(strncmp((char *)buf, "hello", 5) == 0);
-
- quit = true;
-}
-
-static int socket_can_read_hello(void *opaque)
-{
- return 10;
-}
-
-static void char_socket_test_common(Chardev *chr, bool reconnect)
-{
- Chardev *chr_client;
- QObject *addr;
- QDict *qdict;
- const char *port;
- SocketIdleData d = { .chr = chr };
- CharBackend be;
- CharBackend client_be;
- char *tmp;
-
- d.be = &be;
- d.client_be = &be;
-
- g_assert_nonnull(chr);
- g_assert(!object_property_get_bool(OBJECT(chr), "connected", &error_abort));
-
- addr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
- qdict = qobject_to(QDict, addr);
- port = qdict_get_str(qdict, "port");
- tmp = g_strdup_printf("tcp:127.0.0.1:%s%s", port,
- reconnect ? ",reconnect=1" : "");
- qobject_unref(qdict);
-
- qemu_chr_fe_init(&be, chr, &error_abort);
- qemu_chr_fe_set_handlers(&be, socket_can_read, socket_read,
- NULL, NULL, &d, NULL, true);
-
- chr_client = qemu_chr_new("client", tmp);
- qemu_chr_fe_init(&client_be, chr_client, &error_abort);
- qemu_chr_fe_set_handlers(&client_be, socket_can_read_hello,
- socket_read_hello,
- NULL, NULL, &d, NULL, true);
- g_free(tmp);
-
- d.conn_expected = true;
- guint id = g_idle_add(char_socket_test_idle, &d);
- g_source_set_name_by_id(id, "test-idle");
- g_assert_cmpint(id, >, 0);
- main_loop();
-
- d.chr = chr_client;
- id = g_idle_add(char_socket_test_idle, &d);
- g_source_set_name_by_id(id, "test-idle");
- g_assert_cmpint(id, >, 0);
- main_loop();
-
- g_assert(object_property_get_bool(OBJECT(chr), "connected", &error_abort));
- g_assert(object_property_get_bool(OBJECT(chr_client),
- "connected", &error_abort));
-
- qemu_chr_write_all(chr_client, (const uint8_t *)"Z", 1);
- main_loop();
-
- object_unparent(OBJECT(chr_client));
-
- d.chr = chr;
- d.conn_expected = false;
- g_idle_add(char_socket_test_idle, &d);
- main_loop();
-
- object_unparent(OBJECT(chr));
-}
-
-
-static void char_socket_basic_test(void)
-{
- Chardev *chr = qemu_chr_new("server", "tcp:127.0.0.1:0,server,nowait");
-
- char_socket_test_common(chr, false);
-}
-
-
-static void char_socket_reconnect_test(void)
-{
- Chardev *chr = qemu_chr_new("server", "tcp:127.0.0.1:0,server,nowait");
-
- char_socket_test_common(chr, true);
-}
-
-
-static void char_socket_fdpass_test(void)
-{
- Chardev *chr;
- char *optstr;
- QemuOpts *opts;
- int fd;
- SocketAddress *addr = g_new0(SocketAddress, 1);
-
- addr->type = SOCKET_ADDRESS_TYPE_INET;
- addr->u.inet.host = g_strdup("127.0.0.1");
- addr->u.inet.port = g_strdup("0");
-
- fd = socket_listen(addr, &error_abort);
- g_assert(fd >= 0);
-
- qapi_free_SocketAddress(addr);
-
- optstr = g_strdup_printf("socket,id=cdev,fd=%d,server,nowait", fd);
-
- opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"),
- optstr, true);
- g_free(optstr);
- g_assert_nonnull(opts);
-
- chr = qemu_chr_new_from_opts(opts, &error_abort);
-
- qemu_opts_del(opts);
-
- char_socket_test_common(chr, false);
-}
-
static void websock_server_read(void *opaque, const uint8_t *buf, int size)
{
@@ -610,6 +451,28 @@ static void char_pipe_test(void)
}
#endif
+typedef struct SocketIdleData {
+ GMainLoop *loop;
+ Chardev *chr;
+ bool conn_expected;
+ CharBackend *be;
+ CharBackend *client_be;
+} SocketIdleData;
+
+
+static void socket_read_hello(void *opaque, const uint8_t *buf, int size)
+{
+ g_assert_cmpint(size, ==, 5);
+ g_assert(strncmp((char *)buf, "hello", 5) == 0);
+
+ quit = true;
+}
+
+static int socket_can_read_hello(void *opaque)
+{
+ return 10;
+}
+
static int make_udp_socket(int *port)
{
struct sockaddr_in addr = { 0, };
@@ -680,6 +543,391 @@ static void char_udp_test(void)
char_udp_test_internal(NULL, 0);
}
+
+typedef struct {
+ int event;
+ bool got_pong;
+} CharSocketTestData;
+
+
+#define SOCKET_PING "Hello"
+#define SOCKET_PONG "World"
+
+
+static void
+char_socket_event(void *opaque, int event)
+{
+ CharSocketTestData *data = opaque;
+ data->event = event;
+}
+
+
+static void
+char_socket_read(void *opaque, const uint8_t *buf, int size)
+{
+ CharSocketTestData *data = opaque;
+ g_assert_cmpint(size, ==, sizeof(SOCKET_PONG));
+ g_assert(memcmp(buf, SOCKET_PONG, size) == 0);
+ data->got_pong = true;
+}
+
+
+static int
+char_socket_can_read(void *opaque)
+{
+ return sizeof(SOCKET_PONG);
+}
+
+
+static char *
+char_socket_addr_to_opt_str(SocketAddress *addr, bool fd_pass,
+ const char *reconnect, bool is_listen)
+{
+ if (fd_pass) {
+ QIOChannelSocket *ioc = qio_channel_socket_new();
+ int fd;
+ char *optstr;
+ g_assert(!reconnect);
+ if (is_listen) {
+ qio_channel_socket_listen_sync(ioc, addr, &error_abort);
+ } else {
+ qio_channel_socket_connect_sync(ioc, addr, &error_abort);
+ }
+ fd = ioc->fd;
+ ioc->fd = -1;
+ optstr = g_strdup_printf("socket,id=cdev0,fd=%d%s",
+ fd, is_listen ? ",server,nowait" : "");
+ object_unref(OBJECT(ioc));
+ return optstr;
+ } else {
+ switch (addr->type) {
+ case SOCKET_ADDRESS_TYPE_INET:
+ return g_strdup_printf("socket,id=cdev0,host=%s,port=%s%s%s",
+ addr->u.inet.host,
+ addr->u.inet.port,
+ reconnect ? reconnect : "",
+ is_listen ? ",server,nowait" : "");
+
+ case SOCKET_ADDRESS_TYPE_UNIX:
+ return g_strdup_printf("socket,id=cdev0,path=%s%s%s",
+ addr->u.q_unix.path,
+ reconnect ? reconnect : "",
+ is_listen ? ",server,nowait" : "");
+
+ default:
+ g_assert_not_reached();
+ }
+ }
+}
+
+
+static void
+char_socket_ping_pong(QIOChannel *ioc)
+{
+ char greeting[sizeof(SOCKET_PING)];
+ const char *response = SOCKET_PONG;
+
+ qio_channel_read_all(ioc, greeting, sizeof(greeting), &error_abort);
+
+ g_assert(memcmp(greeting, SOCKET_PING, sizeof(greeting)) == 0);
+
+ qio_channel_write_all(ioc, response, sizeof(SOCKET_PONG), &error_abort);
+
+ object_unref(OBJECT(ioc));
+}
+
+
+static gpointer
+char_socket_server_client_thread(gpointer data)
+{
+ SocketAddress *addr = data;
+ QIOChannelSocket *ioc = qio_channel_socket_new();
+
+ qio_channel_socket_connect_sync(ioc, addr, &error_abort);
+
+ char_socket_ping_pong(QIO_CHANNEL(ioc));
+
+ return NULL;
+}
+
+
+typedef struct {
+ SocketAddress *addr;
+ bool wait_connected;
+ bool fd_pass;
+} CharSocketServerTestConfig;
+
+
+static void char_socket_server_test(gconstpointer opaque)
+{
+ const CharSocketServerTestConfig *config = opaque;
+ Chardev *chr;
+ CharBackend be = {0};
+ CharSocketTestData data = {0};
+ QObject *qaddr;
+ SocketAddress *addr;
+ Visitor *v;
+ QemuThread thread;
+ int ret;
+ bool reconnected;
+ char *optstr;
+ QemuOpts *opts;
+
+ g_setenv("QTEST_SILENT_ERRORS", "1", 1);
+ /*
+ * We rely on config->addr containing "nowait", otherwise
+ * qemu_chr_new() will block until a client connects. We
+ * can't spawn our client thread though, because until
+ * qemu_chr_new() returns we don't know what TCP port was
+ * allocated by the OS
+ */
+ optstr = char_socket_addr_to_opt_str(config->addr,
+ config->fd_pass,
+ NULL,
+ true);
+ opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"),
+ optstr, true);
+ g_assert_nonnull(opts);
+ chr = qemu_chr_new_from_opts(opts, &error_abort);
+ qemu_opts_del(opts);
+ g_assert_nonnull(chr);
+ g_assert(!object_property_get_bool(OBJECT(chr), "connected", &error_abort));
+
+ qaddr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
+ g_assert_nonnull(qaddr);
+
+ v = qobject_input_visitor_new(qaddr);
+ visit_type_SocketAddress(v, "addr", &addr, &error_abort);
+ visit_free(v);
+ qobject_unref(qaddr);
+
+ qemu_chr_fe_init(&be, chr, &error_abort);
+
+ reconnect:
+ data.event = -1;
+ qemu_chr_fe_set_handlers(&be, NULL, NULL,
+ char_socket_event, NULL,
+ &data, NULL, true);
+ g_assert(data.event == -1);
+
+ /*
+ * Kick off a thread to act as the "remote" client
+ * which just plays ping-pong with us
+ */
+ qemu_thread_create(&thread, "client",
+ char_socket_server_client_thread,
+ addr, QEMU_THREAD_JOINABLE);
+ g_assert(data.event == -1);
+
+ if (config->wait_connected) {
+ /* Synchronously accept a connection */
+ qemu_chr_wait_connected(chr, &error_abort);
+ } else {
+ /*
+ * Asynchronously accept a connection when the evnt
+ * loop reports the listener socket as readable
+ */
+ while (data.event == -1) {
+ main_loop_wait(false);
+ }
+ }
+ g_assert(object_property_get_bool(OBJECT(chr), "connected", &error_abort));
+ g_assert(data.event == CHR_EVENT_OPENED);
+ data.event = -1;
+
+ /* Send a greeting to the client */
+ ret = qemu_chr_fe_write_all(&be, (const uint8_t *)SOCKET_PING,
+ sizeof(SOCKET_PING));
+ g_assert_cmpint(ret, ==, sizeof(SOCKET_PING));
+ g_assert(data.event == -1);
+
+ /* Setup a callback to receive the reply to our greeting */
+ qemu_chr_fe_set_handlers(&be, char_socket_can_read,
+ char_socket_read,
+ char_socket_event, NULL,
+ &data, NULL, true);
+ g_assert(data.event == CHR_EVENT_OPENED);
+ data.event = -1;
+
+ /* Wait for the client to go away */
+ while (data.event == -1) {
+ main_loop_wait(false);
+ }
+ g_assert(!object_property_get_bool(OBJECT(chr), "connected", &error_abort));
+ g_assert(data.event == CHR_EVENT_CLOSED);
+ g_assert(data.got_pong);
+
+ qemu_thread_join(&thread);
+
+ if (!reconnected) {
+ reconnected = true;
+ goto reconnect;
+ }
+
+ qapi_free_SocketAddress(addr);
+ object_unparent(OBJECT(chr));
+ g_free(optstr);
+ g_unsetenv("QTEST_SILENT_ERRORS");
+}
+
+
+static gpointer
+char_socket_client_server_thread(gpointer data)
+{
+ QIOChannelSocket *ioc = data;
+ QIOChannelSocket *cioc;
+
+ cioc = qio_channel_socket_accept(ioc, &error_abort);
+ g_assert_nonnull(cioc);
+
+ char_socket_ping_pong(QIO_CHANNEL(cioc));
+
+ return NULL;
+}
+
+
+typedef struct {
+ SocketAddress *addr;
+ const char *reconnect;
+ bool wait_connected;
+ bool fd_pass;
+} CharSocketClientTestConfig;
+
+
+static void char_socket_client_test(gconstpointer opaque)
+{
+ const CharSocketClientTestConfig *config = opaque;
+ QIOChannelSocket *ioc;
+ char *optstr;
+ Chardev *chr;
+ CharBackend be = {0};
+ CharSocketTestData data = {0};
+ SocketAddress *addr;
+ QemuThread thread;
+ int ret;
+ bool reconnected = false;
+ QemuOpts *opts;
+
+ /*
+ * Setup a listener socket and determine get its address
+ * so we know the TCP port for the client later
+ */
+ ioc = qio_channel_socket_new();
+ g_assert_nonnull(ioc);
+ qio_channel_socket_listen_sync(ioc, config->addr, &error_abort);
+ addr = qio_channel_socket_get_local_address(ioc, &error_abort);
+ g_assert_nonnull(addr);
+
+ /*
+ * Kick off a thread to act as the "remote" client
+ * which just plays ping-pong with us
+ */
+ qemu_thread_create(&thread, "client",
+ char_socket_client_server_thread,
+ ioc, QEMU_THREAD_JOINABLE);
+
+ /*
+ * Populate the chardev address based on what the server
+ * is actually listening on
+ */
+ optstr = char_socket_addr_to_opt_str(addr,
+ config->fd_pass,
+ config->reconnect,
+ false);
+
+ opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"),
+ optstr, true);
+ g_assert_nonnull(opts);
+ chr = qemu_chr_new_from_opts(opts, &error_abort);
+ qemu_opts_del(opts);
+ g_assert_nonnull(chr);
+
+ if (config->reconnect) {
+ /*
+ * If reconnect is set, the connection will be
+ * established in a background thread and we won't
+ * see the "connected" status updated until we
+ * run the main event loop, or call qemu_chr_wait_connected
+ */
+ g_assert(!object_property_get_bool(OBJECT(chr), "connected",
+ &error_abort));
+ } else {
+ g_assert(object_property_get_bool(OBJECT(chr), "connected",
+ &error_abort));
+ }
+
+ qemu_chr_fe_init(&be, chr, &error_abort);
+
+ reconnect:
+ data.event = -1;
+ qemu_chr_fe_set_handlers(&be, NULL, NULL,
+ char_socket_event, NULL,
+ &data, NULL, true);
+ if (config->reconnect) {
+ g_assert(data.event == -1);
+ } else {
+ g_assert(data.event == CHR_EVENT_OPENED);
+ }
+
+ if (config->wait_connected) {
+ /*
+ * Synchronously wait for the connection to complete
+ * This should be a no-op if reconnect is not set.
+ */
+ qemu_chr_wait_connected(chr, &error_abort);
+ } else {
+ /*
+ * Asynchronously wait for the connection to be reported
+ * as complete when the background thread reports its
+ * status.
+ * The loop will short-circuit if reconnect was set
+ */
+ while (data.event == -1) {
+ main_loop_wait(false);
+ }
+ }
+ g_assert(data.event == CHR_EVENT_OPENED);
+ data.event = -1;
+ g_assert(object_property_get_bool(OBJECT(chr), "connected", &error_abort));
+
+ /* Send a greeting to the server */
+ ret = qemu_chr_fe_write_all(&be, (const uint8_t *)SOCKET_PING,
+ sizeof(SOCKET_PING));
+ g_assert_cmpint(ret, ==, sizeof(SOCKET_PING));
+ g_assert(data.event == -1);
+
+ /* Setup a callback to receive the reply to our greeting */
+ qemu_chr_fe_set_handlers(&be, char_socket_can_read,
+ char_socket_read,
+ char_socket_event, NULL,
+ &data, NULL, true);
+ g_assert(data.event == CHR_EVENT_OPENED);
+ data.event = -1;
+
+ /* Wait for the server to go away */
+ while (data.event == -1) {
+ main_loop_wait(false);
+ }
+ g_assert(data.event == CHR_EVENT_CLOSED);
+ g_assert(!object_property_get_bool(OBJECT(chr), "connected", &error_abort));
+ g_assert(data.got_pong);
+ qemu_thread_join(&thread);
+
+ if (config->reconnect && !reconnected) {
+ reconnected = true;
+ qemu_thread_create(&thread, "client",
+ char_socket_client_server_thread,
+ ioc, QEMU_THREAD_JOINABLE);
+ goto reconnect;
+ }
+
+ object_unref(OBJECT(ioc));
+ object_unparent(OBJECT(chr));
+ qapi_free_SocketAddress(addr);
+ g_free(optstr);
+}
+
+
#ifdef HAVE_CHARDEV_SERIAL
static void char_serial_test(void)
{
@@ -959,9 +1207,71 @@ int main(int argc, char **argv)
#ifndef _WIN32
g_test_add_func("/char/file-fifo", char_file_fifo_test);
#endif
- g_test_add_func("/char/socket/basic", char_socket_basic_test);
- g_test_add_func("/char/socket/reconnect", char_socket_reconnect_test);
- g_test_add_func("/char/socket/fdpass", char_socket_fdpass_test);
+
+ SocketAddress tcpaddr = {
+ .type = SOCKET_ADDRESS_TYPE_INET,
+ .u.inet.host = (char *)"127.0.0.1",
+ .u.inet.port = (char *)"0",
+ };
+#ifndef WIN32
+ SocketAddress unixaddr = {
+ .type = SOCKET_ADDRESS_TYPE_UNIX,
+ .u.q_unix.path = (char *)"test-char.sock",
+ };
+#endif
+
+#define SOCKET_SERVER_TEST(name, addr) \
+ CharSocketServerTestConfig server1 ## name = \
+ { addr, false, false }; \
+ CharSocketServerTestConfig server2 ## name = \
+ { addr, true, false }; \
+ CharSocketServerTestConfig server3 ## name = \
+ { addr, false, true }; \
+ CharSocketServerTestConfig server4 ## name = \
+ { addr, true, true }; \
+ g_test_add_data_func("/char/socket/server/mainloop/" # name, \
+ &server1 ##name, char_socket_server_test); \
+ g_test_add_data_func("/char/socket/server/wait-conn/" # name, \
+ &server2 ##name, char_socket_server_test); \
+ g_test_add_data_func("/char/socket/server/mainloop-fdpass/" # name, \
+ &server3 ##name, char_socket_server_test); \
+ g_test_add_data_func("/char/socket/server/wait-conn-fdpass/" # name, \
+ &server4 ##name, char_socket_server_test)
+
+#define SOCKET_CLIENT_TEST(name, addr) \
+ CharSocketClientTestConfig client1 ## name = \
+ { addr, NULL, false, false }; \
+ CharSocketClientTestConfig client2 ## name = \
+ { addr, NULL, true, false }; \
+ CharSocketClientTestConfig client3 ## name = \
+ { addr, ",reconnect=1", false }; \
+ CharSocketClientTestConfig client4 ## name = \
+ { addr, ",reconnect=1", true }; \
+ CharSocketClientTestConfig client5 ## name = \
+ { addr, NULL, false, true }; \
+ CharSocketClientTestConfig client6 ## name = \
+ { addr, NULL, true, true }; \
+ g_test_add_data_func("/char/socket/client/mainloop/" # name, \
+ &client1 ##name, char_socket_client_test); \
+ g_test_add_data_func("/char/socket/client/wait-conn/" # name, \
+ &client2 ##name, char_socket_client_test); \
+ g_test_add_data_func("/char/socket/client/mainloop-reconnect/" # name, \
+ &client3 ##name, char_socket_client_test); \
+ g_test_add_data_func("/char/socket/client/wait-conn-reconnect/" # name, \
+ &client4 ##name, char_socket_client_test); \
+ g_test_add_data_func("/char/socket/client/mainloop-fdpass/" # name, \
+ &client5 ##name, char_socket_client_test); \
+ g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
+ &client6 ##name, char_socket_client_test)
+
+ SOCKET_SERVER_TEST(tcp, &tcpaddr);
+ SOCKET_CLIENT_TEST(tcp, &tcpaddr);
+#ifndef WIN32
+ SOCKET_SERVER_TEST(unix, &unixaddr);
+ SOCKET_CLIENT_TEST(unix, &unixaddr);
+#endif
+
+
g_test_add_func("/char/udp", char_udp_test);
#ifdef HAVE_CHARDEV_SERIAL
g_test_add_func("/char/serial", char_serial_test);
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 16/18] chardev: ensure termios is fully initialized
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (14 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 15/18] tests: expand coverage of socket chardev test Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 17/18] chardev: fix mess in OPENED/CLOSED events when muxed Marc-André Lureau
` (3 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Daniel P. Berrangé,
Paolo Bonzini
From: Daniel P. Berrangé <berrange@redhat.com>
valgrind on the test-char.c code reports that 'struct termios' contains
uninitialized memory.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190123172740.32452-17-berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
chardev/char-serial.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/chardev/char-serial.c b/chardev/char-serial.c
index 3299b46853..a8bae31b8d 100644
--- a/chardev/char-serial.c
+++ b/chardev/char-serial.c
@@ -57,7 +57,7 @@ static void qmp_chardev_open_serial(Chardev *chr,
static void tty_serial_init(int fd, int speed,
int parity, int data_bits, int stop_bits)
{
- struct termios tty;
+ struct termios tty = {0};
speed_t spd;
#if 0
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 17/18] chardev: fix mess in OPENED/CLOSED events when muxed
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (15 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 16/18] chardev: ensure termios is fully initialized Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-07 16:06 ` [Qemu-devel] [PULL 18/18] tests/test-char: add muxed chardev testing for open/close Marc-André Lureau
` (2 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, peter.maydell, Artem Pisarenko, Paolo Bonzini
From: Artem Pisarenko <artem.k.pisarenko@gmail.com>
When chardev is multiplexed (mux=on) there are a lot of cases where
CHR_EVENT_OPENED/CHR_EVENT_CLOSED events pairing (expected from
frontend side) is broken. There are either generation of multiple
repeated or extra CHR_EVENT_OPENED events, or CHR_EVENT_CLOSED just
isn't generated at all.
This is mostly because 'qemu_chr_fe_set_handlers()' function makes its
own (and often wrong) implicit decision on updated frontend state and
invokes 'fd_event' callback with 'CHR_EVENT_OPENED'. And even worse,
it doesn't do symmetric action in opposite direction, as someone may
expect (i.e. it doesn't invoke previously set 'fd_event' with
'CHR_EVENT_CLOSED'). Muxed chardev uses trick by calling this function
again to replace callback handlers with its own ones, but it doesn't
account for such side effect.
Fix that using extended version of this function with added argument
for disabling side effect and keep original function for compatibility
with lots of frontends already using this interface and being
"tolerant" to its side effects.
One more source of event duplication is just line of code in
char-mux.c, which does far more than comment above says (obvious fix).
Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <7dde6abbd21682857f8294644013173c0b9949b3.1541507990.git.artem.k.pisarenko@gmail.com>
---
include/chardev/char-fe.h | 18 +++++++++++++++++-
chardev/char-fe.c | 33 ++++++++++++++++++++++++---------
chardev/char-mux.c | 16 ++++++++--------
3 files changed, 49 insertions(+), 18 deletions(-)
diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index 46c997d352..c1b7fd9a95 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -67,7 +67,7 @@ bool qemu_chr_fe_backend_connected(CharBackend *be);
bool qemu_chr_fe_backend_open(CharBackend *be);
/**
- * qemu_chr_fe_set_handlers:
+ * qemu_chr_fe_set_handlers_full:
* @b: a CharBackend
* @fd_can_read: callback to get the amount of data the frontend may
* receive
@@ -79,12 +79,28 @@ bool qemu_chr_fe_backend_open(CharBackend *be);
* @context: a main loop context or NULL for the default
* @set_open: whether to call qemu_chr_fe_set_open() implicitely when
* any of the handler is non-NULL
+ * @sync_state: whether to issue event callback with updated state
*
* Set the front end char handlers. The front end takes the focus if
* any of the handler is non-NULL.
*
* Without associated Chardev, nothing is changed.
*/
+void qemu_chr_fe_set_handlers_full(CharBackend *b,
+ IOCanReadHandler *fd_can_read,
+ IOReadHandler *fd_read,
+ IOEventHandler *fd_event,
+ BackendChangeHandler *be_change,
+ void *opaque,
+ GMainContext *context,
+ bool set_open,
+ bool sync_state);
+
+/**
+ * qemu_chr_fe_set_handlers:
+ *
+ * Version of qemu_chr_fe_set_handlers_full() with sync_state = true.
+ */
void qemu_chr_fe_set_handlers(CharBackend *b,
IOCanReadHandler *fd_can_read,
IOReadHandler *fd_read,
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index a8931f7afd..b7bcbd59c0 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -246,14 +246,15 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
}
}
-void qemu_chr_fe_set_handlers(CharBackend *b,
- IOCanReadHandler *fd_can_read,
- IOReadHandler *fd_read,
- IOEventHandler *fd_event,
- BackendChangeHandler *be_change,
- void *opaque,
- GMainContext *context,
- bool set_open)
+void qemu_chr_fe_set_handlers_full(CharBackend *b,
+ IOCanReadHandler *fd_can_read,
+ IOReadHandler *fd_read,
+ IOEventHandler *fd_event,
+ BackendChangeHandler *be_change,
+ void *opaque,
+ GMainContext *context,
+ bool set_open,
+ bool sync_state)
{
Chardev *s;
int fe_open;
@@ -285,7 +286,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
qemu_chr_fe_take_focus(b);
/* We're connecting to an already opened device, so let's make sure we
also get the open event */
- if (s->be_open) {
+ if (sync_state && s->be_open) {
qemu_chr_be_event(s, CHR_EVENT_OPENED);
}
}
@@ -295,6 +296,20 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
}
}
+void qemu_chr_fe_set_handlers(CharBackend *b,
+ IOCanReadHandler *fd_can_read,
+ IOReadHandler *fd_read,
+ IOEventHandler *fd_event,
+ BackendChangeHandler *be_change,
+ void *opaque,
+ GMainContext *context,
+ bool set_open)
+{
+ qemu_chr_fe_set_handlers_full(b, fd_can_read, fd_read, fd_event, be_change,
+ opaque, context, set_open,
+ true);
+}
+
void qemu_chr_fe_take_focus(CharBackend *b)
{
if (!b->chr) {
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 6055e76293..1199d32674 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -283,13 +283,13 @@ void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
MuxChardev *d = MUX_CHARDEV(chr);
/* Fix up the real driver with mux routines */
- qemu_chr_fe_set_handlers(&d->chr,
- mux_chr_can_read,
- mux_chr_read,
- mux_chr_event,
- NULL,
- chr,
- context, true);
+ qemu_chr_fe_set_handlers_full(&d->chr,
+ mux_chr_can_read,
+ mux_chr_read,
+ mux_chr_event,
+ NULL,
+ chr,
+ context, true, false);
}
void mux_set_focus(Chardev *chr, int focus)
@@ -367,7 +367,7 @@ static int open_muxes(Chardev *chr)
* mark mux as OPENED so any new FEs will immediately receive
* OPENED event
*/
- qemu_chr_be_event(chr, CHR_EVENT_OPENED);
+ chr->be_open = 1;
return 0;
}
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PULL 18/18] tests/test-char: add muxed chardev testing for open/close
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (16 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 17/18] chardev: fix mess in OPENED/CLOSED events when muxed Marc-André Lureau
@ 2019-02-07 16:06 ` Marc-André Lureau
2019-02-08 11:44 ` [Qemu-devel] [PULL 00/18] Chardev patches Peter Maydell
2019-02-11 16:50 ` Daniel P. Berrangé
19 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2019-02-07 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, peter.maydell, Artem Pisarenko
From: Artem Pisarenko <artem.k.pisarenko@gmail.com>
Validate that frontend callbacks for CHR_EVENT_OPENED/CHR_EVENT_CLOSED
events are being issued when expected and in strictly pairing order.
Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <ac67ff2d27dd51a0075d5d634355c9e4f7bb53de.1541507990.git.artem.k.pisarenko@gmail.com>
---
tests/test-char.c | 80 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 78 insertions(+), 2 deletions(-)
diff --git a/tests/test-char.c b/tests/test-char.c
index 8b478bb67d..82579e6aa5 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -19,6 +19,9 @@ static bool quit;
typedef struct FeHandler {
int read_count;
+ bool is_open;
+ int openclose_count;
+ bool openclose_mismatch;
int last_event;
char read_buf[128];
} FeHandler;
@@ -52,10 +55,24 @@ static void fe_read(void *opaque, const uint8_t *buf, int size)
static void fe_event(void *opaque, int event)
{
FeHandler *h = opaque;
+ bool new_open_state;
h->last_event = event;
- if (event != CHR_EVENT_BREAK) {
+ switch (event) {
+ case CHR_EVENT_BREAK:
+ break;
+ case CHR_EVENT_OPENED:
+ case CHR_EVENT_CLOSED:
+ h->openclose_count++;
+ new_open_state = (event == CHR_EVENT_OPENED);
+ if (h->is_open == new_open_state) {
+ h->openclose_mismatch = true;
+ }
+ h->is_open = new_open_state;
+ /* no break */
+ default:
quit = true;
+ break;
}
}
@@ -164,7 +181,7 @@ static void char_mux_test(void)
QemuOpts *opts;
Chardev *chr, *base;
char *data;
- FeHandler h1 = { 0, }, h2 = { 0, };
+ FeHandler h1 = { 0, false, 0, false, }, h2 = { 0, false, 0, false, };
CharBackend chr_be1, chr_be2;
opts = qemu_opts_create(qemu_find_opts("chardev"), "mux-label",
@@ -236,6 +253,65 @@ static void char_mux_test(void)
g_assert_cmpint(h1.last_event, ==, CHR_EVENT_BREAK);
g_assert_cmpint(h2.last_event, ==, CHR_EVENT_MUX_OUT);
+ /* open/close state and corresponding events */
+ g_assert_true(qemu_chr_fe_backend_open(&chr_be1));
+ g_assert_true(qemu_chr_fe_backend_open(&chr_be2));
+ g_assert_true(h1.is_open);
+ g_assert_false(h1.openclose_mismatch);
+ g_assert_true(h2.is_open);
+ g_assert_false(h2.openclose_mismatch);
+
+ h1.openclose_count = h2.openclose_count = 0;
+
+ qemu_chr_fe_set_handlers(&chr_be1, NULL, NULL, NULL, NULL,
+ NULL, NULL, false);
+ qemu_chr_fe_set_handlers(&chr_be2, NULL, NULL, NULL, NULL,
+ NULL, NULL, false);
+ g_assert_cmpint(h1.openclose_count, ==, 0);
+ g_assert_cmpint(h2.openclose_count, ==, 0);
+
+ h1.is_open = h2.is_open = false;
+ qemu_chr_fe_set_handlers(&chr_be1,
+ NULL,
+ NULL,
+ fe_event,
+ NULL,
+ &h1,
+ NULL, false);
+ qemu_chr_fe_set_handlers(&chr_be2,
+ NULL,
+ NULL,
+ fe_event,
+ NULL,
+ &h2,
+ NULL, false);
+ g_assert_cmpint(h1.openclose_count, ==, 1);
+ g_assert_false(h1.openclose_mismatch);
+ g_assert_cmpint(h2.openclose_count, ==, 1);
+ g_assert_false(h2.openclose_mismatch);
+
+ qemu_chr_be_event(base, CHR_EVENT_CLOSED);
+ qemu_chr_be_event(base, CHR_EVENT_OPENED);
+ g_assert_cmpint(h1.openclose_count, ==, 3);
+ g_assert_false(h1.openclose_mismatch);
+ g_assert_cmpint(h2.openclose_count, ==, 3);
+ g_assert_false(h2.openclose_mismatch);
+
+ qemu_chr_fe_set_handlers(&chr_be2,
+ fe_can_read,
+ fe_read,
+ fe_event,
+ NULL,
+ &h2,
+ NULL, false);
+ qemu_chr_fe_set_handlers(&chr_be1,
+ fe_can_read,
+ fe_read,
+ fe_event,
+ NULL,
+ &h1,
+ NULL, false);
+
/* remove first handler */
qemu_chr_fe_set_handlers(&chr_be1, NULL, NULL, NULL, NULL,
NULL, NULL, true);
--
2.20.1.519.g8feddda32c
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PULL 00/18] Chardev patches
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (17 preceding siblings ...)
2019-02-07 16:06 ` [Qemu-devel] [PULL 18/18] tests/test-char: add muxed chardev testing for open/close Marc-André Lureau
@ 2019-02-08 11:44 ` Peter Maydell
2019-02-11 17:03 ` Daniel P. Berrangé
2019-02-11 16:50 ` Daniel P. Berrangé
19 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2019-02-08 11:44 UTC (permalink / raw)
To: Marc-André Lureau, Daniel P. Berrange, Artem Pisarenko
Cc: QEMU Developers
On Thu, 7 Feb 2019 at 16:06, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> The following changes since commit 632351e0e1a861f2eaf709b053c53f96a1225825:
>
> Merge remote-tracking branch 'remotes/elmarco/tags/dump-pull-request' into staging (2019-02-07 14:20:46 +0000)
>
> are available in the Git repository at:
>
> https://github.com/elmarco/qemu.git tags/chardev-pull-request
>
> for you to fetch changes up to df3afdedd23ade0c9de55cadeb1d85055689023f:
>
> tests/test-char: add muxed chardev testing for open/close (2019-02-07 16:18:25 +0100)
>
> ----------------------------------------------------------------
> Various chardev fixes
>
> ----------------------------------------------------------------
This seems to result in 'make check' failures on some platforms.
I saw this on s390 and aarch32, I think.
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
tests/test-char -m=quick -k --tap < /dev/null |
./scripts/tap-driver.pl --test-name="test-char
"
PASS 1 test-char /char/null
PASS 2 test-char /char/invalid
PASS 3 test-char /char/ringbuf
PASS 4 test-char /char/mux
PASS 5 test-char /char/stdio
PASS 6 test-char /char/pipe
PASS 7 test-char /char/file
PASS 8 test-char /char/file-fifo
PASS 9 test-char /char/udp
PASS 10 test-char /char/serial
PASS 11 test-char /char/hotswap
PASS 12 test-char /char/websocket
PASS 13 test-char /char/socket/server/mainloop/tcp
PASS 14 test-char /char/socket/server/mainloop/unix
PASS 15 test-char /char/socket/server/wait-conn/tcp
PASS 16 test-char /char/socket/server/wait-conn/unix
PASS 17 test-char /char/socket/server/mainloop-fdpass/tcp
PASS 18 test-char /char/socket/server/mainloop-fdpass/unix
PASS 19 test-char /char/socket/server/wait-conn-fdpass/tcp
PASS 20 test-char /char/socket/server/wait-conn-fdpass/unix
PASS 21 test-char /char/socket/client/mainloop/tcp
PASS 22 test-char /char/socket/client/mainloop/unix
qemu: qemu_mutex_destroy: Device or resource busy
PASS 23 test-char /char/socket/client/wait-conn/tcp
PASS 24 test-char /char/socket/client/wait-conn/unix
Aborted (core dumped)
ERROR - too few tests run (expected 32, got 24)
Here's a backtrace from running tests/test-char under gdb.
Looks like a race condition between a thread trying to
destroy a mutex and a different thread that is still
using it.
qemu: qemu_mutex_destroy: Device or resource busy
test-char: /home/linux1/qemu/util/qemu-thread-posix.c:92:
qemu_mutex_unlock_impl: Assertion `mutex->initialized' failed.
(gdb) thread apply all bt
Thread 17 (Thread 0x3fff77ff910 (LWP 35364)):
#0 0x000003fffd7381b8 in __GI_raise (sig=sig@entry=6) at
../sysdeps/unix/sysv/linux/raise.c:54
#1 0x000003fffd739726 in __GI_abort () at abort.c:89
#2 0x000003fffd7300d6 in __assert_fail_base (fmt=0x3fffd84d18c
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x1000918d2
"mutex->initialized",
file=0x1000918a6 "/home/linux1/qemu/util/qemu-thread-posix.c",
line=<optimized out>,
function=0x100091bb2 <__PRETTY_FUNCTION__.18115>
"qemu_mutex_unlock_impl") at assert.c:92
#3 0x000003fffd730164 in __GI___assert_fail (assertion=0x1000918d2
"mutex->initialized", file=0x1000918a6
"/home/linux1/qemu/util/qemu-thread-posix.c",
line=<optimized out>, function=0x100091bb2
<__PRETTY_FUNCTION__.18115> "qemu_mutex_unlock_impl") at assert.c:101
#4 0x000000010005db16 in qemu_mutex_unlock_impl (mutex=<optimized
out>, file=<optimized out>, line=<optimized out>)
at /home/linux1/qemu/util/qemu-thread-posix.c:92
#5 0x00000001000257cc in qio_task_thread_worker
(opaque=opaque@entry=0x1000f1370) at /home/linux1/qemu/io/task.c:141
#6 0x000000010005d640 in qemu_thread_start (args=<optimized out>) at
/home/linux1/qemu/util/qemu-thread-posix.c:502
#7 0x000003fffd907934 in start_thread (arg=0x3fff77ff910) at
pthread_create.c:335
#8 0x000003fffd7edce2 in thread_start () at
../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:74
Thread 16 (Thread 0x3fff7fff910 (LWP 35363)):
#0 0x000003fffd911774 in __libc_recvmsg (fd=<optimized out>,
msg=0x3fff7ffe7d0, flags=<optimized out>) at
../sysdeps/unix/sysv/linux/recvmsg.c:33
#1 0x000000010001fab6 in qio_channel_socket_readv (ioc=<optimized
out>, iov=<optimized out>, niov=<optimized out>, fds=0x0, nfds=0x0,
errp=0x1000c0320 <error_abort>) at /home/linux1/qemu/io/channel-socket.c:484
#2 0x000000010001ca04 in qio_channel_readv_full (ioc=0x3fff00008c0,
iov=0x3fff00012b0, niov=1, fds=0x0, nfds=0x0, errp=0x1000c0320
<error_abort>)
at /home/linux1/qemu/io/channel.c:65
#3 0x000000010001d478 in qio_channel_readv (errp=0x1000c0320
<error_abort>, niov=<optimized out>, iov=<optimized out>,
ioc=0x3fff00008c0)
at /home/linux1/qemu/io/channel.c:197
#4 qio_channel_readv_all_eof (ioc=0x3fff00008c0, iov=<optimized out>,
niov=<optimized out>, errp=errp@entry=0x1000c0320 <error_abort>)
at /home/linux1/qemu/io/channel.c:106
#5 0x000000010001d576 in qio_channel_readv_all (ioc=<optimized out>,
iov=<optimized out>, niov=<optimized out>, errp=0x1000c0320
<error_abort>)
at /home/linux1/qemu/io/channel.c:142
#6 0x000000010001d602 in qio_channel_read_all (ioc=<optimized out>,
buf=<optimized out>, buflen=<optimized out>, errp=<optimized out>)
at /home/linux1/qemu/io/channel.c:246
#7 0x000000010001c3e0 in char_socket_ping_pong (ioc=0x3fff00008c0) at
/home/linux1/qemu/tests/test-char.c:706
#8 0x000000010001c4a8 in char_socket_client_server_thread
(data=data@entry=0x1000f2730) at
/home/linux1/qemu/tests/test-char.c:859
#9 0x000000010005d640 in qemu_thread_start (args=<optimized out>) at
/home/linux1/qemu/util/qemu-thread-posix.c:502
#10 0x000003fffd907934 in start_thread (arg=0x3fff7fff910) at
pthread_create.c:335
#11 0x000003fffd7edce2 in thread_start () at
../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:74
Thread 3 (Thread 0x3fffc9ff910 (LWP 35350)):
#0 0x000003fffd7e3e54 in ?? () at
../sysdeps/unix/syscall-template.S:84 from
/lib/s390x-linux-gnu/libc.so.6
#1 0x000003fffddd06ee in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0
#2 0x000003fffddd087c in g_main_context_iteration () from
/lib/s390x-linux-gnu/libglib-2.0.so.0
#3 0x000003fffddd08cc in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0
#4 0x000003fffddfaba4 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0
#5 0x000003fffd907934 in start_thread (arg=0x3fffc9ff910) at
pthread_create.c:335
#6 0x000003fffd7edce2 in thread_start () at
../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:74
Thread 2 (Thread 0x3fffd1ff910 (LWP 35346)):
#0 syscall () at ../sysdeps/unix/sysv/linux/s390/s390-64/syscall.S:58
#1 0x000000010005e3ca in qemu_futex_wait (val=<optimized out>,
f=<optimized out>) at /home/linux1/qemu/include/qemu/futex.h:29
#2 qemu_event_wait (ev=0x1000c17f0 <rcu_call_ready_event>) at
/home/linux1/qemu/util/qemu-thread-posix.c:442
#3 0x000000010007d524 in call_rcu_thread (opaque=opaque@entry=0x0) at
/home/linux1/qemu/util/rcu.c:261
#4 0x000000010005d640 in qemu_thread_start (args=<optimized out>) at
/home/linux1/qemu/util/qemu-thread-posix.c:502
#5 0x000003fffd907934 in start_thread (arg=0x3fffd1ff910) at
pthread_create.c:335
#6 0x000003fffd7edce2 in thread_start () at
../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:74
Thread 1 (Thread 0x3fffdff3920 (LWP 35343)):
#0 0x000003fffd7381b8 in __GI_raise (sig=sig@entry=6) at
../sysdeps/unix/sysv/linux/raise.c:54
#1 0x000003fffd739726 in __GI_abort () at abort.c:89
#2 0x0000000100017444 in error_exit (err=<optimized out>,
msg=msg@entry=0x100091c26 <__func__.18092> "qemu_mutex_destroy")
at /home/linux1/qemu/util/qemu-thread-posix.c:36
#3 0x000000010005d772 in qemu_mutex_destroy (mutex=<optimized out>)
at /home/linux1/qemu/util/qemu-thread-posix.c:57
#4 0x0000000100025c36 in qio_task_free (task=0x1000f1370) at
/home/linux1/qemu/io/task.c:97
#5 qio_task_complete (task=task@entry=0x1000f1370) at
/home/linux1/qemu/io/task.c:196
#6 0x0000000100025d0e in qio_task_thread_result (opaque=0x1000f1370)
at /home/linux1/qemu/io/task.c:110
#7 0x000003fffddd03ce in g_main_context_dispatch () from
/lib/s390x-linux-gnu/libglib-2.0.so.0
#8 0x000000010005a16a in glib_pollfds_poll () at
/home/linux1/qemu/util/main-loop.c:215
#9 os_host_main_loop_wait (timeout=<optimized out>) at
/home/linux1/qemu/util/main-loop.c:238
#10 main_loop_wait (nonblocking=<optimized out>) at
/home/linux1/qemu/util/main-loop.c:514
#11 0x00000001000190c2 in char_socket_client_test (opaque=<optimized
out>) at /home/linux1/qemu/tests/test-char.c:962
#12 0x000003fffddf9756 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0
#13 0x000003fffddf9934 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0
#14 0x000003fffddf9934 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0
#15 0x000003fffddf9934 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0
#16 0x000003fffddf9934 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0
#17 0x000003fffddf9b5e in g_test_run_suite () from
/lib/s390x-linux-gnu/libglib-2.0.so.0
#18 0x000003fffddf9b80 in g_test_run () from
/lib/s390x-linux-gnu/libglib-2.0.so.0
#19 0x0000000100017a28 in main (argc=1, argv=0x3fffffff578) at
/home/linux1/qemu/tests/test-char.c:1358
On some other hosts I saw a similar
"qemu: qemu_mutex_destroy: Device or resource busy" and core dump in the
migration tests, I think, which is probably the same underlying bug.
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PULL 00/18] Chardev patches
2019-02-08 11:44 ` [Qemu-devel] [PULL 00/18] Chardev patches Peter Maydell
@ 2019-02-11 17:03 ` Daniel P. Berrangé
2019-02-11 18:29 ` Daniel P. Berrangé
0 siblings, 1 reply; 24+ messages in thread
From: Daniel P. Berrangé @ 2019-02-11 17:03 UTC (permalink / raw)
To: Peter Maydell; +Cc: Marc-André Lureau, Artem Pisarenko, QEMU Developers
On Fri, Feb 08, 2019 at 11:44:42AM +0000, Peter Maydell wrote:
> On Thu, 7 Feb 2019 at 16:06, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > The following changes since commit 632351e0e1a861f2eaf709b053c53f96a1225825:
> >
> > Merge remote-tracking branch 'remotes/elmarco/tags/dump-pull-request' into staging (2019-02-07 14:20:46 +0000)
> >
> > are available in the Git repository at:
> >
> > https://github.com/elmarco/qemu.git tags/chardev-pull-request
> >
> > for you to fetch changes up to df3afdedd23ade0c9de55cadeb1d85055689023f:
> >
> > tests/test-char: add muxed chardev testing for open/close (2019-02-07 16:18:25 +0100)
> >
> > ----------------------------------------------------------------
> > Various chardev fixes
> >
> > ----------------------------------------------------------------
>
> This seems to result in 'make check' failures on some platforms.
> I saw this on s390 and aarch32, I think.
>
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> tests/test-char -m=quick -k --tap < /dev/null |
> ./scripts/tap-driver.pl --test-name="test-char
> "
> PASS 1 test-char /char/null
> PASS 2 test-char /char/invalid
> PASS 3 test-char /char/ringbuf
> PASS 4 test-char /char/mux
> PASS 5 test-char /char/stdio
> PASS 6 test-char /char/pipe
> PASS 7 test-char /char/file
> PASS 8 test-char /char/file-fifo
> PASS 9 test-char /char/udp
> PASS 10 test-char /char/serial
> PASS 11 test-char /char/hotswap
> PASS 12 test-char /char/websocket
> PASS 13 test-char /char/socket/server/mainloop/tcp
> PASS 14 test-char /char/socket/server/mainloop/unix
> PASS 15 test-char /char/socket/server/wait-conn/tcp
> PASS 16 test-char /char/socket/server/wait-conn/unix
> PASS 17 test-char /char/socket/server/mainloop-fdpass/tcp
> PASS 18 test-char /char/socket/server/mainloop-fdpass/unix
> PASS 19 test-char /char/socket/server/wait-conn-fdpass/tcp
> PASS 20 test-char /char/socket/server/wait-conn-fdpass/unix
> PASS 21 test-char /char/socket/client/mainloop/tcp
> PASS 22 test-char /char/socket/client/mainloop/unix
> qemu: qemu_mutex_destroy: Device or resource busy
> PASS 23 test-char /char/socket/client/wait-conn/tcp
> PASS 24 test-char /char/socket/client/wait-conn/unix
> Aborted (core dumped)
> ERROR - too few tests run (expected 32, got 24)
>
> Here's a backtrace from running tests/test-char under gdb.
> Looks like a race condition between a thread trying to
> destroy a mutex and a different thread that is still
> using it.
Thanks, that is very useful. I can see the race condition here
now between qio_task_thread_worker and qio_task_thread_result.
I need to acquire the mutex in qio_task_thread_result in order
to sycnhronize with completion of qio_task_thread_worker.
>
> On some other hosts I saw a similar
> "qemu: qemu_mutex_destroy: Device or resource busy" and core dump in the
> migration tests, I think, which is probably the same underlying bug.
Yes, I expect it is the same problem
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] 24+ messages in thread* Re: [Qemu-devel] [PULL 00/18] Chardev patches
2019-02-11 17:03 ` Daniel P. Berrangé
@ 2019-02-11 18:29 ` Daniel P. Berrangé
0 siblings, 0 replies; 24+ messages in thread
From: Daniel P. Berrangé @ 2019-02-11 18:29 UTC (permalink / raw)
To: Peter Maydell; +Cc: Marc-André Lureau, Artem Pisarenko, QEMU Developers
On Mon, Feb 11, 2019 at 05:03:13PM +0000, Daniel P. Berrangé wrote:
> On Fri, Feb 08, 2019 at 11:44:42AM +0000, Peter Maydell wrote:
> > On Thu, 7 Feb 2019 at 16:06, Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> > >
> > > The following changes since commit 632351e0e1a861f2eaf709b053c53f96a1225825:
> > >
> > > Merge remote-tracking branch 'remotes/elmarco/tags/dump-pull-request' into staging (2019-02-07 14:20:46 +0000)
> > >
> > > are available in the Git repository at:
> > >
> > > https://github.com/elmarco/qemu.git tags/chardev-pull-request
> > >
> > > for you to fetch changes up to df3afdedd23ade0c9de55cadeb1d85055689023f:
> > >
> > > tests/test-char: add muxed chardev testing for open/close (2019-02-07 16:18:25 +0100)
> > >
> > > ----------------------------------------------------------------
> > > Various chardev fixes
> > >
> > > ----------------------------------------------------------------
> >
> > This seems to result in 'make check' failures on some platforms.
> > I saw this on s390 and aarch32, I think.
> >
> > MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> > tests/test-char -m=quick -k --tap < /dev/null |
> > ./scripts/tap-driver.pl --test-name="test-char
> > "
> > PASS 1 test-char /char/null
> > PASS 2 test-char /char/invalid
> > PASS 3 test-char /char/ringbuf
> > PASS 4 test-char /char/mux
> > PASS 5 test-char /char/stdio
> > PASS 6 test-char /char/pipe
> > PASS 7 test-char /char/file
> > PASS 8 test-char /char/file-fifo
> > PASS 9 test-char /char/udp
> > PASS 10 test-char /char/serial
> > PASS 11 test-char /char/hotswap
> > PASS 12 test-char /char/websocket
> > PASS 13 test-char /char/socket/server/mainloop/tcp
> > PASS 14 test-char /char/socket/server/mainloop/unix
> > PASS 15 test-char /char/socket/server/wait-conn/tcp
> > PASS 16 test-char /char/socket/server/wait-conn/unix
> > PASS 17 test-char /char/socket/server/mainloop-fdpass/tcp
> > PASS 18 test-char /char/socket/server/mainloop-fdpass/unix
> > PASS 19 test-char /char/socket/server/wait-conn-fdpass/tcp
> > PASS 20 test-char /char/socket/server/wait-conn-fdpass/unix
> > PASS 21 test-char /char/socket/client/mainloop/tcp
> > PASS 22 test-char /char/socket/client/mainloop/unix
> > qemu: qemu_mutex_destroy: Device or resource busy
> > PASS 23 test-char /char/socket/client/wait-conn/tcp
> > PASS 24 test-char /char/socket/client/wait-conn/unix
> > Aborted (core dumped)
> > ERROR - too few tests run (expected 32, got 24)
> >
> > Here's a backtrace from running tests/test-char under gdb.
> > Looks like a race condition between a thread trying to
> > destroy a mutex and a different thread that is still
> > using it.
>
> Thanks, that is very useful. I can see the race condition here
> now between qio_task_thread_worker and qio_task_thread_result.
> I need to acquire the mutex in qio_task_thread_result in order
> to sycnhronize with completion of qio_task_thread_worker.
In testing this first bug, I found a second bug hiding where
tcp_chr_wait_connected forget to de-register the pending
reconnect timer GSource, leading to a later crash. We would
not have seen this in the test suite except for me adding
a sleep(1) in the right place by chance :-)
I've sent a v3 series for Marc-André to queue for a new PULL
> > On some other hosts I saw a similar
> > "qemu: qemu_mutex_destroy: Device or resource busy" and core dump in the
> > migration tests, I think, which is probably the same underlying bug.
>
> Yes, I expect it is the same problem
I'm confident this is the first problem i mention now.
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] 24+ messages in thread
* Re: [Qemu-devel] [PULL 00/18] Chardev patches
2019-02-07 16:05 [Qemu-devel] [PULL 00/18] Chardev patches Marc-André Lureau
` (18 preceding siblings ...)
2019-02-08 11:44 ` [Qemu-devel] [PULL 00/18] Chardev patches Peter Maydell
@ 2019-02-11 16:50 ` Daniel P. Berrangé
2019-02-11 16:54 ` Peter Maydell
19 siblings, 1 reply; 24+ messages in thread
From: Daniel P. Berrangé @ 2019-02-11 16:50 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel, peter.maydell
On Thu, Feb 07, 2019 at 05:05:59PM +0100, Marc-André Lureau wrote:
> The following changes since commit 632351e0e1a861f2eaf709b053c53f96a1225825:
>
> Merge remote-tracking branch 'remotes/elmarco/tags/dump-pull-request' into staging (2019-02-07 14:20:46 +0000)
>
> are available in the Git repository at:
>
> https://github.com/elmarco/qemu.git tags/chardev-pull-request
>
> for you to fetch changes up to df3afdedd23ade0c9de55cadeb1d85055689023f:
>
> tests/test-char: add muxed chardev testing for open/close (2019-02-07 16:18:25 +0100)
>
> ----------------------------------------------------------------
> Various chardev fixes
>
> ----------------------------------------------------------------
BTW, I think as the maintainer sending the PULL request you are expected
to have added your own S-o-B to every patch, rather than just a R-B.
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] 24+ messages in thread* Re: [Qemu-devel] [PULL 00/18] Chardev patches
2019-02-11 16:50 ` Daniel P. Berrangé
@ 2019-02-11 16:54 ` Peter Maydell
0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2019-02-11 16:54 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: Marc-André Lureau, QEMU Developers
On Mon, 11 Feb 2019 at 16:50, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Feb 07, 2019 at 05:05:59PM +0100, Marc-André Lureau wrote:
> > The following changes since commit 632351e0e1a861f2eaf709b053c53f96a1225825:
> >
> > Merge remote-tracking branch 'remotes/elmarco/tags/dump-pull-request' into staging (2019-02-07 14:20:46 +0000)
> >
> > are available in the Git repository at:
> >
> > https://github.com/elmarco/qemu.git tags/chardev-pull-request
> >
> > for you to fetch changes up to df3afdedd23ade0c9de55cadeb1d85055689023f:
> >
> > tests/test-char: add muxed chardev testing for open/close (2019-02-07 16:18:25 +0100)
> >
> > ----------------------------------------------------------------
> > Various chardev fixes
> >
> > ----------------------------------------------------------------
>
> BTW, I think as the maintainer sending the PULL request you are expected
> to have added your own S-o-B to every patch, rather than just a R-B.
Yes, indeed. Thanks for catching that.
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread