[Qemu-devel] [PATCH 0/7] Handle errors during migration

[Qemu-devel] [PATCH 0/7] Handle errors during migration

[Juan Quintela]

Hi

This patch series contains error handling for migration.

After this series are applied, migrate_cancel after one error don't
hang.  And we add some error checking left and right.

This is the error handling patches that were on the middle of my
migration-cleanup of some months ago.  migration_cancel fix has been
added.

Later, Juan.

Juan Quintela (5):
  migration: simplify state assignmente
  migration: only flush when there are no errors
  migration: Check that migration is active before cancel it
  migration: If there is one error, it makes no sense to continue
  migration: qemu_savevm_iterate has three return values

Yoshiaki Tamura (2):
  savevm: avoid qemu_savevm_state_iterate() to return 1 when qemu file
    has error.
  migration: add error handling to migrate_fd_put_notify().

 buffered_file.c  |   19 +++++++++++--------
 buffered_file.h  |    2 +-
 hw/hw.h          |    4 ++--
 migration-exec.c |    6 +++---
 migration-fd.c   |    4 ++--
 migration-tcp.c  |    4 ++--
 migration-unix.c |    4 ++--
 migration.c      |   33 ++++++++++++++++++---------------
 migration.h      |    4 ++--
 savevm.c         |   27 +++++++++++++++------------
 10 files changed, 58 insertions(+), 49 deletions(-)

-- 
1.7.6.2

[Qemu-devel] [PATCH 1/7] migration: simplify state assignmente

[Juan Quintela]

Once there, make sure that if we already know that there is one error, just
call migration_fd_cleanup() with the ERROR state.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/migration.c b/migration.c
index f5959b4..9a93e3b 100644
--- a/migration.c
+++ b/migration.c
@@ -370,7 +370,6 @@ void migrate_fd_put_ready(void *opaque)

     DPRINTF("iterate\n");
     if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
-        int state;
         int old_vm_running = vm_running;

         DPRINTF("done iterating\n");
@@ -380,17 +379,17 @@ void migrate_fd_put_ready(void *opaque)
             if (old_vm_running) {
                 vm_start();
             }
-            state = MIG_STATE_ERROR;
-        } else {
-            state = MIG_STATE_COMPLETED;
+            s->state = MIG_STATE_ERROR;
         }
         if (migrate_fd_cleanup(s) < 0) {
             if (old_vm_running) {
                 vm_start();
             }
-            state = MIG_STATE_ERROR;
+            s->state = MIG_STATE_ERROR;
+        }
+        if (s->state == MIG_STATE_ACTIVE) {
+            s->state = MIG_STATE_COMPLETED;
         }
-        s->state = state;
         notifier_list_notify(&migration_state_notifiers, NULL);
     }
 }
-- 
1.7.6.2

[Qemu-devel] [PATCH 2/7] migration: only flush when there are no errors

[Juan Quintela]

If we have one error while migrating, and then we issuse a
"migrate_cancel" command, guest hang.  Fix it for flushing only when
migration is in MIG_STATE_ACTIVE.  In case of error of cancellation,
don't flush.

We had an infinite loop at buffered_close()

        while (!s->has_error && s->buffer_size) {
            buffered_flush(s);
            if (s->freeze_output)
                s->wait_for_unfreeze(s);
        }

There was no errors, there were things to send, and connection was
broken.  send() returns -EAGAIN, so we freezed output, but we
unfreeze_output and try again.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c  |   17 ++++++++++-------
 buffered_file.h  |    2 +-
 hw/hw.h          |    4 ++--
 migration-exec.c |    6 +++---
 migration-fd.c   |    4 ++--
 migration-tcp.c  |    4 ++--
 migration-unix.c |    4 ++--
 migration.c      |    6 +++---
 migration.h      |    4 ++--
 savevm.c         |   20 +++++++++++---------
 10 files changed, 38 insertions(+), 33 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 486af57..5ba3d19 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -166,20 +166,23 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
     return offset;
 }

