[Qemu-devel] [PATCH 00/41] Migration cleanups, refactorings, stats, and more

[Qemu-devel] [PATCH 00/41] Migration cleanups, refactorings, stats, and more

[Juan Quintela]

Hi

This is the mergable part of migration thread work that I am doing.
What it does:
- cleanups left and right
- 2nd patch is a (fix migration sync) should be applied to stable IMHO
   Othrewise, we "could" be sending zero pages 2 times.
- It introduces the stats discused previously
   * expected_downtime
   * downtime
   * dirty_pages_rate
     All Eric & Luiz comments have been addressed
- buffered file has been basically unfolded.  It was used only once,
  with one user.  So we de-abstracted it.
  Notice that with migration patches, it makes no sense anymore
- migration bitmap

  Now the bitmap is 1bit per page.  Notice that this only affects to
  migration.  For the rest (aka TCG & VGA), we still use the old bitmap.

- remove qemu_file* functions that were not used or used only once
- last_error: we have made that almost all error paths return one error
  instead of zero, so we use much, much less the error on the qemu_file.
  Some of the last remmants are:
  * still there due to callbacks (can be removed when the thread is
    integrated)
  * are on the read path (I haven't touched a lot of those yet)
- qemu_cpu_is_vcpu()

  We used to test if we were _not_ an vcpu testing if we were the
  iothread.  With the migration thread that is not true anymore.  So
  create a function that does the right thing.

Please review.

Thanks, Juan.

The following changes since commit c26032b2c91721245bfec542d94f37a0238e986e:

  target-xtensa: don't emit extra tcg_gen_goto_tb (2012-09-21 03:07:27 +0400)

are available in the git repository at:

  http://repo.or.cz/r/qemu/quintela.git migration-next-20120921

for you to fetch changes up to 4bce0b88b10ed790ad3669ce4ff61c945cd655eb:

  cpus: create qemu_cpu_is_vcpu() (2012-09-21 10:43:10 +0200)


Juan Quintela (40):
  buffered_file: g_realloc() can't fail
  fix migration sync
  migration: store end_time in a local variable
  migration: print total downtime for final phase of migration
  migration: rename expected_time to expected_downtime
  migration: export migrate_get_current()
  migration: print expected downtime in info migrate
  savevm: Factorize ram globals reset in its own function
  ram: introduce migration_bitmap_set_dirty()
  ram: Introduce migration_bitmap_test_and_reset_dirty()
  ram: Export last_ram_offset()
  ram: introduce migration_bitmap_sync()
  ram: create trace event for migration sync bitmap
  Separate migration bitmap
  migration: Add dirty_pages_rate to query migrate output
  buffered_file: rename opaque to migration_state
  buffered_file: opaque is MigrationState
  buffered_file: unfold migrate_fd_put_buffer
  buffered_file: unfold migrate_fd_put_ready
  buffered_file: unfold migrate_fd_put_buffer
  buffered_file: unfold migrate_fd_put_buffer
  buffered_file: We can access directly to bandwidth_limit
  buffered_file: callers of buffered_flush() already check for errors
  buffered_file: make buffered_flush return the error code
  migration: make migrate_fd_wait_for_unfreeze() return errors
  savevm: unexport qemu_fflush
  virtio-net: use qemu_get_buffer() in a temp buffer
  savevm: Remove qemu_fseek()
  savevm: make qemu_fflush() return an error code
  savevm: unfold qemu_fclose_internal()
  savevm: unexport qemu_ftell()
  savevm: make qemu_fill_buffer() be consistent
  savevm: Only qemu_fflush() can generate errors
  buffered_file: buffered_put_buffer() don't need to set last_error
  block-migration: make flush_blks() return errors
  block-migration:  Switch meaning of return value
  block-migration: handle errors with the return codes correctly
  savevm: un-export qemu_file_set_error()
  savevm: make qemu_file_put_notify() return errors
  cpus: create qemu_cpu_is_vcpu()

Paolo Bonzini (1):
  BufferedFile: append, then flush

 arch_init.c       | 141 +++++++++++++++++++++++++++++++++++++++++-------------
 block-migration.c |  51 ++++++++++----------
 buffered_file.c   | 128 +++++++++++++++++--------------------------------
 buffered_file.h   |  12 +----
 cpu-all.h         |   3 +-
 cpus.c            |   9 +++-
 exec-obsolete.h   |  10 ----
 exec.c            |   2 +-
 hmp.c             |  12 +++++
 hw/virtio-net.c   |   4 +-
 migration.c       |  46 +++++++++---------
 migration.h       |  10 ++++
 qapi-schema.json  |  20 ++++++--
 qemu-file.h       |   8 +---
 qmp-commands.hx   |   9 ++++
 savevm.c          | 117 +++++++++++++++++---------------------------
 trace-events      |   4 ++
 17 files changed, 316 insertions(+), 270 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH 01/41] buffered_file: g_realloc() can't fail

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index f170aa0..4148abb 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -50,20 +50,12 @@ static void buffered_append(QEMUFileBuffered *s,
                             const uint8_t *buf, size_t size)
 {
     if (size > (s->buffer_capacity - s->buffer_size)) {
-        void *tmp;
-
         DPRINTF("increasing buffer capacity from %zu by %zu\n",
                 s->buffer_capacity, size + 1024);

         s->buffer_capacity += size + 1024;

-        tmp = g_realloc(s->buffer, s->buffer_capacity);
-        if (tmp == NULL) {
-            fprintf(stderr, "qemu file buffer expansion failed\n");
-            exit(1);
-        }
-
-        s->buffer = tmp;
+        s->buffer = g_realloc(s->buffer, s->buffer_capacity);
     }

     memcpy(s->buffer + s->buffer_size, buf, size);
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 01/41] buffered_file: g_realloc() can't fail

[Paolo Bonzini]

Il 21/09/2012 10:46, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index f170aa0..4148abb 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -50,20 +50,12 @@ static void buffered_append(QEMUFileBuffered *s,
>                              const uint8_t *buf, size_t size)
>  {
>      if (size > (s->buffer_capacity - s->buffer_size)) {
> -        void *tmp;
> -
>          DPRINTF("increasing buffer capacity from %zu by %zu\n",
>                  s->buffer_capacity, size + 1024);
> 
>          s->buffer_capacity += size + 1024;
> 
> -        tmp = g_realloc(s->buffer, s->buffer_capacity);
> -        if (tmp == NULL) {
> -            fprintf(stderr, "qemu file buffer expansion failed\n");
> -            exit(1);
> -        }
> -
> -        s->buffer = tmp;
> +        s->buffer = g_realloc(s->buffer, s->buffer_capacity);
>      }
> 
>      memcpy(s->buffer + s->buffer_size, buf, size);
> 

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

Paolo

Re: [Qemu-devel] [PATCH 01/41] buffered_file: g_realloc() can't fail

[Orit Wasserman]

On 09/21/2012 11:46 AM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index f170aa0..4148abb 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -50,20 +50,12 @@ static void buffered_append(QEMUFileBuffered *s,
>                              const uint8_t *buf, size_t size)
>  {
>      if (size > (s->buffer_capacity - s->buffer_size)) {
> -        void *tmp;
> -
>          DPRINTF("increasing buffer capacity from %zu by %zu\n",
>                  s->buffer_capacity, size + 1024);
> 
>          s->buffer_capacity += size + 1024;
> 
> -        tmp = g_realloc(s->buffer, s->buffer_capacity);
> -        if (tmp == NULL) {
> -            fprintf(stderr, "qemu file buffer expansion failed\n");
> -            exit(1);
> -        }
> -
> -        s->buffer = tmp;
> +        s->buffer = g_realloc(s->buffer, s->buffer_capacity);
>      }
> 
>      memcpy(s->buffer + s->buffer_size, buf, size);
> 

Reviewed-by: Orit Wasserman <owasserm@redhat.com>

[Qemu-devel] [PATCH 02/41] fix migration sync

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch_init.c b/arch_init.c
index f849f9b..cdd8ab7 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -489,6 +489,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     ram_addr_t addr;
     RAMBlock *block;

+    memory_global_sync_dirty_bitmap(get_system_memory());
     bytes_transferred = 0;
     last_block = NULL;
     last_offset = 0;
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 02/41] fix migration sync

[Paolo Bonzini]

Il 21/09/2012 10:46, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch_init.c b/arch_init.c
> index f849f9b..cdd8ab7 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -489,6 +489,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      ram_addr_t addr;
>      RAMBlock *block;
> 
> +    memory_global_sync_dirty_bitmap(get_system_memory());

Does it make sense to call this function before memory_global_dirty_log_start()?

Also, does this call subsume this loop:

    QLIST_FOREACH(block, &ram_list.blocks, next) {
        for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
            if (!memory_region_get_dirty(block->mr, addr, TARGET_PAGE_SIZE,
                                         DIRTY_MEMORY_MIGRATION)) {
                memory_region_set_dirty(block->mr, addr, TARGET_PAGE_SIZE);
            }
        }
    }

so that the right fix is

-    QLIST_FOREACH(block, &ram_list.blocks, next) {
-        for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
-            if (!memory_region_get_dirty(block->mr, addr, TARGET_PAGE_SIZE,
-                                         DIRTY_MEMORY_MIGRATION)) {
-                memory_region_set_dirty(block->mr, addr, TARGET_PAGE_SIZE);
-            }
-        }
-    }
-
     memory_global_dirty_log_start();
+    memory_global_sync_dirty_bitmap(get_system_memory());

?

Paolo

>      bytes_transferred = 0;
>      last_block = NULL;
>      last_offset = 0;
> 

Re: [Qemu-devel] [PATCH 02/41] fix migration sync

[Paolo Bonzini]

Il 21/09/2012 14:17, Paolo Bonzini ha scritto:
> 
> -    QLIST_FOREACH(block, &ram_list.blocks, next) {
> -        for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
> -            if (!memory_region_get_dirty(block->mr, addr, TARGET_PAGE_SIZE,
> -                                         DIRTY_MEMORY_MIGRATION)) {
> -                memory_region_set_dirty(block->mr, addr, TARGET_PAGE_SIZE);
> -            }
> -        }
> -    }
> -
>      memory_global_dirty_log_start();
> +    memory_global_sync_dirty_bitmap(get_system_memory());

Hmm, perhaps for KVM but probably not for TCG.  But this looks like a bug in
KVM, maybe a fix there would be better?

diff --git a/kvm-all.c b/kvm-all.c
index 78e7a88..fdab4f6 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -744,6 +746,8 @@ static void kvm_log_global_start(struct MemoryListener *listener)
 {
     int r;
 
+    /* Clear the KVM bitmap.  */
+    kvm_physical_sync_dirty_bitmap(section);
     r = kvm_set_migration_log(1);
     assert(r >= 0);
 }

On the other hand, you're doing useless work syncing the KVM bitmap to the
TCG one---even though the TCG bitmap is all-ones!  Something like this ought
to be faster, but I'm not sure whether it is conceptually right:

diff --git a/kvm-all.c b/kvm-all.c
index 78e7a88..fdab4f6 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -399,7 +399,7 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
  * @start_add: start of logged region.
  * @end_addr: end of logged region.
  */
-static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section)
+static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section, bool sync)
 {
     KVMState *s = kvm_state;
     unsigned long size, allocated_size = 0;
@@ -446,7 +446,9 @@ static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section)
             break;
         }
 
-        kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
+        if (sync) {
+            kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
+        }
         start_addr = mem->start_addr + mem->memory_size;
     }
     g_free(d.dirty_bitmap);
@@ -600,7 +602,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
         old = *mem;
 
         if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
-            kvm_physical_sync_dirty_bitmap(section);
+            kvm_physical_sync_dirty_bitmap(section, true);
         }
 
         /* unregister the overlapping slot */
@@ -733,7 +735,7 @@ static void kvm_log_sync(MemoryListener *listener,
 {
     int r;
 
-    r = kvm_physical_sync_dirty_bitmap(section);
+    r = kvm_physical_sync_dirty_bitmap(section, true);
     if (r < 0) {
         abort();
     }
@@ -744,6 +746,8 @@ static void kvm_log_global_start(struct MemoryListener *listener)
 {
     int r;
 
+    /* Clear the KVM bitmap.  */
+    kvm_physical_sync_dirty_bitmap(section, false);
     r = kvm_set_migration_log(1);
     assert(r >= 0);
 }

Re: [Qemu-devel] [PATCH 02/41] fix migration sync

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 21/09/2012 14:17, Paolo Bonzini ha scritto:
>> 
>> -    QLIST_FOREACH(block, &ram_list.blocks, next) {
>> -        for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
>> -            if (!memory_region_get_dirty(block->mr, addr, TARGET_PAGE_SIZE,
>> -                                         DIRTY_MEMORY_MIGRATION)) {
>> -                memory_region_set_dirty(block->mr, addr, TARGET_PAGE_SIZE);
>> -            }
>> -        }
>> -    }
>> -
>>      memory_global_dirty_log_start();
>> +    memory_global_sync_dirty_bitmap(get_system_memory());

With the part of moving it after the memory_global_dirty_log_start() I
agree.

With the other suggestion, I will take another look at it.  The problem
is that vga code can also sync the kvm bitmap, and we want to get that
notifications also.

Later, Juan.

Re: [Qemu-devel] [PATCH 02/41] fix migration sync

[Paolo Bonzini]

Il 02/10/2012 12:43, Juan Quintela ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 21/09/2012 14:17, Paolo Bonzini ha scritto:
>>>
>>> -    QLIST_FOREACH(block, &ram_list.blocks, next) {
>>> -        for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
>>> -            if (!memory_region_get_dirty(block->mr, addr, TARGET_PAGE_SIZE,
>>> -                                         DIRTY_MEMORY_MIGRATION)) {
>>> -                memory_region_set_dirty(block->mr, addr, TARGET_PAGE_SIZE);
>>> -            }
>>> -        }
>>> -    }
>>> -
>>>      memory_global_dirty_log_start();
>>> +    memory_global_sync_dirty_bitmap(get_system_memory());
> 
> With the part of moving it after the memory_global_dirty_log_start() I
> agree.
> 
> With the other suggestion, I will take another look at it.  The problem
> is that vga code can also sync the kvm bitmap, and we want to get that
> notifications also.

I think I had replied to myself later... in any case, nothing that
cannot be improved after merging.  Thanks!

Paolo

[Qemu-devel] [PATCH 03/41] migration: store end_time in a local variable

[Juan Quintela]

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

diff --git a/migration.c b/migration.c
index 1edeec5..1e3f791 100644
--- a/migration.c
+++ b/migration.c
@@ -327,6 +327,7 @@ static void migrate_fd_put_ready(void *opaque)
         migrate_fd_error(s);
     } else if (ret == 1) {
         int old_vm_running = runstate_is_running();
+        int64_t end_time;

         DPRINTF("done iterating\n");
         qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
@@ -337,7 +338,8 @@ static void migrate_fd_put_ready(void *opaque)
         } else {
             migrate_fd_completed(s);
         }
-        s->total_time = qemu_get_clock_ms(rt_clock) - s->total_time;
+        end_time = qemu_get_clock_ms(rt_clock);
+        s->total_time = end_time - s->total_time;
         if (s->state != MIG_STATE_COMPLETED) {
             if (old_vm_running) {
                 vm_start();
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 03/41] migration: store end_time in a local variable

[Paolo Bonzini]

Il 21/09/2012 10:46, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration.c b/migration.c
> index 1edeec5..1e3f791 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -327,6 +327,7 @@ static void migrate_fd_put_ready(void *opaque)
>          migrate_fd_error(s);
>      } else if (ret == 1) {
>          int old_vm_running = runstate_is_running();
> +        int64_t end_time;
> 
>          DPRINTF("done iterating\n");
>          qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> @@ -337,7 +338,8 @@ static void migrate_fd_put_ready(void *opaque)
>          } else {
>              migrate_fd_completed(s);
>          }
> -        s->total_time = qemu_get_clock_ms(rt_clock) - s->total_time;
> +        end_time = qemu_get_clock_ms(rt_clock);
> +        s->total_time = end_time - s->total_time;
>          if (s->state != MIG_STATE_COMPLETED) {
>              if (old_vm_running) {
>                  vm_start();
> 

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

Re: [Qemu-devel] [PATCH 03/41] migration: store end_time in a local variable

[Orit Wasserman]

On 09/21/2012 11:46 AM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration.c b/migration.c
> index 1edeec5..1e3f791 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -327,6 +327,7 @@ static void migrate_fd_put_ready(void *opaque)
>          migrate_fd_error(s);
>      } else if (ret == 1) {
>          int old_vm_running = runstate_is_running();
> +        int64_t end_time;
> 
>          DPRINTF("done iterating\n");
>          qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> @@ -337,7 +338,8 @@ static void migrate_fd_put_ready(void *opaque)
>          } else {
>              migrate_fd_completed(s);
>          }
> -        s->total_time = qemu_get_clock_ms(rt_clock) - s->total_time;
> +        end_time = qemu_get_clock_ms(rt_clock);
> +        s->total_time = end_time - s->total_time;
>          if (s->state != MIG_STATE_COMPLETED) {
>              if (old_vm_running) {
>                  vm_start();
> 

Reviewed-by: Orit Wasserman <owasserm@redhat.com>

[Qemu-devel] [PATCH 04/41] migration: print total downtime for final phase of migration

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hmp.c            | 4 ++++
 migration.c      | 6 +++++-
 migration.h      | 1 +
 qapi-schema.json | 7 ++++++-
 qmp-commands.hx  | 3 +++
 5 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hmp.c b/hmp.c
index ba6fbd3..40b0c05 100644
--- a/hmp.c
+++ b/hmp.c
@@ -152,6 +152,10 @@ void hmp_info_migrate(Monitor *mon)
         monitor_printf(mon, "Migration status: %s\n", info->status);
         monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
                        info->total_time);
+        if (info->has_downtime) {
+            monitor_printf(mon, "downtime: %" PRIu64 " milliseconds\n",
+                           info->downtime);
+        }
     }

     if (info->has_ram) {
diff --git a/migration.c b/migration.c
index 1e3f791..c1655b3 100644
--- a/migration.c
+++ b/migration.c
@@ -195,6 +195,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->has_status = true;
         info->status = g_strdup("completed");
         info->total_time = s->total_time;
+        info->has_downtime = true;
+        info->downtime = s->downtime;

         info->has_ram = true;
         info->ram = g_malloc0(sizeof(*info->ram));
@@ -327,9 +329,10 @@ static void migrate_fd_put_ready(void *opaque)
         migrate_fd_error(s);
     } else if (ret == 1) {
         int old_vm_running = runstate_is_running();
-        int64_t end_time;
+        int64_t start_time, end_time;

         DPRINTF("done iterating\n");
+        start_time = qemu_get_clock_ms(rt_clock);
         qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
         vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);

@@ -340,6 +343,7 @@ static void migrate_fd_put_ready(void *opaque)
         }
         end_time = qemu_get_clock_ms(rt_clock);
         s->total_time = end_time - s->total_time;
+        s->downtime = end_time - start_time;
         if (s->state != MIG_STATE_COMPLETED) {
             if (old_vm_running) {
                 vm_start();
diff --git a/migration.h b/migration.h
index a9852fc..3462917 100644
--- a/migration.h
+++ b/migration.h
@@ -40,6 +40,7 @@ struct MigrationState
     void *opaque;
     MigrationParams params;
     int64_t total_time;
+    int64_t downtime;
     bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
     int64_t xbzrle_cache_size;
 };
diff --git a/qapi-schema.json b/qapi-schema.json
index 14e4419..b5a4360 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -413,13 +413,18 @@
 #        If migration has ended, it returns the total migration
 #        time. (since 1.2)
 #
+# @downtime: #optional only present when migration finishes correctly
+#        total downtime in milliseconds for the guest.
+#        (since 1.3)
+#
 # Since: 0.14.0
 ##
 { 'type': 'MigrationInfo',
   'data': {'*status': 'str', '*ram': 'MigrationStats',
            '*disk': 'MigrationStats',
            '*xbzrle-cache': 'XBZRLECacheStats',
-           '*total-time': 'int'} }
+           '*total-time': 'int',
+           '*downtime': 'int'} }

 ##
 # @query-migrate
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6e21ddb..37be613 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2267,6 +2267,8 @@ The main json-object contains the following:
 - "total-time": total amount of ms since migration started.  If
                 migration has ended, it returns the total migration
 		 time (json-int)
+- "downtime": only present when migration has finished correctly
+                total amount in ms for downtime that happened (json-int)
 - "ram": only present if "status" is "active", it is a json-object with the
   following RAM information (in bytes):
          - "transferred": amount transferred (json-int)
@@ -2304,6 +2306,7 @@ Examples:
           "remaining":123,
           "total":246,
           "total-time":12345,
+          "downtime":12345,
           "duplicate":123,
           "normal":123,
           "normal-bytes":123456
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 04/41] migration: print total downtime for final phase of migration

[Paolo Bonzini]

