diff mbox

[5/6] Purge migration of (almost) everything to do with monitors

Message ID 1328902266-25308-6-git-send-email-lcapitulino@redhat.com
State New
Headers show

Commit Message

Luiz Capitulino Feb. 10, 2012, 7:31 p.m. UTC 
The Monitor object is passed back and forth within the migration/savevm
code so that it can print errors and progress to the user.

However, that approach assumes a HMP monitor, being completely invalid
in QMP.

This commit drops almost every single usage of the Monitor object, all
monitor_printf() calls have been converted into DPRINTF() ones.

There are a few remaining Monitor objects, those are going to be dropped
by the next commit.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 arch_init.c       |    2 +-
 block-migration.c |   58 ++++++++++++++++++++++------------------------------
 migration.c       |    8 +++---
 migration.h       |    2 +-
 savevm.c          |   29 ++++++++++++-------------
 sysemu.h          |    9 +++----
 vmstate.h         |    3 +-
 7 files changed, 50 insertions(+), 61 deletions(-)

Comments

Jan Kiszka Feb. 15, 2012, 9:02 a.m. UTC | #1 
On 2012-02-10 20:31, Luiz Capitulino wrote:
> The Monitor object is passed back and forth within the migration/savevm
> code so that it can print errors and progress to the user.
> 
> However, that approach assumes a HMP monitor, being completely invalid
> in QMP.
> 
> This commit drops almost every single usage of the Monitor object, all
> monitor_printf() calls have been converted into DPRINTF() ones.

Particularly NACK on this. Either the information is useless anyway,
then remove it. Otherwise, keep it for channels that can properly
display it (AKA HMP). I bet the latter can easily be achieved by
providing non-printing Monitor objects over QMP instances.

Jan
Luiz Capitulino Feb. 15, 2012, 12:53 p.m. UTC | #2 
On Wed, 15 Feb 2012 10:02:54 +0100
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2012-02-10 20:31, Luiz Capitulino wrote:
> > The Monitor object is passed back and forth within the migration/savevm
> > code so that it can print errors and progress to the user.
> > 
> > However, that approach assumes a HMP monitor, being completely invalid
> > in QMP.
> > 
> > This commit drops almost every single usage of the Monitor object, all
> > monitor_printf() calls have been converted into DPRINTF() ones.
> 
> Particularly NACK on this. Either the information is useless anyway,
> then remove it. Otherwise, keep it for channels that can properly
> display it (AKA HMP). I bet the latter can easily be achieved by
> providing non-printing Monitor objects over QMP instances.

I will consider dropping it :)

I can think of two ways of displaying status in HMP (considering the new
HMP/QMP split design):

 1. We add all progress status information to a query command, and let HMP
    poll it (manually by the user or automatically from a timer)

 2. We emit an event whenever the progress status changes (seems a bit overkill)
Juan Quintela Feb. 15, 2012, 1:15 p.m. UTC | #3 
Luiz Capitulino <lcapitulino@redhat.com> wrote:
> The Monitor object is passed back and forth within the migration/savevm
> code so that it can print errors and progress to the user.
>
> However, that approach assumes a HMP monitor, being completely invalid
> in QMP.
>
> This commit drops almost every single usage of the Monitor object, all
> monitor_printf() calls have been converted into DPRINTF() ones.
>
> There are a few remaining Monitor objects, those are going to be dropped
> by the next commit.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Basically only block-migration.c uses the monitor at all (rest of code
only pass around for block migration).  Block migration don't work vey
well (to put it midly) at the moment.

I agree with removing the parameter, and also the info if that is
required.

