* [Qemu-devel] [PATCH 0/3] char-socket: Fix race condition
@ 2019-02-07 13:23 Alberto Garcia
2019-02-07 13:23 ` [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext Alberto Garcia
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Alberto Garcia @ 2019-02-07 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: Alberto Garcia, Paolo Bonzini, Daniel P . Berrangé,
Marc-André Lureau
Hi,
this series fixes the crash I reported yesterday in char-socket and
adds a new qemu_idle_add() function, fixing a couple of memory leaks
along the way.
Regards,
Berto
Alberto Garcia (3):
main-loop: Don't leak GSources attached to a GMainContext
main-loop: Add qemu_idle_add()
char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
chardev/char-pty.c | 8 ++------
chardev/char-socket.c | 19 +++++++++++++++++--
include/qemu/main-loop.h | 8 ++++++++
io/task.c | 6 ++----
util/main-loop.c | 9 +++++++++
5 files changed, 38 insertions(+), 12 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext
2019-02-07 13:23 [Qemu-devel] [PATCH 0/3] char-socket: Fix race condition Alberto Garcia
@ 2019-02-07 13:23 ` Alberto Garcia
2019-02-07 13:25 ` Alberto Garcia
` (2 more replies)
2019-02-07 13:23 ` [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add() Alberto Garcia
2019-02-07 13:23 ` [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout() Alberto Garcia
2 siblings, 3 replies; 21+ messages in thread
From: Alberto Garcia @ 2019-02-07 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: Alberto Garcia, Paolo Bonzini, Daniel P . Berrangé,
Marc-André Lureau
After g_source_attach() the GMainContext holds a reference to the
GSource, so the caller does not need to keep it.
pty_chr_state() and qio_task_thread_worker() are not doing this, so
the GSource is being leaked in both cases (pty_chr_open_src_cancel()
is the exception here because it does remove the extra reference
correctly).
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
chardev/char-pty.c | 2 +-
io/task.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index f681d637c1..f16a5e8d59 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -60,7 +60,6 @@ static void pty_chr_open_src_cancel(PtyChardev *s)
{
if (s->open_source) {
g_source_destroy(s->open_source);
- g_source_unref(s->open_source);
s->open_source = NULL;
}
}
@@ -216,6 +215,7 @@ static void pty_chr_state(Chardev *chr, int connected)
qemu_chr_be_generic_open_func,
chr, NULL);
g_source_attach(s->open_source, chr->gcontext);
+ g_source_unref(s->open_source);
}
if (!chr->gsource) {
chr->gsource = io_add_watch_poll(chr, s->ioc,
diff --git a/io/task.c b/io/task.c
index 2886a2c1bc..c8489fb790 100644
--- a/io/task.c
+++ b/io/task.c
@@ -120,6 +120,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
idle = g_idle_source_new();
g_source_set_callback(idle, qio_task_thread_result, data, NULL);
g_source_attach(idle, data->context);
+ g_source_unref(idle);
return NULL;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add()
2019-02-07 13:23 [Qemu-devel] [PATCH 0/3] char-socket: Fix race condition Alberto Garcia
2019-02-07 13:23 ` [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext Alberto Garcia
@ 2019-02-07 13:23 ` Alberto Garcia
2019-02-07 14:24 ` Daniel P. Berrangé
2019-02-07 14:28 ` Marc-André Lureau
2019-02-07 13:23 ` [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout() Alberto Garcia
2 siblings, 2 replies; 21+ messages in thread
From: Alberto Garcia @ 2019-02-07 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: Alberto Garcia, Paolo Bonzini, Daniel P . Berrangé,
Marc-André Lureau
This works like g_idle_add() but allows specifying a different
GMainContext. It also returns the GSource directly instead of its ID
number for convenience.
pty_chr_state() and qio_task_thread_worker() are modified to make use
of this new function.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
chardev/char-pty.c | 8 ++------
include/qemu/main-loop.h | 8 ++++++++
io/task.c | 7 ++-----
util/main-loop.c | 9 +++++++++
4 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index f16a5e8d59..6562c38f39 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -209,13 +209,9 @@ static void pty_chr_state(Chardev *chr, int connected)
pty_chr_timer_cancel(s);
if (!s->connected) {
g_assert(s->open_source == NULL);
- s->open_source = g_idle_source_new();
+ s->open_source = qemu_idle_add(qemu_chr_be_generic_open_func,
+ chr, chr->gcontext);
s->connected = 1;
- g_source_set_callback(s->open_source,
- qemu_chr_be_generic_open_func,
- chr, NULL);
- g_source_attach(s->open_source, chr->gcontext);
- g_source_unref(s->open_source);
}
if (!chr->gsource) {
chr->gsource = io_add_watch_poll(chr, s->ioc,
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index e59f9ae1e9..1865afd993 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -295,6 +295,14 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line);
*/
void qemu_mutex_unlock_iothread(void);
+/**
+ * qemu_idle_add: Add an idle function to a GMainContext
+ *
+ * This function is similar to GLib's g_idle_add() but it allows
+ * specifying a GMainContext instead of using the global one.
+ */
+GSource *qemu_idle_add(GSourceFunc func, gpointer data, GMainContext *ctx);
+
/* internal interfaces */
void qemu_fd_register(int fd);
diff --git a/io/task.c b/io/task.c
index c8489fb790..bdd395b0ad 100644
--- a/io/task.c
+++ b/io/task.c
@@ -19,6 +19,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
#include "io/task.h"
#include "qapi/error.h"
#include "qemu/thread.h"
@@ -105,7 +106,6 @@ static gboolean qio_task_thread_result(gpointer opaque)
static gpointer qio_task_thread_worker(gpointer opaque)
{
struct QIOTaskThreadData *data = opaque;
- GSource *idle;
trace_qio_task_thread_run(data->task);
data->worker(data->task, data->opaque);
@@ -117,10 +117,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
*/
trace_qio_task_thread_exit(data->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_unref(idle);
+ qemu_idle_add(qio_task_thread_result, data, data->context);
return NULL;
}
diff --git a/util/main-loop.c b/util/main-loop.c
index 443cb4cfe8..1c68e3bf5f 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -171,6 +171,15 @@ int qemu_init_main_loop(Error **errp)
return 0;
}
+GSource *qemu_idle_add(GSourceFunc func, gpointer data, GMainContext *ctx)
+{
+ GSource *idle = g_idle_source_new();
+ g_source_set_callback(idle, func, data, NULL);
+ g_source_attach(idle, ctx);
+ g_source_unref(idle);
+ return idle;
+}
+
static int max_priority;
#ifndef _WIN32
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
2019-02-07 13:23 [Qemu-devel] [PATCH 0/3] char-socket: Fix race condition Alberto Garcia
2019-02-07 13:23 ` [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext Alberto Garcia
2019-02-07 13:23 ` [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add() Alberto Garcia
@ 2019-02-07 13:23 ` Alberto Garcia
2019-02-07 14:26 ` Daniel P. Berrangé
2019-02-07 14:46 ` Marc-André Lureau
2 siblings, 2 replies; 21+ messages in thread
From: Alberto Garcia @ 2019-02-07 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: Alberto Garcia, Paolo Bonzini, Daniel P . Berrangé,
Marc-André Lureau
There's a race condition in which the tcp_chr_read() ioc handler can
close a connection that is being written to from another thread.
Running iotest 136 in a loop triggers this problem and crashes QEMU.
(gdb) bt
#0 0x00005558b842902d in object_get_class (obj=0x0) at qom/object.c:860
#1 0x00005558b84f92db in qio_channel_writev_full (ioc=0x0, iov=0x7ffc355decf0, niov=1, fds=0x0, nfds=0, errp=0x0) at io/channel.c:76
#2 0x00005558b84e0e9e in io_channel_send_full (ioc=0x0, buf=0x5558baf5beb0, len=138, fds=0x0, nfds=0) at chardev/char-io.c:123
#3 0x00005558b84e4a69 in tcp_chr_write (chr=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138) at chardev/char-socket.c:135
#4 0x00005558b84dca55 in qemu_chr_write_buffer (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, offset=0x7ffc355dedd0, write_all=false) at chardev/char.c:112
#5 0x00005558b84dcbc2 in qemu_chr_write (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, write_all=false) at chardev/char.c:147
#6 0x00005558b84dfb26 in qemu_chr_fe_write (be=0x5558ba476610, buf=0x5558baf5beb0 "...", len=138) at chardev/char-fe.c:42
#7 0x00005558b8088c86 in monitor_flush_locked (mon=0x5558ba476610) at monitor.c:406
#8 0x00005558b8088e8c in monitor_puts (mon=0x5558ba476610, str=0x5558ba921e49 "") at monitor.c:449
#9 0x00005558b8089178 in qmp_send_response (mon=0x5558ba476610, rsp=0x5558bb161600) at monitor.c:498
#10 0x00005558b808920c in monitor_qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:526
#11 0x00005558b8089307 in monitor_qapi_event_queue_no_reenter (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:551
#12 0x00005558b80896c0 in qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:626
#13 0x00005558b855f23b in qapi_event_send_shutdown (guest=false, reason=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at qapi/qapi-events-run-state.c:43
#14 0x00005558b81911ef in qemu_system_shutdown (cause=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at vl.c:1837
#15 0x00005558b8191308 in main_loop_should_exit () at vl.c:1885
#16 0x00005558b819140d in main_loop () at vl.c:1924
#17 0x00005558b8198c84 in main (argc=18, argv=0x7ffc355df3f8, envp=0x7ffc355df490) at vl.c:4665
This patch adds a lock to protect tcp_chr_disconnect() and
socket_reconnect_timeout()
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
chardev/char-socket.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index eaa8e8b68f..5f421bdc58 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -148,7 +148,9 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
if (ret < 0 && errno != EAGAIN) {
if (tcp_chr_read_poll(chr) <= 0) {
+ qemu_mutex_unlock(&chr->chr_write_lock);
tcp_chr_disconnect(chr);
+ qemu_mutex_lock(&chr->chr_write_lock);
return len;
} /* else let the read handler finish it properly */
}
@@ -440,6 +442,13 @@ static void update_disconnected_filename(SocketChardev *s)
}
}
+static gboolean tcp_chr_be_event_closed(gpointer opaque)
+{
+ Chardev *chr = opaque;
+ qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+ return FALSE;
+}
+
/* 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
@@ -447,7 +456,10 @@ static void update_disconnected_filename(SocketChardev *s)
static void tcp_chr_disconnect(Chardev *chr)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
- bool emit_close = s->connected;
+ bool emit_close;
+
+ qemu_mutex_lock(&chr->chr_write_lock);
+ emit_close = s->connected;
tcp_chr_free_connection(chr);
@@ -457,11 +469,12 @@ static void tcp_chr_disconnect(Chardev *chr)
}
update_disconnected_filename(s);
if (emit_close) {
- qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+ qemu_idle_add(tcp_chr_be_event_closed, chr, chr->gcontext);
}
if (s->reconnect_time) {
qemu_chr_socket_restart_timer(chr);
}
+ qemu_mutex_unlock(&chr->chr_write_lock);
}
static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
@@ -975,8 +988,10 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
Chardev *chr = CHARDEV(opaque);
SocketChardev *s = SOCKET_CHARDEV(opaque);
+ qemu_mutex_lock(&chr->chr_write_lock);
g_source_unref(s->reconnect_timer);
s->reconnect_timer = NULL;
+ qemu_mutex_unlock(&chr->chr_write_lock);
if (chr->be_open) {
return false;
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext
2019-02-07 13:23 ` [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext Alberto Garcia
@ 2019-02-07 13:25 ` Alberto Garcia
2019-02-07 14:21 ` Daniel P. Berrangé
2019-02-07 14:23 ` Marc-André Lureau
2 siblings, 0 replies; 21+ messages in thread
From: Alberto Garcia @ 2019-02-07 13:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Daniel P. Berrangé, Marc-André Lureau
On Thu 07 Feb 2019 02:23:21 PM CET, Alberto Garcia wrote:
> After g_source_attach() the GMainContext holds a reference to the
> GSource, so the caller does not need to keep it.
>
> pty_chr_state() and qio_task_thread_worker() are not doing this, so
s/are not doing this/are not releasing their references/
I'll correct this if I need to send a new version of the series,
otherwise please fix when committing this patch.
Berto
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext
2019-02-07 13:23 ` [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext Alberto Garcia
2019-02-07 13:25 ` Alberto Garcia
@ 2019-02-07 14:21 ` Daniel P. Berrangé
2019-02-07 14:23 ` Marc-André Lureau
2 siblings, 0 replies; 21+ messages in thread
From: Daniel P. Berrangé @ 2019-02-07 14:21 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, Paolo Bonzini, Marc-André Lureau
On Thu, Feb 07, 2019 at 03:23:21PM +0200, Alberto Garcia wrote:
> After g_source_attach() the GMainContext holds a reference to the
> GSource, so the caller does not need to keep it.
>
> pty_chr_state() and qio_task_thread_worker() are not doing this, so
> the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> is the exception here because it does remove the extra reference
> correctly).
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> chardev/char-pty.c | 2 +-
> io/task.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext
2019-02-07 13:23 ` [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext Alberto Garcia
2019-02-07 13:25 ` Alberto Garcia
2019-02-07 14:21 ` Daniel P. Berrangé
@ 2019-02-07 14:23 ` Marc-André Lureau
2019-02-07 14:24 ` Marc-André Lureau
2 siblings, 1 reply; 21+ messages in thread
From: Marc-André Lureau @ 2019-02-07 14:23 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, Paolo Bonzini, Daniel P . Berrangé
On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
>
> After g_source_attach() the GMainContext holds a reference to the
> GSource, so the caller does not need to keep it.
>
> pty_chr_state() and qio_task_thread_worker() are not doing this, so
> the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> is the exception here because it does remove the extra reference
> correctly).
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
You could mention this is a fix for regression from
a17536c594bfed94d05667b419f747b692f5fc7f
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> chardev/char-pty.c | 2 +-
> io/task.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index f681d637c1..f16a5e8d59 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -60,7 +60,6 @@ static void pty_chr_open_src_cancel(PtyChardev *s)
> {
> if (s->open_source) {
> g_source_destroy(s->open_source);
> - g_source_unref(s->open_source);
> s->open_source = NULL;
> }
> }
> @@ -216,6 +215,7 @@ static void pty_chr_state(Chardev *chr, int connected)
> qemu_chr_be_generic_open_func,
> chr, NULL);
> g_source_attach(s->open_source, chr->gcontext);
> + g_source_unref(s->open_source);
> }
> if (!chr->gsource) {
> chr->gsource = io_add_watch_poll(chr, s->ioc,
> diff --git a/io/task.c b/io/task.c
> index 2886a2c1bc..c8489fb790 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -120,6 +120,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
> idle = g_idle_source_new();
> g_source_set_callback(idle, qio_task_thread_result, data, NULL);
> g_source_attach(idle, data->context);
> + g_source_unref(idle);
>
> return NULL;
> }
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add()
2019-02-07 13:23 ` [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add() Alberto Garcia
@ 2019-02-07 14:24 ` Daniel P. Berrangé
2019-02-07 14:28 ` Marc-André Lureau
1 sibling, 0 replies; 21+ messages in thread
From: Daniel P. Berrangé @ 2019-02-07 14:24 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, Paolo Bonzini, Marc-André Lureau
On Thu, Feb 07, 2019 at 03:23:22PM +0200, Alberto Garcia wrote:
> This works like g_idle_add() but allows specifying a different
> GMainContext. It also returns the GSource directly instead of its ID
> number for convenience.
>
> pty_chr_state() and qio_task_thread_worker() are modified to make use
> of this new function.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> chardev/char-pty.c | 8 ++------
> include/qemu/main-loop.h | 8 ++++++++
> io/task.c | 7 ++-----
> util/main-loop.c | 9 +++++++++
> 4 files changed, 21 insertions(+), 11 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext
2019-02-07 14:23 ` Marc-André Lureau
@ 2019-02-07 14:24 ` Marc-André Lureau
2019-02-07 14:27 ` Alberto Garcia
0 siblings, 1 reply; 21+ messages in thread
From: Marc-André Lureau @ 2019-02-07 14:24 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, Paolo Bonzini, Daniel P . Berrangé
On Thu, Feb 7, 2019 at 3:23 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
> >
> > After g_source_attach() the GMainContext holds a reference to the
> > GSource, so the caller does not need to keep it.
> >
> > pty_chr_state() and qio_task_thread_worker() are not doing this, so
> > the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> > is the exception here because it does remove the extra reference
> > correctly).
> >
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> You could mention this is a fix for regression from
> a17536c594bfed94d05667b419f747b692f5fc7f
>
and 938eb9e9c83d34d90cac6ec5c5388e7998840e4e
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
> > ---
> > chardev/char-pty.c | 2 +-
> > io/task.c | 1 +
> > 2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > index f681d637c1..f16a5e8d59 100644
> > --- a/chardev/char-pty.c
> > +++ b/chardev/char-pty.c
> > @@ -60,7 +60,6 @@ static void pty_chr_open_src_cancel(PtyChardev *s)
> > {
> > if (s->open_source) {
> > g_source_destroy(s->open_source);
> > - g_source_unref(s->open_source);
> > s->open_source = NULL;
> > }
> > }
> > @@ -216,6 +215,7 @@ static void pty_chr_state(Chardev *chr, int connected)
> > qemu_chr_be_generic_open_func,
> > chr, NULL);
> > g_source_attach(s->open_source, chr->gcontext);
> > + g_source_unref(s->open_source);
> > }
> > if (!chr->gsource) {
> > chr->gsource = io_add_watch_poll(chr, s->ioc,
> > diff --git a/io/task.c b/io/task.c
> > index 2886a2c1bc..c8489fb790 100644
> > --- a/io/task.c
> > +++ b/io/task.c
> > @@ -120,6 +120,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
> > idle = g_idle_source_new();
> > g_source_set_callback(idle, qio_task_thread_result, data, NULL);
> > g_source_attach(idle, data->context);
> > + g_source_unref(idle);
> >
> > return NULL;
> > }
> > --
> > 2.11.0
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
2019-02-07 13:23 ` [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout() Alberto Garcia
@ 2019-02-07 14:26 ` Daniel P. Berrangé
2019-02-08 8:50 ` Alberto Garcia
2019-02-08 9:34 ` Alberto Garcia
2019-02-07 14:46 ` Marc-André Lureau
1 sibling, 2 replies; 21+ messages in thread
From: Daniel P. Berrangé @ 2019-02-07 14:26 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, Paolo Bonzini, Marc-André Lureau
On Thu, Feb 07, 2019 at 03:23:23PM +0200, Alberto Garcia wrote:
> There's a race condition in which the tcp_chr_read() ioc handler can
> close a connection that is being written to from another thread.
>
> Running iotest 136 in a loop triggers this problem and crashes QEMU.
>
> (gdb) bt
> #0 0x00005558b842902d in object_get_class (obj=0x0) at qom/object.c:860
> #1 0x00005558b84f92db in qio_channel_writev_full (ioc=0x0, iov=0x7ffc355decf0, niov=1, fds=0x0, nfds=0, errp=0x0) at io/channel.c:76
> #2 0x00005558b84e0e9e in io_channel_send_full (ioc=0x0, buf=0x5558baf5beb0, len=138, fds=0x0, nfds=0) at chardev/char-io.c:123
> #3 0x00005558b84e4a69 in tcp_chr_write (chr=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138) at chardev/char-socket.c:135
> #4 0x00005558b84dca55 in qemu_chr_write_buffer (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, offset=0x7ffc355dedd0, write_all=false) at chardev/char.c:112
> #5 0x00005558b84dcbc2 in qemu_chr_write (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, write_all=false) at chardev/char.c:147
> #6 0x00005558b84dfb26 in qemu_chr_fe_write (be=0x5558ba476610, buf=0x5558baf5beb0 "...", len=138) at chardev/char-fe.c:42
> #7 0x00005558b8088c86 in monitor_flush_locked (mon=0x5558ba476610) at monitor.c:406
> #8 0x00005558b8088e8c in monitor_puts (mon=0x5558ba476610, str=0x5558ba921e49 "") at monitor.c:449
> #9 0x00005558b8089178 in qmp_send_response (mon=0x5558ba476610, rsp=0x5558bb161600) at monitor.c:498
> #10 0x00005558b808920c in monitor_qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:526
> #11 0x00005558b8089307 in monitor_qapi_event_queue_no_reenter (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:551
> #12 0x00005558b80896c0 in qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:626
> #13 0x00005558b855f23b in qapi_event_send_shutdown (guest=false, reason=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at qapi/qapi-events-run-state.c:43
> #14 0x00005558b81911ef in qemu_system_shutdown (cause=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at vl.c:1837
> #15 0x00005558b8191308 in main_loop_should_exit () at vl.c:1885
> #16 0x00005558b819140d in main_loop () at vl.c:1924
> #17 0x00005558b8198c84 in main (argc=18, argv=0x7ffc355df3f8, envp=0x7ffc355df490) at vl.c:4665
>
> This patch adds a lock to protect tcp_chr_disconnect() and
> socket_reconnect_timeout()
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> chardev/char-socket.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index eaa8e8b68f..5f421bdc58 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -148,7 +148,9 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>
> if (ret < 0 && errno != EAGAIN) {
> if (tcp_chr_read_poll(chr) <= 0) {
> + qemu_mutex_unlock(&chr->chr_write_lock);
> tcp_chr_disconnect(chr);
> + qemu_mutex_lock(&chr->chr_write_lock);
> return len;
> } /* else let the read handler finish it properly */
> }
> @@ -440,6 +442,13 @@ static void update_disconnected_filename(SocketChardev *s)
> }
> }
>
> +static gboolean tcp_chr_be_event_closed(gpointer opaque)
> +{
> + Chardev *chr = opaque;
> + qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> + return FALSE;
> +}
> +
> /* 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
> @@ -447,7 +456,10 @@ static void update_disconnected_filename(SocketChardev *s)
> static void tcp_chr_disconnect(Chardev *chr)
> {
> SocketChardev *s = SOCKET_CHARDEV(chr);
> - bool emit_close = s->connected;
> + bool emit_close;
> +
> + qemu_mutex_lock(&chr->chr_write_lock);
> + emit_close = s->connected;
>
> tcp_chr_free_connection(chr);
>
> @@ -457,11 +469,12 @@ static void tcp_chr_disconnect(Chardev *chr)
> }
> update_disconnected_filename(s);
> if (emit_close) {
> - qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> + qemu_idle_add(tcp_chr_be_event_closed, chr, chr->gcontext);
I'm a little concerned that this change might trigger some unexpected
interactions with the qemu_chr_wait_connected() function. I added some
more testing around that code, so if it passes the new tests I have in:
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05962.html
and also works with the vhostuser reconnect device, then we are probably
ok.
Assuming it passes the test above:
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> }
> if (s->reconnect_time) {
> qemu_chr_socket_restart_timer(chr);
> }
> + qemu_mutex_unlock(&chr->chr_write_lock);
> }
>
> static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
> @@ -975,8 +988,10 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
> Chardev *chr = CHARDEV(opaque);
> SocketChardev *s = SOCKET_CHARDEV(opaque);
>
> + qemu_mutex_lock(&chr->chr_write_lock);
> g_source_unref(s->reconnect_timer);
> s->reconnect_timer = NULL;
> + qemu_mutex_unlock(&chr->chr_write_lock);
>
> if (chr->be_open) {
> return false;
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] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext
2019-02-07 14:24 ` Marc-André Lureau
@ 2019-02-07 14:27 ` Alberto Garcia
2019-02-07 14:43 ` Marc-André Lureau
0 siblings, 1 reply; 21+ messages in thread
From: Alberto Garcia @ 2019-02-07 14:27 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel, Paolo Bonzini, Daniel P. Berrangé
On Thu 07 Feb 2019 03:24:40 PM CET, Marc-André Lureau wrote:
> On Thu, Feb 7, 2019 at 3:23 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>>
>> On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
>> >
>> > After g_source_attach() the GMainContext holds a reference to the
>> > GSource, so the caller does not need to keep it.
>> >
>> > pty_chr_state() and qio_task_thread_worker() are not doing this, so
>> > the GSource is being leaked in both cases (pty_chr_open_src_cancel()
>> > is the exception here because it does remove the extra reference
>> > correctly).
>> >
>> > Signed-off-by: Alberto Garcia <berto@igalia.com>
>>
>> You could mention this is a fix for regression from
>> a17536c594bfed94d05667b419f747b692f5fc7f
>
> and 938eb9e9c83d34d90cac6ec5c5388e7998840e4e
Ok, feel free to add this to the commit message if I don't need to
resend the series.
Berto
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add()
2019-02-07 13:23 ` [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add() Alberto Garcia
2019-02-07 14:24 ` Daniel P. Berrangé
@ 2019-02-07 14:28 ` Marc-André Lureau
1 sibling, 0 replies; 21+ messages in thread
From: Marc-André Lureau @ 2019-02-07 14:28 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, Paolo Bonzini, Daniel P . Berrangé
Hi
On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
>
> This works like g_idle_add() but allows specifying a different
> GMainContext. It also returns the GSource directly instead of its ID
> number for convenience.
>
> pty_chr_state() and qio_task_thread_worker() are modified to make use
> of this new function.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> chardev/char-pty.c | 8 ++------
> include/qemu/main-loop.h | 8 ++++++++
> io/task.c | 7 ++-----
> util/main-loop.c | 9 +++++++++
> 4 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index f16a5e8d59..6562c38f39 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -209,13 +209,9 @@ static void pty_chr_state(Chardev *chr, int connected)
> pty_chr_timer_cancel(s);
> if (!s->connected) {
> g_assert(s->open_source == NULL);
> - s->open_source = g_idle_source_new();
> + s->open_source = qemu_idle_add(qemu_chr_be_generic_open_func,
> + chr, chr->gcontext);
> s->connected = 1;
> - g_source_set_callback(s->open_source,
> - qemu_chr_be_generic_open_func,
> - chr, NULL);
> - g_source_attach(s->open_source, chr->gcontext);
> - g_source_unref(s->open_source);
> }
> if (!chr->gsource) {
> chr->gsource = io_add_watch_poll(chr, s->ioc,
> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
> index e59f9ae1e9..1865afd993 100644
> --- a/include/qemu/main-loop.h
> +++ b/include/qemu/main-loop.h
> @@ -295,6 +295,14 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line);
> */
> void qemu_mutex_unlock_iothread(void);
>
> +/**
> + * qemu_idle_add: Add an idle function to a GMainContext
> + *
> + * This function is similar to GLib's g_idle_add() but it allows
> + * specifying a GMainContext instead of using the global one.
> + */
> +GSource *qemu_idle_add(GSourceFunc func, gpointer data, GMainContext *ctx);
I think you should document that returned GSource* isn't ref/owned.
> +
> /* internal interfaces */
>
> void qemu_fd_register(int fd);
> diff --git a/io/task.c b/io/task.c
> index c8489fb790..bdd395b0ad 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -19,6 +19,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
> #include "io/task.h"
> #include "qapi/error.h"
> #include "qemu/thread.h"
> @@ -105,7 +106,6 @@ static gboolean qio_task_thread_result(gpointer opaque)
> static gpointer qio_task_thread_worker(gpointer opaque)
> {
> struct QIOTaskThreadData *data = opaque;
> - GSource *idle;
>
> trace_qio_task_thread_run(data->task);
> data->worker(data->task, data->opaque);
> @@ -117,10 +117,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
> */
> trace_qio_task_thread_exit(data->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_unref(idle);
> + qemu_idle_add(qio_task_thread_result, data, data->context);
>
> return NULL;
> }
> diff --git a/util/main-loop.c b/util/main-loop.c
> index 443cb4cfe8..1c68e3bf5f 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -171,6 +171,15 @@ int qemu_init_main_loop(Error **errp)
> return 0;
> }
>
> +GSource *qemu_idle_add(GSourceFunc func, gpointer data, GMainContext *ctx)
> +{
> + GSource *idle = g_idle_source_new();
> + g_source_set_callback(idle, func, data, NULL);
> + g_source_attach(idle, ctx);
> + g_source_unref(idle);
> + return idle;
> +}
> +
> static int max_priority;
>
> #ifndef _WIN32
> --
> 2.11.0
>
looks ok otherwise
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext
2019-02-07 14:27 ` Alberto Garcia
@ 2019-02-07 14:43 ` Marc-André Lureau
2019-02-07 14:47 ` Alberto Garcia
0 siblings, 1 reply; 21+ messages in thread
From: Marc-André Lureau @ 2019-02-07 14:43 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Paolo Bonzini, qemu-devel
Hi
On Thu, Feb 7, 2019 at 3:37 PM Alberto Garcia <berto@igalia.com> wrote:
>
> On Thu 07 Feb 2019 03:24:40 PM CET, Marc-André Lureau wrote:
> > On Thu, Feb 7, 2019 at 3:23 PM Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> >>
> >> On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
> >> >
> >> > After g_source_attach() the GMainContext holds a reference to the
> >> > GSource, so the caller does not need to keep it.
> >> >
> >> > pty_chr_state() and qio_task_thread_worker() are not doing this, so
> >> > the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> >> > is the exception here because it does remove the extra reference
> >> > correctly).
> >> >
> >> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> >>
> >> You could mention this is a fix for regression from
> >> a17536c594bfed94d05667b419f747b692f5fc7f
> >
> > and 938eb9e9c83d34d90cac6ec5c5388e7998840e4e
>
> Ok, feel free to add this to the commit message if I don't need to
> resend the series.
Well, the series conflicts with the currently queued chardev series. I
think I will send the pullreq and let you rebase.
(I also have a related series pending review "[PATCH v2 0/6] chardev:
various cleanups and improvements")
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
2019-02-07 13:23 ` [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout() Alberto Garcia
2019-02-07 14:26 ` Daniel P. Berrangé
@ 2019-02-07 14:46 ` Marc-André Lureau
2019-02-07 15:05 ` Paolo Bonzini
1 sibling, 1 reply; 21+ messages in thread
From: Marc-André Lureau @ 2019-02-07 14:46 UTC (permalink / raw)
To: Alberto Garcia; +Cc: QEMU, Paolo Bonzini
Hi
On Thu, Feb 7, 2019 at 2:25 PM Alberto Garcia <berto@igalia.com> wrote:
>
> There's a race condition in which the tcp_chr_read() ioc handler can
> close a connection that is being written to from another thread.
>
> Running iotest 136 in a loop triggers this problem and crashes QEMU.
>
> (gdb) bt
> #0 0x00005558b842902d in object_get_class (obj=0x0) at qom/object.c:860
> #1 0x00005558b84f92db in qio_channel_writev_full (ioc=0x0, iov=0x7ffc355decf0, niov=1, fds=0x0, nfds=0, errp=0x0) at io/channel.c:76
> #2 0x00005558b84e0e9e in io_channel_send_full (ioc=0x0, buf=0x5558baf5beb0, len=138, fds=0x0, nfds=0) at chardev/char-io.c:123
> #3 0x00005558b84e4a69 in tcp_chr_write (chr=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138) at chardev/char-socket.c:135
> #4 0x00005558b84dca55 in qemu_chr_write_buffer (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, offset=0x7ffc355dedd0, write_all=false) at chardev/char.c:112
> #5 0x00005558b84dcbc2 in qemu_chr_write (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, write_all=false) at chardev/char.c:147
> #6 0x00005558b84dfb26 in qemu_chr_fe_write (be=0x5558ba476610, buf=0x5558baf5beb0 "...", len=138) at chardev/char-fe.c:42
> #7 0x00005558b8088c86 in monitor_flush_locked (mon=0x5558ba476610) at monitor.c:406
> #8 0x00005558b8088e8c in monitor_puts (mon=0x5558ba476610, str=0x5558ba921e49 "") at monitor.c:449
> #9 0x00005558b8089178 in qmp_send_response (mon=0x5558ba476610, rsp=0x5558bb161600) at monitor.c:498
> #10 0x00005558b808920c in monitor_qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:526
> #11 0x00005558b8089307 in monitor_qapi_event_queue_no_reenter (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:551
> #12 0x00005558b80896c0 in qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:626
> #13 0x00005558b855f23b in qapi_event_send_shutdown (guest=false, reason=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at qapi/qapi-events-run-state.c:43
> #14 0x00005558b81911ef in qemu_system_shutdown (cause=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at vl.c:1837
> #15 0x00005558b8191308 in main_loop_should_exit () at vl.c:1885
> #16 0x00005558b819140d in main_loop () at vl.c:1924
> #17 0x00005558b8198c84 in main (argc=18, argv=0x7ffc355df3f8, envp=0x7ffc355df490) at vl.c:4665
>
> This patch adds a lock to protect tcp_chr_disconnect() and
> socket_reconnect_timeout()
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
That looks like a subset of "[Qemu-devel] Segfaults in chardev due to
races". The problem is likely to occur with other chardevs I am
afraid.
> ---
> chardev/char-socket.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index eaa8e8b68f..5f421bdc58 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -148,7 +148,9 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>
> if (ret < 0 && errno != EAGAIN) {
> if (tcp_chr_read_poll(chr) <= 0) {
> + qemu_mutex_unlock(&chr->chr_write_lock);
> tcp_chr_disconnect(chr);
> + qemu_mutex_lock(&chr->chr_write_lock);
> return len;
> } /* else let the read handler finish it properly */
> }
> @@ -440,6 +442,13 @@ static void update_disconnected_filename(SocketChardev *s)
> }
> }
>
> +static gboolean tcp_chr_be_event_closed(gpointer opaque)
> +{
> + Chardev *chr = opaque;
> + qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> + return FALSE;
> +}
> +
> /* 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
> @@ -447,7 +456,10 @@ static void update_disconnected_filename(SocketChardev *s)
> static void tcp_chr_disconnect(Chardev *chr)
> {
> SocketChardev *s = SOCKET_CHARDEV(chr);
> - bool emit_close = s->connected;
> + bool emit_close;
> +
> + qemu_mutex_lock(&chr->chr_write_lock);
> + emit_close = s->connected;
>
> tcp_chr_free_connection(chr);
>
> @@ -457,11 +469,12 @@ static void tcp_chr_disconnect(Chardev *chr)
> }
> update_disconnected_filename(s);
> if (emit_close) {
> - qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> + qemu_idle_add(tcp_chr_be_event_closed, chr, chr->gcontext);
> }
> if (s->reconnect_time) {
> qemu_chr_socket_restart_timer(chr);
> }
> + qemu_mutex_unlock(&chr->chr_write_lock);
> }
>
> static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
> @@ -975,8 +988,10 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
> Chardev *chr = CHARDEV(opaque);
> SocketChardev *s = SOCKET_CHARDEV(opaque);
>
> + qemu_mutex_lock(&chr->chr_write_lock);
> g_source_unref(s->reconnect_timer);
> s->reconnect_timer = NULL;
> + qemu_mutex_unlock(&chr->chr_write_lock);
>
> if (chr->be_open) {
> return false;
> --
> 2.11.0
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext
2019-02-07 14:43 ` Marc-André Lureau
@ 2019-02-07 14:47 ` Alberto Garcia
0 siblings, 0 replies; 21+ messages in thread
From: Alberto Garcia @ 2019-02-07 14:47 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Paolo Bonzini, qemu-devel
On Thu 07 Feb 2019 03:43:59 PM CET, Marc-André Lureau wrote:
> Well, the series conflicts with the currently queued chardev series. I
> think I will send the pullreq and let you rebase.
I'll wait for it then
Berto
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
2019-02-07 14:46 ` Marc-André Lureau
@ 2019-02-07 15:05 ` Paolo Bonzini
0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2019-02-07 15:05 UTC (permalink / raw)
To: Marc-André Lureau, Alberto Garcia; +Cc: QEMU
On 07/02/19 15:46, Marc-André Lureau wrote:
> That looks like a subset of "[Qemu-devel] Segfaults in chardev due to
> races". The problem is likely to occur with other chardevs I am
> afraid.
Yes, it could possibly happen with pty at least. However, Alberto's
patch is a start.
Paolo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
2019-02-07 14:26 ` Daniel P. Berrangé
@ 2019-02-08 8:50 ` Alberto Garcia
2019-02-08 9:34 ` Alberto Garcia
1 sibling, 0 replies; 21+ messages in thread
From: Alberto Garcia @ 2019-02-08 8:50 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Paolo Bonzini, Marc-André Lureau
On Thu 07 Feb 2019 03:26:49 PM CET, Daniel P. Berrangé wrote:
>> @@ -457,11 +469,12 @@ static void tcp_chr_disconnect(Chardev *chr)
>> }
>> update_disconnected_filename(s);
>> if (emit_close) {
>> - qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>> + qemu_idle_add(tcp_chr_be_event_closed, chr, chr->gcontext);
>
> I'm a little concerned that this change might trigger some unexpected
> interactions with the qemu_chr_wait_connected() function. I added some
> more testing around that code, so if it passes the new tests I have
> in:
>
> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05962.html
>
> and also works with the vhostuser reconnect device, then we are
> probably ok.
Seems that it doesn't ...
/char/null: OK
/char/invalid: OK
/char/ringbuf: OK
/char/mux: OK
/char/stdio: OK
/char/pipe: OK
/char/file: OK
/char/file-fifo: OK
/char/udp: OK
/char/serial: OK
/char/hotswap: OK
/char/websocket: OK
/char/socket/server/mainloop/tcp: OK
/char/socket/server/mainloop/unix: OK
/char/socket/server/wait-conn/tcp: OK
/char/socket/server/wait-conn/unix: OK
/char/socket/server/mainloop-fdpass/tcp: OK
/char/socket/server/mainloop-fdpass/unix: OK
/char/socket/server/wait-conn-fdpass/tcp: OK
/char/socket/server/wait-conn-fdpass/unix: OK
/char/socket/client/mainloop/tcp: OK
/char/socket/client/mainloop/unix: OK
/char/socket/client/wait-conn/tcp: OK
/char/socket/client/wait-conn/unix: OK
/char/socket/client/mainloop-reconnect/tcp: OK
/char/socket/client/mainloop-reconnect/unix: OK
/char/socket/client/wait-conn-reconnect/tcp: Unexpected error in qio_channel_readv_all() at /home/berto/work/qemu/qemu/io/channel.c:147:
Unexpected end-of-file before all bytes were read
Berto
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
2019-02-07 14:26 ` Daniel P. Berrangé
2019-02-08 8:50 ` Alberto Garcia
@ 2019-02-08 9:34 ` Alberto Garcia
2019-02-08 13:46 ` Paolo Bonzini
1 sibling, 1 reply; 21+ messages in thread
From: Alberto Garcia @ 2019-02-08 9:34 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Paolo Bonzini, Marc-André Lureau
On Thu 07 Feb 2019 03:26:49 PM CET, Daniel P. Berrangé wrote:
>> @@ -457,11 +469,12 @@ static void tcp_chr_disconnect(Chardev *chr)
>> }
>> update_disconnected_filename(s);
>> if (emit_close) {
>> - qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>> + qemu_idle_add(tcp_chr_be_event_closed, chr, chr->gcontext);
>
> I'm a little concerned that this change might trigger some unexpected
> interactions with the qemu_chr_wait_connected() function. I added some
> more testing around that code, so if it passes the new tests I have in:
>
> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05962.html
Forget my previous e-mail, there was a mistake on my side. The test does
pass after rebasing my series on top of that one.
> and also works with the vhostuser reconnect device, then we are
> probably ok.
How do I test this?
Berto
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
2019-02-08 9:34 ` Alberto Garcia
@ 2019-02-08 13:46 ` Paolo Bonzini
2019-02-08 15:01 ` Alberto Garcia
0 siblings, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2019-02-08 13:46 UTC (permalink / raw)
To: Alberto Garcia, Daniel P. Berrangé
Cc: qemu-devel, Marc-André Lureau
On 08/02/19 10:34, Alberto Garcia wrote:
>> and also works with the vhostuser reconnect device, then we are
>> probably ok.
> How do I test this?
QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 \
QTEST_VHOST_USER_FIXME=1 \
tests/vhost-user-test
Paolo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
2019-02-08 13:46 ` Paolo Bonzini
@ 2019-02-08 15:01 ` Alberto Garcia
2019-02-08 17:27 ` Paolo Bonzini
0 siblings, 1 reply; 21+ messages in thread
From: Alberto Garcia @ 2019-02-08 15:01 UTC (permalink / raw)
To: Paolo Bonzini, Daniel P. Berrangé; +Cc: qemu-devel, Marc-André Lureau
On Fri 08 Feb 2019 02:46:27 PM CET, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 08/02/19 10:34, Alberto Garcia wrote:
>>> and also works with the vhostuser reconnect device, then we are
>>> probably ok.
>> How do I test this?
>
> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 \
> QTEST_VHOST_USER_FIXME=1 \
> tests/vhost-user-test
I see. The ones enabled by QTEST_VHOST_USER_FIXME never finish with this
patch applied.
Berto
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
2019-02-08 15:01 ` Alberto Garcia
@ 2019-02-08 17:27 ` Paolo Bonzini
0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2019-02-08 17:27 UTC (permalink / raw)
To: Alberto Garcia, Daniel P. Berrangé
Cc: qemu-devel, Marc-André Lureau
On 08/02/19 16:01, Alberto Garcia wrote:
> On Fri 08 Feb 2019 02:46:27 PM CET, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 08/02/19 10:34, Alberto Garcia wrote:
>>>> and also works with the vhostuser reconnect device, then we are
>>>> probably ok.
>>> How do I test this?
>>
>> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 \
>> QTEST_VHOST_USER_FIXME=1 \
>> tests/vhost-user-test
>
> I see. The ones enabled by QTEST_VHOST_USER_FIXME never finish with this
> patch applied.
I can look at it next week.
Paolo
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-02-08 17:27 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07 13:23 [Qemu-devel] [PATCH 0/3] char-socket: Fix race condition Alberto Garcia
2019-02-07 13:23 ` [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext Alberto Garcia
2019-02-07 13:25 ` Alberto Garcia
2019-02-07 14:21 ` Daniel P. Berrangé
2019-02-07 14:23 ` Marc-André Lureau
2019-02-07 14:24 ` Marc-André Lureau
2019-02-07 14:27 ` Alberto Garcia
2019-02-07 14:43 ` Marc-André Lureau
2019-02-07 14:47 ` Alberto Garcia
2019-02-07 13:23 ` [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add() Alberto Garcia
2019-02-07 14:24 ` Daniel P. Berrangé
2019-02-07 14:28 ` Marc-André Lureau
2019-02-07 13:23 ` [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout() Alberto Garcia
2019-02-07 14:26 ` Daniel P. Berrangé
2019-02-08 8:50 ` Alberto Garcia
2019-02-08 9:34 ` Alberto Garcia
2019-02-08 13:46 ` Paolo Bonzini
2019-02-08 15:01 ` Alberto Garcia
2019-02-08 17:27 ` Paolo Bonzini
2019-02-07 14:46 ` Marc-André Lureau
2019-02-07 15:05 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.