Re: [Qemu-devel] [PATCH] migrate_fd_close: delete associated io-handler before closing the fd

[Mark McLoughlin]

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);