[Qemu-devel] [PATCH v2 00/11] Handle errors during migration

[Qemu-devel] [PATCH v2 00/11] Handle errors during migration

[Juan Quintela]

Hi

[v2]
- dropped migration_flush patch.  Substituted by
  migration: don't write when migration is not active (based on danp suggestion)
- we were using wrong "opaque"
- merge has_error field (Paolo suggested to handle both)
- has_error now contains errno values

Later, Juan.

[v1]
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 (10):
  ds1225y: Use stdio instead of QEMUFile
  migration: simplify state assignmente
  migration: Check that migration is active before cancel it
  migration: return real error code
  migration: If there is one error, it makes no sense to continue
  buffered_file: Use right "opaque"
  buffered_file: reuse QEMUFile has_error field
  migration: don't "write" when migration is not active
  migration: set error if select return one error
  migration: change has_error to contain errno values

Yoshiaki Tamura (1):
  migration: add error handling to migrate_fd_put_notify().

 arch_init.c       |    2 +-
 block-migration.c |    6 +++---
 buffered_file.c   |   23 +++++++++++------------
 hw/ds1225y.c      |   28 ++++++++++++++++------------
 hw/hw.h           |    2 +-
 migration.c       |   35 ++++++++++++++++++++++-------------
 savevm.c          |   41 +++++++++++++++++++----------------------
 7 files changed, 73 insertions(+), 64 deletions(-)

-- 
1.7.6.2

[Qemu-devel] [PATCH 01/11] ds1225y: Use stdio instead of QEMUFile

[Juan Quintela]

QEMUFile * is only intended for migration nowadays.  Using it for
anything else just adds pain and a layer of buffers for no good
reason.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ds1225y.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/ds1225y.c b/hw/ds1225y.c
index 9875c44..6852a61 100644
--- a/hw/ds1225y.c
+++ b/hw/ds1225y.c
@@ -29,7 +29,7 @@ typedef struct {
     DeviceState qdev;
     uint32_t chip_size;
     char *filename;
-    QEMUFile *file;
+    FILE *file;
     uint8_t *contents;
 } NvRamState;

@@ -70,9 +70,9 @@ static void nvram_writeb (void *opaque, target_phys_addr_t addr, uint32_t val)

     s->contents[addr] = val;
     if (s->file) {
-        qemu_fseek(s->file, addr, SEEK_SET);
-        qemu_put_byte(s->file, (int)val);
-        qemu_fflush(s->file);
+        fseek(s->file, addr, SEEK_SET);
+        fputc(val, s->file);
+        fflush(s->file);
     }
 }

@@ -108,15 +108,17 @@ static int nvram_post_load(void *opaque, int version_id)

     /* Close file, as filename may has changed in load/store process */
     if (s->file) {
-        qemu_fclose(s->file);
+        fclose(s->file);
     }

     /* Write back nvram contents */
-    s->file = qemu_fopen(s->filename, "wb");
+    s->file = fopen(s->filename, "wb");
     if (s->file) {
         /* Write back contents, as 'wb' mode cleaned the file */
-        qemu_put_buffer(s->file, s->contents, s->chip_size);
-        qemu_fflush(s->file);
+        if (fwrite(s->contents, s->chip_size, 1, s->file) != 1) {
+            printf("nvram_post_load: short write\n");
+        }
+        fflush(s->file);
     }

     return 0;