Later, Juan.
Jan Kiszka Feb. 15, 2012, 1:32 p.m. UTC | #4 
On 2012-02-15 13:53, Luiz Capitulino wrote:
> On Wed, 15 Feb 2012 10:02:54 +0100
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> On 2012-02-10 20:31, Luiz Capitulino wrote:
>>> The Monitor object is passed back and forth within the migration/savevm
>>> code so that it can print errors and progress to the user.
>>>
>>> However, that approach assumes a HMP monitor, being completely invalid
>>> in QMP.
>>>
>>> This commit drops almost every single usage of the Monitor object, all
>>> monitor_printf() calls have been converted into DPRINTF() ones.
>>
>> Particularly NACK on this. Either the information is useless anyway,
>> then remove it. Otherwise, keep it for channels that can properly
>> display it (AKA HMP). I bet the latter can easily be achieved by
>> providing non-printing Monitor objects over QMP instances.
> 
> I will consider dropping it :)
> 
> I can think of two ways of displaying status in HMP (considering the new
> HMP/QMP split design):
> 
>  1. We add all progress status information to a query command, and let HMP
>     poll it (manually by the user or automatically from a timer)

As migration can also be synchronous, polling has to be time-driven.

> 
>  2. We emit an event whenever the progress status changes (seems a bit overkill)

If there is a use in QMP as well... dunno.

Jan
diff mbox

Patch

diff --git a/arch_init.c b/arch_init.c
index 699bdd1..2988964 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -259,7 +259,7 @@  static void sort_ram_list(void)
     g_free(blocks);
 }
 
-int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
+int ram_save_live(QEMUFile *f, int stage, void *opaque)
 {
     ram_addr_t addr;
     uint64_t bytes_transferred_last;
diff --git a/block-migration.c b/block-migration.c
index 4467468..fd2ffff 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -18,7 +18,6 @@ 
 #include "hw/hw.h"
 #include "qemu-queue.h"
 #include "qemu-timer.h"
-#include "monitor.h"
 #include "block-migration.h"
 #include "migration.h"
 #include "blockdev.h"
@@ -204,8 +203,7 @@  static void blk_mig_read_cb(void *opaque, int ret)
     assert(block_mig_state.submitted >= 0);
 }
 
-static int mig_save_device_bulk(Monitor *mon, QEMUFile *f,
-                                BlkMigDevState *bmds)
+static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
 {
     int64_t total_sectors = bmds->total_sectors;
     int64_t cur_sector = bmds->cur_sector;
@@ -272,7 +270,6 @@  static void set_dirty_tracking(int enable)
 
 static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
 {
-    Monitor *mon = opaque;
     BlkMigDevState *bmds;
     int64_t sectors;
 
@@ -295,19 +292,17 @@  static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
         block_mig_state.total_sector_sum += sectors;
 
         if (bmds->shared_base) {
-            monitor_printf(mon, "Start migration for %s with shared base "
-                                "image\n",
-                           bs->device_name);
+            DPRINTF("Start migration for %s with shared base image\n",
+                    bs->device_name);
         } else {
-            monitor_printf(mon, "Start full migration for %s\n",
-                           bs->device_name);
+            DPRINTF("Start full migration for %s\n", bs->device_name);
         }
 
         QSIMPLEQ_INSERT_TAIL(&block_mig_state.bmds_list, bmds, entry);
     }
 }
 
-static void init_blk_migration(Monitor *mon, QEMUFile *f)
+static void init_blk_migration(QEMUFile *f)
 {
     block_mig_state.submitted = 0;
     block_mig_state.read_done = 0;
@@ -318,10 +313,10 @@  static void init_blk_migration(Monitor *mon, QEMUFile *f)
     block_mig_state.total_time = 0;
     block_mig_state.reads = 0;
 
-    bdrv_iterate(init_blk_migration_it, mon);
+    bdrv_iterate(init_blk_migration_it, NULL);
 }
 