Il 21/09/2012 10:46, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hmp.c            | 4 ++++
>  migration.c      | 6 +++++-
>  migration.h      | 1 +
>  qapi-schema.json | 7 ++++++-
>  qmp-commands.hx  | 3 +++
>  5 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index ba6fbd3..40b0c05 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -152,6 +152,10 @@ void hmp_info_migrate(Monitor *mon)
>          monitor_printf(mon, "Migration status: %s\n", info->status);
>          monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
>                         info->total_time);
> +        if (info->has_downtime) {
> +            monitor_printf(mon, "downtime: %" PRIu64 " milliseconds\n",
> +                           info->downtime);
> +        }
>      }
> 
>      if (info->has_ram) {
> diff --git a/migration.c b/migration.c
> index 1e3f791..c1655b3 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -195,6 +195,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->has_status = true;
>          info->status = g_strdup("completed");
>          info->total_time = s->total_time;
> +        info->has_downtime = true;
> +        info->downtime = s->downtime;
> 
>          info->has_ram = true;
>          info->ram = g_malloc0(sizeof(*info->ram));
> @@ -327,9 +329,10 @@ static void migrate_fd_put_ready(void *opaque)
>          migrate_fd_error(s);
>      } else if (ret == 1) {
>          int old_vm_running = runstate_is_running();
> -        int64_t end_time;
> +        int64_t start_time, end_time;
> 
>          DPRINTF("done iterating\n");
> +        start_time = qemu_get_clock_ms(rt_clock);
>          qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>          vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> 
> @@ -340,6 +343,7 @@ static void migrate_fd_put_ready(void *opaque)
>          }
>          end_time = qemu_get_clock_ms(rt_clock);
>          s->total_time = end_time - s->total_time;
> +        s->downtime = end_time - start_time;
>          if (s->state != MIG_STATE_COMPLETED) {
>              if (old_vm_running) {
>                  vm_start();
> diff --git a/migration.h b/migration.h
> index a9852fc..3462917 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -40,6 +40,7 @@ struct MigrationState
>      void *opaque;
>      MigrationParams params;
>      int64_t total_time;
> +    int64_t downtime;
>      bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
>      int64_t xbzrle_cache_size;
>  };
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 14e4419..b5a4360 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -413,13 +413,18 @@
>  #        If migration has ended, it returns the total migration
>  #        time. (since 1.2)
>  #
> +# @downtime: #optional only present when migration finishes correctly
> +#        total downtime in milliseconds for the guest.
> +#        (since 1.3)
> +#
>  # Since: 0.14.0
>  ##
>  { 'type': 'MigrationInfo',
>    'data': {'*status': 'str', '*ram': 'MigrationStats',
>             '*disk': 'MigrationStats',
>             '*xbzrle-cache': 'XBZRLECacheStats',
> -           '*total-time': 'int'} }
> +           '*total-time': 'int',
> +           '*downtime': 'int'} }
> 
>  ##
>  # @query-migrate
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6e21ddb..37be613 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2267,6 +2267,8 @@ The main json-object contains the following:
>  - "total-time": total amount of ms since migration started.  If
>                  migration has ended, it returns the total migration
>  		 time (json-int)
> +- "downtime": only present when migration has finished correctly
> +                total amount in ms for downtime that happened (json-int)

Indeed it's only present if

         info->status = g_strdup("completed");

>  - "ram": only present if "status" is "active", it is a json-object with the
>    following RAM information (in bytes):
>           - "transferred": amount transferred (json-int)
> @@ -2304,6 +2306,7 @@ Examples:
>            "remaining":123,
>            "total":246,
>            "total-time":12345,
> +          "downtime":12345,
>            "duplicate":123,
>            "normal":123,
>            "normal-bytes":123456
> 

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

Paolo

Re: [Qemu-devel] [PATCH 04/41] migration: print total downtime for final phase of migration

[Orit Wasserman]

On 09/21/2012 11:46 AM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hmp.c            | 4 ++++
>  migration.c      | 6 +++++-
>  migration.h      | 1 +
>  qapi-schema.json | 7 ++++++-
>  qmp-commands.hx  | 3 +++
>  5 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index ba6fbd3..40b0c05 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -152,6 +152,10 @@ void hmp_info_migrate(Monitor *mon)
>          monitor_printf(mon, "Migration status: %s\n", info->status);
>          monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
>                         info->total_time);
> +        if (info->has_downtime) {
> +            monitor_printf(mon, "downtime: %" PRIu64 " milliseconds\n",
> +                           info->downtime);
> +        }
>      }
> 
>      if (info->has_ram) {
> diff --git a/migration.c b/migration.c
> index 1e3f791..c1655b3 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -195,6 +195,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->has_status = true;
>          info->status = g_strdup("completed");
>          info->total_time = s->total_time;
> +        info->has_downtime = true;
> +        info->downtime = s->downtime;
> 
>          info->has_ram = true;
>          info->ram = g_malloc0(sizeof(*info->ram));
> @@ -327,9 +329,10 @@ static void migrate_fd_put_ready(void *opaque)
>          migrate_fd_error(s);
>      } else if (ret == 1) {
>          int old_vm_running = runstate_is_running();
> -        int64_t end_time;
> +        int64_t start_time, end_time;
> 
>          DPRINTF("done iterating\n");
> +        start_time = qemu_get_clock_ms(rt_clock);
>          qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>          vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> 
> @@ -340,6 +343,7 @@ static void migrate_fd_put_ready(void *opaque)
>          }
>          end_time = qemu_get_clock_ms(rt_clock);
>          s->total_time = end_time - s->total_time;
> +        s->downtime = end_time - start_time;
>          if (s->state != MIG_STATE_COMPLETED) {
>              if (old_vm_running) {
>                  vm_start();
> diff --git a/migration.h b/migration.h
> index a9852fc..3462917 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -40,6 +40,7 @@ struct MigrationState
>      void *opaque;
>      MigrationParams params;
>      int64_t total_time;
> +    int64_t downtime;
>      bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
>      int64_t xbzrle_cache_size;
>  };
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 14e4419..b5a4360 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -413,13 +413,18 @@
>  #        If migration has ended, it returns the total migration
>  #        time. (since 1.2)
>  #
> +# @downtime: #optional only present when migration finishes correctly
> +#        total downtime in milliseconds for the guest.
> +#        (since 1.3)
> +#
>  # Since: 0.14.0
>  ##
>  { 'type': 'MigrationInfo',
>    'data': {'*status': 'str', '*ram': 'MigrationStats',
>             '*disk': 'MigrationStats',
>             '*xbzrle-cache': 'XBZRLECacheStats',
> -           '*total-time': 'int'} }
> +           '*total-time': 'int',
> +           '*downtime': 'int'} }
> 
>  ##
>  # @query-migrate
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6e21ddb..37be613 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2267,6 +2267,8 @@ The main json-object contains the following:
>  - "total-time": total amount of ms since migration started.  If
>                  migration has ended, it returns the total migration
>  		 time (json-int)
> +- "downtime": only present when migration has finished correctly
> +                total amount in ms for downtime that happened (json-int)
>  - "ram": only present if "status" is "active", it is a json-object with the
>    following RAM information (in bytes):
>           - "transferred": amount transferred (json-int)
> @@ -2304,6 +2306,7 @@ Examples:
>            "remaining":123,
>            "total":246,
>            "total-time":12345,
> +          "downtime":12345,
>            "duplicate":123,
>            "normal":123,
>            "normal-bytes":123456
> 

Reviewed-by: Orit Wasserman <owasserm@redhat.com>

[Qemu-devel] [PATCH 05/41] migration: rename expected_time to expected_downtime

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index cdd8ab7..013e5e5 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -539,7 +539,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     double bwidth = 0;
     int ret;
     int i;
-    uint64_t expected_time;
+    uint64_t expected_downtime;

     bytes_transferred_last = bytes_transferred;
     bwidth = qemu_get_clock_ns(rt_clock);
@@ -578,24 +578,24 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     bwidth = qemu_get_clock_ns(rt_clock) - bwidth;
     bwidth = (bytes_transferred - bytes_transferred_last) / bwidth;

-    /* if we haven't transferred anything this round, force expected_time to a
-     * a very high value, but without crashing */
+    /* if we haven't transferred anything this round, force
+     * expected_downtime to a very high value, but without
+     * crashing */
     if (bwidth == 0) {
         bwidth = 0.000001;
     }

     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);

-    expected_time = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
+    expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
+    DPRINTF("ram_save_live: expected(%" PRIu64 ") <= max(" PRIu64 ")?\n",
+            expected_downtime, migrate_max_downtime());

-    DPRINTF("ram_save_live: expected(%" PRIu64 ") <= max(%" PRIu64 ")?\n",
-            expected_time, migrate_max_downtime());
-
-    if (expected_time <= migrate_max_downtime()) {
+    if (expected_downtime <= migrate_max_downtime()) {
         memory_global_sync_dirty_bitmap(get_system_memory());
-        expected_time = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
+        expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;

-        return expected_time <= migrate_max_downtime();
+        return expected_downtime <= migrate_max_downtime();
     }
     return 0;
 }
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 05/41] migration: rename expected_time to expected_downtime

[Paolo Bonzini]

Il 21/09/2012 10:46, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index cdd8ab7..013e5e5 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -539,7 +539,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      double bwidth = 0;
>      int ret;
>      int i;
> -    uint64_t expected_time;
> +    uint64_t expected_downtime;
> 
>      bytes_transferred_last = bytes_transferred;
>      bwidth = qemu_get_clock_ns(rt_clock);
> @@ -578,24 +578,24 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      bwidth = qemu_get_clock_ns(rt_clock) - bwidth;
>      bwidth = (bytes_transferred - bytes_transferred_last) / bwidth;
> 
> -    /* if we haven't transferred anything this round, force expected_time to a
> -     * a very high value, but without crashing */
> +    /* if we haven't transferred anything this round, force
> +     * expected_downtime to a very high value, but without
> +     * crashing */
>      if (bwidth == 0) {
>          bwidth = 0.000001;
>      }
> 
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> 
> -    expected_time = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
> +    expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
> +    DPRINTF("ram_save_live: expected(%" PRIu64 ") <= max(" PRIu64 ")?\n",
> +            expected_downtime, migrate_max_downtime());
> 
> -    DPRINTF("ram_save_live: expected(%" PRIu64 ") <= max(%" PRIu64 ")?\n",
> -            expected_time, migrate_max_downtime());
> -
> -    if (expected_time <= migrate_max_downtime()) {
> +    if (expected_downtime <= migrate_max_downtime()) {
>          memory_global_sync_dirty_bitmap(get_system_memory());
> -        expected_time = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
> +        expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
> 
> -        return expected_time <= migrate_max_downtime();
> +        return expected_downtime <= migrate_max_downtime();
>      }
>      return 0;
>  }
> 

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

Re: [Qemu-devel] [PATCH 05/41] migration: rename expected_time to expected_downtime

[Orit Wasserman]

On 09/21/2012 11:46 AM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index cdd8ab7..013e5e5 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -539,7 +539,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      double bwidth = 0;
>      int ret;
>      int i;
> -    uint64_t expected_time;
> +    uint64_t expected_downtime;
> 
>      bytes_transferred_last = bytes_transferred;
>      bwidth = qemu_get_clock_ns(rt_clock);
> @@ -578,24 +578,24 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      bwidth = qemu_get_clock_ns(rt_clock) - bwidth;
>      bwidth = (bytes_transferred - bytes_transferred_last) / bwidth;
> 
> -    /* if we haven't transferred anything this round, force expected_time to a
> -     * a very high value, but without crashing */
> +    /* if we haven't transferred anything this round, force
> +     * expected_downtime to a very high value, but without
> +     * crashing */
>      if (bwidth == 0) {
>          bwidth = 0.000001;
>      }
> 
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> 
> -    expected_time = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
> +    expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
> +    DPRINTF("ram_save_live: expected(%" PRIu64 ") <= max(" PRIu64 ")?\n",
> +            expected_downtime, migrate_max_downtime());
> 
> -    DPRINTF("ram_save_live: expected(%" PRIu64 ") <= max(%" PRIu64 ")?\n",
> -            expected_time, migrate_max_downtime());
> -
> -    if (expected_time <= migrate_max_downtime()) {
> +    if (expected_downtime <= migrate_max_downtime()) {
>          memory_global_sync_dirty_bitmap(get_system_memory());
> -        expected_time = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
> +        expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
> 
> -        return expected_time <= migrate_max_downtime();
> +        return expected_downtime <= migrate_max_downtime();
>      }
>      return 0;
>  }
> 

Reviewed-by: Orit Wasserman <owasserm@redhat.com>

[Qemu-devel] [PATCH 06/41] migration: export migrate_get_current()

[Juan Quintela]

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

diff --git a/migration.c b/migration.c
index c1655b3..2827663 100644
--- a/migration.c
+++ b/migration.c
@@ -53,7 +53,7 @@ static NotifierList migration_state_notifiers =
    migrations at once.  For now we don't need to add
    dynamic creation of migration */

-static MigrationState *migrate_get_current(void)
+MigrationState *migrate_get_current(void)
 {
     static MigrationState current_migration = {
         .state = MIG_STATE_SETUP,
diff --git a/migration.h b/migration.h
index 3462917..dabc333 100644
--- a/migration.h
+++ b/migration.h
@@ -81,6 +81,7 @@ void remove_migration_state_change_notifier(Notifier *notify);
 bool migration_is_active(MigrationState *);
 bool migration_has_finished(MigrationState *);
 bool migration_has_failed(MigrationState *);
+MigrationState *migrate_get_current(void);

 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_transferred(void);
-- 
1.7.11.4

[Qemu-devel] [PATCH 07/41] migration: print expected downtime in info migrate

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c      | 2 ++
 hmp.c            | 4 ++++
 migration.c      | 2 ++
 migration.h      | 1 +
 qapi-schema.json | 5 +++++
 qmp-commands.hx  | 6 ++++++
 6 files changed, 20 insertions(+)

diff --git a/arch_init.c b/arch_init.c
index 013e5e5..52ccc7b 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -540,6 +540,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     int ret;
     int i;
     uint64_t expected_downtime;
+    MigrationState *s = migrate_get_current();

     bytes_transferred_last = bytes_transferred;
     bwidth = qemu_get_clock_ns(rt_clock);
@@ -594,6 +595,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     if (expected_downtime <= migrate_max_downtime()) {
         memory_global_sync_dirty_bitmap(get_system_memory());
         expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
+        s->expected_downtime = expected_downtime / 1000000; /* ns -> ms */

         return expected_downtime <= migrate_max_downtime();
     }
diff --git a/hmp.c b/hmp.c
index 40b0c05..71c9292 100644
--- a/hmp.c
+++ b/hmp.c
@@ -152,6 +152,10 @@ void hmp_info_migrate(Monitor *mon)
         monitor_printf(mon, "Migration status: %s\n", info->status);
         monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
                        info->total_time);
+        if (info->has_expected_downtime) {
+            monitor_printf(mon, "expected downtime: %" PRIu64 " milliseconds\n",
+                           info->expected_downtime);
+        }
         if (info->has_downtime) {
             monitor_printf(mon, "downtime: %" PRIu64 " milliseconds\n",
                            info->downtime);
diff --git a/migration.c b/migration.c
index 2827663..62c8fe9 100644
--- a/migration.c
+++ b/migration.c
@@ -169,6 +169,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->has_total_time = true;
         info->total_time = qemu_get_clock_ms(rt_clock)
             - s->total_time;
+        info->has_expected_downtime = true;
+        info->expected_downtime = s->expected_downtime;

         info->has_ram = true;
         info->ram = g_malloc0(sizeof(*info->ram));
diff --git a/migration.h b/migration.h
index dabc333..552200c 100644
--- a/migration.h
+++ b/migration.h
@@ -41,6 +41,7 @@ struct MigrationState
     MigrationParams params;
     int64_t total_time;
     int64_t downtime;
+    int64_t expected_downtime;
     bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
     int64_t xbzrle_cache_size;
 };
diff --git a/qapi-schema.json b/qapi-schema.json
index b5a4360..b8a1244 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -417,6 +417,10 @@
 #        total downtime in milliseconds for the guest.
 #        (since 1.3)
 #
+# @expected-downtime: #optional only present while migration is active
+#        expected downtime in milliseconds for the guest in last walk
+#        of the dirty bitmap. (since 1.3)
+#
 # Since: 0.14.0
 ##
 { 'type': 'MigrationInfo',
@@ -424,6 +428,7 @@
            '*disk': 'MigrationStats',
            '*xbzrle-cache': 'XBZRLECacheStats',
            '*total-time': 'int',
+           '*expected-downtime': 'int',
            '*downtime': 'int'} }

 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 37be613..68b6580 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2269,6 +2269,9 @@ The main json-object contains the following:
 		 time (json-int)
 - "downtime": only present when migration has finished correctly
                 total amount in ms for downtime that happened (json-int)
+- "expected-downtime": only present while migration is active
+                total amount in ms for downtime that was calculated on
+		the last bitmap round (json-int)
 - "ram": only present if "status" is "active", it is a json-object with the
   following RAM information (in bytes):
          - "transferred": amount transferred (json-int)
@@ -2330,6 +2333,7 @@ Examples:
             "remaining":123,
             "total":246,
             "total-time":12345,
+            "expected-downtime":12345,
             "duplicate":123,
             "normal":123,
             "normal-bytes":123456
@@ -2348,6 +2352,7 @@ Examples:
             "remaining":1053304,
             "transferred":3720,
             "total-time":12345,
+            "expected-downtime":12345,
             "duplicate":123,
             "normal":123,
             "normal-bytes":123456
@@ -2372,6 +2377,7 @@ Examples:
             "remaining":1053304,
             "transferred":3720,
             "total-time":12345,
+            "expected-downtime":12345,
             "duplicate":10,
             "normal":3333,
             "normal-bytes":3412992
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 07/41] migration: print expected downtime in info migrate

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c      | 2 ++
>  hmp.c            | 4 ++++
>  migration.c      | 2 ++
>  migration.h      | 1 +
>  qapi-schema.json | 5 +++++
>  qmp-commands.hx  | 6 ++++++
>  6 files changed, 20 insertions(+)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 013e5e5..52ccc7b 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -540,6 +540,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      int ret;
>      int i;
>      uint64_t expected_downtime;
> +    MigrationState *s = migrate_get_current();
> 
>      bytes_transferred_last = bytes_transferred;
>      bwidth = qemu_get_clock_ns(rt_clock);
> @@ -594,6 +595,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      if (expected_downtime <= migrate_max_downtime()) {
>          memory_global_sync_dirty_bitmap(get_system_memory());
>          expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
> +        s->expected_downtime = expected_downtime / 1000000; /* ns -> ms */
> 
>          return expected_downtime <= migrate_max_downtime();
>      }
> diff --git a/hmp.c b/hmp.c
> index 40b0c05..71c9292 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -152,6 +152,10 @@ void hmp_info_migrate(Monitor *mon)
>          monitor_printf(mon, "Migration status: %s\n", info->status);
>          monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
>                         info->total_time);
> +        if (info->has_expected_downtime) {
> +            monitor_printf(mon, "expected downtime: %" PRIu64 " milliseconds\n",
> +                           info->expected_downtime);
> +        }
>          if (info->has_downtime) {
>              monitor_printf(mon, "downtime: %" PRIu64 " milliseconds\n",
>                             info->downtime);
> diff --git a/migration.c b/migration.c
> index 2827663..62c8fe9 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -169,6 +169,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->has_total_time = true;
>          info->total_time = qemu_get_clock_ms(rt_clock)
>              - s->total_time;
> +        info->has_expected_downtime = true;
> +        info->expected_downtime = s->expected_downtime;
> 
>          info->has_ram = true;
>          info->ram = g_malloc0(sizeof(*info->ram));
> diff --git a/migration.h b/migration.h
> index dabc333..552200c 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -41,6 +41,7 @@ struct MigrationState
>      MigrationParams params;
>      int64_t total_time;
>      int64_t downtime;
> +    int64_t expected_downtime;
>      bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
>      int64_t xbzrle_cache_size;
>  };
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b5a4360..b8a1244 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -417,6 +417,10 @@
>  #        total downtime in milliseconds for the guest.
>  #        (since 1.3)
>  #
> +# @expected-downtime: #optional only present while migration is active
> +#        expected downtime in milliseconds for the guest in last walk
> +#        of the dirty bitmap. (since 1.3)
> +#
>  # Since: 0.14.0
>  ##
>  { 'type': 'MigrationInfo',
> @@ -424,6 +428,7 @@
>             '*disk': 'MigrationStats',
>             '*xbzrle-cache': 'XBZRLECacheStats',
>             '*total-time': 'int',
> +           '*expected-downtime': 'int',
>             '*downtime': 'int'} }
> 
>  ##
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 37be613..68b6580 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2269,6 +2269,9 @@ The main json-object contains the following:
>  		 time (json-int)
>  - "downtime": only present when migration has finished correctly
>                  total amount in ms for downtime that happened (json-int)
> +- "expected-downtime": only present while migration is active
> +                total amount in ms for downtime that was calculated on
> +		the last bitmap round (json-int)
>  - "ram": only present if "status" is "active", it is a json-object with the
>    following RAM information (in bytes):
>           - "transferred": amount transferred (json-int)
> @@ -2330,6 +2333,7 @@ Examples:
>              "remaining":123,
>              "total":246,
>              "total-time":12345,
> +            "expected-downtime":12345,
>              "duplicate":123,
>              "normal":123,
>              "normal-bytes":123456
> @@ -2348,6 +2352,7 @@ Examples:
>              "remaining":1053304,
>              "transferred":3720,
>              "total-time":12345,
> +            "expected-downtime":12345,
>              "duplicate":123,
>              "normal":123,
>              "normal-bytes":123456
> @@ -2372,6 +2377,7 @@ Examples:
>              "remaining":1053304,
>              "transferred":3720,
>              "total-time":12345,
> +            "expected-downtime":12345,
>              "duplicate":10,
>              "normal":3333,
>              "normal-bytes":3412992
> 

