Handle errors during migration

PATCH Superseded Juan Quintela 3 participants 19 messages Sep 23, 2011 – Oct 5, 2011

Share & tools

Juan Quintela

author

2011-09-23 12:51 UTC #0 lore raw

v2 cover Handle errors during migration

[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(-)

Signature · 1 line

1.7.6.2

Juan Quintela

author

2011-09-23 12:51 UTC #1 lore raw

PATCH v2 1/11 ds1225y: Use stdio instead of QEMUFile

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 <[email protected]>

---
 hw/ds1225y.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

hw/ds1225y.c +16 ~~-12~~

index 9875c44..6852a61 100644
@@ -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);

Signature · 1 line

1.7.6.2

Juan Quintela

author

2011-09-23 12:51 UTC #2 lore raw

PATCH v2 2/11 migration: simplify state assignmente

In reply to 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 <[email protected]>

---
 migration.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

migration.c +4 -7

index 7dd8f4e..531fe83 100644
@@ -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);
     }
 }

Signature · 1 line

1.7.6.2

Juan Quintela

author

2011-09-23 12:51 UTC #3 lore raw

PATCH v2 3/11 migration: Check that migration is active before cancel it

In reply to Juan Quintela

Signed-off-by Juan Quintela <[email protected]>

---
 migration.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

migration.c +2 -2

index 531fe83..ea7bcc8 100644
@@ -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;
 }

Signature · 1 line

1.7.6.2

Juan Quintela

author

2011-09-23 12:51 UTC #4 lore raw

PATCH v2 4/11 migration: return real error code

In reply to Juan Quintela

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

Signed-off-by Juan Quintela <[email protected]>

---
 migration.c |    6 +++++-
 savevm.c    |   33 +++++++++++++++------------------
 2 files changed, 20 insertions(+), 19 deletions(-)

migration.c +5 -1

index ea7bcc8..9498e20 100644
@@ -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");

savevm.c +15 ~~-18~~

index 46f2447..c382076 100644
@@ -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();

Signature · 1 line

1.7.6.2

Anthony Liguori

2011-10-04 18:08 UTC #5 lore raw

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

Quoted 3 lines · Juan Quintela

make functions propaget errno, instead of just using -EIO.
Signed-off-by Juan Quintela<[email protected]>

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

Quoted 106 lines

---
  migration.c |    6 +++++-
  savevm.c    |   33 +++++++++++++++------------------
  2 files changed, 20 insertions(+), 19 deletions(-)

migration.c +5 -1

index ea7bcc8..9498e20 100644
@@ -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");

savevm.c +15 ~~-18~~

index 46f2447..c382076 100644
@@ -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();

Juan Quintela

author

2011-10-04 18:36 UTC #6 lore raw

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

Quoted 8 lines · Anthony Liguori

Quoted 3 lines · Juan Quintela
make functions propaget errno, instead of just using -EIO.
Signed-off-by Juan Quintela<[email protected]>
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.

Anthony Liguori

2011-10-05 19:52 UTC #7 lore raw

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

Quoted 4 lines · Juan Quintela

Quoted 3 lines · Anthony Liguori
Quoted 2 lines · Juan Quintela
make functions propaget errno, instead of just using -EIO.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Juan Quintela<[email protected]>
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

Quoted 2 lines

Later, Juan.

Juan Quintela

author

2011-10-05 20:07 UTC #8 lore raw

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

Quoted 11 lines · Anthony Liguori

Quoted 10 lines · Juan Quintela
Quoted 8 lines · Anthony Liguori
Quoted 3 lines · Juan Quintela
make functions propaget errno, instead of just using -EIO.
Signed-off-by Juan Quintela<[email protected]>
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?

Quoted 6 lines

Quoted 2 lines
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.

Juan Quintela

author

2011-09-23 12:51 UTC #9 lore raw

PATCH v2 5/11 migration: add error handling to migrate_fd_put_notify().

In reply to Juan Quintela

From: Yoshiaki Tamura <[email protected]>

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 <[email protected]>

Signed-off-by Juan Quintela <[email protected]>

---
 migration.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

migration.c +3 -3

index 9498e20..0b284ff 100644
@@ -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;

Signature · 1 line

1.7.6.2

Paolo Bonzini

2011-09-23 13:45 UTC #10 lore raw

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

Quoted 28 lines · Juan Quintela