-static int blk_mig_save_bulked_block(Monitor *mon, QEMUFile *f)
+static int blk_mig_save_bulked_block(QEMUFile *f)
 {
     int64_t completed_sector_sum = 0;
     BlkMigDevState *bmds;
@@ -330,7 +325,7 @@  static int blk_mig_save_bulked_block(Monitor *mon, QEMUFile *f)
 
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
         if (bmds->bulk_completed == 0) {
-            if (mig_save_device_bulk(mon, f, bmds) == 1) {
+            if (mig_save_device_bulk(f, bmds) == 1) {
                 /* completed bulk section for this device */
                 bmds->bulk_completed = 1;
             }
@@ -352,8 +347,7 @@  static int blk_mig_save_bulked_block(Monitor *mon, QEMUFile *f)
         block_mig_state.prev_progress = progress;
         qemu_put_be64(f, (progress << BDRV_SECTOR_BITS)
                          | BLK_MIG_FLAG_PROGRESS);
-        monitor_printf(mon, "Completed %d %%\r", progress);
-        monitor_flush(mon);
+        DPRINTF("Completed %d %%\r", progress);
     }
 
     return ret;
@@ -368,8 +362,8 @@  static void blk_mig_reset_dirty_cursor(void)
     }
 }
 
-static int mig_save_device_dirty(Monitor *mon, QEMUFile *f,
-                                 BlkMigDevState *bmds, int is_async)
+static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
+                                 int is_async)
 {
     BlkMigBlock *blk;
     int64_t total_sectors = bmds->total_sectors;
@@ -428,20 +422,20 @@  static int mig_save_device_dirty(Monitor *mon, QEMUFile *f,
     return (bmds->cur_dirty >= bmds->total_sectors);
 
 error:
-    monitor_printf(mon, "Error reading sector %" PRId64 "\n", sector);
+    DPRINTF("Error reading sector %" PRId64 "\n", sector);
     qemu_file_set_error(f, ret);
     g_free(blk->buf);
     g_free(blk);
     return 0;
 }
 
-static int blk_mig_save_dirty_block(Monitor *mon, QEMUFile *f, int is_async)
+static int blk_mig_save_dirty_block(QEMUFile *f, int is_async)
 {
     BlkMigDevState *bmds;
     int ret = 0;
 
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
-        if (mig_save_device_dirty(mon, f, bmds, is_async) == 0) {
+        if (mig_save_device_dirty(f, bmds, is_async) == 0) {
             ret = 1;
             break;
         }
@@ -520,7 +514,7 @@  static int is_stage2_completed(void)
     return 0;
 }
 
-static void blk_mig_cleanup(Monitor *mon)
+static void blk_mig_cleanup(void)
 {
     BlkMigDevState *bmds;
     BlkMigBlock *blk;
@@ -540,11 +534,9 @@  static void blk_mig_cleanup(Monitor *mon)
         g_free(blk->buf);
         g_free(blk);
     }
-
-    monitor_printf(mon, "\n");
 }
 
-static int block_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
+static int block_save_live(QEMUFile *f, int stage, void *opaque)
 {
     int ret;
 
@@ -552,7 +544,7 @@  static int block_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
             stage, block_mig_state.submitted, block_mig_state.transferred);
 
     if (stage < 0) {
-        blk_mig_cleanup(mon);
+        blk_mig_cleanup();
         return 0;
     }
 
@@ -563,7 +555,7 @@  static int block_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
     }
 
     if (stage == 1) {
-        init_blk_migration(mon, f);
+        init_blk_migration(f);
 
         /* start track dirty blocks */
         set_dirty_tracking(1);
@@ -573,7 +565,7 @@  static int block_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
 
     ret = qemu_file_get_error(f);
     if (ret) {
-        blk_mig_cleanup(mon);
+        blk_mig_cleanup();
         return ret;
     }
 
@@ -586,12 +578,12 @@  static int block_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
                qemu_file_get_rate_limit(f)) {
             if (block_mig_state.bulk_completed == 0) {
                 /* first finish the bulk phase */
-                if (blk_mig_save_bulked_block(mon, f) == 0) {
+                if (blk_mig_save_bulked_block(f) == 0) {
                     /* finished saving bulk on all devices */
                     block_mig_state.bulk_completed = 1;
                 }
             } else {
-                if (blk_mig_save_dirty_block(mon, f, 1) == 0) {
+                if (blk_mig_save_dirty_block(f, 1) == 0) {
                     /* no more dirty blocks */
                     break;
                 }
@@ -602,7 +594,7 @@  static int block_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
 
         ret = qemu_file_get_error(f);
         if (ret) {
-            blk_mig_cleanup(mon);
+            blk_mig_cleanup();
             return ret;
         }
     }
@@ -612,8 +604,8 @@  static int block_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
            all async read completed */
         assert(block_mig_state.submitted == 0);
 
-        while (blk_mig_save_dirty_block(mon, f, 0) != 0);
-        blk_mig_cleanup(mon);
+        while (blk_mig_save_dirty_block(f, 0) != 0);
+        blk_mig_cleanup();
 
         /* report completion */
         qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);
@@ -623,7 +615,7 @@  static int block_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
             return ret;
         }
 
-        monitor_printf(mon, "Block migration completed\n");
+        DPRINTF("Block migration completed\n");
     }
 
     qemu_put_be64(f, BLK_MIG_FLAG_EOS);
