From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MDhrc-0003VP-Rw for qemu-devel@nongnu.org; Mon, 08 Jun 2009 12:38:24 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MDhrY-0003P0-5u for qemu-devel@nongnu.org; Mon, 08 Jun 2009 12:38:24 -0400 Received: from [199.232.76.173] (port=46461 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MDhrX-0003Og-Pe for qemu-devel@nongnu.org; Mon, 08 Jun 2009 12:38:19 -0400 Received: from mx2.redhat.com ([66.187.237.31]:52251) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MDhrX-0007lG-7n for qemu-devel@nongnu.org; Mon, 08 Jun 2009 12:38:19 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n58GcIAc025358 for ; Mon, 8 Jun 2009 12:38:18 -0400 Subject: Re: [Qemu-devel] [PATCH] migrate_fd_close: delete associated io-handler before closing the fd From: Mark McLoughlin In-Reply-To: <4A2D34A7.1080208@redhat.com> References: <1244460481-13575-1-git-send-email-uril@redhat.com> <1244476182.3632.40.camel@blaa> <4A2D34A7.1080208@redhat.com> Content-Type: text/plain Date: Mon, 08 Jun 2009 17:38:14 +0100 Message-Id: <1244479094.3632.60.camel@blaa> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Reply-To: Mark McLoughlin List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Uri Lublin Cc: qemu-devel@nongnu.org On Mon, 2009-06-08 at 18:56 +0300, Uri Lublin wrote: > >> + > >> + qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL); > > > > Looks good, but perhaps a comment explaining how the I/O handler could > > possibly be registered here would be useful - at first glance, it seemed > > to me that the I/O handler should always be de-registered in > > migrate_fd_cleanup() before getting here. > > > > The key to understanding the problem is that qemu_fclose() on a buffered > > file can cause I/O to be flushed. > > Do you mean in addition to the log-message (copy part of the log message as a > comment in the code) ? It's subtle, so yeah - I think it deserves a comment. Would the below patch working equally well? But then again, we should really remove the I/O handler before closing the fd. (The close(s->fd) in migration_fd_cleanup() looks like it can never happen - perhaps we should remove it) Cheers, Mark. diff --git a/migration.c b/migration.c index 401383c..078967f 100644 --- a/migration.c +++ b/migration.c @@ -154,13 +154,21 @@ void migrate_fd_error(FdMigrationState *s) void migrate_fd_cleanup(FdMigrationState *s) { - qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL); + int fd; + + /* qemu_fclose() can cause I/O to be flushed (see buffered_close()) + * which, in turn, can cause an I/O handler to be registered. We + * need to delay removing the I/O handler until after qemu_fclose(). + */ + fd = s->fd; if (s->file) { dprintf("closing file\n"); qemu_fclose(s->file); } + qemu_set_fd_handler2(fd, NULL, NULL, NULL, NULL); + if (s->fd != -1) close(s->fd);