@@ -143,7 +145,7 @@ typedef struct {
 static int nvram_sysbus_initfn(SysBusDevice *dev)
 {
     NvRamState *s = &FROM_SYSBUS(SysBusNvRamState, dev)->nvram;
-    QEMUFile *file;
+    FILE *file;
     int s_io;

     s->contents = g_malloc0(s->chip_size);
@@ -153,11 +155,13 @@ static int nvram_sysbus_initfn(SysBusDevice *dev)
     sysbus_init_mmio(dev, s->chip_size, s_io);

     /* Read current file */
-    file = qemu_fopen(s->filename, "rb");
+    file = fopen(s->filename, "rb");
     if (file) {
         /* Read nvram contents */
-        qemu_get_buffer(file, s->contents, s->chip_size);
-        qemu_fclose(file);
+        if (fread(s->contents, s->chip_size, 1, file) != 1) {
+            printf("nvram_sysbus_initfn: short read\n");
+        }
+        fclose(file);
     }
     nvram_post_load(s, 0);

-- 
1.7.6.2

[Qemu-devel] [PATCH 02/11] 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, 4 insertions(+), 7 deletions(-)

diff --git a/migration.c b/migration.c
index 7dd8f4e..531fe83 100644
--- a/migration.c
+++ b/migration.c
@@ -371,7 +371,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 = runstate_is_running();

         DPRINTF("done iterating\n");
@@ -381,20 +380,18 @@ 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 (state == MIG_STATE_COMPLETED) {
+        if (s->state == MIG_STATE_ACTIVE) {
+            s->state = MIG_STATE_COMPLETED;
             runstate_set(RSTATE_POST_MIGRATE);
         }
-        s->state = state;
         notifier_list_notify(&migration_state_notifiers, NULL);
     }
 }
-- 
1.7.6.2

[Qemu-devel] [PATCH 03/11] 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 531fe83..ea7bcc8 100644
--- a/migration.c
+++ b/migration.c
@@ -133,9 +133,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 04/11] migration: return real error code

[Juan Quintela]

make functions propaget errno, instead of just using -EIO.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    6 +++++-
 savevm.c    |   33 +++++++++++++++------------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/migration.c b/migration.c
index ea7bcc8..9498e20 100644
--- a/migration.c
+++ b/migration.c
@@ -363,6 +363,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");
@@ -370,7 +371,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 < 0) {
+        migrate_fd_error(s);
+    } else if (ret == 1) {
         int old_vm_running = runstate_is_running();

         DPRINTF("done iterating\n");
diff --git a/savevm.c b/savevm.c
index 46f2447..c382076 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1466,6 +1466,7 @@ int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
                             int shared)
 {
     SaveStateEntry *se;
+    int ret;

     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         if(se->set_params == NULL) {
@@ -1497,13 +1498,13 @@ int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,

         se->save_live_state(mon, f, QEMU_VM_SECTION_START, se->opaque);
     }
-
-    if (qemu_file_has_error(f)) {
+    ret = qemu_file_has_error(f);
+    if (ret != 0) {
         qemu_savevm_state_cancel(mon, f);
-        return -EIO;
     }

-    return 0;
+    return ret;
+
 }

 int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
@@ -1528,16 +1529,14 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
             break;
         }
     }
-
-    if (ret)
-        return 1;
-
-    if (qemu_file_has_error(f)) {
+    if (ret != 0) {
+        return ret;
+    }
+    ret = qemu_file_has_error(f);
+    if (ret != 0) {
         qemu_savevm_state_cancel(mon, f);
-        return -EIO;
     }
-
-    return 0;
+    return ret;
 }

 int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
@@ -1580,10 +1579,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)

     qemu_put_byte(f, QEMU_VM_EOF);

-    if (qemu_file_has_error(f))
-        return -EIO;
-
-    return 0;
+    return qemu_file_has_error(f);
 }

 void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f)
@@ -1623,8 +1619,9 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
     ret = qemu_savevm_state_complete(mon, f);

 out:
-    if (qemu_file_has_error(f))
-        ret = -EIO;
+    if (ret == 0) {
+        ret = qemu_file_has_error(f);
+    }

     if (!ret && saved_vm_running)
         vm_start();
-- 
1.7.6.2