Consider making the save_live functions return the expected downtime in
an int64_t* argument.  The loop then can sum all the expected downtimes
and store them in s->expected_downtime, removing the need for patch 6.

Paolo

Re: [Qemu-devel] [PATCH 07/41] migration: print expected downtime in info migrate

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 21/09/2012 10:47, Juan Quintela ha scritto:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> Consider making the save_live functions return the expected downtime in
> an int64_t* argument.  The loop then can sum all the expected downtimes
> and store them in s->expected_downtime, removing the need for patch 6.

If you look at the migration thread patches, I change the functions to
return the "pending amount".  And the migration thread does the
bandwidth calculations.  It is just impossible to do the right
bandwidth calculation locally.

Later, Juan.

[Qemu-devel] [PATCH 08/41] savevm: Factorize ram globals reset in its own function

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 52ccc7b..57f7f1a 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -482,6 +482,14 @@ static void ram_migration_cancel(void *opaque)
     migration_end();
 }

+
+static void reset_ram_globals(void)
+{
+    last_block = NULL;
+    last_offset = 0;
+    sort_ram_list();
+}
+
 #define MAX_WAIT 50 /* ms, half buffered_file limit */

 static int ram_save_setup(QEMUFile *f, void *opaque)
@@ -491,9 +499,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)

     memory_global_sync_dirty_bitmap(get_system_memory());
     bytes_transferred = 0;
-    last_block = NULL;
-    last_offset = 0;
-    sort_ram_list();
+    reset_ram_globals();

     if (migrate_use_xbzrle()) {
         XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 08/41] savevm: Factorize ram globals reset in its own function

[Orit Wasserman]

On 09/21/2012 11:47 AM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 52ccc7b..57f7f1a 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -482,6 +482,14 @@ static void ram_migration_cancel(void *opaque)
>      migration_end();
>  }
> 
> +
> +static void reset_ram_globals(void)
> +{
> +    last_block = NULL;
> +    last_offset = 0;
> +    sort_ram_list();
> +}
> +
>  #define MAX_WAIT 50 /* ms, half buffered_file limit */
> 
>  static int ram_save_setup(QEMUFile *f, void *opaque)
> @@ -491,9 +499,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> 
>      memory_global_sync_dirty_bitmap(get_system_memory());
>      bytes_transferred = 0;
> -    last_block = NULL;
> -    last_offset = 0;
> -    sort_ram_list();
> +    reset_ram_globals();
> 
>      if (migrate_use_xbzrle()) {
>          XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
> 

Reviewed-by: Orit Wasserman <owasserm@redhat.com>

[Qemu-devel] [PATCH 09/41] ram: introduce migration_bitmap_set_dirty()

[Juan Quintela]

It just marks a region of memory as dirty.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 57f7f1a..b2dcc24 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -332,6 +332,18 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
 static RAMBlock *last_block;
 static ram_addr_t last_offset;

+static inline void migration_bitmap_set_dirty(MemoryRegion *mr, int length)
+{
+    ram_addr_t addr;
+
+    for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
+        if (!memory_region_get_dirty(mr, addr, TARGET_PAGE_SIZE,
+                                     DIRTY_MEMORY_MIGRATION)) {
+            memory_region_set_dirty(mr, addr, TARGET_PAGE_SIZE);
+        }
+    }
+}
+
 /*
  * ram_save_block: Writes a page of memory to the stream f
  *
@@ -494,7 +506,6 @@ static void reset_ram_globals(void)

 static int ram_save_setup(QEMUFile *f, void *opaque)
 {
-    ram_addr_t addr;
     RAMBlock *block;

     memory_global_sync_dirty_bitmap(get_system_memory());
@@ -516,12 +527,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)

     /* Make sure all dirty bits are set */
     QLIST_FOREACH(block, &ram_list.blocks, next) {
-        for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
-            if (!memory_region_get_dirty(block->mr, addr, TARGET_PAGE_SIZE,
-                                         DIRTY_MEMORY_MIGRATION)) {
-                memory_region_set_dirty(block->mr, addr, TARGET_PAGE_SIZE);
-            }
-        }
+        migration_bitmap_set_dirty(block->mr, block->length);
     }

     memory_global_dirty_log_start();
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 09/41] ram: introduce migration_bitmap_set_dirty()

[Orit Wasserman]

On 09/21/2012 11:47 AM, Juan Quintela wrote:
> It just marks a region of memory as dirty.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 57f7f1a..b2dcc24 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -332,6 +332,18 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
>  static RAMBlock *last_block;
>  static ram_addr_t last_offset;
> 
> +static inline void migration_bitmap_set_dirty(MemoryRegion *mr, int length)
> +{
> +    ram_addr_t addr;
> +
> +    for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
> +        if (!memory_region_get_dirty(mr, addr, TARGET_PAGE_SIZE,
> +                                     DIRTY_MEMORY_MIGRATION)) {
> +            memory_region_set_dirty(mr, addr, TARGET_PAGE_SIZE);
> +        }
> +    }
> +}
> +
>  /*
>   * ram_save_block: Writes a page of memory to the stream f
>   *
> @@ -494,7 +506,6 @@ static void reset_ram_globals(void)
> 
>  static int ram_save_setup(QEMUFile *f, void *opaque)
>  {
> -    ram_addr_t addr;
>      RAMBlock *block;
> 
>      memory_global_sync_dirty_bitmap(get_system_memory());
> @@ -516,12 +527,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> 
>      /* Make sure all dirty bits are set */
>      QLIST_FOREACH(block, &ram_list.blocks, next) {
> -        for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
> -            if (!memory_region_get_dirty(block->mr, addr, TARGET_PAGE_SIZE,
> -                                         DIRTY_MEMORY_MIGRATION)) {
> -                memory_region_set_dirty(block->mr, addr, TARGET_PAGE_SIZE);
> -            }
> -        }
> +        migration_bitmap_set_dirty(block->mr, block->length);
>      }
> 
>      memory_global_dirty_log_start();
> 
Reviewed-by: Orit Wasserman <owasserm@redhat.com>

[Qemu-devel] [PATCH 10/41] ram: Introduce migration_bitmap_test_and_reset_dirty()

[Juan Quintela]

It just test if the dirty bit is set, and clears it.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index b2dcc24..acc057f 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -332,6 +332,19 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
 static RAMBlock *last_block;
 static ram_addr_t last_offset;

+static inline bool migration_bitmap_test_and_reset_dirty(MemoryRegion *mr,
+                                                         ram_addr_t offset)
+{
+    bool ret = memory_region_get_dirty(mr, offset, TARGET_PAGE_SIZE,
+                                       DIRTY_MEMORY_MIGRATION);
+
+    if (ret) {
+        memory_region_reset_dirty(mr, offset, TARGET_PAGE_SIZE,
+                                  DIRTY_MEMORY_MIGRATION);
+    }
+    return ret;
+}
+
 static inline void migration_bitmap_set_dirty(MemoryRegion *mr, int length)
 {
     ram_addr_t addr;
@@ -365,14 +378,10 @@ static int ram_save_block(QEMUFile *f, bool last_stage)

     do {
         mr = block->mr;
-        if (memory_region_get_dirty(mr, offset, TARGET_PAGE_SIZE,
-                                    DIRTY_MEMORY_MIGRATION)) {
+        if (migration_bitmap_test_and_reset_dirty(mr, offset)) {
             uint8_t *p;
             int cont = (block == last_block) ? RAM_SAVE_FLAG_CONTINUE : 0;

-            memory_region_reset_dirty(mr, offset, TARGET_PAGE_SIZE,
-                                      DIRTY_MEMORY_MIGRATION);
-
             p = memory_region_get_ram_ptr(mr) + offset;

             if (is_dup_page(p)) {
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 10/41] ram: Introduce migration_bitmap_test_and_reset_dirty()

[Orit Wasserman]

On 09/21/2012 11:47 AM, Juan Quintela wrote:
> It just test if the dirty bit is set, and clears it.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index b2dcc24..acc057f 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -332,6 +332,19 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
>  static RAMBlock *last_block;
>  static ram_addr_t last_offset;
> 
> +static inline bool migration_bitmap_test_and_reset_dirty(MemoryRegion *mr,
> +                                                         ram_addr_t offset)
> +{
> +    bool ret = memory_region_get_dirty(mr, offset, TARGET_PAGE_SIZE,
> +                                       DIRTY_MEMORY_MIGRATION);
> +
> +    if (ret) {
> +        memory_region_reset_dirty(mr, offset, TARGET_PAGE_SIZE,
> +                                  DIRTY_MEMORY_MIGRATION);
> +    }
> +    return ret;
> +}
> +
>  static inline void migration_bitmap_set_dirty(MemoryRegion *mr, int length)
>  {
>      ram_addr_t addr;
> @@ -365,14 +378,10 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
> 
>      do {
>          mr = block->mr;
> -        if (memory_region_get_dirty(mr, offset, TARGET_PAGE_SIZE,
> -                                    DIRTY_MEMORY_MIGRATION)) {
> +        if (migration_bitmap_test_and_reset_dirty(mr, offset)) {
>              uint8_t *p;
>              int cont = (block == last_block) ? RAM_SAVE_FLAG_CONTINUE : 0;
> 
> -            memory_region_reset_dirty(mr, offset, TARGET_PAGE_SIZE,
> -                                      DIRTY_MEMORY_MIGRATION);
> -
>              p = memory_region_get_ram_ptr(mr) + offset;
> 
>              if (is_dup_page(p)) {
> 

Reviewed-by: Orit Wasserman <owasserm@redhat.com>

[Qemu-devel] [PATCH 11/41] ram: Export last_ram_offset()

[Juan Quintela]

Is the only way of knowing the RAM size.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 cpu-all.h | 2 ++
 exec.c    | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/cpu-all.h b/cpu-all.h
index 74d3681..5408782 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -517,6 +517,8 @@ extern int mem_prealloc;
 void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
 #endif /* !CONFIG_USER_ONLY */

+ram_addr_t last_ram_offset(void);
+
 int cpu_memory_rw_debug(CPUArchState *env, target_ulong addr,
                         uint8_t *buf, int len, int is_write);

diff --git a/exec.c b/exec.c
index f22e9e6..ad2cc2e 100644
--- a/exec.c
+++ b/exec.c
@@ -2464,7 +2464,7 @@ static ram_addr_t find_ram_offset(ram_addr_t size)
     return offset;
 }

-static ram_addr_t last_ram_offset(void)
+ram_addr_t last_ram_offset(void)
 {
     RAMBlock *block;
     ram_addr_t last = 0;
-- 
1.7.11.4

[Qemu-devel] [PATCH 12/41] ram: introduce migration_bitmap_sync()

[Juan Quintela]

Helper that we use each time that we need to syncronize the migration
bitmap with the other dirty bitmaps.

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

diff --git a/arch_init.c b/arch_init.c
index acc057f..a58e8c3 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -357,6 +357,12 @@ static inline void migration_bitmap_set_dirty(MemoryRegion *mr, int length)
     }
 }

+static void migration_bitmap_sync(void)
+{
+    memory_global_sync_dirty_bitmap(get_system_memory());
+}
+
+
 /*
  * ram_save_block: Writes a page of memory to the stream f
  *
@@ -614,7 +620,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
             expected_downtime, migrate_max_downtime());

     if (expected_downtime <= migrate_max_downtime()) {
-        memory_global_sync_dirty_bitmap(get_system_memory());
+        migration_bitmap_sync();
         expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
         s->expected_downtime = expected_downtime / 1000000; /* ns -> ms */

@@ -625,7 +631,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)

 static int ram_save_complete(QEMUFile *f, void *opaque)
 {
-    memory_global_sync_dirty_bitmap(get_system_memory());
+    migration_bitmap_sync();

     /* try transferring iterative blocks of memory */

-- 
1.7.11.4

[Qemu-devel] [PATCH 13/41] ram: create trace event for migration sync bitmap

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c  | 6 ++++++
 trace-events | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/arch_init.c b/arch_init.c
index a58e8c3..6e0d7c4 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -45,6 +45,7 @@
 #include "hw/pcspk.h"
 #include "qemu/page_cache.h"
 #include "qmp-commands.h"
+#include "trace.h"

 #ifdef DEBUG_ARCH_INIT
 #define DPRINTF(fmt, ...) \
@@ -359,7 +360,12 @@ static inline void migration_bitmap_set_dirty(MemoryRegion *mr, int length)

 static void migration_bitmap_sync(void)
 {
+    uint64_t num_dirty_pages_init = ram_list.dirty_pages;
+
+    trace_migration_bitmap_sync_start();
     memory_global_sync_dirty_bitmap(get_system_memory());
+    trace_migration_bitmap_sync_end(ram_list.dirty_pages
+                                    - num_dirty_pages_init);
 }


diff --git a/trace-events b/trace-events
index b48fe2d..2666191 100644
--- a/trace-events
+++ b/trace-events
@@ -914,6 +914,10 @@ ppm_save(const char *filename, void *display_surface) "%s surface=%p"
 savevm_section_start(void) ""
 savevm_section_end(unsigned int section_id) "section_id %u"

+# arch_init.c
+migration_bitmap_sync_start(void) ""
+migration_bitmap_sync_end(uint64_t dirty_pages) "dirty_pages %" PRIu64""
+
 # hw/qxl.c
 disable qxl_interface_set_mm_time(int qid, uint32_t mm_time) "%d %d"
 disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
-- 
1.7.11.4

[Qemu-devel] [PATCH 14/41] Separate migration bitmap

[Juan Quintela]

This patch creates a migration bitmap, which is periodically kept in
sync with the qemu bitmap. A separate copy of the dirty bitmap for the
migration limits the amount of concurrent access to the qemu bitmap
from iothread and migration thread (which requires taking the big
lock).

We use the qemu bitmap type.  We have to "undo" the dirty_pages
counting optimization on the general dirty bitmap and do the counting
optimization with the migration local bitmap.

Signed-off-by: Umesh Deshpande <udeshpan@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c     | 63 +++++++++++++++++++++++++++++++++++++++------------------
 cpu-all.h       |  1 -
 exec-obsolete.h | 10 ---------
 3 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 6e0d7c4..0279d06 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -31,6 +31,8 @@
 #include "config.h"
 #include "monitor.h"
 #include "sysemu.h"
+#include "bitops.h"
+#include "bitmap.h"
 #include "arch_init.h"
 #include "audio/audio.h"
 #include "hw/pc.h"
@@ -332,39 +334,57 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,

 static RAMBlock *last_block;
 static ram_addr_t last_offset;
+static unsigned long *migration_bitmap;
+static uint64_t migration_dirty_pages;

 static inline bool migration_bitmap_test_and_reset_dirty(MemoryRegion *mr,
                                                          ram_addr_t offset)
 {
-    bool ret = memory_region_get_dirty(mr, offset, TARGET_PAGE_SIZE,
-                                       DIRTY_MEMORY_MIGRATION);
+    bool ret;
+    int nr = (mr->ram_addr + offset) >> TARGET_PAGE_BITS;
+
+    ret = test_and_clear_bit(nr, migration_bitmap);

     if (ret) {
-        memory_region_reset_dirty(mr, offset, TARGET_PAGE_SIZE,
-                                  DIRTY_MEMORY_MIGRATION);
+        migration_dirty_pages--;
     }
     return ret;
 }

-static inline void migration_bitmap_set_dirty(MemoryRegion *mr, int length)
+static inline bool migration_bitmap_set_dirty(MemoryRegion *mr,
+                                              ram_addr_t offset)
 {
-    ram_addr_t addr;
+    bool ret;
+    int nr = (mr->ram_addr + offset) >> TARGET_PAGE_BITS;

-    for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
-        if (!memory_region_get_dirty(mr, addr, TARGET_PAGE_SIZE,
-                                     DIRTY_MEMORY_MIGRATION)) {
-            memory_region_set_dirty(mr, addr, TARGET_PAGE_SIZE);
-        }
+    ret = test_and_set_bit(nr, migration_bitmap);
+
+    if (!ret) {
+        migration_dirty_pages++;
     }
+    return ret;
 }

 static void migration_bitmap_sync(void)
 {
-    uint64_t num_dirty_pages_init = ram_list.dirty_pages;
+    RAMBlock *block;
+    ram_addr_t addr;
+    uint64_t num_dirty_pages_init = migration_dirty_pages;

     trace_migration_bitmap_sync_start();
     memory_global_sync_dirty_bitmap(get_system_memory());
-    trace_migration_bitmap_sync_end(ram_list.dirty_pages
+
+    QLIST_FOREACH(block, &ram_list.blocks, next) {
+        for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
+            if (memory_region_get_dirty(block->mr, addr, TARGET_PAGE_SIZE,
+                                        DIRTY_MEMORY_MIGRATION)) {
+                migration_bitmap_set_dirty(block->mr, addr);
+            }
+        }
+        memory_region_reset_dirty(block->mr, 0, block->length,
+                                  DIRTY_MEMORY_MIGRATION);
+    }
+    trace_migration_bitmap_sync_end(migration_dirty_pages
                                     - num_dirty_pages_init);
 }

@@ -443,7 +463,7 @@ static uint64_t bytes_transferred;

 static ram_addr_t ram_save_remaining(void)
 {
-    return ram_list.dirty_pages;
+    return migration_dirty_pages;
 }

 uint64_t ram_bytes_remaining(void)
@@ -528,8 +548,13 @@ static void reset_ram_globals(void)
 static int ram_save_setup(QEMUFile *f, void *opaque)
 {
     RAMBlock *block;
+    int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;

-    memory_global_sync_dirty_bitmap(get_system_memory());
+    migration_bitmap = bitmap_new(ram_pages);
+    bitmap_set(migration_bitmap, 1, ram_pages);
+    migration_dirty_pages = ram_pages;
+
+    migration_bitmap_sync();
     bytes_transferred = 0;
     reset_ram_globals();

@@ -546,11 +571,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
         acct_clear();
     }

-    /* Make sure all dirty bits are set */
-    QLIST_FOREACH(block, &ram_list.blocks, next) {
-        migration_bitmap_set_dirty(block->mr, block->length);
-    }
-
     memory_global_dirty_log_start();

     qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
@@ -656,6 +676,9 @@ static int ram_save_complete(QEMUFile *f, void *opaque)

     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);

+    g_free(migration_bitmap);
+    migration_bitmap = NULL;
+
     return 0;
 }

diff --git a/cpu-all.h b/cpu-all.h
index 5408782..db25f73 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -497,7 +497,6 @@ typedef struct RAMBlock {
 typedef struct RAMList {
     uint8_t *phys_dirty;
     QLIST_HEAD(, RAMBlock) blocks;
-    uint64_t dirty_pages;
 } RAMList;
 extern RAMList ram_list;

diff --git a/exec-obsolete.h b/exec-obsolete.h
index c099256..f8ffce6 100644
--- a/exec-obsolete.h
+++ b/exec-obsolete.h
@@ -74,11 +74,6 @@ static inline int cpu_physical_memory_get_dirty(ram_addr_t start,
 static inline int cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
                                                       int dirty_flags)
 {
-    if ((dirty_flags & MIGRATION_DIRTY_FLAG) &&
-        !cpu_physical_memory_get_dirty(addr, TARGET_PAGE_SIZE,
-                                       MIGRATION_DIRTY_FLAG)) {
-        ram_list.dirty_pages++;
-    }
     return ram_list.phys_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
 }

@@ -92,11 +87,6 @@ static inline int cpu_physical_memory_clear_dirty_flags(ram_addr_t addr,
 {
     int mask = ~dirty_flags;

-    if ((dirty_flags & MIGRATION_DIRTY_FLAG) &&
-        cpu_physical_memory_get_dirty(addr, TARGET_PAGE_SIZE,
-                                      MIGRATION_DIRTY_FLAG)) {
-        ram_list.dirty_pages--;
-    }
     return ram_list.phys_dirty[addr >> TARGET_PAGE_BITS] &= mask;
 }

-- 
1.7.11.4

[Qemu-devel] [PATCH 15/41] migration: Add dirty_pages_rate to query migrate output

[Juan Quintela]

It indicates how many pages were dirtied during the last second.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c      | 18 ++++++++++++++++++
 hmp.c            |  4 ++++
 migration.c      |  2 ++
 migration.h      |  1 +
 qapi-schema.json |  8 ++++++--
 5 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 0279d06..d96e888 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -370,6 +370,14 @@ static void migration_bitmap_sync(void)
     RAMBlock *block;
     ram_addr_t addr;
     uint64_t num_dirty_pages_init = migration_dirty_pages;
+    MigrationState *s = migrate_get_current();
+    static int64_t start_time;
+    static int64_t num_dirty_pages_period;
+    int64_t end_time;
+
+    if (!start_time) {
+        start_time = qemu_get_clock_ms(rt_clock);
+    }

     trace_migration_bitmap_sync_start();
     memory_global_sync_dirty_bitmap(get_system_memory());
@@ -386,6 +394,16 @@ static void migration_bitmap_sync(void)
     }
     trace_migration_bitmap_sync_end(migration_dirty_pages
                                     - num_dirty_pages_init);
+    num_dirty_pages_period += migration_dirty_pages - num_dirty_pages_init;
+    end_time = qemu_get_clock_ms(rt_clock);
+
+    /* more than 1 second = 1000 millisecons */
+    if (end_time > start_time + 1000) {
+        s->dirty_pages_rate = num_dirty_pages_period * 1000
+            / (end_time - start_time);
+        start_time = end_time;
+        num_dirty_pages_period = 0;
+    }
 }


diff --git a/hmp.c b/hmp.c
index 71c9292..67a529a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -175,6 +175,10 @@ void hmp_info_migrate(Monitor *mon)
                        info->ram->normal);
         monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",
                        info->ram->normal_bytes >> 10);
+        if (info->ram->dirty_pages_rate) {
+            monitor_printf(mon, "dirty pages rate: %" PRIu64 " pages\n",
+                           info->ram->dirty_pages_rate);
+        }
     }

     if (info->has_disk) {
diff --git a/migration.c b/migration.c
index 62c8fe9..05634d5 100644
--- a/migration.c
+++ b/migration.c
@@ -180,6 +180,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->ram->duplicate = dup_mig_pages_transferred();
         info->ram->normal = norm_mig_pages_transferred();
         info->ram->normal_bytes = norm_mig_bytes_transferred();
+        info->ram->dirty_pages_rate = s->dirty_pages_rate;
+

         if (blk_mig_active()) {
             info->has_disk = true;
diff --git a/migration.h b/migration.h
index 552200c..66d7f68 100644
--- a/migration.h
+++ b/migration.h
@@ -42,6 +42,7 @@ struct MigrationState
     int64_t total_time;
     int64_t downtime;
     int64_t expected_downtime;
+    int64_t dirty_pages_rate;
     bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
     int64_t xbzrle_cache_size;
 };
diff --git a/qapi-schema.json b/qapi-schema.json
index b8a1244..4a9ae52 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -358,13 +358,17 @@
 #
 # @normal : number of normal pages (since 1.2)
 #
-# @normal-bytes : number of normal bytes sent (since 1.2)
+# @normal-bytes: number of normal bytes sent (since 1.2)
+#
+# @dirty-pages-rate: number of pages dirtied by second by the
+#        guest (since 1.3)
 #
 # Since: 0.14.0
 ##
 { 'type': 'MigrationStats',
   'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
-           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int' } }
+           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int',
+           'dirty-pages-rate' : 'int' } }

 ##
 # @XBZRLECacheStats
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 15/41] migration: Add dirty_pages_rate to query migrate output

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> It indicates how many pages were dirtied during the last second.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c      | 18 ++++++++++++++++++
>  hmp.c            |  4 ++++
>  migration.c      |  2 ++
>  migration.h      |  1 +
>  qapi-schema.json |  8 ++++++--
>  5 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 0279d06..d96e888 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -370,6 +370,14 @@ static void migration_bitmap_sync(void)
>      RAMBlock *block;
>      ram_addr_t addr;
>      uint64_t num_dirty_pages_init = migration_dirty_pages;
> +    MigrationState *s = migrate_get_current();
> +    static int64_t start_time;
> +    static int64_t num_dirty_pages_period;
> +    int64_t end_time;
> +
> +    if (!start_time) {
> +        start_time = qemu_get_clock_ms(rt_clock);
> +    }
> 
>      trace_migration_bitmap_sync_start();
>      memory_global_sync_dirty_bitmap(get_system_memory());
> @@ -386,6 +394,16 @@ static void migration_bitmap_sync(void)
>      }
>      trace_migration_bitmap_sync_end(migration_dirty_pages
>                                      - num_dirty_pages_init);
> +    num_dirty_pages_period += migration_dirty_pages - num_dirty_pages_init;
> +    end_time = qemu_get_clock_ms(rt_clock);
> +
> +    /* more than 1 second = 1000 millisecons */
> +    if (end_time > start_time + 1000) {
> +        s->dirty_pages_rate = num_dirty_pages_period * 1000
> +            / (end_time - start_time);
> +        start_time = end_time;
> +        num_dirty_pages_period = 0;
> +    }
>  }

