From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46694) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vsqef-00078O-Ks for qemu-devel@nongnu.org; Tue, 17 Dec 2013 04:09:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VsqeW-00069L-Pj for qemu-devel@nongnu.org; Tue, 17 Dec 2013 04:09:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43814) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VsqeW-00069F-HZ for qemu-devel@nongnu.org; Tue, 17 Dec 2013 04:09:20 -0500 Message-ID: <52B01562.60700@redhat.com> Date: Tue, 17 Dec 2013 11:12:02 +0200 From: Gal Hammer MIME-Version: 1.0 References: <1387103197-2238-1-git-send-email-ghammer@redhat.com> <20131216203245.GD6582@grmbl.mre> In-Reply-To: <20131216203245.GD6582@grmbl.mre> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V4] char: restore read callback on a reattached (hotplug) chardev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: qemu-devel@nongnu.org, Anthony Liguori , Gerd Hoffmann 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 >> >> --- >> 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.