From: Yoshiaki Tamura<[email protected]>
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<[email protected]>
Signed-off-by Juan Quintela<[email protected]>
---
  migration.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)
migration.c +3 -0
index 9498e20..0b284ff 100644
@@ -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)

migration.c ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size) · line 337

Quoted 9 lines

migration.c +0 -3

@@ -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

Juan Quintela

author

2011-10-04 20:49 UTC #11 lore raw

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

(Adding new Yoshiaki email address)

Quoted 37 lines · Paolo Bonzini

Quoted 36 lines · Juan Quintela

From: Yoshiaki Tamura<[email protected]>

Signed-off-by Yoshiaki Tamura<[email protected]>

Signed-off-by Juan Quintela<[email protected]>

---
  migration.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)

migration.c +3 -3

index 9498e20..0b284ff 100644
@@ -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);
      }

Quoted 4 lines

migration.c ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size) · line 337
Quoted 1 line
migration.c +0 -0
      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.

Juan Quintela

author

2011-09-23 12:51 UTC #12 lore raw

PATCH v2 6/11 migration: If there is one error, it makes no sense to continue

In reply to Juan Quintela

Signed-off-by Juan Quintela <[email protected]>

---
 buffered_file.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

buffered_file.c +3 -3

index 486af57..bcdf04f 100644
@@ -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;

Signature · 1 line

1.7.6.2

Juan Quintela

author

2011-09-23 12:51 UTC #13 lore raw

PATCH v2 7/11 buffered_file: Use right "opaque"

In reply to 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 <[email protected]>

---
 buffered_file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

buffered_file.c +1 -1

index bcdf04f..701b440 100644
@@ -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);

Signature · 1 line

1.7.6.2

Juan Quintela

author

2011-09-23 12:51 UTC #14 lore raw

PATCH v2 8/11 buffered_file: reuse QEMUFile has_error field

In reply to 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 <[email protected]>

---
 buffered_file.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

buffered_file.c +8 -9

index 701b440..3dadec0 100644
@@ -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;
     }

Signature · 1 line

1.7.6.2

Juan Quintela

author

2011-09-23 12:51 UTC #15 lore raw

PATCH v2 9/11 migration: don't "write" when migration is not active

In reply to Juan Quintela

If migration is not active, just ignore writes.

[Based on Daniel Berrange suggestion]

Signed-off-by Juan Quintela <[email protected]>

---
 migration.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

migration.c +4 -0

index 0b284ff..755b96b 100644
@@ -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));

Signature · 1 line

1.7.6.2

Paolo Bonzini

Acked-by on v2 9/11

2011-09-23 13:46 UTC #16 lore raw

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

Quoted 14 lines · Juan Quintela

If migration is not active, just ignore writes.
[Based on Daniel Berrange suggestion]
Signed-off-by Juan Quintela<[email protected]>
---
  migration.c |    4 ++++
  1 files changed, 4 insertions(+), 0 deletions(-)
migration.c +0 -0
index 0b284ff..755b96b 100644

migration.c ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size) · line 332

Quoted 10 lines

migration.c +4 -0

@@ -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 <[email protected]>

Juan Quintela

author

2011-09-23 12:51 UTC #17 lore raw

PATCH v2 10/11 migration: set error if select return one error

In reply to Juan Quintela

Signed-off-by Juan Quintela <[email protected]>

---
 migration.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

migration.c +4 -0

index 755b96b..9265b16 100644
@@ -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)

Signature · 1 line

1.7.6.2

Juan Quintela

author

2011-09-23 12:51 UTC #18 lore raw

PATCH v2 11/11 migration: change has_error to contain errno values

In reply to Juan Quintela

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

Signed-off-by Juan Quintela <[email protected]>

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

arch_init.c +1 -1

index 9a5a0e3..12d9c8b 100644
@@ -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;
     }

block-migration.c +3 -3

index e2775ee..2638f51 100644
@@ -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);

buffered_file.c +2 -2

index 3dadec0..3e5333c 100644
@@ -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;
         }

hw/hw.h +1 -1

index a124da9..6cf8cd2 100644
@@ -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

migration.c +1 -1

index 9265b16..08688aa 100644
@@ -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));
     }
 }

savevm.c +4 -4

index c382076..9199c24 100644
@@ -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)

Signature · 1 line

1.7.6.2