Ok, this makes use of patch 6 as well.  I'd still prefer the interface
change to the save_live functions for cumulating the expected downtime
across all save_live functions, but feel free to ignore me.

Paolo

> 
> diff --git a/hmp.c b/hmp.c
> index 71c9292..67a529a 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -175,6 +175,10 @@ void hmp_info_migrate(Monitor *mon)
>                         info->ram->normal);
>          monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",
>                         info->ram->normal_bytes >> 10);
> +        if (info->ram->dirty_pages_rate) {
> +            monitor_printf(mon, "dirty pages rate: %" PRIu64 " pages\n",
> +                           info->ram->dirty_pages_rate);
> +        }
>      }
> 
>      if (info->has_disk) {
> diff --git a/migration.c b/migration.c
> index 62c8fe9..05634d5 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -180,6 +180,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->ram->duplicate = dup_mig_pages_transferred();
>          info->ram->normal = norm_mig_pages_transferred();
>          info->ram->normal_bytes = norm_mig_bytes_transferred();
> +        info->ram->dirty_pages_rate = s->dirty_pages_rate;
> +
> 
>          if (blk_mig_active()) {
>              info->has_disk = true;
> diff --git a/migration.h b/migration.h
> index 552200c..66d7f68 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -42,6 +42,7 @@ struct MigrationState
>      int64_t total_time;
>      int64_t downtime;
>      int64_t expected_downtime;
> +    int64_t dirty_pages_rate;
>      bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
>      int64_t xbzrle_cache_size;
>  };
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b8a1244..4a9ae52 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -358,13 +358,17 @@
>  #
>  # @normal : number of normal pages (since 1.2)
>  #
> -# @normal-bytes : number of normal bytes sent (since 1.2)
> +# @normal-bytes: number of normal bytes sent (since 1.2)
> +#
> +# @dirty-pages-rate: number of pages dirtied by second by the
> +#        guest (since 1.3)
>  #
>  # Since: 0.14.0
>  ##
>  { 'type': 'MigrationStats',
>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
> -           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int' } }
> +           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int',
> +           'dirty-pages-rate' : 'int' } }
> 
>  ##
>  # @XBZRLECacheStats
> 

[Qemu-devel] [PATCH 16/41] BufferedFile: append, then flush

[Juan Quintela]

From: Paolo Bonzini <pbonzini@redhat.com>

Simplify the logic for pushing data from the buffer to the output
pipe/socket.  This also matches more closely what will be the
operation of the migration thread.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 50 +++++++++++---------------------------------------
 1 file changed, 11 insertions(+), 39 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 4148abb..7155800 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -75,7 +75,7 @@ static void buffered_flush(QEMUFileBuffered *s)

     DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);

-    while (offset < s->buffer_size) {
+    while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
         ssize_t ret;

         ret = s->put_buffer(s->opaque, s->buffer + offset,
@@ -93,6 +93,7 @@ static void buffered_flush(QEMUFileBuffered *s)
         } else {
             DPRINTF("flushed %zd byte(s)\n", ret);
             offset += ret;
+            s->bytes_xfer += ret;
         }
     }