diff --git a/migration.c b/migration.c
index 37af438..5c5c94c 100644
--- a/migration.c
+++ b/migration.c
@@ -258,7 +258,7 @@  static void migrate_fd_put_ready(void *opaque)
     }
 
     DPRINTF("iterate\n");
-    ret = qemu_savevm_state_iterate(s->mon, s->file);
+    ret = qemu_savevm_state_iterate(s->file);
     if (ret < 0) {
         migrate_fd_error(s);
     } else if (ret == 1) {
@@ -267,7 +267,7 @@  static void migrate_fd_put_ready(void *opaque)
         DPRINTF("done iterating\n");
         vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
 
-        if (qemu_savevm_state_complete(s->mon, s->file) < 0) {
+        if (qemu_savevm_state_complete(s->file) < 0) {
             migrate_fd_error(s);
         } else {
             migrate_fd_completed(s);
@@ -289,7 +289,7 @@  static void migrate_fd_cancel(MigrationState *s)
 
     s->state = MIG_STATE_CANCELLED;
     notifier_list_notify(&migration_state_notifiers, s);
-    qemu_savevm_state_cancel(s->mon, s->file);
+    qemu_savevm_state_cancel(s->file);
 
     migrate_fd_cleanup(s);
 }
@@ -367,7 +367,7 @@  void migrate_fd_connect(MigrationState *s)
                                       migrate_fd_close);
 
     DPRINTF("beginning savevm\n");
-    ret = qemu_savevm_state_begin(s->mon, s->file, s->blk, s->shared);
+    ret = qemu_savevm_state_begin(s->file, s->blk, s->shared);
     if (ret < 0) {
         DPRINTF("failed, %d\n", ret);
         migrate_fd_error(s);
diff --git a/migration.h b/migration.h
index 372b066..0e44197 100644
--- a/migration.h
+++ b/migration.h
@@ -78,7 +78,7 @@  uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_transferred(void);
 uint64_t ram_bytes_total(void);
 
-int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque);
+int ram_save_live(QEMUFile *f, int stage, void *opaque);
 int ram_load(QEMUFile *f, void *opaque, int version_id);
 
 /**
diff --git a/savevm.c b/savevm.c
index 80be1ff..70f5c4f 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1554,8 +1554,7 @@  bool qemu_savevm_state_blocked(Monitor *mon)
     return false;
 }
 
-int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
-                            int shared)
+int qemu_savevm_state_begin(QEMUFile *f, int blk_enable, int shared)
 {
     SaveStateEntry *se;
     int ret;
@@ -1588,15 +1587,15 @@  int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
         qemu_put_be32(f, se->instance_id);
         qemu_put_be32(f, se->version_id);
 
-        ret = se->save_live_state(mon, f, QEMU_VM_SECTION_START, se->opaque);
+        ret = se->save_live_state(f, QEMU_VM_SECTION_START, se->opaque);
         if (ret < 0) {
-            qemu_savevm_state_cancel(mon, f);
+            qemu_savevm_state_cancel(f);
             return ret;
         }
     }
     ret = qemu_file_get_error(f);
     if (ret != 0) {
-        qemu_savevm_state_cancel(mon, f);
+        qemu_savevm_state_cancel(f);
     }
 
     return ret;
@@ -1609,7 +1608,7 @@  int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
  *   0 : We haven't finished, caller have to go again
  *   1 : We have finished, we can go to complete phase
  */
-int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
+int qemu_savevm_state_iterate(QEMUFile *f)
 {
     SaveStateEntry *se;
     int ret = 1;
@@ -1622,7 +1621,7 @@  int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
         qemu_put_byte(f, QEMU_VM_SECTION_PART);
         qemu_put_be32(f, se->section_id);
 
-        ret = se->save_live_state(mon, f, QEMU_VM_SECTION_PART, se->opaque);
+        ret = se->save_live_state(f, QEMU_VM_SECTION_PART, se->opaque);
         if (ret <= 0) {
             /* Do not proceed to the next vmstate before this one reported
                completion of the current stage. This serializes the migration
@@ -1636,12 +1635,12 @@  int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
     }
     ret = qemu_file_get_error(f);
     if (ret != 0) {
-        qemu_savevm_state_cancel(mon, f);
+        qemu_savevm_state_cancel(f);
     }
     return ret;
 }
 
-int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
+int qemu_savevm_state_complete(QEMUFile *f)
 {
     SaveStateEntry *se;
     int ret;
@@ -1656,7 +1655,7 @@  int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
         qemu_put_byte(f, QEMU_VM_SECTION_END);
         qemu_put_be32(f, se->section_id);
 
-        ret = se->save_live_state(mon, f, QEMU_VM_SECTION_END, se->opaque);
+        ret = se->save_live_state(f, QEMU_VM_SECTION_END, se->opaque);
         if (ret < 0) {
             return ret;
         }
@@ -1688,13 +1687,13 @@  int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
     return qemu_file_get_error(f);
 }
 
-void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f)
+void qemu_savevm_state_cancel(QEMUFile *f)
 {
     SaveStateEntry *se;
 
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         if (se->save_live_state) {
-            se->save_live_state(mon, f, -1, se->opaque);
+            se->save_live_state(f, -1, se->opaque);
         }
     }
 }
@@ -1708,17 +1707,17 @@  static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
         goto out;
     }
 
-    ret = qemu_savevm_state_begin(mon, f, 0, 0);
+    ret = qemu_savevm_state_begin(f, 0, 0);
     if (ret < 0)
         goto out;
 
     do {
-        ret = qemu_savevm_state_iterate(mon, f);
+        ret = qemu_savevm_state_iterate(f);
         if (ret < 0)
             goto out;
     } while (ret == 0);
 
-    ret = qemu_savevm_state_complete(mon, f);
+    ret = qemu_savevm_state_complete(f);
 
 out:
     if (ret == 0) {
diff --git a/sysemu.h b/sysemu.h
index 9d5ce33..3a0f683 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -66,11 +66,10 @@  void do_info_snapshots(Monitor *mon);
 void qemu_announce_self(void);
 
 bool qemu_savevm_state_blocked(Monitor *mon);
-int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
-                            int shared);
-int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
-int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
-void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
+int qemu_savevm_state_begin(QEMUFile *f, int blk_enable, int shared);
+int qemu_savevm_state_iterate(QEMUFile *f);
+int qemu_savevm_state_complete(QEMUFile *f);
+void qemu_savevm_state_cancel(QEMUFile *f);
 int qemu_loadvm_state(QEMUFile *f);
 
 /* SLIRP */
diff --git a/vmstate.h b/vmstate.h
index 9d3c49c..82d97ae 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -28,8 +28,7 @@ 
 
 typedef void SaveSetParamsHandler(int blk_enable, int shared, void * opaque);
 typedef void SaveStateHandler(QEMUFile *f, void *opaque);
-typedef int SaveLiveStateHandler(Monitor *mon, QEMUFile *f, int stage,
-                                 void *opaque);
+typedef int SaveLiveStateHandler(QEMUFile *f, int stage, void *opaque);
 typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
 
 int register_savevm(DeviceState *dev,