-static int buffered_close(void *opaque)
+static int buffered_close(void *opaque, bool flush)
 {
     QEMUFileBuffered *s = opaque;
     int ret;

     DPRINTF("closing\n");

-    while (!s->has_error && s->buffer_size) {
-        buffered_flush(s);
-        if (s->freeze_output)
-            s->wait_for_unfreeze(s);
+    if (flush) {
+        while (!s->has_error && s->buffer_size) {
+            buffered_flush(s);
+            if (s->freeze_output) {
+                s->wait_for_unfreeze(s);
+            }
+        }
     }

-    ret = s->close(s->opaque);
+    ret = s->close(s->opaque, flush);

     qemu_del_timer(s->timer);
     qemu_free_timer(s->timer);
@@ -233,7 +236,7 @@ static void buffered_rate_tick(void *opaque)
     QEMUFileBuffered *s = opaque;

     if (s->has_error) {
-        buffered_close(s);
+        buffered_close(s, false);
         return;
     }

diff --git a/buffered_file.h b/buffered_file.h
index 98d358b..16162ec 100644
--- a/buffered_file.h
+++ b/buffered_file.h
@@ -19,7 +19,7 @@
 typedef ssize_t (BufferedPutFunc)(void *opaque, const void *data, size_t size);
 typedef void (BufferedPutReadyFunc)(void *opaque);
 typedef void (BufferedWaitForUnfreezeFunc)(void *opaque);
-typedef int (BufferedCloseFunc)(void *opaque);
+typedef int (BufferedCloseFunc)(void *opaque, bool flush);

 QEMUFile *qemu_fopen_ops_buffered(void *opaque, size_t xfer_limit,
                                   BufferedPutFunc *put_buffer,
diff --git a/hw/hw.h b/hw/hw.h
index a124da9..129de0e 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -28,7 +28,7 @@ typedef int (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
                                     int64_t pos, int size);

 /* Close a file and return an error code */
-typedef int (QEMUFileCloseFunc)(void *opaque);
+typedef int (QEMUFileCloseFunc)(void *opaque, bool flush);

 /* Called to determine if the file has exceeded it's bandwidth allocation.  The
  * bandwidth capping is a soft limit, not a hard limit.
@@ -55,7 +55,7 @@ QEMUFile *qemu_popen(FILE *popen_file, const char *mode);
 QEMUFile *qemu_popen_cmd(const char *command, const char *mode);
 int qemu_stdio_fd(QEMUFile *f);
 void qemu_fflush(QEMUFile *f);
-int qemu_fclose(QEMUFile *f);
+int qemu_fclose(QEMUFile *f, bool flush);
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
 void qemu_put_byte(QEMUFile *f, int v);

diff --git a/migration-exec.c b/migration-exec.c
index 2cfb6f2..5fe09e3 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -42,12 +42,12 @@ static int file_write(FdMigrationState *s, const void * buf, size_t size)
     return write(s->fd, buf, size);
 }

-static int exec_close(FdMigrationState *s)
+static int exec_close(FdMigrationState *s, bool flush)
 {
     int ret = 0;
     DPRINTF("exec_close\n");
     if (s->opaque) {
-        ret = qemu_fclose(s->opaque);
+        ret = qemu_fclose(s->opaque, flush);
         s->opaque = NULL;
         s->fd = -1;
         if (ret != -1 &&
@@ -123,7 +123,7 @@ static void exec_accept_incoming_migration(void *opaque)

     process_incoming_migration(f);
     qemu_set_fd_handler2(qemu_stdio_fd(f), NULL, NULL, NULL, NULL);
-    qemu_fclose(f);
+    qemu_fclose(f, false);
 }

 int exec_start_incoming_migration(const char *command)
diff --git a/migration-fd.c b/migration-fd.c
index aee690a..3908850 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -40,7 +40,7 @@ static int fd_write(FdMigrationState *s, const void * buf, size_t size)
     return write(s->fd, buf, size);
 }

-static int fd_close(FdMigrationState *s)
+static int fd_close(FdMigrationState *s, bool flush)
 {
     DPRINTF("fd_close\n");
     if (s->fd != -1) {
@@ -106,7 +106,7 @@ static void fd_accept_incoming_migration(void *opaque)

     process_incoming_migration(f);
     qemu_set_fd_handler2(qemu_stdio_fd(f), NULL, NULL, NULL, NULL);
-    qemu_fclose(f);
+    qemu_fclose(f, false);
 }

 int fd_start_incoming_migration(const char *infd)
diff --git a/migration-tcp.c b/migration-tcp.c
index c431e03..cb061f6 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -38,7 +38,7 @@ static int socket_write(FdMigrationState *s, const void * buf, size_t size)
     return send(s->fd, buf, size, 0);
 }

-static int tcp_close(FdMigrationState *s)
+static int tcp_close(FdMigrationState *s, bool flush)
 {
     DPRINTF("tcp_close\n");
     if (s->fd != -1) {
@@ -160,7 +160,7 @@ static void tcp_accept_incoming_migration(void *opaque)
     }

     process_incoming_migration(f);
-    qemu_fclose(f);
+    qemu_fclose(f, false);
 out:
     close(c);
 out2:
diff --git a/migration-unix.c b/migration-unix.c
index 6dc985d..bafb855 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -38,7 +38,7 @@ static int unix_write(FdMigrationState *s, const void * buf, size_t size)
     return write(s->fd, buf, size);
 }

-static int unix_close(FdMigrationState *s)
+static int unix_close(FdMigrationState *s, bool flush)
 {
     DPRINTF("unix_close\n");
     if (s->fd != -1) {
@@ -168,7 +168,7 @@ static void unix_accept_incoming_migration(void *opaque)
     }

     process_incoming_migration(f);
-    qemu_fclose(f);
+    qemu_fclose(f, false);
 out:
     qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
     close(s);
diff --git a/migration.c b/migration.c
index 9a93e3b..15d001e 100644
--- a/migration.c
+++ b/migration.c
@@ -288,7 +288,7 @@ int migrate_fd_cleanup(FdMigrationState *s)

     if (s->file) {
         DPRINTF("closing file\n");
-        if (qemu_fclose(s->file) != 0) {
+        if (qemu_fclose(s->file, s->state == MIG_STATE_ACTIVE) != 0) {
             ret = -1;
         }
         s->file = NULL;
@@ -449,7 +449,7 @@ void migrate_fd_wait_for_unfreeze(void *opaque)
     } while (ret == -1 && (s->get_error(s)) == EINTR);
 }

-int migrate_fd_close(void *opaque)
+int migrate_fd_close(void *opaque, bool flush)
 {
     FdMigrationState *s = opaque;

@@ -457,7 +457,7 @@ int migrate_fd_close(void *opaque)
         monitor_resume(s->mon);
     }
     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-    return s->close(s);
+    return s->close(s, flush);
 }

 void add_migration_state_change_notifier(Notifier *notify)
diff --git a/migration.h b/migration.h
index 050c56c..27a1f25 100644
--- a/migration.h
+++ b/migration.h
@@ -46,7 +46,7 @@ struct FdMigrationState
     Monitor *mon;
     int state;
     int (*get_error)(struct FdMigrationState*);
-    int (*close)(struct FdMigrationState*);
+    int (*close)(struct FdMigrationState*, bool flush);
     int (*write)(struct FdMigrationState*, const void *, size_t);
     void *opaque;
 };
@@ -128,7 +128,7 @@ void migrate_fd_release(MigrationState *mig_state);

 void migrate_fd_wait_for_unfreeze(void *opaque);

-int migrate_fd_close(void *opaque);
+int migrate_fd_close(void *opaque, bool flush);

 static inline FdMigrationState *migrate_to_fms(MigrationState *mig_state)
 {
diff --git a/savevm.c b/savevm.c
index 1feaa70..a793137 100644
--- a/savevm.c
+++ b/savevm.c
@@ -203,7 +203,7 @@ static int socket_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
     return len;
 }

-static int socket_close(void *opaque)
+static int socket_close(void *opaque, bool flush)
 {
     QEMUFileSocket *s = opaque;
     g_free(s);
@@ -229,7 +229,7 @@ static int stdio_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
     return bytes;
 }

-static int stdio_pclose(void *opaque)
+static int stdio_pclose(void *opaque, bool flush)
 {
     QEMUFileStdio *s = opaque;
     int ret;
@@ -238,7 +238,7 @@ static int stdio_pclose(void *opaque)
     return ret;
 }

-static int stdio_fclose(void *opaque)
+static int stdio_fclose(void *opaque, bool flush)
 {
     QEMUFileStdio *s = opaque;
     fclose(s->stdio_file);
@@ -389,7 +389,7 @@ static int block_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
     return bdrv_load_vmstate(opaque, buf, pos, size);
 }

-static int bdrv_fclose(void *opaque)
+static int bdrv_fclose(void *opaque, bool flush)
 {
     return 0;
 }
@@ -471,12 +471,14 @@ static void qemu_fill_buffer(QEMUFile *f)
         f->has_error = 1;
 }

-int qemu_fclose(QEMUFile *f)
+int qemu_fclose(QEMUFile *f, bool flush)
 {
     int ret = 0;
-    qemu_fflush(f);
+    if (flush) {
+        qemu_fflush(f);
+    }
     if (f->close)
-        ret = f->close(f->opaque);
+        ret = f->close(f->opaque, flush);
     g_free(f);
     return ret;
 }
@@ -1980,7 +1982,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     }
     ret = qemu_savevm_state(mon, f);
     vm_state_size = qemu_ftell(f);
-    qemu_fclose(f);
+    qemu_fclose(f, true);
     if (ret < 0) {
         monitor_printf(mon, "Error %d while writing VM\n", ret);
         goto the_end;
@@ -2077,7 +2079,7 @@ int load_vmstate(const char *name)
     qemu_system_reset(VMRESET_SILENT);
     ret = qemu_loadvm_state(f);

-    qemu_fclose(f);
+    qemu_fclose(f, false);
     if (ret < 0) {
         error_report("Error %d while loading VM state", ret);
         return ret;
-- 
1.7.6.2

[Qemu-devel] [PATCH 3/7] migration: Check that migration is active before cancel it

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration.c b/migration.c
index 15d001e..c56d29c 100644
--- a/migration.c
+++ b/migration.c
@@ -132,9 +132,9 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     MigrationState *s = current_migration;

-    if (s)
+    if (s && s->get_status(s) == MIG_STATE_ACTIVE) {
         s->cancel(s);
-
+    }
     return 0;
 }

-- 
1.7.6.2

[Qemu-devel] [PATCH 4/7] savevm: avoid qemu_savevm_state_iterate() to return 1 when qemu file has error.

[Juan Quintela]

From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>

When qemu on the receiver gets killed during live migration, if debug
is turned on, migrate_fd_put_ready() says,

migration: done iterating

and proceeds.  The reason was qemu_savevm_state_iterate() returning 1
even when qemu file has error.  This patch checks
qemu_file_has_error() before returning 1/0, and avoids
migrate_fd_put_ready() to proceed in case of error.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/savevm.c b/savevm.c
index a793137..af0d0e7 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1531,14 +1531,15 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
         }
     }

-    if (ret)
-        return 1;
-
     if (qemu_file_has_error(f)) {
         qemu_savevm_state_cancel(mon, f);
         return -EIO;
     }

+    if (ret) {
+        return 1;
+    }
+
     return 0;
 }

-- 
1.7.6.2

[Qemu-devel] [PATCH 5/7] migration: add error handling to migrate_fd_put_notify().

[Juan Quintela]

From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>

Although migrate_fd_put_buffer() sets MIG_STATE_ERROR if it failed,
since migrate_fd_put_notify() isn't checking error of underlying
QEMUFile, those resources are kept open.  This patch checks it and
calls migrate_fd_error() in case of error.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration.c b/migration.c
index c56d29c..7f8928a 100644
--- a/migration.c
+++ b/migration.c
@@ -312,6 +312,9 @@ void migrate_fd_put_notify(void *opaque)

     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
     qemu_file_put_notify(s->file);
+    if (qemu_file_has_error(s->file)) {
+        migrate_fd_error(s);
+    }
 }

 ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
@@ -328,9 +331,6 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)

     if (ret == -EAGAIN) {
         qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_fd_put_notify, s);
-    } else if (ret < 0) {
-        s->state = MIG_STATE_ERROR;
-        notifier_list_notify(&migration_state_notifiers, NULL);
     }

     return ret;
-- 
1.7.6.2

[Qemu-devel] [PATCH 6/7] migration: If there is one error, it makes no sense to continue

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 5ba3d19..10d14f9 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -197,7 +197,7 @@ static int buffered_rate_limit(void *opaque)
     QEMUFileBuffered *s = opaque;

     if (s->has_error)
-        return 0;
+        return -1;

     if (s->freeze_output)
         return 1;
-- 
1.7.6.2

[Qemu-devel] [PATCH 7/7] migration: qemu_savevm_iterate has three return values

[Juan Quintela]

We were retrying when there was one error, entering a loop.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/migration.c b/migration.c
index 7f8928a..0baed23 100644
--- a/migration.c
+++ b/migration.c
@@ -362,6 +362,7 @@ void migrate_fd_connect(FdMigrationState *s)
 void migrate_fd_put_ready(void *opaque)
 {
     FdMigrationState *s = opaque;
+    int ret;

     if (s->state != MIG_STATE_ACTIVE) {
         DPRINTF("put_ready returning because of non-active state\n");
@@ -369,7 +370,10 @@ void migrate_fd_put_ready(void *opaque)
     }

     DPRINTF("iterate\n");
-    if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
+    ret = qemu_savevm_state_iterate(s->mon, s->file);
+    if (ret == -1) {
+        migrate_fd_error(s);
+    } else if (ret == 1) {
         int old_vm_running = vm_running;

         DPRINTF("done iterating\n");
-- 
1.7.6.2

Re: [Qemu-devel] [PATCH 2/7] migration: only flush when there are no errors

[Daniel P. Berrange]

On Tue, Sep 20, 2011 at 03:24:41PM +0200, Juan Quintela wrote:
> If we have one error while migrating, and then we issuse a
> "migrate_cancel" command, guest hang.  Fix it for flushing only when
> migration is in MIG_STATE_ACTIVE.  In case of error of cancellation,
> don't flush.
> 
> We had an infinite loop at buffered_close()
> 
>         while (!s->has_error && s->buffer_size) {
>             buffered_flush(s);
>             if (s->freeze_output)
>                 s->wait_for_unfreeze(s);
>         }
> 
> There was no errors, there were things to send, and connection was
> broken.  send() returns -EAGAIN, so we freezed output, but we
> unfreeze_output and try again.

I posted a couple of alternative approaches to fixing this
hang problem

http://lists.nongnu.org/archive/html/qemu-devel/2011-08/msg03248.html

My second approach of checking the migration state in migrate_fd_put_buffer()
seems like it would be worthwhile, even with your patch as an additional
safety net against bad code.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 2/7] migration: only flush when there are no errors

[Juan Quintela]

"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Tue, Sep 20, 2011 at 03:24:41PM +0200, Juan Quintela wrote:
>> If we have one error while migrating, and then we issuse a
>> "migrate_cancel" command, guest hang.  Fix it for flushing only when
>> migration is in MIG_STATE_ACTIVE.  In case of error of cancellation,
>> don't flush.
>> 
>> We had an infinite loop at buffered_close()
>> 
>>         while (!s->has_error && s->buffer_size) {
>>             buffered_flush(s);
>>             if (s->freeze_output)
>>                 s->wait_for_unfreeze(s);
>>         }
>> 
>> There was no errors, there were things to send, and connection was
>> broken.  send() returns -EAGAIN, so we freezed output, but we
>> unfreeze_output and try again.
>
> I posted a couple of alternative approaches to fixing this
> hang problem
>
> http://lists.nongnu.org/archive/html/qemu-devel/2011-08/msg03248.html
>
> My second approach of checking the migration state in migrate_fd_put_buffer()
> seems like it would be worthwhile, even with your patch as an additional
> safety net against bad code.

We can add that there, but in my tests, the s->write() was returning
correctly an error (or -EAGAIN).  The problem was that we were not
exiting when we didn't needed to.

I agree that we can have *both* tests.  I will add your patch to my
series.

Thanks for the fast review.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/7] migration: only flush when there are no errors

[Paolo Bonzini]

On 09/20/2011 03:24 PM, Juan Quintela wrote:
> If we have one error while migrating, and then we issuse a
> "migrate_cancel" command, guest hang.  Fix it for flushing only when
> migration is in MIG_STATE_ACTIVE.  In case of error of cancellation,
> don't flush.
>
> We had an infinite loop at buffered_close()
>
>          while (!s->has_error && s->buffer_size) {
>              buffered_flush(s);
>              if (s->freeze_output)
>                  s->wait_for_unfreeze(s);
>          }
>
> There was no errors, there were things to send, and connection was
> broken.  send() returns -EAGAIN, so we freezed output, but we
> unfreeze_output and try again.
>
> Signed-off-by: Juan Quintela<quintela@redhat.com>

I don't like the idea of adding an extra argument to fix a bug 
elsewhere.  I don't consider this even a safety net, since it is relying 
anyway on the migration code setting the new argument correctly:

diff --git a/migration.c b/migration.c
index 9a93e3b..15d001e 100644
--- a/migration.c
+++ b/migration.c
@@ -288,7 +288,7 @@ int migrate_fd_cleanup(FdMigrationState *s)

      if (s->file) {
          DPRINTF("closing file\n");
-        if (qemu_fclose(s->file) != 0) {
+        if (qemu_fclose(s->file, s->state == MIG_STATE_ACTIVE) != 0) {
              ret = -1;
          }
          s->file = NULL;

Dan's patch is the right fix, this one can be dropped altogether.

If anything, you may consider making wait_for_unfreeze return an error 
code, and set has_error when wait_for_unfreeze returns an error. 
Alternatively, merge QEMUFile and BufferedFile's has_error flags, and 
use qemu_file_set_error when migration is canceled.

Paolo

Re: [Qemu-devel] [PATCH 5/7] migration: add error handling to migrate_fd_put_notify().

[Paolo Bonzini]

On 09/20/2011 03:24 PM, Juan Quintela wrote:
>
> diff --git a/migration.c b/migration.c
> index c56d29c..7f8928a 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -312,6 +312,9 @@ void migrate_fd_put_notify(void *opaque)
>
>       qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>       qemu_file_put_notify(s->file);
> +    if (qemu_file_has_error(s->file)) {
> +        migrate_fd_error(s);
> +    }

Note this is testing QEMUFile's has_error, not QEMUBufferedFile's. 
Merging the two flags would be a good idea.

Paolo

Re: [Qemu-devel] [PATCH 7/7] migration: qemu_savevm_iterate has three return values

[Wayne Xia]

于 2011-9-20 21:24, Juan Quintela 写道:
> We were retrying when there was one error, entering a loop.
> 
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> ---
>   migration.c |    6 +++++-
>   1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 7f8928a..0baed23 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -362,6 +362,7 @@ void migrate_fd_connect(FdMigrationState *s)
>   void migrate_fd_put_ready(void *opaque)
>   {
>       FdMigrationState *s = opaque;
> +    int ret;
> 
>       if (s->state != MIG_STATE_ACTIVE) {
>           DPRINTF("put_ready returning because of non-active state\n");
> @@ -369,7 +370,10 @@ void migrate_fd_put_ready(void *opaque)
>       }
> 
>       DPRINTF("iterate\n");
> -    if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
> +    ret = qemu_savevm_state_iterate(s->mon, s->file);
> +    if (ret == -1) {
> +        migrate_fd_error(s);
> +    } else if (ret == 1) {
>           int old_vm_running = vm_running;
> 
>           DPRINTF("done iterating\n");
Maybe macro could be used in the situation that more than 2 possible
types of value may return.


-- 
Best Regards

Wayne Xia
mail:xiawenc@linux.vnet.ibm.com
tel:86-010-82450803