@@ -104,8 +105,7 @@ static void buffered_flush(QEMUFileBuffered *s)
 static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size)
 {
     QEMUFileBuffered *s = opaque;
-    int offset = 0, error;
-    ssize_t ret;
+    int error;

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

@@ -118,48 +118,22 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
     DPRINTF("unfreezing output\n");
     s->freeze_output = 0;

-    buffered_flush(s);
-
-    while (!s->freeze_output && offset < size) {
-        if (s->bytes_xfer > s->xfer_limit) {
-            DPRINTF("transfer limit exceeded when putting\n");
-            break;
-        }
-
-        ret = s->put_buffer(s->opaque, buf + offset, size - offset);
-        if (ret == -EAGAIN) {
-            DPRINTF("backend not ready, freezing\n");
-            s->freeze_output = 1;
-            break;
-        }
-
-        if (ret <= 0) {
-            DPRINTF("error putting\n");
-            qemu_file_set_error(s->file, ret);
-            offset = -EINVAL;
-            break;
-        }
-
-        DPRINTF("put %zd byte(s)\n", ret);
-        offset += ret;
-        s->bytes_xfer += ret;
-    }
-
-    if (offset >= 0) {
+    if (size > 0) {
         DPRINTF("buffering %d bytes\n", size - offset);
-        buffered_append(s, buf + offset, size - offset);
-        offset = size;
+        buffered_append(s, buf, size);
     }

+    buffered_flush(s);
+
     if (pos == 0 && size == 0) {
         DPRINTF("file is ready\n");
-        if (s->bytes_xfer <= s->xfer_limit) {
+        if (!s->freeze_output && s->bytes_xfer < s->xfer_limit) {
             DPRINTF("notifying client\n");
             s->put_ready(s->opaque);
         }
     }

-    return offset;
+    return size;
 }

 static int buffered_close(void *opaque)
@@ -169,6 +143,7 @@ static int buffered_close(void *opaque)

     DPRINTF("closing\n");

+    s->xfer_limit = INT_MAX;
     while (!qemu_file_get_error(s->file) && s->buffer_size) {
         buffered_flush(s);
         if (s->freeze_output)
@@ -248,10 +223,7 @@ static void buffered_rate_tick(void *opaque)

     s->bytes_xfer = 0;

-    buffered_flush(s);
-
-    /* Add some checks around this */
-    s->put_ready(s->opaque);
+    buffered_put_buffer(s, NULL, 0, 0);
 }

 QEMUFile *qemu_fopen_ops_buffered(void *opaque,
-- 
1.7.11.4

[Qemu-devel] [PATCH 17/41] buffered_file: rename opaque to migration_state

[Juan Quintela]

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

diff --git a/buffered_file.c b/buffered_file.c
index 7155800..33b700b 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -27,7 +27,7 @@ typedef struct QEMUFileBuffered
     BufferedPutReadyFunc *put_ready;
     BufferedWaitForUnfreezeFunc *wait_for_unfreeze;
     BufferedCloseFunc *close;
-    void *opaque;
+    void *migration_state;
     QEMUFile *file;
     int freeze_output;
     size_t bytes_xfer;
@@ -78,7 +78,7 @@ static void buffered_flush(QEMUFileBuffered *s)
     while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
         ssize_t ret;

-        ret = s->put_buffer(s->opaque, s->buffer + offset,
+        ret = s->put_buffer(s->migration_state, s->buffer + offset,
                             s->buffer_size - offset);
         if (ret == -EAGAIN) {
             DPRINTF("backend not ready, freezing\n");
@@ -129,7 +129,7 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         DPRINTF("file is ready\n");
         if (!s->freeze_output && s->bytes_xfer < s->xfer_limit) {
             DPRINTF("notifying client\n");
-            s->put_ready(s->opaque);
+            s->put_ready(s->migration_state);
         }
     }

@@ -147,10 +147,10 @@ static int buffered_close(void *opaque)
     while (!qemu_file_get_error(s->file) && s->buffer_size) {
         buffered_flush(s);
         if (s->freeze_output)
-            s->wait_for_unfreeze(s->opaque);
+            s->wait_for_unfreeze(s->migration_state);
     }

-    ret = s->close(s->opaque);
+    ret = s->close(s->migration_state);

     qemu_del_timer(s->timer);
     qemu_free_timer(s->timer);
@@ -237,7 +237,7 @@ QEMUFile *qemu_fopen_ops_buffered(void *opaque,

     s = g_malloc0(sizeof(*s));

-    s->opaque = opaque;
+    s->migration_state = opaque;
     s->xfer_limit = bytes_per_sec / 10;
     s->put_buffer = put_buffer;
     s->put_ready = put_ready;
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 17/41] buffered_file: rename opaque to migration_state

[Orit Wasserman]

On 09/21/2012 11:47 AM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index 7155800..33b700b 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -27,7 +27,7 @@ typedef struct QEMUFileBuffered
>      BufferedPutReadyFunc *put_ready;
>      BufferedWaitForUnfreezeFunc *wait_for_unfreeze;
>      BufferedCloseFunc *close;
> -    void *opaque;
> +    void *migration_state;
Why ?
This doesn't have to be for migration use only.
Orit
>      QEMUFile *file;
>      int freeze_output;
>      size_t bytes_xfer;
> @@ -78,7 +78,7 @@ static void buffered_flush(QEMUFileBuffered *s)
>      while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
>          ssize_t ret;
> 
> -        ret = s->put_buffer(s->opaque, s->buffer + offset,
> +        ret = s->put_buffer(s->migration_state, s->buffer + offset,
>                              s->buffer_size - offset);
>          if (ret == -EAGAIN) {
>              DPRINTF("backend not ready, freezing\n");
> @@ -129,7 +129,7 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
>          DPRINTF("file is ready\n");
>          if (!s->freeze_output && s->bytes_xfer < s->xfer_limit) {
>              DPRINTF("notifying client\n");
> -            s->put_ready(s->opaque);
> +            s->put_ready(s->migration_state);
>          }
>      }
> 
> @@ -147,10 +147,10 @@ static int buffered_close(void *opaque)
>      while (!qemu_file_get_error(s->file) && s->buffer_size) {
>          buffered_flush(s);
>          if (s->freeze_output)
> -            s->wait_for_unfreeze(s->opaque);
> +            s->wait_for_unfreeze(s->migration_state);
>      }
> 
> -    ret = s->close(s->opaque);
> +    ret = s->close(s->migration_state);
> 
>      qemu_del_timer(s->timer);
>      qemu_free_timer(s->timer);
> @@ -237,7 +237,7 @@ QEMUFile *qemu_fopen_ops_buffered(void *opaque,
> 
>      s = g_malloc0(sizeof(*s));
> 
> -    s->opaque = opaque;
> +    s->migration_state = opaque;
>      s->xfer_limit = bytes_per_sec / 10;
>      s->put_buffer = put_buffer;
>      s->put_ready = put_ready;
> 

Re: [Qemu-devel] [PATCH 17/41] buffered_file: rename opaque to migration_state

[Juan Quintela]

Orit Wasserman <owasserm@redhat.com> wrote:
> On 09/21/2012 11:47 AM, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  buffered_file.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/buffered_file.c b/buffered_file.c
>> index 7155800..33b700b 100644
>> --- a/buffered_file.c
>> +++ b/buffered_file.c
>> @@ -27,7 +27,7 @@ typedef struct QEMUFileBuffered
>>      BufferedPutReadyFunc *put_ready;
>>      BufferedWaitForUnfreezeFunc *wait_for_unfreeze;
>>      BufferedCloseFunc *close;
>> -    void *opaque;
>> +    void *migration_state;
> Why ?
> This doesn't have to be for migration use only.

Idea is to remove it altogether.  Not useful anymore once that migration
thread is there.

Later, Juan.

[Qemu-devel] [PATCH 18/41] buffered_file: opaque is MigrationState

[Juan Quintela]

It always have that type, just change it.

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

diff --git a/buffered_file.c b/buffered_file.c
index 33b700b..59d952d 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -27,7 +27,7 @@ typedef struct QEMUFileBuffered
     BufferedPutReadyFunc *put_ready;
     BufferedWaitForUnfreezeFunc *wait_for_unfreeze;
     BufferedCloseFunc *close;
-    void *migration_state;
+    MigrationState *migration_state;
     QEMUFile *file;
     int freeze_output;
     size_t bytes_xfer;
@@ -226,7 +226,7 @@ static void buffered_rate_tick(void *opaque)
     buffered_put_buffer(s, NULL, 0, 0);
 }

-QEMUFile *qemu_fopen_ops_buffered(void *opaque,
+QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
                                   size_t bytes_per_sec,
                                   BufferedPutFunc *put_buffer,
                                   BufferedPutReadyFunc *put_ready,
@@ -237,7 +237,7 @@ QEMUFile *qemu_fopen_ops_buffered(void *opaque,

     s = g_malloc0(sizeof(*s));

-    s->migration_state = opaque;
+    s->migration_state = migration_state;
     s->xfer_limit = bytes_per_sec / 10;
     s->put_buffer = put_buffer;
     s->put_ready = put_ready;
diff --git a/buffered_file.h b/buffered_file.h
index 98d358b..39f7fa0 100644
--- a/buffered_file.h
+++ b/buffered_file.h
@@ -15,13 +15,15 @@
 #define QEMU_BUFFERED_FILE_H

 #include "hw/hw.h"
+#include "migration.h"

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

-QEMUFile *qemu_fopen_ops_buffered(void *opaque, size_t xfer_limit,
+QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
+                                  size_t xfer_limit,
                                   BufferedPutFunc *put_buffer,
                                   BufferedPutReadyFunc *put_ready,
                                   BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 18/41] buffered_file: opaque is MigrationState

[Orit Wasserman]

On 09/21/2012 11:47 AM, Juan Quintela wrote:
> It always have that type, just change it.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 6 +++---
>  buffered_file.h | 4 +++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index 33b700b..59d952d 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -27,7 +27,7 @@ typedef struct QEMUFileBuffered
>      BufferedPutReadyFunc *put_ready;
>      BufferedWaitForUnfreezeFunc *wait_for_unfreeze;
>      BufferedCloseFunc *close;
> -    void *migration_state;
> +    MigrationState *migration_state;
>      QEMUFile *file;
>      int freeze_output;
>      size_t bytes_xfer;
> @@ -226,7 +226,7 @@ static void buffered_rate_tick(void *opaque)
>      buffered_put_buffer(s, NULL, 0, 0);
>  }
> 
> -QEMUFile *qemu_fopen_ops_buffered(void *opaque,
> +QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
>                                    size_t bytes_per_sec,
>                                    BufferedPutFunc *put_buffer,
>                                    BufferedPutReadyFunc *put_ready,
> @@ -237,7 +237,7 @@ QEMUFile *qemu_fopen_ops_buffered(void *opaque,
> 
>      s = g_malloc0(sizeof(*s));
> 
> -    s->migration_state = opaque;
> +    s->migration_state = migration_state;
>      s->xfer_limit = bytes_per_sec / 10;
>      s->put_buffer = put_buffer;
>      s->put_ready = put_ready;
> diff --git a/buffered_file.h b/buffered_file.h
> index 98d358b..39f7fa0 100644
> --- a/buffered_file.h
> +++ b/buffered_file.h
> @@ -15,13 +15,15 @@
>  #define QEMU_BUFFERED_FILE_H
> 
>  #include "hw/hw.h"
> +#include "migration.h"
> 
>  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);
> 
> -QEMUFile *qemu_fopen_ops_buffered(void *opaque, size_t xfer_limit,
> +QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
> +                                  size_t xfer_limit,
>                                    BufferedPutFunc *put_buffer,
>                                    BufferedPutReadyFunc *put_ready,
>                                    BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
> 
Again why ? this is a general buffered file not just for migration use.

Orit

Re: [Qemu-devel] [PATCH 18/41] buffered_file: opaque is MigrationState

[Juan Quintela]

Orit Wasserman <owasserm@redhat.com> wrote:
> On 09/21/2012 11:47 AM, Juan Quintela wrote:
>> It always have that type, just change it.
>> -QEMUFile *qemu_fopen_ops_buffered(void *opaque, size_t xfer_limit,
>> +QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
>> +                                  size_t xfer_limit,
>>                                    BufferedPutFunc *put_buffer,
>>                                    BufferedPutReadyFunc *put_ready,
>>                                    BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
>> 
> Again why ? this is a general buffered file not just for migration use.

It is not used for anything else.  It is used only, and inlining it
removes one level of indirection.  All wins on my book.

Notice that once the migration-thread patches end, this file basically
dissapears (its only reason to exist was to allow non-blocking writes").

Later, Juan.

Re: [Qemu-devel] [PATCH 18/41] buffered_file: opaque is MigrationState

[Orit Wasserman]

On 09/28/2012 09:33 AM, Juan Quintela wrote:
> Orit Wasserman <owasserm@redhat.com> wrote:
>> On 09/21/2012 11:47 AM, Juan Quintela wrote:
>>> It always have that type, just change it.
>>> -QEMUFile *qemu_fopen_ops_buffered(void *opaque, size_t xfer_limit,
>>> +QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
>>> +                                  size_t xfer_limit,
>>>                                    BufferedPutFunc *put_buffer,
>>>                                    BufferedPutReadyFunc *put_ready,
>>>                                    BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
>>>
>> Again why ? this is a general buffered file not just for migration use.
> 
> It is not used for anything else.  It is used only, and inlining it
> removes one level of indirection.  All wins on my book.
> 
> Notice that once the migration-thread patches end, this file basically
> dissapears (its only reason to exist was to allow non-blocking writes").
> 
yes I read the second series and get it now (maybe more details in the commit comment can help).

Cheers,
Orit
> Later, Juan.
> 

[Qemu-devel] [PATCH 19/41] buffered_file: unfold migrate_fd_put_buffer

[Juan Quintela]

We only used it once, just remove the callback indirection

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 7 ++-----
 buffered_file.h | 2 --
 migration.c     | 6 ++----
 migration.h     | 3 +++
 4 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 59d952d..702a726 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -23,7 +23,6 @@

 typedef struct QEMUFileBuffered
 {
-    BufferedPutFunc *put_buffer;
     BufferedPutReadyFunc *put_ready;
     BufferedWaitForUnfreezeFunc *wait_for_unfreeze;
     BufferedCloseFunc *close;
@@ -78,8 +77,8 @@ static void buffered_flush(QEMUFileBuffered *s)
     while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
         ssize_t ret;

-        ret = s->put_buffer(s->migration_state, s->buffer + offset,
-                            s->buffer_size - offset);
+        ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset,
+                                    s->buffer_size - offset);
         if (ret == -EAGAIN) {
             DPRINTF("backend not ready, freezing\n");
             s->freeze_output = 1;
@@ -228,7 +227,6 @@ static void buffered_rate_tick(void *opaque)

 QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
                                   size_t bytes_per_sec,
-                                  BufferedPutFunc *put_buffer,
                                   BufferedPutReadyFunc *put_ready,
                                   BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
                                   BufferedCloseFunc *close)
@@ -239,7 +237,6 @@ QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,

     s->migration_state = migration_state;
     s->xfer_limit = bytes_per_sec / 10;
-    s->put_buffer = put_buffer;
     s->put_ready = put_ready;
     s->wait_for_unfreeze = wait_for_unfreeze;
     s->close = close;
diff --git a/buffered_file.h b/buffered_file.h
index 39f7fa0..ca7e62d 100644
--- a/buffered_file.h
+++ b/buffered_file.h
@@ -17,14 +17,12 @@
 #include "hw/hw.h"
 #include "migration.h"

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

 QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
                                   size_t xfer_limit,
-                                  BufferedPutFunc *put_buffer,
                                   BufferedPutReadyFunc *put_ready,
                                   BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
                                   BufferedCloseFunc *close);
diff --git a/migration.c b/migration.c
index 05634d5..a958f4f 100644
--- a/migration.c
+++ b/migration.c
@@ -293,10 +293,9 @@ static void migrate_fd_put_notify(void *opaque)
     }
 }

-static ssize_t migrate_fd_put_buffer(void *opaque, const void *data,
-                                     size_t size)
+ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
+                              size_t size)
 {
-    MigrationState *s = opaque;
     ssize_t ret;

     if (s->state != MIG_STATE_ACTIVE) {
@@ -434,7 +433,6 @@ void migrate_fd_connect(MigrationState *s)
     s->state = MIG_STATE_ACTIVE;
     s->file = qemu_fopen_ops_buffered(s,
                                       s->bandwidth_limit,
-                                      migrate_fd_put_buffer,
                                       migrate_fd_put_ready,
                                       migrate_fd_wait_for_unfreeze,
                                       migrate_fd_close);
diff --git a/migration.h b/migration.h
index 66d7f68..02d0219 100644
--- a/migration.h
+++ b/migration.h
@@ -78,6 +78,9 @@ void migrate_fd_error(MigrationState *s);

 void migrate_fd_connect(MigrationState *s);

+ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
+                              size_t size);
+
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
 bool migration_is_active(MigrationState *);
-- 
1.7.11.4

[Qemu-devel] [PATCH 20/41] buffered_file: unfold migrate_fd_put_ready

[Juan Quintela]

We only use it once, just remove the callback indirection.

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

diff --git a/buffered_file.c b/buffered_file.c
index 702a726..4c6a797 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -23,7 +23,6 @@

 typedef struct QEMUFileBuffered
 {
-    BufferedPutReadyFunc *put_ready;
     BufferedWaitForUnfreezeFunc *wait_for_unfreeze;
     BufferedCloseFunc *close;
     MigrationState *migration_state;
@@ -128,7 +127,7 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         DPRINTF("file is ready\n");
         if (!s->freeze_output && s->bytes_xfer < s->xfer_limit) {
             DPRINTF("notifying client\n");
-            s->put_ready(s->migration_state);
+            migrate_fd_put_ready(s->migration_state);
         }
     }

@@ -227,7 +226,6 @@ static void buffered_rate_tick(void *opaque)

 QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
                                   size_t bytes_per_sec,
-                                  BufferedPutReadyFunc *put_ready,
                                   BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
                                   BufferedCloseFunc *close)
 {
@@ -237,7 +235,6 @@ QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,

     s->migration_state = migration_state;
     s->xfer_limit = bytes_per_sec / 10;
-    s->put_ready = put_ready;
     s->wait_for_unfreeze = wait_for_unfreeze;
     s->close = close;

diff --git a/buffered_file.h b/buffered_file.h
index ca7e62d..dd239b3 100644
--- a/buffered_file.h
+++ b/buffered_file.h
@@ -17,13 +17,11 @@
 #include "hw/hw.h"
 #include "migration.h"

-typedef void (BufferedPutReadyFunc)(void *opaque);
 typedef void (BufferedWaitForUnfreezeFunc)(void *opaque);
 typedef int (BufferedCloseFunc)(void *opaque);

 QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
                                   size_t xfer_limit,
-                                  BufferedPutReadyFunc *put_ready,
                                   BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
                                   BufferedCloseFunc *close);

diff --git a/migration.c b/migration.c
index a958f4f..d0d1014 100644
--- a/migration.c
+++ b/migration.c
@@ -316,9 +316,8 @@ ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
     return ret;
 }

-static void migrate_fd_put_ready(void *opaque)
+void migrate_fd_put_ready(MigrationState *s)
 {
-    MigrationState *s = opaque;
     int ret;

     if (s->state != MIG_STATE_ACTIVE) {
@@ -433,7 +432,6 @@ void migrate_fd_connect(MigrationState *s)
     s->state = MIG_STATE_ACTIVE;
     s->file = qemu_fopen_ops_buffered(s,
                                       s->bandwidth_limit,
-                                      migrate_fd_put_ready,
                                       migrate_fd_wait_for_unfreeze,
                                       migrate_fd_close);

diff --git a/migration.h b/migration.h
index 02d0219..031c2ab 100644
--- a/migration.h
+++ b/migration.h
@@ -80,6 +80,7 @@ void migrate_fd_connect(MigrationState *s);

 ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
                               size_t size);
+void migrate_fd_put_ready(MigrationState *s);

 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
-- 
1.7.11.4

[Qemu-devel] [PATCH 21/41] buffered_file: unfold migrate_fd_put_buffer

[Juan Quintela]

We only used it once, just remove the callback indirection.

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

diff --git a/buffered_file.c b/buffered_file.c
index 4c6a797..d257496 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -23,7 +23,6 @@

 typedef struct QEMUFileBuffered
 {
-    BufferedWaitForUnfreezeFunc *wait_for_unfreeze;
     BufferedCloseFunc *close;
     MigrationState *migration_state;
     QEMUFile *file;
@@ -145,7 +144,7 @@ static int buffered_close(void *opaque)
     while (!qemu_file_get_error(s->file) && s->buffer_size) {
         buffered_flush(s);
         if (s->freeze_output)
-            s->wait_for_unfreeze(s->migration_state);
+            migrate_fd_wait_for_unfreeze(s->migration_state);
     }

     ret = s->close(s->migration_state);
@@ -226,7 +225,6 @@ static void buffered_rate_tick(void *opaque)

 QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
                                   size_t bytes_per_sec,
-                                  BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
                                   BufferedCloseFunc *close)
 {
     QEMUFileBuffered *s;
@@ -235,7 +233,6 @@ QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,

     s->migration_state = migration_state;
     s->xfer_limit = bytes_per_sec / 10;
-    s->wait_for_unfreeze = wait_for_unfreeze;
     s->close = close;

     s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
diff --git a/buffered_file.h b/buffered_file.h
index dd239b3..926e5c6 100644
--- a/buffered_file.h
+++ b/buffered_file.h
@@ -17,12 +17,10 @@
 #include "hw/hw.h"
 #include "migration.h"

-typedef void (BufferedWaitForUnfreezeFunc)(void *opaque);
 typedef int (BufferedCloseFunc)(void *opaque);

 QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
                                   size_t xfer_limit,
-                                  BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
                                   BufferedCloseFunc *close);

 #endif
diff --git a/migration.c b/migration.c
index d0d1014..add4632 100644
--- a/migration.c
+++ b/migration.c
@@ -368,9 +368,8 @@ static void migrate_fd_cancel(MigrationState *s)
     migrate_fd_cleanup(s);
 }

-static void migrate_fd_wait_for_unfreeze(void *opaque)
+void migrate_fd_wait_for_unfreeze(MigrationState *s)
 {
-    MigrationState *s = opaque;
     int ret;

     DPRINTF("wait for unfreeze\n");
@@ -432,7 +431,6 @@ void migrate_fd_connect(MigrationState *s)
     s->state = MIG_STATE_ACTIVE;
     s->file = qemu_fopen_ops_buffered(s,
                                       s->bandwidth_limit,
-                                      migrate_fd_wait_for_unfreeze,
                                       migrate_fd_close);

     DPRINTF("beginning savevm\n");
diff --git a/migration.h b/migration.h
index 031c2ab..d6341d6 100644
--- a/migration.h
+++ b/migration.h
@@ -81,6 +81,7 @@ void migrate_fd_connect(MigrationState *s);
 ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
                               size_t size);
 void migrate_fd_put_ready(MigrationState *s);
+void migrate_fd_wait_for_unfreeze(MigrationState *s);

 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 21/41] buffered_file: unfold migrate_fd_put_buffer

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> We only used it once, just remove the callback indirection.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 5 +----
>  buffered_file.h | 2 --
>  migration.c     | 4 +---
>  migration.h     | 1 +
>  4 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index 4c6a797..d257496 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -23,7 +23,6 @@
> 
>  typedef struct QEMUFileBuffered
>  {
> -    BufferedWaitForUnfreezeFunc *wait_for_unfreeze;

Should be "buffered_file: unfold migrate_fd_wait_for_unfreeze".
Otherwise looks good.

I misaddressed some answers just to Juan, omitting qemu-devel.  All
patches I haven't replied to publicly, up to this one, are Reviewed-by me.

Paolo

>      BufferedCloseFunc *close;
>      MigrationState *migration_state;
>      QEMUFile *file;
> @@ -145,7 +144,7 @@ static int buffered_close(void *opaque)
>      while (!qemu_file_get_error(s->file) && s->buffer_size) {
>          buffered_flush(s);
>          if (s->freeze_output)
> -            s->wait_for_unfreeze(s->migration_state);
> +            migrate_fd_wait_for_unfreeze(s->migration_state);
>      }
> 
>      ret = s->close(s->migration_state);
> @@ -226,7 +225,6 @@ static void buffered_rate_tick(void *opaque)
> 
>  QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
>                                    size_t bytes_per_sec,
> -                                  BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
>                                    BufferedCloseFunc *close)
>  {
>      QEMUFileBuffered *s;
> @@ -235,7 +233,6 @@ QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
> 
>      s->migration_state = migration_state;
>      s->xfer_limit = bytes_per_sec / 10;
> -    s->wait_for_unfreeze = wait_for_unfreeze;
>      s->close = close;
> 
>      s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
> diff --git a/buffered_file.h b/buffered_file.h
> index dd239b3..926e5c6 100644
> --- a/buffered_file.h
> +++ b/buffered_file.h
> @@ -17,12 +17,10 @@
>  #include "hw/hw.h"
>  #include "migration.h"
> 
> -typedef void (BufferedWaitForUnfreezeFunc)(void *opaque);
>  typedef int (BufferedCloseFunc)(void *opaque);
> 
>  QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
>                                    size_t xfer_limit,
> -                                  BufferedWaitForUnfreezeFunc *wait_for_unfreeze,
>                                    BufferedCloseFunc *close);
> 
>  #endif
> diff --git a/migration.c b/migration.c
> index d0d1014..add4632 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -368,9 +368,8 @@ static void migrate_fd_cancel(MigrationState *s)
>      migrate_fd_cleanup(s);
>  }
> 
> -static void migrate_fd_wait_for_unfreeze(void *opaque)
> +void migrate_fd_wait_for_unfreeze(MigrationState *s)
>  {
> -    MigrationState *s = opaque;
>      int ret;
> 
>      DPRINTF("wait for unfreeze\n");
> @@ -432,7 +431,6 @@ void migrate_fd_connect(MigrationState *s)
>      s->state = MIG_STATE_ACTIVE;
>      s->file = qemu_fopen_ops_buffered(s,
>                                        s->bandwidth_limit,
> -                                      migrate_fd_wait_for_unfreeze,
>                                        migrate_fd_close);
> 
>      DPRINTF("beginning savevm\n");
> diff --git a/migration.h b/migration.h
> index 031c2ab..d6341d6 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -81,6 +81,7 @@ void migrate_fd_connect(MigrationState *s);
>  ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
>                                size_t size);
>  void migrate_fd_put_ready(MigrationState *s);
> +void migrate_fd_wait_for_unfreeze(MigrationState *s);
> 
>  void add_migration_state_change_notifier(Notifier *notify);
>  void remove_migration_state_change_notifier(Notifier *notify);
> 

[Qemu-devel] [PATCH 22/41] buffered_file: unfold migrate_fd_put_buffer

[Juan Quintela]

We only used it once, just remove the callback indirection.

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

diff --git a/buffered_file.c b/buffered_file.c
index d257496..4fca774 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -23,7 +23,6 @@

 typedef struct QEMUFileBuffered
 {
-    BufferedCloseFunc *close;
     MigrationState *migration_state;
     QEMUFile *file;
     int freeze_output;
@@ -147,7 +146,7 @@ static int buffered_close(void *opaque)
             migrate_fd_wait_for_unfreeze(s->migration_state);
     }

-    ret = s->close(s->migration_state);
+    ret = migrate_fd_close(s->migration_state);

     qemu_del_timer(s->timer);
     qemu_free_timer(s->timer);
@@ -224,8 +223,7 @@ static void buffered_rate_tick(void *opaque)
 }

 QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
-                                  size_t bytes_per_sec,
-                                  BufferedCloseFunc *close)
+                                  size_t bytes_per_sec)
 {
     QEMUFileBuffered *s;

@@ -233,7 +231,6 @@ QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,

     s->migration_state = migration_state;
     s->xfer_limit = bytes_per_sec / 10;
-    s->close = close;

     s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
                              buffered_close, buffered_rate_limit,
diff --git a/buffered_file.h b/buffered_file.h
index 926e5c6..8a38754 100644
--- a/buffered_file.h
+++ b/buffered_file.h
@@ -17,10 +17,7 @@
 #include "hw/hw.h"
 #include "migration.h"

-typedef int (BufferedCloseFunc)(void *opaque);
-
 QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
-                                  size_t xfer_limit,
-                                  BufferedCloseFunc *close);
+                                  size_t xfer_limit);

 #endif
diff --git a/migration.c b/migration.c
index add4632..6f1e4d3 100644
--- a/migration.c
+++ b/migration.c
@@ -390,10 +390,8 @@ void migrate_fd_wait_for_unfreeze(MigrationState *s)
     }
 }

-static int migrate_fd_close(void *opaque)
+int migrate_fd_close(MigrationState *s)
 {
-    MigrationState *s = opaque;
-
     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
     return s->close(s);
 }
@@ -429,9 +427,7 @@ void migrate_fd_connect(MigrationState *s)
     int ret;

     s->state = MIG_STATE_ACTIVE;
-    s->file = qemu_fopen_ops_buffered(s,
-                                      s->bandwidth_limit,
-                                      migrate_fd_close);
+    s->file = qemu_fopen_ops_buffered(s, s->bandwidth_limit);

     DPRINTF("beginning savevm\n");
     ret = qemu_savevm_state_begin(s->file, &s->params);
diff --git a/migration.h b/migration.h
index d6341d6..ec022d6 100644
--- a/migration.h
+++ b/migration.h
@@ -82,6 +82,7 @@ ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
                               size_t size);
 void migrate_fd_put_ready(MigrationState *s);
 void migrate_fd_wait_for_unfreeze(MigrationState *s);
+int migrate_fd_close(MigrationState *s);

 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 22/41] buffered_file: unfold migrate_fd_put_buffer

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> -    ret = s->close(s->migration_state);
> +    ret = migrate_fd_close(s->migration_state);

This one should also be "buffered_file: unfold migrate_fd_close".

Paolo

[Qemu-devel] [PATCH 23/41] buffered_file: We can access directly to bandwidth_limit

[Juan Quintela]

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

diff --git a/buffered_file.c b/buffered_file.c
index 4fca774..43e68b6 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -222,15 +222,14 @@ static void buffered_rate_tick(void *opaque)
     buffered_put_buffer(s, NULL, 0, 0);
 }

-QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
-                                  size_t bytes_per_sec)
+QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state)
 {
     QEMUFileBuffered *s;

     s = g_malloc0(sizeof(*s));

     s->migration_state = migration_state;
-    s->xfer_limit = bytes_per_sec / 10;
+    s->xfer_limit = migration_state->bandwidth_limit / 10;

     s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
                              buffered_close, buffered_rate_limit,
diff --git a/buffered_file.h b/buffered_file.h
index 8a38754..ef010fe 100644
--- a/buffered_file.h
+++ b/buffered_file.h
@@ -17,7 +17,6 @@
 #include "hw/hw.h"
 #include "migration.h"

-QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
-                                  size_t xfer_limit);
+QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state);

 #endif
diff --git a/migration.c b/migration.c
index 6f1e4d3..56014dd 100644
--- a/migration.c
+++ b/migration.c
@@ -427,7 +427,7 @@ void migrate_fd_connect(MigrationState *s)
     int ret;

     s->state = MIG_STATE_ACTIVE;
-    s->file = qemu_fopen_ops_buffered(s, s->bandwidth_limit);
+    s->file = qemu_fopen_ops_buffered(s);

     DPRINTF("beginning savevm\n");
     ret = qemu_savevm_state_begin(s->file, &s->params);
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 23/41] buffered_file: We can access directly to bandwidth_limit

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 5 ++---
>  buffered_file.h | 3 +--
>  migration.c     | 2 +-
>  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index 4fca774..43e68b6 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -222,15 +222,14 @@ static void buffered_rate_tick(void *opaque)
>      buffered_put_buffer(s, NULL, 0, 0);
>  }
> 
> -QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
> -                                  size_t bytes_per_sec)
> +QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state)
>  {
>      QEMUFileBuffered *s;
> 
>      s = g_malloc0(sizeof(*s));
> 
>      s->migration_state = migration_state;
> -    s->xfer_limit = bytes_per_sec / 10;
> +    s->xfer_limit = migration_state->bandwidth_limit / 10;
> 
>      s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
>                               buffered_close, buffered_rate_limit,
> diff --git a/buffered_file.h b/buffered_file.h
> index 8a38754..ef010fe 100644
> --- a/buffered_file.h
> +++ b/buffered_file.h
> @@ -17,7 +17,6 @@
>  #include "hw/hw.h"
>  #include "migration.h"
> 
> -QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state,
> -                                  size_t xfer_limit);
> +QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state);
> 
>  #endif
> diff --git a/migration.c b/migration.c
> index 6f1e4d3..56014dd 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -427,7 +427,7 @@ void migrate_fd_connect(MigrationState *s)
>      int ret;
> 
>      s->state = MIG_STATE_ACTIVE;
> -    s->file = qemu_fopen_ops_buffered(s, s->bandwidth_limit);
> +    s->file = qemu_fopen_ops_buffered(s);
> 
>      DPRINTF("beginning savevm\n");
>      ret = qemu_savevm_state_begin(s->file, &s->params);
> 

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

Paolo

