* [Qemu-devel] [PATCH v2 1/3] char: move backends' io watch tag to CharDriverState
2013-08-28 12:29 [Qemu-devel] [PATCH v2 0/3] char: fix segfault on chardev detach Amit Shah
@ 2013-08-28 12:29 ` Amit Shah
2013-08-28 12:29 ` [Qemu-devel] [PATCH v2 2/3] char: use common function to disable callbacks on chardev close Amit Shah
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Amit Shah @ 2013-08-28 12:29 UTC (permalink / raw)
To: qemu list
Cc: qemu-stable, Hans de Goede, Gerd Hoffmann, Anthony Liguori,
Amit Shah, Paolo Bonzini
All the backends implement an io watcher tag for callbacks. Move it to
CharDriverState from each backend's struct to make accessing the tag from
backend-neutral functions easier.
This will be used later to cancel a callback on chardev detach from a
frontend.
CC: <qemu-stable@nongnu.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
include/sysemu/char.h | 1 +
qemu-char.c | 77 ++++++++++++++++++++++++++-------------------------
2 files changed, 40 insertions(+), 38 deletions(-)
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 8053130..ad101d9 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -78,6 +78,7 @@ struct CharDriverState {
int explicit_be_open;
int avail_connections;
int is_mux;
+ guint fd_in_tag;
QemuOpts *opts;
QTAILQ_ENTRY(CharDriverState) next;
};
diff --git a/qemu-char.c b/qemu-char.c
index 6259496..cb4a311 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -798,7 +798,6 @@ static int io_channel_send(GIOChannel *fd, const void *buf, size_t len)
typedef struct FDCharDriver {
CharDriverState *chr;
GIOChannel *fd_in, *fd_out;
- guint fd_in_tag;
int max_size;
QTAILQ_ENTRY(FDCharDriver) node;
} FDCharDriver;
@@ -830,9 +829,9 @@ static gboolean fd_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
status = g_io_channel_read_chars(chan, (gchar *)buf,
len, &bytes_read, NULL);
if (status == G_IO_STATUS_EOF) {
- if (s->fd_in_tag) {
- io_remove_watch_poll(s->fd_in_tag);
- s->fd_in_tag = 0;
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
}
qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
return FALSE;
@@ -863,13 +862,14 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
{
FDCharDriver *s = chr->opaque;
- if (s->fd_in_tag) {
- io_remove_watch_poll(s->fd_in_tag);
- s->fd_in_tag = 0;
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
}
if (s->fd_in) {
- s->fd_in_tag = io_add_watch_poll(s->fd_in, fd_chr_read_poll, fd_chr_read, chr);
+ chr->fd_in_tag = io_add_watch_poll(s->fd_in, fd_chr_read_poll, fd_chr_read,
+ chr);
}
}
@@ -877,9 +877,9 @@ static void fd_chr_close(struct CharDriverState *chr)
{
FDCharDriver *s = chr->opaque;
- if (s->fd_in_tag) {
- io_remove_watch_poll(s->fd_in_tag);
- s->fd_in_tag = 0;
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
}
if (s->fd_in) {
@@ -1012,7 +1012,6 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
typedef struct {
GIOChannel *fd;
- guint fd_tag;
int connected;
int read_bytes;
guint timer_tag;
@@ -1127,9 +1126,9 @@ static void pty_chr_state(CharDriverState *chr, int connected)
PtyCharDriver *s = chr->opaque;
if (!connected) {
- if (s->fd_tag) {
- io_remove_watch_poll(s->fd_tag);
- s->fd_tag = 0;
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
}
s->connected = 0;
/* (re-)connect poll interval for idle guests: once per second.
@@ -1144,7 +1143,8 @@ static void pty_chr_state(CharDriverState *chr, int connected)
if (!s->connected) {
s->connected = 1;
qemu_chr_be_generic_open(chr);
- s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr);
+ chr->fd_in_tag = io_add_watch_poll(s->fd, pty_chr_read_poll,
+ pty_chr_read, chr);
}
}
}
@@ -1155,9 +1155,9 @@ static void pty_chr_close(struct CharDriverState *chr)
PtyCharDriver *s = chr->opaque;
int fd;
- if (s->fd_tag) {
- io_remove_watch_poll(s->fd_tag);
- s->fd_tag = 0;
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
}
fd = g_io_channel_unix_get_fd(s->fd);
g_io_channel_unref(s->fd);
@@ -2165,7 +2165,6 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
typedef struct {
int fd;
GIOChannel *chan;
- guint tag;
uint8_t buf[READ_BUF_LEN];
int bufcnt;
int bufptr;
@@ -2221,9 +2220,9 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
s->bufcnt = bytes_read;
s->bufptr = s->bufcnt;
if (status != G_IO_STATUS_NORMAL) {
- if (s->tag) {
- io_remove_watch_poll(s->tag);
- s->tag = 0;
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
}
return FALSE;
}
@@ -2242,22 +2241,23 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
{
NetCharDriver *s = chr->opaque;
- if (s->tag) {
- io_remove_watch_poll(s->tag);
- s->tag = 0;
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
}
if (s->chan) {
- s->tag = io_add_watch_poll(s->chan, udp_chr_read_poll, udp_chr_read, chr);
+ chr->fd_in_tag = io_add_watch_poll(s->chan, udp_chr_read_poll, udp_chr_read,
+ chr);
}
}
static void udp_chr_close(CharDriverState *chr)
{
NetCharDriver *s = chr->opaque;
- if (s->tag) {
- io_remove_watch_poll(s->tag);
- s->tag = 0;
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
}
if (s->chan) {
g_io_channel_unref(s->chan);
@@ -2308,7 +2308,7 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
typedef struct {
GIOChannel *chan, *listen_chan;
- guint tag, listen_tag;
+ guint listen_tag;
int fd, listen_fd;
int connected;
int max_size;
@@ -2493,9 +2493,9 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
if (s->listen_chan) {
s->listen_tag = g_io_add_watch(s->listen_chan, G_IO_IN, tcp_chr_accept, chr);
}
- if (s->tag) {
- io_remove_watch_poll(s->tag);
- s->tag = 0;
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
}
g_io_channel_unref(s->chan);
s->chan = NULL;
@@ -2526,7 +2526,8 @@ static void tcp_chr_connect(void *opaque)
s->connected = 1;
if (s->chan) {
- s->tag = io_add_watch_poll(s->chan, tcp_chr_read_poll, tcp_chr_read, chr);
+ chr->fd_in_tag = io_add_watch_poll(s->chan, tcp_chr_read_poll, tcp_chr_read,
+ chr);
}
qemu_chr_be_generic_open(chr);
}
@@ -2609,9 +2610,9 @@ static void tcp_chr_close(CharDriverState *chr)
{
TCPCharDriver *s = chr->opaque;
if (s->fd >= 0) {
- if (s->tag) {
- io_remove_watch_poll(s->tag);
- s->tag = 0;
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
}
if (s->chan) {
g_io_channel_unref(s->chan);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH v2 2/3] char: use common function to disable callbacks on chardev close
2013-08-28 12:29 [Qemu-devel] [PATCH v2 0/3] char: fix segfault on chardev detach Amit Shah
2013-08-28 12:29 ` [Qemu-devel] [PATCH v2 1/3] char: move backends' io watch tag to CharDriverState Amit Shah
@ 2013-08-28 12:29 ` Amit Shah
2013-08-28 12:29 ` [Qemu-devel] [PATCH v2 3/3] char: remove watch callback on chardev detach from frontend Amit Shah
2013-08-28 12:33 ` [Qemu-devel] [PATCH v2 0/3] char: fix segfault on chardev detach Gerd Hoffmann
3 siblings, 0 replies; 6+ messages in thread
From: Amit Shah @ 2013-08-28 12:29 UTC (permalink / raw)
To: qemu list
Cc: qemu-stable, Hans de Goede, Gerd Hoffmann, Anthony Liguori,
Amit Shah, Paolo Bonzini
This deduplicates code used a lot of times.
CC: <qemu-stable@nongnu.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
qemu-char.c | 62 +++++++++++++++++++------------------------------------------
1 file changed, 19 insertions(+), 43 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index cb4a311..44d993a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -725,6 +725,14 @@ static void io_remove_watch_poll(guint tag)
g_source_destroy(&iwp->parent);
}
+static void remove_fd_in_watch(CharDriverState *chr)
+{
+ if (chr->fd_in_tag) {
+ io_remove_watch_poll(chr->fd_in_tag);
+ chr->fd_in_tag = 0;
+ }
+}
+
#ifndef _WIN32
static GIOChannel *io_channel_from_fd(int fd)
{
@@ -829,10 +837,7 @@ static gboolean fd_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
status = g_io_channel_read_chars(chan, (gchar *)buf,
len, &bytes_read, NULL);
if (status == G_IO_STATUS_EOF) {
- if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
- chr->fd_in_tag = 0;
- }
+ remove_fd_in_watch(chr);
qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
return FALSE;
}
@@ -862,11 +867,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
{
FDCharDriver *s = chr->opaque;
- if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
- chr->fd_in_tag = 0;
- }
-
+ remove_fd_in_watch(chr);
if (s->fd_in) {
chr->fd_in_tag = io_add_watch_poll(s->fd_in, fd_chr_read_poll, fd_chr_read,
chr);
@@ -877,11 +878,7 @@ static void fd_chr_close(struct CharDriverState *chr)
{
FDCharDriver *s = chr->opaque;
- if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
- chr->fd_in_tag = 0;
- }
-
+ remove_fd_in_watch(chr);
if (s->fd_in) {
g_io_channel_unref(s->fd_in);
}
@@ -1126,10 +1123,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
PtyCharDriver *s = chr->opaque;
if (!connected) {
- if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
- chr->fd_in_tag = 0;
- }
+ remove_fd_in_watch(chr);
s->connected = 0;
/* (re-)connect poll interval for idle guests: once per second.
* We check more frequently in case the guests sends data to
@@ -1155,10 +1149,7 @@ static void pty_chr_close(struct CharDriverState *chr)
PtyCharDriver *s = chr->opaque;
int fd;
- if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
- chr->fd_in_tag = 0;
- }
+ remove_fd_in_watch(chr);
fd = g_io_channel_unix_get_fd(s->fd);
g_io_channel_unref(s->fd);
close(fd);
@@ -2220,10 +2211,7 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
s->bufcnt = bytes_read;
s->bufptr = s->bufcnt;
if (status != G_IO_STATUS_NORMAL) {
- if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
- chr->fd_in_tag = 0;
- }
+ remove_fd_in_watch(chr);
return FALSE;
}
@@ -2241,11 +2229,7 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
{
NetCharDriver *s = chr->opaque;
- if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
- chr->fd_in_tag = 0;
- }
-
+ remove_fd_in_watch(chr);
if (s->chan) {
chr->fd_in_tag = io_add_watch_poll(s->chan, udp_chr_read_poll, udp_chr_read,
chr);
@@ -2255,10 +2239,8 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
static void udp_chr_close(CharDriverState *chr)
{
NetCharDriver *s = chr->opaque;
- if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
- chr->fd_in_tag = 0;
- }
+
+ remove_fd_in_watch(chr);
if (s->chan) {
g_io_channel_unref(s->chan);
closesocket(s->fd);
@@ -2493,10 +2475,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
if (s->listen_chan) {
s->listen_tag = g_io_add_watch(s->listen_chan, G_IO_IN, tcp_chr_accept, chr);
}
- if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
- chr->fd_in_tag = 0;
- }
+ remove_fd_in_watch(chr);
g_io_channel_unref(s->chan);
s->chan = NULL;
closesocket(s->fd);
@@ -2610,10 +2589,7 @@ static void tcp_chr_close(CharDriverState *chr)
{
TCPCharDriver *s = chr->opaque;
if (s->fd >= 0) {
- if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
- chr->fd_in_tag = 0;
- }
+ remove_fd_in_watch(chr);
if (s->chan) {
g_io_channel_unref(s->chan);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH v2 3/3] char: remove watch callback on chardev detach from frontend
2013-08-28 12:29 [Qemu-devel] [PATCH v2 0/3] char: fix segfault on chardev detach Amit Shah
2013-08-28 12:29 ` [Qemu-devel] [PATCH v2 1/3] char: move backends' io watch tag to CharDriverState Amit Shah
2013-08-28 12:29 ` [Qemu-devel] [PATCH v2 2/3] char: use common function to disable callbacks on chardev close Amit Shah
@ 2013-08-28 12:29 ` Amit Shah
2013-08-28 12:33 ` [Qemu-devel] [PATCH v2 0/3] char: fix segfault on chardev detach Gerd Hoffmann
3 siblings, 0 replies; 6+ messages in thread
From: Amit Shah @ 2013-08-28 12:29 UTC (permalink / raw)
To: qemu list
Cc: qemu-stable, Hans de Goede, Gerd Hoffmann, Anthony Liguori,
Amit Shah, Paolo Bonzini
If a frontend device releases the chardev (via unplug), the chr handlers
are set to NULL via qdev's exit callbacks invoking
qemu_chr_add_handlers(). If the chardev had a pending operation, a
callback will be invoked, which will try to access data in the
just-released frontend, causing a segfault.
Ensure the callbacks are disabled when frontends release chardevs.
This was seen when a virtio-serial port was unplugged when heavy
guest->host IO was in progress (causing a callback to be registered).
In the window in which the throttling was active, unplugging ports
caused a qemu segfault.
https://bugzilla.redhat.com/show_bug.cgi?id=985205
CC: <qemu-stable@nongnu.org>
Reported-by: Sibiao Luo <sluo@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
qemu-char.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/qemu-char.c b/qemu-char.c
index 44d993a..0cba7d4 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -193,6 +193,8 @@ void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
va_end(ap);
}
+static void remove_fd_in_watch(CharDriverState *chr);
+
void qemu_chr_add_handlers(CharDriverState *s,
IOCanReadHandler *fd_can_read,
IOReadHandler *fd_read,
@@ -203,6 +205,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
if (!opaque && !fd_can_read && !fd_read && !fd_event) {
fe_open = 0;
+ remove_fd_in_watch(s);
} else {
fe_open = 1;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread