Re: [Qemu-devel] [PATCH V4] char: restore read callback on a reattached (hotplug) chardev
On 18/12/2013 07:35, Amit Shah wrote: As far as I could tell the pty backend doesn't suffer from this issue. That's why I didn't change anything there. pty_chr_update_read_handler() calls pty_chr_state(), which calls remove_fd_in_watch(). Yes, but the pty_chr_update_read_handler() isn't called on device removal so remove_fd_in_watch() is not executed. However I did find that there was a problem in the pty backend that I missed and I'll post an updated patch. Thanks, Gal.
Re: [Qemu-devel] [PATCH V4] char: restore read callback on a reattached (hotplug) chardev
On 16/12/2013 22:32, Amit Shah wrote:
On (Sun) 15 Dec 2013 [12:26:37], Gal Hammer wrote:
Fix a bug that was introduced in commit 386a5a1e. A removal of a device
set the chr handlers to NULL. However when the device is plugged back,
its read callback is not restored so data can't be transferred from the
host to the guest (e.g. via the virtio-serial port).
https://bugzilla.redhat.com/show_bug.cgi?id=1027181
Signed-off-by: Gal Hammer gham...@redhat.com
---
qemu-char.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
V4: - Same as V3, but this time done right.
V3: - fix a typo in comment.
- move the revision history after the signed-off-by tag.
V2: - do not call chr_update_read_handler on device removal.
- add asserts to verify chr_update_read_handler is not called
with an assigned fd_in_tag to prevent fd leaks.
- update fd and udp backends' chr_update_read_handler function
so it won't remove fd_in to prevent a double release.
Looks like you missed the pty backend. Can't blame you -- this
chardev stuff is really messy. pty is doing is own handling + polling
+ HUP detection, which really is equally applicable to tcp and udp as
well.
As far as I could tell the pty backend doesn't suffer from this issue.
That's why I didn't change anything there.
More below.
diff --git a/qemu-char.c b/qemu-char.c
index e00f84c..69649f0 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -213,7 +213,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
s-chr_read = fd_read;
s-chr_event = fd_event;
s-handler_opaque = opaque;
-if (s-chr_update_read_handler)
+if (fe_open s-chr_update_read_handler)
s-chr_update_read_handler(s);
if (!s-explicit_fe_open) {
@@ -870,6 +870,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
{
FDCharDriver *s = chr-opaque;
+assert(!chr-fd_in_tag);
remove_fd_in_watch(chr);
if (s-fd_in) {
chr-fd_in_tag = io_add_watch_poll(s-fd_in, fd_chr_read_poll,
@@ -2228,7 +2229,7 @@ static void udp_chr_update_read_handler(CharDriverState
*chr)
{
NetCharDriver *s = chr-opaque;
-remove_fd_in_watch(chr);
+assert(!chr-fd_in_tag);
You're removing remove_fd_in_watch() here but not in
fd_chr_update_read_handler() above. Any particular reason?
I missed it while commit -p the changes. Fixed.
Also, this assert would be a risky thing in pty, since it has its own
HUP detection method. A common way to deal with all this would be
nice, but chardevs are hacks upon hacks already, and this fixes a
regression, so I'm fine with applying the patch in this form now.
Please update for pty.
Gerd, can you please look at this and pick it up? I'm going to be
away till Jan.
Thanks,
Amit
Gal.
Re: [Qemu-devel] [PATCH V4] char: restore read callback on a reattached (hotplug) chardev
On (Tue) 17 Dec 2013 [11:12:02], Gal Hammer wrote: On 16/12/2013 22:32, Amit Shah wrote: On (Sun) 15 Dec 2013 [12:26:37], Gal Hammer wrote: Fix a bug that was introduced in commit 386a5a1e. A removal of a device set the chr handlers to NULL. However when the device is plugged back, its read callback is not restored so data can't be transferred from the host to the guest (e.g. via the virtio-serial port). https://bugzilla.redhat.com/show_bug.cgi?id=1027181 Signed-off-by: Gal Hammer gham...@redhat.com --- qemu-char.c | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) V4: - Same as V3, but this time done right. V3: - fix a typo in comment. - move the revision history after the signed-off-by tag. V2: - do not call chr_update_read_handler on device removal. - add asserts to verify chr_update_read_handler is not called with an assigned fd_in_tag to prevent fd leaks. - update fd and udp backends' chr_update_read_handler function so it won't remove fd_in to prevent a double release. Looks like you missed the pty backend. Can't blame you -- this chardev stuff is really messy. pty is doing is own handling + polling + HUP detection, which really is equally applicable to tcp and udp as well. As far as I could tell the pty backend doesn't suffer from this issue. That's why I didn't change anything there. pty_chr_update_read_handler() calls pty_chr_state(), which calls remove_fd_in_watch(). Amit
Re: [Qemu-devel] [PATCH V4] char: restore read callback on a reattached (hotplug) chardev
On (Sun) 15 Dec 2013 [12:26:37], Gal Hammer wrote:
Fix a bug that was introduced in commit 386a5a1e. A removal of a device
set the chr handlers to NULL. However when the device is plugged back,
its read callback is not restored so data can't be transferred from the
host to the guest (e.g. via the virtio-serial port).
https://bugzilla.redhat.com/show_bug.cgi?id=1027181
Signed-off-by: Gal Hammer gham...@redhat.com
---
qemu-char.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
V4: - Same as V3, but this time done right.
V3: - fix a typo in comment.
- move the revision history after the signed-off-by tag.
V2: - do not call chr_update_read_handler on device removal.
- add asserts to verify chr_update_read_handler is not called
with an assigned fd_in_tag to prevent fd leaks.
- update fd and udp backends' chr_update_read_handler function
so it won't remove fd_in to prevent a double release.
Looks like you missed the pty backend. Can't blame you -- this
chardev stuff is really messy. pty is doing is own handling + polling
+ HUP detection, which really is equally applicable to tcp and udp as
well.
More below.
diff --git a/qemu-char.c b/qemu-char.c
index e00f84c..69649f0 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -213,7 +213,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
s-chr_read = fd_read;
s-chr_event = fd_event;
s-handler_opaque = opaque;
-if (s-chr_update_read_handler)
+if (fe_open s-chr_update_read_handler)
s-chr_update_read_handler(s);
if (!s-explicit_fe_open) {
@@ -870,6 +870,7 @@ static void fd_chr_update_read_handler(CharDriverState
*chr)
{
FDCharDriver *s = chr-opaque;
+assert(!chr-fd_in_tag);
remove_fd_in_watch(chr);
if (s-fd_in) {
chr-fd_in_tag = io_add_watch_poll(s-fd_in, fd_chr_read_poll,
@@ -2228,7 +2229,7 @@ static void udp_chr_update_read_handler(CharDriverState
*chr)
{
NetCharDriver *s = chr-opaque;
-remove_fd_in_watch(chr);
+assert(!chr-fd_in_tag);
You're removing remove_fd_in_watch() here but not in
fd_chr_update_read_handler() above. Any particular reason?
Also, this assert would be a risky thing in pty, since it has its own
HUP detection method. A common way to deal with all this would be
nice, but chardevs are hacks upon hacks already, and this fixes a
regression, so I'm fine with applying the patch in this form now.
Please update for pty.
Gerd, can you please look at this and pick it up? I'm going to be
away till Jan.
Thanks,
Amit