[Qemu-devel] [PATCH 24/41] buffered_file: callers of buffered_flush() already check for errors

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 43e68b6..747d672 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -61,13 +61,6 @@ static void buffered_append(QEMUFileBuffered *s,
 static void buffered_flush(QEMUFileBuffered *s)
 {
     size_t offset = 0;
-    int error;
-
-    error = qemu_file_get_error(s->file);
-    if (error != 0) {
-        DPRINTF("flush when error, bailing: %s\n", strerror(-error));
-        return;
-    }

     DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);

-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 24/41] buffered_file: callers of buffered_flush() already check for errors

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index 43e68b6..747d672 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -61,13 +61,6 @@ static void buffered_append(QEMUFileBuffered *s,
>  static void buffered_flush(QEMUFileBuffered *s)
>  {
>      size_t offset = 0;
> -    int error;
> -
> -    error = qemu_file_get_error(s->file);
> -    if (error != 0) {
> -        DPRINTF("flush when error, bailing: %s\n", strerror(-error));
> -        return;
> -    }
> 
>      DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);
> 

Right, they do so _before_ checking buffered_flush which matches the
code you are removing here.

Perhaps an assertion would be better, but still:

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

[Qemu-devel] [PATCH 25/41] buffered_file: make buffered_flush return the error code

[Juan Quintela]

Or the amount of data written if there is no error.  Adjust all callers.

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

diff --git a/buffered_file.c b/buffered_file.c
index 747d672..9db73dc 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -58,26 +58,26 @@ static void buffered_append(QEMUFileBuffered *s,
     s->buffer_size += size;
 }

-static void buffered_flush(QEMUFileBuffered *s)
+static int buffered_flush(QEMUFileBuffered *s)
 {
     size_t offset = 0;
+    int ret = 0;

     DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);

     while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
-        ssize_t ret;

         ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset,
                                     s->buffer_size - offset);
         if (ret == -EAGAIN) {
             DPRINTF("backend not ready, freezing\n");
+            ret = 0;
             s->freeze_output = 1;
             break;
         }

         if (ret <= 0) {
             DPRINTF("error flushing data, %zd\n", ret);
-            qemu_file_set_error(s->file, ret);
             break;
         } else {
             DPRINTF("flushed %zd byte(s)\n", ret);
@@ -89,6 +89,11 @@ static void buffered_flush(QEMUFileBuffered *s)
     DPRINTF("flushed %zu of %zu byte(s)\n", offset, s->buffer_size);
     memmove(s->buffer, s->buffer + offset, s->buffer_size - offset);
     s->buffer_size -= offset;
+
+    if (ret < 0) {
+        return ret;
+    }
+    return offset;
 }

 static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size)
@@ -112,7 +117,13 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         buffered_append(s, buf, size);
     }

-    buffered_flush(s);
+    error = buffered_flush(s);
+    if (error < 0) {
+        DPRINTF("buffered flush error. bailing: %s\n", strerror(-error));
+        qemu_file_set_error(s->file, error);
+
+        return error;
+    }

     if (pos == 0 && size == 0) {
         DPRINTF("file is ready\n");
@@ -128,19 +139,24 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
 static int buffered_close(void *opaque)
 {
     QEMUFileBuffered *s = opaque;
-    int ret;
+    int ret = 0, ret2;

     DPRINTF("closing\n");

     s->xfer_limit = INT_MAX;
     while (!qemu_file_get_error(s->file) && s->buffer_size) {
-        buffered_flush(s);
+        ret = buffered_flush(s);
+        if (ret < 0) {
+            break;
+        }
         if (s->freeze_output)
             migrate_fd_wait_for_unfreeze(s->migration_state);
     }

-    ret = migrate_fd_close(s->migration_state);
-
+    ret2 = migrate_fd_close(s->migration_state);
+    if (ret >= 0) {
+        ret = ret2;
+    }
     qemu_del_timer(s->timer);
     qemu_free_timer(s->timer);
     g_free(s->buffer);
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 25/41] buffered_file: make buffered_flush return the error code

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Or the amount of data written if there is no error.  Adjust all callers.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index 747d672..9db73dc 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -58,26 +58,26 @@ static void buffered_append(QEMUFileBuffered *s,
>      s->buffer_size += size;
>  }
> 
> -static void buffered_flush(QEMUFileBuffered *s)
> +static int buffered_flush(QEMUFileBuffered *s)

Should this be ssize_t?

>  {
>      size_t offset = 0;
> +    int ret = 0;
> 
>      DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);
> 
>      while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
> -        ssize_t ret;
> 
>          ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset,
>                                      s->buffer_size - offset);
>          if (ret == -EAGAIN) {
>              DPRINTF("backend not ready, freezing\n");
> +            ret = 0;
>              s->freeze_output = 1;
>              break;
>          }
> 
>          if (ret <= 0) {
>              DPRINTF("error flushing data, %zd\n", ret);
> -            qemu_file_set_error(s->file, ret);
>              break;
>          } else {
>              DPRINTF("flushed %zd byte(s)\n", ret);
> @@ -89,6 +89,11 @@ static void buffered_flush(QEMUFileBuffered *s)
>      DPRINTF("flushed %zu of %zu byte(s)\n", offset, s->buffer_size);
>      memmove(s->buffer, s->buffer + offset, s->buffer_size - offset);
>      s->buffer_size -= offset;
> +
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return offset;
>  }
> 
>  static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size)
> @@ -112,7 +117,13 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
>          buffered_append(s, buf, size);
>      }
> 
> -    buffered_flush(s);
> +    error = buffered_flush(s);
> +    if (error < 0) {
> +        DPRINTF("buffered flush error. bailing: %s\n", strerror(-error));
> +        qemu_file_set_error(s->file, error);
> +
> +        return error;
> +    }
> 
>      if (pos == 0 && size == 0) {
>          DPRINTF("file is ready\n");
> @@ -128,19 +139,24 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
>  static int buffered_close(void *opaque)
>  {
>      QEMUFileBuffered *s = opaque;
> -    int ret;
> +    int ret = 0, ret2;

Same here too (ssize_t).

Paolo

> 
>      DPRINTF("closing\n");
> 
>      s->xfer_limit = INT_MAX;
>      while (!qemu_file_get_error(s->file) && s->buffer_size) {
> -        buffered_flush(s);
> +        ret = buffered_flush(s);
> +        if (ret < 0) {
> +            break;
> +        }
>          if (s->freeze_output)
>              migrate_fd_wait_for_unfreeze(s->migration_state);
>      }
> 
> -    ret = migrate_fd_close(s->migration_state);
> -
> +    ret2 = migrate_fd_close(s->migration_state);
> +    if (ret >= 0) {
> +        ret = ret2;
> +    }

Perhaps the other way round:

    if (ret < 0) {
        ret2 = -1;
    }
    ...
    return ret2;

so that buffered_close can still return int without having a suspicious
truncation of its return value from ssize_t to int.

Paolo

>      qemu_del_timer(s->timer);
>      qemu_free_timer(s->timer);
>      g_free(s->buffer);
> 

Re: [Qemu-devel] [PATCH 25/41] buffered_file: make buffered_flush return the error code

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 21/09/2012 10:47, Juan Quintela ha scritto:
>> Or the amount of data written if there is no error.  Adjust all callers.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  buffered_file.c | 32 ++++++++++++++++++++++++--------
>>  1 file changed, 24 insertions(+), 8 deletions(-)
>> 
>> diff --git a/buffered_file.c b/buffered_file.c
>> index 747d672..9db73dc 100644
>> --- a/buffered_file.c
>> +++ b/buffered_file.c
>> @@ -58,26 +58,26 @@ static void buffered_append(QEMUFileBuffered *s,
>>      s->buffer_size += size;
>>  }
>> 
>> -static void buffered_flush(QEMUFileBuffered *s)
>> +static int buffered_flush(QEMUFileBuffered *s)
>
> Should this be ssize_t?

Done.


> Perhaps the other way round:
>
>     if (ret < 0) {
>         ret2 = -1;
>     }
>     ...
>     return ret2;

This lost the 1st errno value.  The other way around we preserve it.

Later, Juan.

Re: [Qemu-devel] [PATCH 25/41] buffered_file: make buffered_flush return the error code

[Paolo Bonzini]

Il 02/10/2012 13:06, Juan Quintela ha scritto:
>> Perhaps the other way round:
>> >
>> >     if (ret < 0) {
>> >         ret2 = -1;
>> >     }
>> >     ...
>> >     return ret2;
> This lost the 1st errno value.

Right, I meant ret2 = ret;

> The other way around we preserve it.

I think with "ret2 = ret;" it is the same.

The way you have it in your patch, you could truncate a positive ssize_t
return value from buffered_flush that does not fit in an int.
Theoretical, I know, but I can see Coverity spotting it from a mile...

Paolo

[Qemu-devel] [PATCH 26/41] migration: make migrate_fd_wait_for_unfreeze() return errors

[Juan Quintela]

Adjust all callers

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 8 ++++++--
 migration.c     | 7 ++++---
 migration.h     | 2 +-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 9db73dc..6d9a50b 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -149,8 +149,12 @@ static int buffered_close(void *opaque)
         if (ret < 0) {
             break;
         }
-        if (s->freeze_output)
-            migrate_fd_wait_for_unfreeze(s->migration_state);
+        if (s->freeze_output) {
+            ret = migrate_fd_wait_for_unfreeze(s->migration_state);
+            if (ret < 0) {
+                break;
+            }
+        }
     }

     ret2 = migrate_fd_close(s->migration_state);
diff --git a/migration.c b/migration.c
index 56014dd..6a505c1 100644
--- a/migration.c
+++ b/migration.c
@@ -368,13 +368,13 @@ static void migrate_fd_cancel(MigrationState *s)
     migrate_fd_cleanup(s);
 }

-void migrate_fd_wait_for_unfreeze(MigrationState *s)
+int migrate_fd_wait_for_unfreeze(MigrationState *s)
 {
     int ret;

     DPRINTF("wait for unfreeze\n");
     if (s->state != MIG_STATE_ACTIVE)
-        return;
+        return -EINVAL;

     do {
         fd_set wfds;
@@ -386,8 +386,9 @@ void migrate_fd_wait_for_unfreeze(MigrationState *s)
     } while (ret == -1 && (s->get_error(s)) == EINTR);

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

 int migrate_fd_close(MigrationState *s)
diff --git a/migration.h b/migration.h
index ec022d6..1c3e9b7 100644
--- a/migration.h
+++ b/migration.h
@@ -81,7 +81,7 @@ void migrate_fd_connect(MigrationState *s);
 ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
                               size_t size);
 void migrate_fd_put_ready(MigrationState *s);
-void migrate_fd_wait_for_unfreeze(MigrationState *s);
+int migrate_fd_wait_for_unfreeze(MigrationState *s);
 int migrate_fd_close(MigrationState *s);

 void add_migration_state_change_notifier(Notifier *notify);
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 26/41] migration: make migrate_fd_wait_for_unfreeze() return errors

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Adjust all callers
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 8 ++++++--
>  migration.c     | 7 ++++---
>  migration.h     | 2 +-
>  3 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index 9db73dc..6d9a50b 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -149,8 +149,12 @@ static int buffered_close(void *opaque)
>          if (ret < 0) {
>              break;
>          }
> -        if (s->freeze_output)
> -            migrate_fd_wait_for_unfreeze(s->migration_state);
> +        if (s->freeze_output) {
> +            ret = migrate_fd_wait_for_unfreeze(s->migration_state);
> +            if (ret < 0) {
> +                break;
> +            }
> +        }
>      }
> 
>      ret2 = migrate_fd_close(s->migration_state);
> diff --git a/migration.c b/migration.c
> index 56014dd..6a505c1 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -368,13 +368,13 @@ static void migrate_fd_cancel(MigrationState *s)
>      migrate_fd_cleanup(s);
>  }
> 
> -void migrate_fd_wait_for_unfreeze(MigrationState *s)
> +int migrate_fd_wait_for_unfreeze(MigrationState *s)
>  {
>      int ret;
> 
>      DPRINTF("wait for unfreeze\n");
>      if (s->state != MIG_STATE_ACTIVE)
> -        return;
> +        return -EINVAL;
> 
>      do {
>          fd_set wfds;
> @@ -386,8 +386,9 @@ void migrate_fd_wait_for_unfreeze(MigrationState *s)
>      } while (ret == -1 && (s->get_error(s)) == EINTR);
> 
>      if (ret == -1) {
> -        qemu_file_set_error(s->file, -s->get_error(s));
> +        return -s->get_error(s);
>      }
> +    return 0;
>  }
> 
>  int migrate_fd_close(MigrationState *s)
> diff --git a/migration.h b/migration.h
> index ec022d6..1c3e9b7 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -81,7 +81,7 @@ void migrate_fd_connect(MigrationState *s);
>  ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
>                                size_t size);
>  void migrate_fd_put_ready(MigrationState *s);
> -void migrate_fd_wait_for_unfreeze(MigrationState *s);
> +int migrate_fd_wait_for_unfreeze(MigrationState *s);
>  int migrate_fd_close(MigrationState *s);
> 
>  void add_migration_state_change_notifier(Notifier *notify);
> 

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

[Qemu-devel] [PATCH 27/41] savevm: unexport qemu_fflush

[Juan Quintela]

It is not used outside of savevm.c

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 qemu-file.h | 1 -
 savevm.c    | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/qemu-file.h b/qemu-file.h
index 31b83f6..d8487cd 100644
--- a/qemu-file.h
+++ b/qemu-file.h
@@ -71,7 +71,6 @@ QEMUFile *qemu_fopen_socket(int fd);
 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);
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
 void qemu_put_byte(QEMUFile *f, int v);
diff --git a/savevm.c b/savevm.c
index c7fe283..1ec6ff1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -461,7 +461,7 @@ static void qemu_file_set_if_error(QEMUFile *f, int ret)
  *
  * In case of error, last_error is set.
  */
-void qemu_fflush(QEMUFile *f)
+static void qemu_fflush(QEMUFile *f)
 {
     if (!f->put_buffer)
         return;
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 27/41] savevm: unexport qemu_fflush

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> It is not used outside of savevm.c
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  qemu-file.h | 1 -
>  savevm.c    | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/qemu-file.h b/qemu-file.h
> index 31b83f6..d8487cd 100644
> --- a/qemu-file.h
> +++ b/qemu-file.h
> @@ -71,7 +71,6 @@ QEMUFile *qemu_fopen_socket(int fd);
>  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);
>  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
>  void qemu_put_byte(QEMUFile *f, int v);
> diff --git a/savevm.c b/savevm.c
> index c7fe283..1ec6ff1 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -461,7 +461,7 @@ static void qemu_file_set_if_error(QEMUFile *f, int ret)
>   *
>   * In case of error, last_error is set.
>   */
> -void qemu_fflush(QEMUFile *f)
> +static void qemu_fflush(QEMUFile *f)
>  {
>      if (!f->put_buffer)
>          return;
> 

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

[Qemu-devel] [PATCH 28/41] virtio-net: use qemu_get_buffer() in a temp buffer

[Juan Quintela]

qemu_fseek() is known to be wrong.  Would be removed on the next
commit.  This code should never been used (value has been
MAC_TABLE_ENTRIES since 2009).

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 6490743..e8c43af 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -921,7 +921,9 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
             qemu_get_buffer(f, n->mac_table.macs,
                             n->mac_table.in_use * ETH_ALEN);
         } else if (n->mac_table.in_use) {
-            qemu_fseek(f, n->mac_table.in_use * ETH_ALEN, SEEK_CUR);
+            uint8_t *buf = g_malloc0(n->mac_table.in_use);
+            qemu_get_buffer(f, buf, n->mac_table.in_use * ETH_ALEN);
+            g_free(buf);
             n->mac_table.multi_overflow = n->mac_table.uni_overflow = 1;
             n->mac_table.in_use = 0;
         }
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 28/41] virtio-net: use qemu_get_buffer() in a temp buffer

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> qemu_fseek() is known to be wrong.  Would be removed on the next
> commit.  This code should never been used (value has been
> MAC_TABLE_ENTRIES since 2009).
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-net.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 6490743..e8c43af 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -921,7 +921,9 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>              qemu_get_buffer(f, n->mac_table.macs,
>                              n->mac_table.in_use * ETH_ALEN);
>          } else if (n->mac_table.in_use) {
> -            qemu_fseek(f, n->mac_table.in_use * ETH_ALEN, SEEK_CUR);
> +            uint8_t *buf = g_malloc0(n->mac_table.in_use);
> +            qemu_get_buffer(f, buf, n->mac_table.in_use * ETH_ALEN);
> +            g_free(buf);
>              n->mac_table.multi_overflow = n->mac_table.uni_overflow = 1;
>              n->mac_table.in_use = 0;
>          }
> 

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

[Qemu-devel] [PATCH 29/41] savevm: Remove qemu_fseek()

[Juan Quintela]

It has no users, and is only half implemented.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 qemu-file.h |  1 -
 savevm.c    | 21 ---------------------
 2 files changed, 22 deletions(-)

diff --git a/qemu-file.h b/qemu-file.h
index d8487cd..7fe7274 100644
--- a/qemu-file.h
+++ b/qemu-file.h
@@ -232,6 +232,5 @@ static inline void qemu_get_sbe64s(QEMUFile *f, int64_t *pv)
 }

 int64_t qemu_ftell(QEMUFile *f);
-int64_t qemu_fseek(QEMUFile *f, int64_t pos, int whence);

 #endif
diff --git a/savevm.c b/savevm.c
index 1ec6ff1..6865862 100644
--- a/savevm.c
+++ b/savevm.c
@@ -676,27 +676,6 @@ int64_t qemu_ftell(QEMUFile *f)
     return f->buf_offset - f->buf_size + f->buf_index;
 }

-int64_t qemu_fseek(QEMUFile *f, int64_t pos, int whence)
-{
-    if (whence == SEEK_SET) {
-        /* nothing to do */
-    } else if (whence == SEEK_CUR) {
-        pos += qemu_ftell(f);
-    } else {
-        /* SEEK_END not supported */
-        return -1;
-    }
-    if (f->put_buffer) {
-        qemu_fflush(f);
-        f->buf_offset = pos;
-    } else {
-        f->buf_offset = pos;
-        f->buf_index = 0;
-        f->buf_size = 0;
-    }
-    return pos;
-}
-
 int qemu_file_rate_limit(QEMUFile *f)
 {
     if (f->rate_limit)
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 29/41] savevm: Remove qemu_fseek()

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> It has no users, and is only half implemented.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  qemu-file.h |  1 -
>  savevm.c    | 21 ---------------------
>  2 files changed, 22 deletions(-)
> 
> diff --git a/qemu-file.h b/qemu-file.h
> index d8487cd..7fe7274 100644
> --- a/qemu-file.h
> +++ b/qemu-file.h
> @@ -232,6 +232,5 @@ static inline void qemu_get_sbe64s(QEMUFile *f, int64_t *pv)
>  }
> 
>  int64_t qemu_ftell(QEMUFile *f);
> -int64_t qemu_fseek(QEMUFile *f, int64_t pos, int whence);
> 
>  #endif
> diff --git a/savevm.c b/savevm.c
> index 1ec6ff1..6865862 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -676,27 +676,6 @@ int64_t qemu_ftell(QEMUFile *f)
>      return f->buf_offset - f->buf_size + f->buf_index;
>  }
> 
> -int64_t qemu_fseek(QEMUFile *f, int64_t pos, int whence)
> -{
> -    if (whence == SEEK_SET) {
> -        /* nothing to do */
> -    } else if (whence == SEEK_CUR) {
> -        pos += qemu_ftell(f);
> -    } else {
> -        /* SEEK_END not supported */
> -        return -1;
> -    }
> -    if (f->put_buffer) {
> -        qemu_fflush(f);
> -        f->buf_offset = pos;
> -    } else {
> -        f->buf_offset = pos;
> -        f->buf_index = 0;
> -        f->buf_size = 0;
> -    }
> -    return pos;
> -}
> -
>  int qemu_file_rate_limit(QEMUFile *f)
>  {
>      if (f->rate_limit)
> 

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

[Qemu-devel] [PATCH 30/41] savevm: make qemu_fflush() return an error code

[Juan Quintela]

Adjust all the callers.  We moved the set of last_error from inside
qemu_fflush() to all the callers.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/savevm.c b/savevm.c
index 6865862..0953695 100644
--- a/savevm.c
+++ b/savevm.c
@@ -459,23 +459,22 @@ static void qemu_file_set_if_error(QEMUFile *f, int ret)

 /** Flushes QEMUFile buffer
  *
- * In case of error, last_error is set.
  */
-static void qemu_fflush(QEMUFile *f)
+static int qemu_fflush(QEMUFile *f)
 {
+    int ret = 0;
+
     if (!f->put_buffer)
-        return;
+        return 0;

     if (f->is_write && f->buf_index > 0) {
-        int len;
-
-        len = f->put_buffer(f->opaque, f->buf, f->buf_offset, f->buf_index);
-        if (len > 0)
+        ret = f->put_buffer(f->opaque, f->buf, f->buf_offset, f->buf_index);
+        if (ret >= 0) {
             f->buf_offset += f->buf_index;
-        else
-            qemu_file_set_error(f, -EINVAL);
+        }
         f->buf_index = 0;
     }
+    return ret;
 }

 static void qemu_fill_buffer(QEMUFile *f)
@@ -533,9 +532,13 @@ static int qemu_fclose_internal(QEMUFile *f)
  */
 int qemu_fclose(QEMUFile *f)
 {
-    int ret;
-    qemu_fflush(f);
-    ret = qemu_fclose_internal(f);
+    int ret, ret2;
+    ret = qemu_fflush(f);
+    ret2 = qemu_fclose_internal(f);
+
+    if (ret >= 0) {
+        ret = ret2;
+    }
     /* If any error was spotted before closing, we should report it
      * instead of the close() return value.
      */
@@ -570,8 +573,10 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         f->buf_index += l;
         buf += l;
         size -= l;
-        if (f->buf_index >= IO_BUF_SIZE)
-            qemu_fflush(f);
+        if (f->buf_index >= IO_BUF_SIZE) {
+            int ret = qemu_fflush(f);
+            qemu_file_set_if_error(f, ret);
+        }
     }
 }

@@ -585,8 +590,10 @@ void qemu_put_byte(QEMUFile *f, int v)

     f->buf[f->buf_index++] = v;
     f->is_write = 1;
-    if (f->buf_index >= IO_BUF_SIZE)
-        qemu_fflush(f);
+    if (f->buf_index >= IO_BUF_SIZE) {
+        int ret = qemu_fflush(f);
+        qemu_file_set_if_error(f, ret);
+    }
 }

 static void qemu_file_skip(QEMUFile *f, int size)
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 30/41] savevm: make qemu_fflush() return an error code

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Adjust all the callers.  We moved the set of last_error from inside
> qemu_fflush() to all the callers.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  savevm.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 6865862..0953695 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -459,23 +459,22 @@ static void qemu_file_set_if_error(QEMUFile *f, int ret)
> 
>  /** Flushes QEMUFile buffer
>   *
> - * In case of error, last_error is set.
>   */
> -static void qemu_fflush(QEMUFile *f)
> +static int qemu_fflush(QEMUFile *f)
>  {
> +    int ret = 0;
> +
>      if (!f->put_buffer)
> -        return;
> +        return 0;
> 
>      if (f->is_write && f->buf_index > 0) {
> -        int len;
> -
> -        len = f->put_buffer(f->opaque, f->buf, f->buf_offset, f->buf_index);
> -        if (len > 0)
> +        ret = f->put_buffer(f->opaque, f->buf, f->buf_offset, f->buf_index);
> +        if (ret >= 0) {
>              f->buf_offset += f->buf_index;
> -        else
> -            qemu_file_set_error(f, -EINVAL);
> +        }
>          f->buf_index = 0;
>      }
> +    return ret;
>  }
> 
>  static void qemu_fill_buffer(QEMUFile *f)
> @@ -533,9 +532,13 @@ static int qemu_fclose_internal(QEMUFile *f)
>   */
>  int qemu_fclose(QEMUFile *f)
>  {
> -    int ret;
> -    qemu_fflush(f);
> -    ret = qemu_fclose_internal(f);
> +    int ret, ret2;
> +    ret = qemu_fflush(f);
> +    ret2 = qemu_fclose_internal(f);
> +
> +    if (ret >= 0) {
> +        ret = ret2;
> +    }
>      /* If any error was spotted before closing, we should report it
>       * instead of the close() return value.
>       */
> @@ -570,8 +573,10 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>          f->buf_index += l;
>          buf += l;
>          size -= l;
> -        if (f->buf_index >= IO_BUF_SIZE)
> -            qemu_fflush(f);
> +        if (f->buf_index >= IO_BUF_SIZE) {
> +            int ret = qemu_fflush(f);
> +            qemu_file_set_if_error(f, ret);
> +        }
>      }
>  }
> 
> @@ -585,8 +590,10 @@ void qemu_put_byte(QEMUFile *f, int v)
> 
>      f->buf[f->buf_index++] = v;
>      f->is_write = 1;
> -    if (f->buf_index >= IO_BUF_SIZE)
> -        qemu_fflush(f);
> +    if (f->buf_index >= IO_BUF_SIZE) {
> +        int ret = qemu_fflush(f);
> +        qemu_file_set_if_error(f, ret);
> +    }
>  }
> 
>  static void qemu_file_skip(QEMUFile *f, int size)
> 

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

[Qemu-devel] [PATCH 31/41] savevm: unfold qemu_fclose_internal()

[Juan Quintela]

It was used only one, and was only one if.  It makes error handling
saner.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/savevm.c b/savevm.c
index 0953695..8efa7cc 100644
--- a/savevm.c
+++ b/savevm.c
@@ -506,22 +506,6 @@ static void qemu_fill_buffer(QEMUFile *f)
         qemu_file_set_error(f, len);
 }

-/** Calls close function and set last_error if needed
- *
- * Internal function. qemu_fflush() must be called before this.
- *
- * Returns f->close() return value, or 0 if close function is not set.
- */
-static int qemu_fclose_internal(QEMUFile *f)
-{
-    int ret = 0;
-    if (f->close) {
-        ret = f->close(f->opaque);
-        qemu_file_set_if_error(f, ret);
-    }
-    return ret;
-}
-
 /** Closes the file
  *
  * Returns negative error value if any error happened on previous operations or
@@ -532,12 +516,14 @@ static int qemu_fclose_internal(QEMUFile *f)
  */
 int qemu_fclose(QEMUFile *f)
 {
-    int ret, ret2;
+    int ret;
     ret = qemu_fflush(f);
-    ret2 = qemu_fclose_internal(f);

-    if (ret >= 0) {
-        ret = ret2;
+    if (f->close) {
+        int ret2 = f->close(f->opaque);
+        if (ret >= 0) {
+            ret = ret2;
+        }
     }
     /* If any error was spotted before closing, we should report it
      * instead of the close() return value.
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 31/41] savevm: unfold qemu_fclose_internal()

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> It was used only one, and was only one if.  It makes error handling
> saner.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  savevm.c | 26 ++++++--------------------
>  1 file changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 0953695..8efa7cc 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -506,22 +506,6 @@ static void qemu_fill_buffer(QEMUFile *f)
>          qemu_file_set_error(f, len);
>  }
> 
> -/** Calls close function and set last_error if needed
> - *
> - * Internal function. qemu_fflush() must be called before this.
> - *
> - * Returns f->close() return value, or 0 if close function is not set.
> - */
> -static int qemu_fclose_internal(QEMUFile *f)
> -{
> -    int ret = 0;
> -    if (f->close) {
> -        ret = f->close(f->opaque);
> -        qemu_file_set_if_error(f, ret);
> -    }
> -    return ret;
> -}
> -
>  /** Closes the file
>   *
>   * Returns negative error value if any error happened on previous operations or
> @@ -532,12 +516,14 @@ static int qemu_fclose_internal(QEMUFile *f)
>   */
>  int qemu_fclose(QEMUFile *f)
>  {
> -    int ret, ret2;
> +    int ret;
>      ret = qemu_fflush(f);
> -    ret2 = qemu_fclose_internal(f);
> 
> -    if (ret >= 0) {
> -        ret = ret2;
> +    if (f->close) {
> +        int ret2 = f->close(f->opaque);
> +        if (ret >= 0) {
> +            ret = ret2;
> +        }
>      }
>      /* If any error was spotted before closing, we should report it
>       * instead of the close() return value.
> 

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

[Qemu-devel] [PATCH 32/41] savevm: unexport qemu_ftell()

[Juan Quintela]

It was unused out of savevm.c.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 qemu-file.h | 3 ---
 savevm.c    | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/qemu-file.h b/qemu-file.h
index 7fe7274..289849a 100644
--- a/qemu-file.h
+++ b/qemu-file.h
@@ -230,7 +230,4 @@ static inline void qemu_get_sbe64s(QEMUFile *f, int64_t *pv)
 {
     qemu_get_be64s(f, (uint64_t *)pv);
 }
-
-int64_t qemu_ftell(QEMUFile *f);
-
 #endif
diff --git a/savevm.c b/savevm.c
index 8efa7cc..8ddb9d5 100644
--- a/savevm.c
+++ b/savevm.c
@@ -664,7 +664,7 @@ int qemu_get_byte(QEMUFile *f)
     return result;
 }

-int64_t qemu_ftell(QEMUFile *f)
+static int64_t qemu_ftell(QEMUFile *f)
 {
     return f->buf_offset - f->buf_size + f->buf_index;
 }
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 32/41] savevm: unexport qemu_ftell()

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> It was unused out of savevm.c.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  qemu-file.h | 3 ---
>  savevm.c    | 2 +-
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/qemu-file.h b/qemu-file.h
> index 7fe7274..289849a 100644
> --- a/qemu-file.h
> +++ b/qemu-file.h
> @@ -230,7 +230,4 @@ static inline void qemu_get_sbe64s(QEMUFile *f, int64_t *pv)
>  {
>      qemu_get_be64s(f, (uint64_t *)pv);
>  }
> -
> -int64_t qemu_ftell(QEMUFile *f);
> -
>  #endif
> diff --git a/savevm.c b/savevm.c
> index 8efa7cc..8ddb9d5 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -664,7 +664,7 @@ int qemu_get_byte(QEMUFile *f)
>      return result;
>  }
> 
> -int64_t qemu_ftell(QEMUFile *f)
> +static int64_t qemu_ftell(QEMUFile *f)
>  {
>      return f->buf_offset - f->buf_size + f->buf_index;
>  }
> 

static inline?

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

Re: [Qemu-devel] [PATCH 32/41] savevm: unexport qemu_ftell()

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 21/09/2012 10:47, Juan Quintela ha scritto:
>> It was unused out of savevm.c.
>> 
>> -int64_t qemu_ftell(QEMUFile *f)
>> +static int64_t qemu_ftell(QEMUFile *f)
>>  {
>>      return f->buf_offset - f->buf_size + f->buf_index;
>>  }
>> 
>
> static inline?

Used only once, when doing one snapshot ....

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

[Qemu-devel] [PATCH 33/41] savevm: make qemu_fill_buffer() be consistent

[Juan Quintela]

It was setting last_error directly once, and with the helper the other time.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/savevm.c b/savevm.c
index 8ddb9d5..4e4aa3c 100644
--- a/savevm.c
+++ b/savevm.c
@@ -501,7 +501,7 @@ static void qemu_fill_buffer(QEMUFile *f)
         f->buf_size += len;
         f->buf_offset += len;
     } else if (len == 0) {
-        f->last_error = -EIO;
+        qemu_file_set_error(f, -EIO);
     } else if (len != -EAGAIN)
         qemu_file_set_error(f, len);
 }
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 33/41] savevm: make qemu_fill_buffer() be consistent

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> It was setting last_error directly once, and with the helper the other time.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  savevm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 8ddb9d5..4e4aa3c 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -501,7 +501,7 @@ static void qemu_fill_buffer(QEMUFile *f)
>          f->buf_size += len;
>          f->buf_offset += len;
>      } else if (len == 0) {
> -        f->last_error = -EIO;
> +        qemu_file_set_error(f, -EIO);
>      } else if (len != -EAGAIN)
>          qemu_file_set_error(f, len);
>  }
> 

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

[Qemu-devel] [PATCH 34/41] savevm: Only qemu_fflush() can generate errors

[Juan Quintela]

Move the error check to the beggining of the callers.  Once this is fixed
qemu_file_set_if_error() is not used anymore, so remove it.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/savevm.c b/savevm.c
index 4e4aa3c..59ec8bf 100644
--- a/savevm.c
+++ b/savevm.c
@@ -445,18 +445,6 @@ void qemu_file_set_error(QEMUFile *f, int ret)
     f->last_error = ret;
 }

-/** Sets last_error conditionally
- *
- * Sets last_error only if ret is negative _and_ no error
- * was set before.
- */
-static void qemu_file_set_if_error(QEMUFile *f, int ret)
-{
-    if (ret < 0 && !f->last_error) {
-        qemu_file_set_error(f, ret);
-    }
-}
-
 /** Flushes QEMUFile buffer
  *
  */
@@ -544,13 +532,17 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
 {
     int l;

-    if (!f->last_error && f->is_write == 0 && f->buf_index > 0) {
+    if (f->last_error) {
+        return;
+    }
+
+    if (f->is_write == 0 && f->buf_index > 0) {
         fprintf(stderr,
                 "Attempted to write to buffer while read buffer is not empty\n");
         abort();
     }

-    while (!f->last_error && size > 0) {
+    while (size > 0) {
         l = IO_BUF_SIZE - f->buf_index;
         if (l > size)
             l = size;
@@ -561,14 +553,21 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         size -= l;
         if (f->buf_index >= IO_BUF_SIZE) {
             int ret = qemu_fflush(f);
-            qemu_file_set_if_error(f, ret);
+            if (ret < 0) {
+                qemu_file_set_error(f, ret);
+                break;
+            }
         }
     }
 }

 void qemu_put_byte(QEMUFile *f, int v)
 {
-    if (!f->last_error && f->is_write == 0 && f->buf_index > 0) {
+    if (f->last_error) {
+        return;
+    }
+
+    if (f->is_write == 0 && f->buf_index > 0) {
         fprintf(stderr,
                 "Attempted to write to buffer while read buffer is not empty\n");
         abort();
@@ -578,7 +577,9 @@ void qemu_put_byte(QEMUFile *f, int v)
     f->is_write = 1;
     if (f->buf_index >= IO_BUF_SIZE) {
         int ret = qemu_fflush(f);
-        qemu_file_set_if_error(f, ret);
+        if (ret < 0) {
+            qemu_file_set_error(f, ret);
+        }
     }
 }

-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 34/41] savevm: Only qemu_fflush() can generate errors

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Move the error check to the beggining of the callers.  Once this is fixed
> qemu_file_set_if_error() is not used anymore, so remove it.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  savevm.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 4e4aa3c..59ec8bf 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -445,18 +445,6 @@ void qemu_file_set_error(QEMUFile *f, int ret)
>      f->last_error = ret;
>  }
> 
> -/** Sets last_error conditionally
> - *
> - * Sets last_error only if ret is negative _and_ no error
> - * was set before.
> - */
> -static void qemu_file_set_if_error(QEMUFile *f, int ret)
> -{
> -    if (ret < 0 && !f->last_error) {
> -        qemu_file_set_error(f, ret);
> -    }
> -}
> -
>  /** Flushes QEMUFile buffer
>   *
>   */
> @@ -544,13 +532,17 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>  {
>      int l;
> 
> -    if (!f->last_error && f->is_write == 0 && f->buf_index > 0) {
> +    if (f->last_error) {
> +        return;
> +    }
> +
> +    if (f->is_write == 0 && f->buf_index > 0) {
>          fprintf(stderr,
>                  "Attempted to write to buffer while read buffer is not empty\n");
>          abort();
>      }
> 
> -    while (!f->last_error && size > 0) {
> +    while (size > 0) {
>          l = IO_BUF_SIZE - f->buf_index;
>          if (l > size)
>              l = size;
> @@ -561,14 +553,21 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>          size -= l;
>          if (f->buf_index >= IO_BUF_SIZE) {
>              int ret = qemu_fflush(f);
> -            qemu_file_set_if_error(f, ret);
> +            if (ret < 0) {
> +                qemu_file_set_error(f, ret);
> +                break;
> +            }
>          }
>      }
>  }
> 
>  void qemu_put_byte(QEMUFile *f, int v)
>  {
> -    if (!f->last_error && f->is_write == 0 && f->buf_index > 0) {
> +    if (f->last_error) {
> +        return;
> +    }
> +
> +    if (f->is_write == 0 && f->buf_index > 0) {
>          fprintf(stderr,
>                  "Attempted to write to buffer while read buffer is not empty\n");
>          abort();
> @@ -578,7 +577,9 @@ void qemu_put_byte(QEMUFile *f, int v)
>      f->is_write = 1;
>      if (f->buf_index >= IO_BUF_SIZE) {
>          int ret = qemu_fflush(f);
> -        qemu_file_set_if_error(f, ret);
> +        if (ret < 0) {
> +            qemu_file_set_error(f, ret);
> +        }
>      }
>  }
> 

Just a matter of taste, but I happen to agree.

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

[Qemu-devel] [PATCH 35/41] buffered_file: buffered_put_buffer() don't need to set last_error

[Juan Quintela]

Callers on savevm.c:qemu_fflush() will set it.

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

diff --git a/buffered_file.c b/buffered_file.c
index 6d9a50b..318d0f0 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -120,8 +120,6 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
     error = buffered_flush(s);
     if (error < 0) {
         DPRINTF("buffered flush error. bailing: %s\n", strerror(-error));
-        qemu_file_set_error(s->file, error);
-
         return error;
     }

-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 35/41] buffered_file: buffered_put_buffer() don't need to set last_error

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Callers on savevm.c:qemu_fflush() will set it.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index 6d9a50b..318d0f0 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -120,8 +120,6 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
>      error = buffered_flush(s);
>      if (error < 0) {
>          DPRINTF("buffered flush error. bailing: %s\n", strerror(-error));
> -        qemu_file_set_error(s->file, error);
> -
>          return error;
>      }
> 

Yo dawg, I heard you like buffers so I put a buffer in your buffered
file so you buffer while the file buffers.

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

[Qemu-devel] [PATCH 36/41] block-migration: make flush_blks() return errors

[Juan Quintela]

This means we don't need to pass through qemu_file to get the errors.
Adjust all callers.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block-migration.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 7def8ab..a822bb2 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -444,9 +444,10 @@ static int blk_mig_save_dirty_block(QEMUFile *f, int is_async)
     return ret;
 }

-static void flush_blks(QEMUFile* f)
+static int flush_blks(QEMUFile *f)
 {
     BlkMigBlock *blk;
+    int ret = 0;

     DPRINTF("%s Enter submitted %d read_done %d transferred %d\n",
             __FUNCTION__, block_mig_state.submitted, block_mig_state.read_done,
@@ -457,7 +458,7 @@ static void flush_blks(QEMUFile* f)
             break;
         }
         if (blk->ret < 0) {
-            qemu_file_set_error(f, blk->ret);
+            ret = blk->ret;
             break;
         }
         blk_send(f, blk);
@@ -474,6 +475,7 @@ static void flush_blks(QEMUFile* f)
     DPRINTF("%s Exit submitted %d read_done %d transferred %d\n", __FUNCTION__,
             block_mig_state.submitted, block_mig_state.read_done,
             block_mig_state.transferred);
+    return ret;
 }

 static int64_t get_remaining_dirty(void)
@@ -553,9 +555,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
     /* start track dirty blocks */
     set_dirty_tracking(1);

-    flush_blks(f);
-
-    ret = qemu_file_get_error(f);
+    ret = flush_blks(f);
     if (ret) {
         blk_mig_cleanup();
         return ret;
@@ -575,9 +575,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
     DPRINTF("Enter save live iterate submitted %d transferred %d\n",
             block_mig_state.submitted, block_mig_state.transferred);

-    flush_blks(f);
-
-    ret = qemu_file_get_error(f);
+    ret = flush_blks(f);
     if (ret) {
         blk_mig_cleanup();
         return ret;
@@ -603,9 +601,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
         }
     }

-    flush_blks(f);
-
-    ret = qemu_file_get_error(f);
+    ret = flush_blks(f);
     if (ret) {
         blk_mig_cleanup();
         return ret;
@@ -623,9 +619,7 @@ static int block_save_complete(QEMUFile *f, void *opaque)
     DPRINTF("Enter save live complete submitted %d transferred %d\n",
             block_mig_state.submitted, block_mig_state.transferred);

-    flush_blks(f);
-
-    ret = qemu_file_get_error(f);
+    ret = flush_blks(f);
     if (ret) {
         blk_mig_cleanup();
         return ret;
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 36/41] block-migration: make flush_blks() return errors

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> This means we don't need to pass through qemu_file to get the errors.
> Adjust all callers.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  block-migration.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index 7def8ab..a822bb2 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -444,9 +444,10 @@ static int blk_mig_save_dirty_block(QEMUFile *f, int is_async)
>      return ret;
>  }
> 
> -static void flush_blks(QEMUFile* f)
> +static int flush_blks(QEMUFile *f)
>  {
>      BlkMigBlock *blk;
> +    int ret = 0;
> 
>      DPRINTF("%s Enter submitted %d read_done %d transferred %d\n",
>              __FUNCTION__, block_mig_state.submitted, block_mig_state.read_done,
> @@ -457,7 +458,7 @@ static void flush_blks(QEMUFile* f)
>              break;
>          }
>          if (blk->ret < 0) {
> -            qemu_file_set_error(f, blk->ret);
> +            ret = blk->ret;
>              break;
>          }
>          blk_send(f, blk);
> @@ -474,6 +475,7 @@ static void flush_blks(QEMUFile* f)
>      DPRINTF("%s Exit submitted %d read_done %d transferred %d\n", __FUNCTION__,
>              block_mig_state.submitted, block_mig_state.read_done,
>              block_mig_state.transferred);
> +    return ret;
>  }
> 
>  static int64_t get_remaining_dirty(void)
> @@ -553,9 +555,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
>      /* start track dirty blocks */
>      set_dirty_tracking(1);
> 
> -    flush_blks(f);
> -
> -    ret = qemu_file_get_error(f);
> +    ret = flush_blks(f);
>      if (ret) {
>          blk_mig_cleanup();
>          return ret;
> @@ -575,9 +575,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>      DPRINTF("Enter save live iterate submitted %d transferred %d\n",
>              block_mig_state.submitted, block_mig_state.transferred);
> 
> -    flush_blks(f);
> -
> -    ret = qemu_file_get_error(f);
> +    ret = flush_blks(f);
>      if (ret) {
>          blk_mig_cleanup();
>          return ret;
> @@ -603,9 +601,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>          }
>      }
> 
> -    flush_blks(f);
> -
> -    ret = qemu_file_get_error(f);
> +    ret = flush_blks(f);
>      if (ret) {
>          blk_mig_cleanup();
>          return ret;
> @@ -623,9 +619,7 @@ static int block_save_complete(QEMUFile *f, void *opaque)
>      DPRINTF("Enter save live complete submitted %d transferred %d\n",
>              block_mig_state.submitted, block_mig_state.transferred);
> 
> -    flush_blks(f);
> -
> -    ret = qemu_file_get_error(f);
> +    ret = flush_blks(f);
>      if (ret) {
>          blk_mig_cleanup();
>          return ret;
> 

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

[Qemu-devel] [PATCH 37/41] block-migration: Switch meaning of return value

[Juan Quintela]

Make consistent the result of blk_mig_save_dirty_block() and
mig_save_device_dirty()

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

diff --git a/block-migration.c b/block-migration.c
index a822bb2..565628f 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -429,14 +429,18 @@ error:
     return 0;
 }

+/* return value:
+ * 0: too much data for max_downtime
+ * 1: few enough data for max_downtime
+*/
 static int blk_mig_save_dirty_block(QEMUFile *f, int is_async)
 {
     BlkMigDevState *bmds;
-    int ret = 0;
+    int ret = 1;

     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
-        if (mig_save_device_dirty(f, bmds, is_async) == 0) {
-            ret = 1;
+        ret = mig_save_device_dirty(f, bmds, is_async);
+        if (ret == 0) {
             break;
         }
     }
@@ -594,7 +598,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
                 block_mig_state.bulk_completed = 1;
             }
         } else {
-            if (blk_mig_save_dirty_block(f, 1) == 0) {
+            if (blk_mig_save_dirty_block(f, 1) != 0) {
                 /* no more dirty blocks */
                 break;
             }
@@ -631,7 +635,7 @@ static int block_save_complete(QEMUFile *f, void *opaque)
        all async read completed */
     assert(block_mig_state.submitted == 0);

-    while (blk_mig_save_dirty_block(f, 0) != 0) {
+    while (blk_mig_save_dirty_block(f, 0) == 0) {
         /* Do nothing */
     }
     blk_mig_cleanup();
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 37/41] block-migration: Switch meaning of return value

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Make consistent the result of blk_mig_save_dirty_block() and
> mig_save_device_dirty()
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  block-migration.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index a822bb2..565628f 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -429,14 +429,18 @@ error:
>      return 0;
>  }
> 
> +/* return value:
> + * 0: too much data for max_downtime
> + * 1: few enough data for max_downtime
> +*/
>  static int blk_mig_save_dirty_block(QEMUFile *f, int is_async)
>  {
>      BlkMigDevState *bmds;
> -    int ret = 0;
> +    int ret = 1;
> 
>      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
> -        if (mig_save_device_dirty(f, bmds, is_async) == 0) {
> -            ret = 1;
> +        ret = mig_save_device_dirty(f, bmds, is_async);
> +        if (ret == 0) {
>              break;
>          }
>      }
> @@ -594,7 +598,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>                  block_mig_state.bulk_completed = 1;
>              }
>          } else {
> -            if (blk_mig_save_dirty_block(f, 1) == 0) {
> +            if (blk_mig_save_dirty_block(f, 1) != 0) {
>                  /* no more dirty blocks */
>                  break;
>              }
> @@ -631,7 +635,7 @@ static int block_save_complete(QEMUFile *f, void *opaque)
>         all async read completed */
>      assert(block_mig_state.submitted == 0);
> 
> -    while (blk_mig_save_dirty_block(f, 0) != 0) {
> +    while (blk_mig_save_dirty_block(f, 0) == 0) {
>          /* Do nothing */
>      }
>      blk_mig_cleanup();
> 

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

[Qemu-devel] [PATCH 38/41] block-migration: handle errors with the return codes correctly

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block-migration.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 565628f..9f94733 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -423,10 +423,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,

 error:
     DPRINTF("Error reading sector %" PRId64 "\n", sector);
-    qemu_file_set_error(f, ret);
     g_free(blk->buf);
     g_free(blk);
-    return 0;
+    return ret;
 }

 /* return value:
@@ -440,7 +439,7 @@ static int blk_mig_save_dirty_block(QEMUFile *f, int is_async)

     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
         ret = mig_save_device_dirty(f, bmds, is_async);
-        if (ret == 0) {
+        if (ret <= 0) {
             break;
         }
     }
@@ -598,12 +597,17 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
                 block_mig_state.bulk_completed = 1;
             }
         } else {
-            if (blk_mig_save_dirty_block(f, 1) != 0) {
+            ret = blk_mig_save_dirty_block(f, 1);
+            if (ret != 0) {
                 /* no more dirty blocks */
                 break;
             }
         }
     }
+    if (ret) {
+        blk_mig_cleanup();
+        return ret;
+    }

     ret = flush_blks(f);
     if (ret) {
@@ -635,18 +639,15 @@ static int block_save_complete(QEMUFile *f, void *opaque)
        all async read completed */
     assert(block_mig_state.submitted == 0);

-    while (blk_mig_save_dirty_block(f, 0) == 0) {
+    while ((ret = blk_mig_save_dirty_block(f, 0)) == 0) {
         /* Do nothing */
     }
     blk_mig_cleanup();
-
-    /* report completion */
-    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);
-
-    ret = qemu_file_get_error(f);
     if (ret) {
         return ret;
     }
+    /* report completion */
+    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);

     DPRINTF("Block migration completed\n");

-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 38/41] block-migration: handle errors with the return codes correctly

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  block-migration.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index 565628f..9f94733 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -423,10 +423,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
> 
>  error:
>      DPRINTF("Error reading sector %" PRId64 "\n", sector);
> -    qemu_file_set_error(f, ret);
>      g_free(blk->buf);
>      g_free(blk);
> -    return 0;
> +    return ret;
>  }
> 
>  /* return value:
> @@ -440,7 +439,7 @@ static int blk_mig_save_dirty_block(QEMUFile *f, int is_async)
> 
>      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
>          ret = mig_save_device_dirty(f, bmds, is_async);
> -        if (ret == 0) {
> +        if (ret <= 0) {
>              break;
>          }
>      }
> @@ -598,12 +597,17 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>                  block_mig_state.bulk_completed = 1;
>              }
>          } else {
> -            if (blk_mig_save_dirty_block(f, 1) != 0) {
> +            ret = blk_mig_save_dirty_block(f, 1);
> +            if (ret != 0) {
>                  /* no more dirty blocks */
>                  break;
>              }
>          }
>      }
> +    if (ret) {
> +        blk_mig_cleanup();
> +        return ret;
> +    }
> 
>      ret = flush_blks(f);
>      if (ret) {
> @@ -635,18 +639,15 @@ static int block_save_complete(QEMUFile *f, void *opaque)
>         all async read completed */
>      assert(block_mig_state.submitted == 0);
> 

Not clear what the fixes are, I'll take it as a cleanup.  Then:

> -    while (blk_mig_save_dirty_block(f, 0) == 0) {
> +    while ((ret = blk_mig_save_dirty_block(f, 0)) == 0) {
>          /* Do nothing */
>      }

use a do...while instead:

    do {
        ret = blk_mig_save_dirty_block(f, 0);
    } while (ret == 0);

>      blk_mig_cleanup();
> -
> -    /* report completion */
> -    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);
> -
> -    ret = qemu_file_get_error(f);
>      if (ret) {
>          return ret;
>      }

Is it correct that we now return 1?  We used to return 0.

Paolo

> +    /* report completion */
> +    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);
> 
>      DPRINTF("Block migration completed\n");
> 

Re: [Qemu-devel] [PATCH 38/41] block-migration: handle errors with the return codes correctly

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 21/09/2012 10:47, Juan Quintela ha scritto:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> @@ -635,18 +639,15 @@ static int block_save_complete(QEMUFile *f, void *opaque)
>>         all async read completed */
>>      assert(block_mig_state.submitted == 0);
>> 
>
> Not clear what the fixes are, I'll take it as a cleanup.  Then:

We are returning now the errors directly.  Until now, we were 

>
>> -    while (blk_mig_save_dirty_block(f, 0) == 0) {
>> +    while ((ret = blk_mig_save_dirty_block(f, 0)) == 0) {
>>          /* Do nothing */
>>      }
>
> use a do...while instead:
>
>     do {
>         ret = blk_mig_save_dirty_block(f, 0);
>     } while (ret == 0);

I wanted to minimize the changes, but I don't really care, 

>
>>      blk_mig_cleanup();
>> -
>> -    /* report completion */
>> -    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);
>> -
>> -    ret = qemu_file_get_error(f);
>>      if (ret) {
>>          return ret;
>>      }
>
> Is it correct that we now return 1?  We used to return 0.

If you look at the whole change, old code was:


    while (blk_mig_save_dirty_block(f, 0) != 0) {
        /* Do nothing */
    }
    blk_mig_cleanup();

    /* report completion */
    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);

    ret = qemu_file_get_error(f);
    if (ret) {
        return ret;
    }

    DPRINTF("Block migration completed\n");

    qemu_put_be64(f, BLK_MIG_FLAG_EOS);

    return 0;

i.e. if there is one error, we return it, otherwise, we return 0.

    while ((ret = blk_mig_save_dirty_block(f, 0)) == 0) {
        /* Do nothing */
    }
    blk_mig_cleanup();
    if (ret) {
        return ret;
    }
    /* report completion */
    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);

    DPRINTF("Block migration completed\n");

    qemu_put_be64(f, BLK_MIG_FLAG_EOS);

    return 0;