[Qemu-devel] [PATCH 05/11] 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 9498e20..0b284ff 100644
--- a/migration.c
+++ b/migration.c
@@ -313,6 +313,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)
@@ -329,9 +332,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 06/11] 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 |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 486af57..bcdf04f 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -193,9 +193,9 @@ static int buffered_rate_limit(void *opaque)
 {
     QEMUFileBuffered *s = opaque;

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

-- 
1.7.6.2

[Qemu-devel] [PATCH 07/11] buffered_file: Use right "opaque"

[Juan Quintela]

buffered_close 's' variable is of type QEMUFileBuffered, and
wait_for_unfreeze() expect to receive a MigrationState, that
'coincidentaly' is s->opaque.

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 bcdf04f..701b440 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -176,7 +176,7 @@ static int buffered_close(void *opaque)
     while (!s->has_error && s->buffer_size) {
         buffered_flush(s);
         if (s->freeze_output)
-            s->wait_for_unfreeze(s);
+            s->wait_for_unfreeze(s->opaque);
     }

     ret = s->close(s->opaque);
-- 
1.7.6.2

[Qemu-devel] [PATCH 08/11] buffered_file: reuse QEMUFile has_error field

[Juan Quintela]

Instead of having two has_error fields in QEMUFile & QEMUBufferedFile, reuse
the 1st one.  Notice that the one in buffered_file is only set after a
file operation.

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

diff --git a/buffered_file.c b/buffered_file.c
index 701b440..3dadec0 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -27,7 +27,6 @@ typedef struct QEMUFileBuffered
     BufferedCloseFunc *close;
     void *opaque;
     QEMUFile *file;
-    int has_error;
     int freeze_output;
     size_t bytes_xfer;
     size_t xfer_limit;
@@ -73,7 +72,7 @@ static void buffered_flush(QEMUFileBuffered *s)
 {
     size_t offset = 0;

-    if (s->has_error) {
+    if (qemu_file_has_error(s->file)) {
         DPRINTF("flush when error, bailing\n");
         return;
     }
@@ -93,7 +92,7 @@ static void buffered_flush(QEMUFileBuffered *s)

         if (ret <= 0) {
             DPRINTF("error flushing data, %zd\n", ret);
-            s->has_error = 1;
+            qemu_file_set_error(s->file);
             break;
         } else {
             DPRINTF("flushed %zd byte(s)\n", ret);
@@ -114,7 +113,7 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in

     DPRINTF("putting %d bytes at %" PRId64 "\n", size, pos);

-    if (s->has_error) {
+    if (qemu_file_has_error(s->file)) {
         DPRINTF("flush when error, bailing\n");
         return -EINVAL;
     }
@@ -139,7 +138,7 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in

         if (ret <= 0) {
             DPRINTF("error putting\n");
-            s->has_error = 1;
+            qemu_file_set_error(s->file);
             offset = -EINVAL;
             break;
         }
@@ -173,7 +172,7 @@ static int buffered_close(void *opaque)

     DPRINTF("closing\n");

-    while (!s->has_error && s->buffer_size) {
+    while (!qemu_file_has_error(s->file) && s->buffer_size) {
         buffered_flush(s);
         if (s->freeze_output)
             s->wait_for_unfreeze(s->opaque);
@@ -193,7 +192,7 @@ static int buffered_rate_limit(void *opaque)
 {
     QEMUFileBuffered *s = opaque;

-    if (s->has_error) {
+    if (qemu_file_has_error(s->file)) {
         return 1;
     }
     if (s->freeze_output)
@@ -208,7 +207,7 @@ static int buffered_rate_limit(void *opaque)
 static int64_t buffered_set_rate_limit(void *opaque, int64_t new_rate)
 {
     QEMUFileBuffered *s = opaque;
-    if (s->has_error)
+    if (qemu_file_has_error(s->file))
         goto out;

     if (new_rate > SIZE_MAX) {
@@ -232,7 +231,7 @@ static void buffered_rate_tick(void *opaque)
 {
     QEMUFileBuffered *s = opaque;

-    if (s->has_error) {
+    if (qemu_file_has_error(s->file)) {
         buffered_close(s);
         return;
     }
-- 
1.7.6.2

[Qemu-devel] [PATCH 09/11] migration: don't "write" when migration is not active

[Juan Quintela]

If migration is not active, just ignore writes.

[Based on Daniel Berrange suggestion]

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

diff --git a/migration.c b/migration.c
index 0b284ff..755b96b 100644
--- a/migration.c
+++ b/migration.c
@@ -323,6 +323,10 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
     FdMigrationState *s = opaque;
     ssize_t ret;

+    if (s->state != MIG_STATE_ACTIVE) {
+        return -EIO;
+    }
+
     do {
         ret = s->write(s, data, size);
     } while (ret == -1 && ((s->get_error(s)) == EINTR));
-- 
1.7.6.2

[Qemu-devel] [PATCH 10/11] migration: set error if select return one error

[Juan Quintela]

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

diff --git a/migration.c b/migration.c
index 755b96b..9265b16 100644
--- a/migration.c
+++ b/migration.c
@@ -457,6 +457,10 @@ void migrate_fd_wait_for_unfreeze(void *opaque)

         ret = select(s->fd + 1, NULL, &wfds, NULL, NULL);
     } while (ret == -1 && (s->get_error(s)) == EINTR);
+
+    if (ret == -1) {
+        qemu_file_set_error(s->file);
+    }
 }

 int migrate_fd_close(void *opaque)
-- 
1.7.6.2

[Qemu-devel] [PATCH 11/11] migration: change has_error to contain errno values

[Juan Quintela]

We normally already have an errno value.  When not, abuse EINVAL.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c       |    2 +-
 block-migration.c |    6 +++---
 buffered_file.c   |    4 ++--
 hw/hw.h           |    2 +-
 migration.c       |    2 +-
 savevm.c          |    8 ++++----
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 9a5a0e3..12d9c8b 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -263,7 +263,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
     }

     if (cpu_physical_sync_dirty_bitmap(0, TARGET_PHYS_ADDR_MAX) != 0) {
-        qemu_file_set_error(f);
+        qemu_file_set_error(f, -EINVAL);
         return 0;
     }

diff --git a/block-migration.c b/block-migration.c
index e2775ee..2638f51 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -263,7 +263,7 @@ static int mig_save_device_bulk(Monitor *mon, QEMUFile *f,

 error:
     monitor_printf(mon, "Error reading sector %" PRId64 "\n", cur_sector);
-    qemu_file_set_error(f);
+    qemu_file_set_error(f, -EINVAL);
     g_free(blk->buf);
     g_free(blk);
     return 0;
@@ -439,7 +439,7 @@ static int mig_save_device_dirty(Monitor *mon, QEMUFile *f,

 error:
     monitor_printf(mon, "Error reading sector %" PRId64 "\n", sector);
-    qemu_file_set_error(f);
+    qemu_file_set_error(f, -EINVAL);
     g_free(blk->buf);
     g_free(blk);
     return 0;
@@ -473,7 +473,7 @@ static void flush_blks(QEMUFile* f)
             break;
         }
         if (blk->ret < 0) {
-            qemu_file_set_error(f);
+            qemu_file_set_error(f, -EINVAL);
             break;
         }
         blk_send(f, blk);
diff --git a/buffered_file.c b/buffered_file.c
index 3dadec0..3e5333c 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -92,7 +92,7 @@ static void buffered_flush(QEMUFileBuffered *s)

         if (ret <= 0) {
             DPRINTF("error flushing data, %zd\n", ret);
-            qemu_file_set_error(s->file);
+            qemu_file_set_error(s->file, ret);
             break;
         } else {
             DPRINTF("flushed %zd byte(s)\n", ret);
@@ -138,7 +138,7 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in

         if (ret <= 0) {
             DPRINTF("error putting\n");
-            qemu_file_set_error(s->file);
+            qemu_file_set_error(s->file, ret);
             offset = -EINVAL;
             break;
         }
diff --git a/hw/hw.h b/hw/hw.h
index a124da9..6cf8cd2 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -86,7 +86,7 @@ int qemu_file_rate_limit(QEMUFile *f);
 int64_t qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
 int64_t qemu_file_get_rate_limit(QEMUFile *f);
 int qemu_file_has_error(QEMUFile *f);
-void qemu_file_set_error(QEMUFile *f);
+void qemu_file_set_error(QEMUFile *f, int error);

 /* Try to send any outstanding data.  This function is useful when output is
  * halted due to rate limiting or EAGAIN errors occur as it can be used to
diff --git a/migration.c b/migration.c
index 9265b16..08688aa 100644
--- a/migration.c
+++ b/migration.c
@@ -459,7 +459,7 @@ void migrate_fd_wait_for_unfreeze(void *opaque)
     } while (ret == -1 && (s->get_error(s)) == EINTR);

     if (ret == -1) {
-        qemu_file_set_error(s->file);
+        qemu_file_set_error(s->file, s->get_error(s));
     }
 }

diff --git a/savevm.c b/savevm.c
index c382076..9199c24 100644
--- a/savevm.c
+++ b/savevm.c
@@ -430,9 +430,9 @@ int qemu_file_has_error(QEMUFile *f)
     return f->has_error;
 }

-void qemu_file_set_error(QEMUFile *f)
+void qemu_file_set_error(QEMUFile *f, int ret)
 {
-    f->has_error = 1;
+    f->has_error = ret;
 }

 void qemu_fflush(QEMUFile *f)
@@ -447,7 +447,7 @@ void qemu_fflush(QEMUFile *f)
         if (len > 0)
             f->buf_offset += f->buf_index;
         else
-            f->has_error = 1;
+            f->has_error = -EINVAL;
         f->buf_index = 0;
     }
 }
@@ -468,7 +468,7 @@ static void qemu_fill_buffer(QEMUFile *f)
         f->buf_size = len;
         f->buf_offset += len;
     } else if (len != -EAGAIN)
-        f->has_error = 1;
+        f->has_error = len;
 }

 int qemu_fclose(QEMUFile *f)
-- 
1.7.6.2

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

[Paolo Bonzini]

On 09/23/2011 02:50 PM, Juan Quintela wrote:
> 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 9498e20..0b284ff 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -313,6 +313,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)
> @@ -329,9 +332,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;

Why not leave both (or even better, call migrate_fd_error in the else 
branch)?

Paolo

Re: [Qemu-devel] [PATCH 09/11] migration: don't "write" when migration is not active

[Paolo Bonzini]

On 09/23/2011 02:50 PM, Juan Quintela wrote:
> If migration is not active, just ignore writes.
>
> [Based on Daniel Berrange suggestion]
>
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> ---
>   migration.c |    4 ++++
>   1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 0b284ff..755b96b 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -323,6 +323,10 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
>       FdMigrationState *s = opaque;
>       ssize_t ret;
>
> +    if (s->state != MIG_STATE_ACTIVE) {
> +        return -EIO;
> +    }
> +
>       do {
>           ret = s->write(s, data, size);
>       } while (ret == -1&&  ((s->get_error(s)) == EINTR));

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 04/11] migration: return real error code

[Anthony Liguori]

On 09/23/2011 07:50 AM, Juan Quintela wrote:
> make functions propaget errno, instead of just using -EIO.
>
> Signed-off-by: Juan Quintela<quintela@redhat.com>

qemu_file_has_error() implies a boolean response.  Wouldn't 
qemu_file_get_error() make more sense if you're going to rely on the return value?

Regards,

Anthony Liguori

> ---
>   migration.c |    6 +++++-
>   savevm.c    |   33 +++++++++++++++------------------
>   2 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index ea7bcc8..9498e20 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -363,6 +363,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");
> @@ -370,7 +371,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<  0) {
> +        migrate_fd_error(s);
> +    } else if (ret == 1) {
>           int old_vm_running = runstate_is_running();
>
>           DPRINTF("done iterating\n");
> diff --git a/savevm.c b/savevm.c
> index 46f2447..c382076 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1466,6 +1466,7 @@ int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
>                               int shared)
>   {
>       SaveStateEntry *se;
> +    int ret;
>
>       QTAILQ_FOREACH(se,&savevm_handlers, entry) {
>           if(se->set_params == NULL) {
> @@ -1497,13 +1498,13 @@ int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
>
>           se->save_live_state(mon, f, QEMU_VM_SECTION_START, se->opaque);
>       }
> -
> -    if (qemu_file_has_error(f)) {
> +    ret = qemu_file_has_error(f);
> +    if (ret != 0) {
>           qemu_savevm_state_cancel(mon, f);
> -        return -EIO;
>       }
>
> -    return 0;
> +    return ret;
> +
>   }
>
>   int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
> @@ -1528,16 +1529,14 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
>               break;
>           }
>       }
> -
> -    if (ret)
> -        return 1;
> -
> -    if (qemu_file_has_error(f)) {
> +    if (ret != 0) {
> +        return ret;
> +    }
> +    ret = qemu_file_has_error(f);
> +    if (ret != 0) {
>           qemu_savevm_state_cancel(mon, f);
> -        return -EIO;
>       }
> -
> -    return 0;
> +    return ret;
>   }
>
>   int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> @@ -1580,10 +1579,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
>
>       qemu_put_byte(f, QEMU_VM_EOF);
>
> -    if (qemu_file_has_error(f))
> -        return -EIO;
> -
> -    return 0;
> +    return qemu_file_has_error(f);
>   }
>
>   void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f)
> @@ -1623,8 +1619,9 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
>       ret = qemu_savevm_state_complete(mon, f);
>
>   out:
> -    if (qemu_file_has_error(f))
> -        ret = -EIO;
> +    if (ret == 0) {
> +        ret = qemu_file_has_error(f);
> +    }
>
>       if (!ret&&  saved_vm_running)
>           vm_start();

Re: [Qemu-devel] [PATCH 04/11] migration: return real error code

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 09/23/2011 07:50 AM, Juan Quintela wrote:
>> make functions propaget errno, instead of just using -EIO.
>>
>> Signed-off-by: Juan Quintela<quintela@redhat.com>
>
> qemu_file_has_error() implies a boolean response.  Wouldn't
> qemu_file_get_error() make more sense if you're going to rely on the
> return value?

I just didn't want to change more things on the same patch.
I can add a patch on top of this series?

Later, Juan.

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

[Juan Quintela]

(Adding new Yoshiaki email address)

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 09/23/2011 02:50 PM, Juan Quintela wrote:
>> 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 9498e20..0b284ff 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -313,6 +313,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)
>> @@ -329,9 +332,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;
>
> Why not leave both (or even better, call migrate_fd_error in the else
> branch)?

In the big scheme of things, it don't matter.  migration_fd_put_buffer()
still returns the errno.  it is only called from buffered_put_buffer()
and buffered_flush(), both check the error handling and setup
s->has_error correctly (after this series).

Yoshi, do you remember better?

Later, Juan.

Re: [Qemu-devel] [PATCH 04/11] migration: return real error code

[Anthony Liguori]

On 10/04/2011 01:35 PM, Juan Quintela wrote:
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 09/23/2011 07:50 AM, Juan Quintela wrote:
>>> make functions propaget errno, instead of just using -EIO.
>>>
>>> Signed-off-by: Juan Quintela<quintela@redhat.com>
>>
>> qemu_file_has_error() implies a boolean response.  Wouldn't
>> qemu_file_get_error() make more sense if you're going to rely on the
>> return value?
>
> I just didn't want to change more things on the same patch.
> I can add a patch on top of this series?

It's terribly odd to make a function that looks like a bool return a non-boolean 
value.  It can't be that much of a change to do it in this patch, it's just a 
matter of running sed.

Regards,

Anthony Liguori

>
> Later, Juan.

Re: [Qemu-devel] [PATCH 04/11] migration: return real error code

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 10/04/2011 01:35 PM, Juan Quintela wrote:
>> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>> On 09/23/2011 07:50 AM, Juan Quintela wrote:
>>>> make functions propaget errno, instead of just using -EIO.
>>>>
>>>> Signed-off-by: Juan Quintela<quintela@redhat.com>
>>>
>>> qemu_file_has_error() implies a boolean response.  Wouldn't
>>> qemu_file_get_error() make more sense if you're going to rely on the
>>> return value?
>>
>> I just didn't want to change more things on the same patch.
>> I can add a patch on top of this series?
>
> It's terribly odd to make a function that looks like a bool return a
> non-boolean value.  It can't be that much of a change to do it in this
> patch, it's just a matter of running sed.

That is already fixed.  Problem was not doing that change, was to fix
the rejects after that on the following patches.

Later, Juan.