Now, we give exactly the same error, just that we return the error
directly from blk_mig_save_dirty_block() instead of storing it on
QEMUFile and getting it through qemu_file_get_error().

Notice that between old code and the _newer_ code, we have reverted the
return value of blk_mig_save_block to make things more consistent.

Later, Juan.

Re: [Qemu-devel] [PATCH 38/41] block-migration: handle errors with the return codes correctly

[Paolo Bonzini]

Il 24/09/2012 20:24, Juan Quintela ha scritto:
> Now, we give exactly the same error, just that we return the error
> directly from blk_mig_save_dirty_block() instead of storing it on
> QEMUFile and getting it through qemu_file_get_error().

Right you are.  Apart from the do/while,

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

[Qemu-devel] [PATCH 39/41] savevm: un-export qemu_file_set_error()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 qemu-file.h | 1 -
 savevm.c    | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/qemu-file.h b/qemu-file.h
index 289849a..8dd9207 100644
--- a/qemu-file.h
+++ b/qemu-file.h
@@ -103,7 +103,6 @@ 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_get_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/savevm.c b/savevm.c
index 59ec8bf..68c0464 100644
--- a/savevm.c
+++ b/savevm.c
@@ -440,7 +440,7 @@ int qemu_file_get_error(QEMUFile *f)
     return f->last_error;
 }

-void qemu_file_set_error(QEMUFile *f, int ret)
+static void qemu_file_set_error(QEMUFile *f, int ret)
 {
     f->last_error = ret;
 }
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 39/41] savevm: un-export qemu_file_set_error()

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  qemu-file.h | 1 -
>  savevm.c    | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/qemu-file.h b/qemu-file.h
> index 289849a..8dd9207 100644
> --- a/qemu-file.h
> +++ b/qemu-file.h
> @@ -103,7 +103,6 @@ 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_get_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/savevm.c b/savevm.c
> index 59ec8bf..68c0464 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -440,7 +440,7 @@ int qemu_file_get_error(QEMUFile *f)
>      return f->last_error;
>  }
> 
> -void qemu_file_set_error(QEMUFile *f, int ret)
> +static void qemu_file_set_error(QEMUFile *f, int ret)
>  {
>      f->last_error = ret;
>  }
> 

Yay.

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

[Qemu-devel] [PATCH 40/41] savevm: make qemu_file_put_notify() return errors

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c | 5 +++--
 qemu-file.h | 2 +-
 savevm.c    | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration.c b/migration.c
index 6a505c1..2c29d04 100644
--- a/migration.c
+++ b/migration.c
@@ -285,10 +285,11 @@ static void migrate_fd_completed(MigrationState *s)
 static void migrate_fd_put_notify(void *opaque)
 {
     MigrationState *s = opaque;
+    int ret;

     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-    qemu_file_put_notify(s->file);
-    if (s->file && qemu_file_get_error(s->file)) {
+    ret = qemu_file_put_notify(s->file);
+    if (ret) {
         migrate_fd_error(s);
     }
 }
diff --git a/qemu-file.h b/qemu-file.h
index 8dd9207..9c8985b 100644
--- a/qemu-file.h
+++ b/qemu-file.h
@@ -107,7 +107,7 @@ int qemu_file_get_error(QEMUFile *f);
 /* 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
  * resume output. */
-void qemu_file_put_notify(QEMUFile *f);
+int qemu_file_put_notify(QEMUFile *f);

 static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
 {
diff --git a/savevm.c b/savevm.c
index 68c0464..2ea1fa6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -523,9 +523,9 @@ int qemu_fclose(QEMUFile *f)
     return ret;
 }

-void qemu_file_put_notify(QEMUFile *f)
+int qemu_file_put_notify(QEMUFile *f)
 {
-    f->put_buffer(f->opaque, NULL, 0, 0);
+    return f->put_buffer(f->opaque, NULL, 0, 0);
 }

 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 40/41] savevm: make qemu_file_put_notify() return errors

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c | 5 +++--
>  qemu-file.h | 2 +-
>  savevm.c    | 4 ++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 6a505c1..2c29d04 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -285,10 +285,11 @@ static void migrate_fd_completed(MigrationState *s)
>  static void migrate_fd_put_notify(void *opaque)
>  {
>      MigrationState *s = opaque;
> +    int ret;
> 
>      qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> -    qemu_file_put_notify(s->file);
> -    if (s->file && qemu_file_get_error(s->file)) {
> +    ret = qemu_file_put_notify(s->file);
> +    if (ret) {
>          migrate_fd_error(s);
>      }
>  }
> diff --git a/qemu-file.h b/qemu-file.h
> index 8dd9207..9c8985b 100644
> --- a/qemu-file.h
> +++ b/qemu-file.h
> @@ -107,7 +107,7 @@ int qemu_file_get_error(QEMUFile *f);
>  /* 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
>   * resume output. */
> -void qemu_file_put_notify(QEMUFile *f);
> +int qemu_file_put_notify(QEMUFile *f);
> 
>  static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
>  {
> diff --git a/savevm.c b/savevm.c
> index 68c0464..2ea1fa6 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -523,9 +523,9 @@ int qemu_fclose(QEMUFile *f)
>      return ret;
>  }
> 
> -void qemu_file_put_notify(QEMUFile *f)
> +int qemu_file_put_notify(QEMUFile *f)
>  {
> -    f->put_buffer(f->opaque, NULL, 0, 0);
> +    return f->put_buffer(f->opaque, NULL, 0, 0);
>  }
> 
>  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
> 

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

[Qemu-devel] [PATCH 41/41] cpus: create qemu_cpu_is_vcpu()

[Juan Quintela]

Old code used !io_thread to know if a thread was an vcpu or not.  That
fails when we introduce the iothread.

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

diff --git a/cpus.c b/cpus.c
index e476a3c..1b7061a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -902,6 +902,11 @@ int qemu_cpu_is_self(void *_env)
     return qemu_thread_is_self(cpu->thread);
 }

+static bool qemu_cpu_is_vcpu(void)
+{
+    return cpu_single_env && qemu_cpu_is_self(&cpu_single_env);
+}
+
 void qemu_mutex_lock_iothread(void)
 {
     if (!tcg_enabled()) {
@@ -947,7 +952,7 @@ void pause_all_vcpus(void)
         penv = penv->next_cpu;
     }

-    if (!qemu_thread_is_self(&io_thread)) {
+    if (qemu_cpu_is_vcpu()) {
         cpu_stop_current();
         if (!kvm_enabled()) {
             while (penv) {
@@ -1064,7 +1069,7 @@ void cpu_stop_current(void)

 void vm_stop(RunState state)
 {
-    if (!qemu_thread_is_self(&io_thread)) {
+    if (qemu_cpu_is_vcpu()) {
         qemu_system_vmstop_request(state);
         /*
          * FIXME: should not return to device code in case
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 41/41] cpus: create qemu_cpu_is_vcpu()

[Paolo Bonzini]

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Old code used !io_thread to know if a thread was an vcpu or not.  That
> fails when we introduce the iothread.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  cpus.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index e476a3c..1b7061a 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -902,6 +902,11 @@ int qemu_cpu_is_self(void *_env)
>      return qemu_thread_is_self(cpu->thread);
>  }
> 
> +static bool qemu_cpu_is_vcpu(void)
> +{
> +    return cpu_single_env && qemu_cpu_is_self(&cpu_single_env);

Should be "cpu_single_env && qemu_cpu_is_self(&cpu_single_env)".

Please named the function qemu_in_vcpu_thread.

Paolo

> +}
> +
>  void qemu_mutex_lock_iothread(void)
>  {
>      if (!tcg_enabled()) {
> @@ -947,7 +952,7 @@ void pause_all_vcpus(void)
>          penv = penv->next_cpu;
>      }
> 
> -    if (!qemu_thread_is_self(&io_thread)) {
> +    if (qemu_cpu_is_vcpu()) {
>          cpu_stop_current();
>          if (!kvm_enabled()) {
>              while (penv) {
> @@ -1064,7 +1069,7 @@ void cpu_stop_current(void)
> 
>  void vm_stop(RunState state)
>  {
> -    if (!qemu_thread_is_self(&io_thread)) {
> +    if (qemu_cpu_is_vcpu()) {
>          qemu_system_vmstop_request(state);
>          /*
>           * FIXME: should not return to device code in case
> 

Re: [Qemu-devel] [PATCH 41/41] cpus: create qemu_cpu_is_vcpu()

[Andreas Färber]

Am 21.09.2012 14:54, schrieb Paolo Bonzini:
> Il 21/09/2012 10:47, Juan Quintela ha scritto:
>> Old code used !io_thread to know if a thread was an vcpu or not.  That
>> fails when we introduce the iothread.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  cpus.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index e476a3c..1b7061a 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -902,6 +902,11 @@ int qemu_cpu_is_self(void *_env)
>>      return qemu_thread_is_self(cpu->thread);
>>  }
>>
>> +static bool qemu_cpu_is_vcpu(void)
>> +{
>> +    return cpu_single_env && qemu_cpu_is_self(&cpu_single_env);
> 
> Should be "cpu_single_env && qemu_cpu_is_self(&cpu_single_env)".

"cpu_single_env && qemu_cpu_is_self(cpu_single_env)" maybe?

> Please named the function qemu_in_vcpu_thread.

Seconded, it does not take a CPU argument.

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 41/41] cpus: create qemu_cpu_is_vcpu()

[Paolo Bonzini]

Il 21/09/2012 16:48, Andreas Färber ha scritto:
>>> >>
>>> >> +static bool qemu_cpu_is_vcpu(void)
>>> >> +{
>>> >> +    return cpu_single_env && qemu_cpu_is_self(&cpu_single_env);
>> > 
>> > Should be "cpu_single_env && qemu_cpu_is_self(&cpu_single_env)".
> "cpu_single_env && qemu_cpu_is_self(cpu_single_env)" maybe?

O:-)

Paolo

Re: [Qemu-devel] [PATCH 41/41] cpus: create qemu_cpu_is_vcpu()

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 21/09/2012 10:47, Juan Quintela ha scritto:
>> Old code used !io_thread to know if a thread was an vcpu or not.  That
>> fails when we introduce the iothread.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  cpus.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/cpus.c b/cpus.c
>> index e476a3c..1b7061a 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -902,6 +902,11 @@ int qemu_cpu_is_self(void *_env)
>>      return qemu_thread_is_self(cpu->thread);
>>  }
>> 
>> +static bool qemu_cpu_is_vcpu(void)
>> +{
>> +    return cpu_single_env && qemu_cpu_is_self(&cpu_single_env);
>
> Should be "cpu_single_env && qemu_cpu_is_self(&cpu_single_env)".
>
> Please named the function qemu_in_vcpu_thread.

Agreed to both changes.

Later, Juan.