[Qemu-devel] [PATCH v2 0/4] qapi: Convert migrate

[Qemu-devel] [PATCH v2 0/4] qapi: Convert migrate

[Luiz Capitulino]

v2

o Drop qemu_get_fd() (use monitor_get_fd(cur_mon, instead)
o Drop qdict_copy() (use refcount instead)
o Check for monitor_suspend() return value
o Other minor changes

Original intro:

This is a rebase of Anthony's conversion, from his glib branch; and this is
also the beginning of the conversion of complex commands to the qapi.

There are two important changes that should be observed:

 1. patch 3/4 purges the 'mon' object from migration code. One of the
    consequences is that we loose the ability to print progress status to
    the HMP user (esp. in block migration)

 2. The HMP hmp_migrate() command is a bit tricky when in non-detached
    mode: we lock the monitor and poll for the migration status from a
    timer handler. This obviously assumes that migration will end at some
    point

 arch_init.c       |    2 +-
 block-migration.c |   58 ++++++++++++++++++-----------------------
 error.c           |   13 ++++++++++
 error.h           |    5 ++++
 hmp-commands.hx   |    3 +--
 hmp.c             |   56 ++++++++++++++++++++++++++++++++++++++++
 hmp.h             |    1 +
 migration-fd.c    |    2 +-
 migration.c       |   74 +++++++++++++++--------------------------------------
 migration.h       |    5 +---
 qapi-schema.json  |   21 +++++++++++++++
 qerror.c          |    8 ++++++
 qerror.h          |    6 +++++
 qmp-commands.hx   |    9 +------
 savevm.c          |   42 +++++++++++++++---------------
 sysemu.h          |   11 ++++----
 vmstate.h         |    3 +--
 17 files changed, 187 insertions(+), 132 deletions(-)

[Qemu-devel] [PATCH 1/4] QError: Introduce new errors for the migration command

[Luiz Capitulino]

The new errors are QERR_MIGRATION_ACTIVE and QERR_MIGRATION_NOT_SUPPORTED,
which are going to be used by the QAPI converted migration command.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    8 ++++++++
 qerror.h |    6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/qerror.c b/qerror.c
index f55d435..41c729a 100644
--- a/qerror.c
+++ b/qerror.c
@@ -193,6 +193,14 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Using KVM without %(capability), %(feature) unavailable",
     },
     {
+        .error_fmt = QERR_MIGRATION_ACTIVE,
+        .desc      = "There's a migration process in progress",
+    },
+    {
+        .error_fmt = QERR_MIGRATION_NOT_SUPPORTED,
+        .desc      = "State blocked by non-migratable device '%(device)'",
+    },
+    {
         .error_fmt = QERR_MIGRATION_EXPECTED,
         .desc      = "An incoming migration is expected before this command can be executed",
     },
diff --git a/qerror.h b/qerror.h
index e26c635..e16f9c2 100644
--- a/qerror.h
+++ b/qerror.h
@@ -166,6 +166,12 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_KVM_MISSING_CAP \
     "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }"
 
+#define QERR_MIGRATION_ACTIVE \
+    "{ 'class': 'MigrationActive', 'data': {} }"
+
+#define QERR_MIGRATION_NOT_SUPPORTED \
+    "{ 'class': 'MigrationNotSupported', 'data': {'device': %s} }"
+
 #define QERR_MIGRATION_EXPECTED \
     "{ 'class': 'MigrationExpected', 'data': {} }"
 
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 2/4] Error: Introduce error_copy()

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 error.c |   13 +++++++++++++
 error.h |    5 +++++
 2 files changed, 18 insertions(+)

diff --git a/error.c b/error.c
index 990050f..d3455ab 100644
--- a/error.c
+++ b/error.c
@@ -43,6 +43,19 @@ void error_set(Error **errp, const char *fmt, ...)
     *errp = err;
 }
 
+Error *error_copy(const Error *err)
+{
+    Error *err_new;
+
+    err_new = g_malloc0(sizeof(*err));
+    err_new->msg = g_strdup(err->msg);
+    err_new->fmt = err->fmt;
+    err_new->obj = err->obj;
+    QINCREF(err_new->obj);
+
+    return err_new;
+}
+
 bool error_is_set(Error **errp)
 {
     return (errp && *errp);
diff --git a/error.h b/error.h
index 6361f40..45ff6c1 100644
--- a/error.h
+++ b/error.h
@@ -35,6 +35,11 @@ void error_set(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
 bool error_is_set(Error **err);
 
 /**
+ * Returns an exact copy of the error passed as an argument.
+ */
+Error *error_copy(const Error *err);
+
+/**
  * Get a human readable representation of an error object.
  */
 const char *error_get_pretty(Error *err);
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Luiz Capitulino]

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

diff --git a/arch_init.c b/arch_init.c
index a95ef49..595badf 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -260,7 +260,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 00fa1e3..b21b2df 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 98118cc..29b0e96 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -77,11 +77,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,
-- 
1.7.9.2.384.g4a92a

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Jan Kiszka]

On 2012-03-09 19:13, 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.

I guess for most printfs, this is OK. But do you provide an alternative
for the block migration progress output? I did not find anything on
first glance. That is not a debugging feature.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Luiz Capitulino]

On Fri, 09 Mar 2012 19:20:56 +0100
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2012-03-09 19:13, 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.
> 
> I guess for most printfs, this is OK. But do you provide an alternative
> for the block migration progress output? I did not find anything on
> first glance. That is not a debugging feature.

Can't the info migrate command be used for that?

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Jan Kiszka]

On 2012-03-09 19:30, Luiz Capitulino wrote:
> On Fri, 09 Mar 2012 19:20:56 +0100
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> On 2012-03-09 19:13, 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.
>>
>> I guess for most printfs, this is OK. But do you provide an alternative
>> for the block migration progress output? I did not find anything on
>> first glance. That is not a debugging feature.
> 
> Can't the info migrate command be used for that?

This was introduced for synchronous migration, as usability improvement.

Either we expose this feature or we drop it completely. But converting
it to a /dev/null output is pointless.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Anthony Liguori]

On 03/09/2012 01:05 PM, Jan Kiszka wrote:
> On 2012-03-09 19:30, Luiz Capitulino wrote:
>> On Fri, 09 Mar 2012 19:20:56 +0100
>> Jan Kiszka<jan.kiszka@siemens.com>  wrote:
>>
>>> On 2012-03-09 19:13, 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.
>>>
>>> I guess for most printfs, this is OK. But do you provide an alternative
>>> for the block migration progress output? I did not find anything on
>>> first glance. That is not a debugging feature.
>>
>> Can't the info migrate command be used for that?
>
> This was introduced for synchronous migration, as usability improvement.

Synchronous migration doesn't exist in QMP.

It's certainly possible to make the synchronous monitor command spit out status 
as it already uses a polling loop to determine when migration completes.

Regards,

Anthony Liguori

>
> Either we expose this feature or we drop it completely. But converting
> it to a /dev/null output is pointless.
>
> Jan
>

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Luiz Capitulino]

On Fri, 09 Mar 2012 13:12:43 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/09/2012 01:05 PM, Jan Kiszka wrote:
> > On 2012-03-09 19:30, Luiz Capitulino wrote:
> >> On Fri, 09 Mar 2012 19:20:56 +0100
> >> Jan Kiszka<jan.kiszka@siemens.com>  wrote:
> >>
> >>> On 2012-03-09 19:13, 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.
> >>>
> >>> I guess for most printfs, this is OK. But do you provide an alternative
> >>> for the block migration progress output? I did not find anything on
> >>> first glance. That is not a debugging feature.
> >>
> >> Can't the info migrate command be used for that?
> >
> > This was introduced for synchronous migration, as usability improvement.
> 
> Synchronous migration doesn't exist in QMP.
> 
> It's certainly possible to make the synchronous monitor command spit out status 
> as it already uses a polling loop to determine when migration completes.

As HMP now uses QMP as a real client, we'd have to make that information
available for all QMP users. Best place is probably query-migrate.

Don't you prefer to drop the whole thing instead? :-)

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Luiz Capitulino]

On Fri, 9 Mar 2012 16:48:10 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> > It's certainly possible to make the synchronous monitor command spit out status 
> > as it already uses a polling loop to determine when migration completes.
> 
> As HMP now uses QMP as a real client, we'd have to make that information
> available for all QMP users. Best place is probably query-migrate.
> 
> Don't you prefer to drop the whole thing instead? :-)

Oh, we actually have block migration information in query-migrate already,
maybe we can use that to have a progress bar (not sure it's the same info
used by current progress bar though).

But dropping it is still a viable alternative :)

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Kevin Wolf]

Am 09.03.2012 20:57, schrieb Luiz Capitulino:
> On Fri, 9 Mar 2012 16:48:10 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
>>> It's certainly possible to make the synchronous monitor command spit out status 
>>> as it already uses a polling loop to determine when migration completes.
>>
>> As HMP now uses QMP as a real client, we'd have to make that information
>> available for all QMP users. Best place is probably query-migrate.
>>
>> Don't you prefer to drop the whole thing instead? :-)
> 
> Oh, we actually have block migration information in query-migrate already,
> maybe we can use that to have a progress bar (not sure it's the same info
> used by current progress bar though).
> 
> But dropping it is still a viable alternative :)

We should use the decoupling of the human monitor to make it better
rather than worse. :-)

Kevin

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Luiz Capitulino]

On Mon, 12 Mar 2012 15:51:04 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 09.03.2012 20:57, schrieb Luiz Capitulino:
> > On Fri, 9 Mar 2012 16:48:10 -0300
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > 
> >>> It's certainly possible to make the synchronous monitor command spit out status 
> >>> as it already uses a polling loop to determine when migration completes.
> >>
> >> As HMP now uses QMP as a real client, we'd have to make that information
> >> available for all QMP users. Best place is probably query-migrate.
> >>
> >> Don't you prefer to drop the whole thing instead? :-)
> > 
> > Oh, we actually have block migration information in query-migrate already,
> > maybe we can use that to have a progress bar (not sure it's the same info
> > used by current progress bar though).
> > 
> > But dropping it is still a viable alternative :)
> 
> We should use the decoupling of the human monitor to make it better
> rather than worse. :-)

I've implemented the progress counting and will submit a new version of
the series later today.

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Anthony Liguori]

On 03/09/2012 12:20 PM, Jan Kiszka wrote:
> On 2012-03-09 19:13, 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.
>
> I guess for most printfs, this is OK. But do you provide an alternative
> for the block migration progress output? I did not find anything on
> first glance. That is not a debugging feature.

The block migration progress stuff is horribly broken and I regret ever merging 
it.  Are you actively relying on this?

Does block migration even work?

Could we just remove block migration entirely...

Regards,

Anthony Liguori

>
> Jan
>

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1812 bytes --]

On 03/09/2012 11:31 AM, Anthony Liguori wrote:
> On 03/09/2012 12:20 PM, Jan Kiszka wrote:
>> On 2012-03-09 19:13, 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.
>>
>> I guess for most printfs, this is OK. But do you provide an alternative
>> for the block migration progress output? I did not find anything on
>> first glance. That is not a debugging feature.
> 
> The block migration progress stuff is horribly broken and I regret ever
> merging it.  Are you actively relying on this?
> 
> Does block migration even work?
> 
> Could we just remove block migration entirely...

Libvirt has already exposed block migration to users, but it is
untested; if, as you say, it is horribly broken, then libvirt needs to
pass on the error message back to the user that it is broken.  But we
regularly get people complaining that they don't have shared storage set
up, and they are looking for a magic bullet that will do block migration
alongside the domain migration; perhaps libvirt can still provide that
with block mirroring or other (working) solutions from the same API it
gives to the user if you end up pulling it out of the migrate QMP
command.  At the end of the day, it's your call whether to pull it out,
and libvirt will adapt, but it would be nice to know what kosher working
alternative(s) to use instead.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Anthony Liguori]

On 03/09/2012 12:45 PM, Eric Blake wrote:
> On 03/09/2012 11:31 AM, Anthony Liguori wrote:
>> On 03/09/2012 12:20 PM, Jan Kiszka wrote:
>>> On 2012-03-09 19:13, 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.
>>>
>>> I guess for most printfs, this is OK. But do you provide an alternative
>>> for the block migration progress output? I did not find anything on
>>> first glance. That is not a debugging feature.
>>
>> The block migration progress stuff is horribly broken and I regret ever
>> merging it.  Are you actively relying on this?
>>
>> Does block migration even work?
>>
>> Could we just remove block migration entirely...
>
> Libvirt has already exposed block migration to users,

:-(

Do you rely on the progress stats that are printed?

> but it is
> untested; if, as you say, it is horribly broken, then libvirt needs to
> pass on the error message back to the user that it is broken.  But we
> regularly get people complaining that they don't have shared storage set
> up, and they are looking for a magic bullet that will do block migration
> alongside the domain migration; perhaps libvirt can still provide that
> with block mirroring or other (working) solutions from the same API it
> gives to the user if you end up pulling it out of the migrate QMP
> command.

Well block mirroring is hopefully around the corner.

Regards,

Anthony Liguori

> At the end of the day, it's your call whether to pull it out,
> and libvirt will adapt, but it would be nice to know what kosher working
> alternative(s) to use instead.
>

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

On 03/09/2012 11:53 AM, Anthony Liguori wrote:
>>>
>>> Could we just remove block migration entirely...
>>
>> Libvirt has already exposed block migration to users,
> 
> :-(
> 
> Do you rely on the progress stats that are printed?

I've never personally used it (as I said earlier, the libvirt support
for blk and inc doesn't get much testing), but I can state that if you do:

virsh migrate --copy-storage-all --copy-storage-inc --verbose ...

that virsh will cause libvirt to start up a detached migration, then
repeatedly call the query-migrate monitor command to update an on-screen
progress monitor to the user; this progress monitor tracks both ram and
disk status to compute the percentage complete showed to the user, so at
some level, libvirt really is trying to rely on accurate block migration
progress numbers.

>> regularly get people complaining that they don't have shared storage set
>> up, and they are looking for a magic bullet that will do block migration
>> alongside the domain migration; perhaps libvirt can still provide that
>> with block mirroring or other (working) solutions from the same API it
>> gives to the user if you end up pulling it out of the migrate QMP
>> command.
> 
> Well block mirroring is hopefully around the corner.

Just so I'm clear, are you talking about a more complete mirroring
solution than just the storage migration use of mirroring that enables
live block migration without also migrating VM state?  Also, is this a
qemu 1.1 planned feature, or will it more likely be in the 1.2 timeframe?

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

On 03/09/2012 12:42 PM, Eric Blake wrote:
> On 03/09/2012 11:53 AM, Anthony Liguori wrote:
>>>>
>>>> Could we just remove block migration entirely...
>>>
>>> Libvirt has already exposed block migration to users,
>>
>> :-(
>>
>> Do you rely on the progress stats that are printed?
> 
> I've never personally used it (as I said earlier, the libvirt support
> for blk and inc doesn't get much testing), but I can state that if you do:
> 
> virsh migrate --copy-storage-all --copy-storage-inc --verbose ...
> 
> that virsh will cause libvirt to start up a detached migration, then
> repeatedly call the query-migrate monitor command to update an on-screen
> progress monitor to the user; this progress monitor tracks both ram and
> disk status to compute the percentage complete showed to the user, so at
> some level, libvirt really is trying to rely on accurate block migration
> progress numbers.

Just realized I wasn't very clear:

libvirt does _not_ play back qemu's status numbers, so much as computing
its _own_ progress monitor based on parsing the output of the repeated
query-migrate commands.  And since libvirt tends to favor QMP over HMP,
the only monitoring we need from QMP is what we query for; we don't need
HMP-style update messages.  Having HMP update a progress line each time
it rearms the one second timer while waiting for the migration to
complete is probably good enough for HMP, and libvirt won't care.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 3/4] Purge migration of (almost) everything to do with monitors

[Jan Kiszka]

On 2012-03-09 19:31, Anthony Liguori wrote:
> On 03/09/2012 12:20 PM, Jan Kiszka wrote:
>> On 2012-03-09 19:13, 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.
>>
>> I guess for most printfs, this is OK. But do you provide an alternative
>> for the block migration progress output? I did not find anything on
>> first glance. That is not a debugging feature.
> 
> The block migration progress stuff is horribly broken and I regret ever merging 
> it.  Are you actively relying on this?
> 
> Does block migration even work?

The last time I tried (a while back), it worked for me, but I also
received reports that it was not working reliably.

> 
> Could we just remove block migration entirely...

Well, if we have a better alternative, replace it.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Luiz Capitulino]

The migrate command is one of those commands where HMP and QMP completely
mix up together. This made the conversion to the QAPI (which separates the
command into QMP and HMP parts) a bit difficult.

The first important change to be noticed is that this commit completes the
removal of the Monitor object from migration code, started by the previous
commit.

Another important and tricky change is about supporting the non-detached
mode. That's, if the user doesn't pass '-d' the migrate command will lock
the monitor and will only release it when migration is finished.

To support that in the new HMP command (hmp_migrate()), it is necessary
to create a timer which runs every second and checks if the migration is
still active. If it's, the timer callback will re-schedule itself to run
one second in the future. If the migration has already finished, the
monitor lock is relased and the user can use it normally.

All these changes should be transparent to the user.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx  |    3 +--
 hmp.c            |   56 +++++++++++++++++++++++++++++++++++++++++++++
 hmp.h            |    1 +
 migration-fd.c   |    2 +-
 migration.c      |   66 ++++++++++++++----------------------------------------
 migration.h      |    3 ---
 qapi-schema.json |   21 +++++++++++++++++
 qmp-commands.hx  |    9 +-------
 savevm.c         |   13 +++++------
 sysemu.h         |    2 +-
 10 files changed, 105 insertions(+), 71 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index ed88877..a86f53f 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -806,8 +806,7 @@ ETEXI
 		      " full copy of disk\n\t\t\t -i for migration without "
 		      "shared storage with incremental copy of disk "
 		      "(base image shared between src and destination)",
-        .user_print = monitor_user_noop,	
-	.mhandler.cmd_new = do_migrate,
+        .mhandler.cmd = hmp_migrate,
     },
 
 
diff --git a/hmp.c b/hmp.c
index 3a54455..0f62b3f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -14,6 +14,7 @@
  */
 
 #include "hmp.h"
+#include "qemu-timer.h"
 #include "qmp-commands.h"
 
 static void hmp_handle_error(Monitor *mon, Error **errp)
@@ -856,3 +857,58 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
 
     hmp_handle_error(mon, &error);
 }
+
+typedef struct MigrationStatus
+{
+    QEMUTimer *timer;
+    Monitor *mon;
+} MigrationStatus;
+
+static void hmp_migrate_status_cb(void *opaque)
+{
+    MigrationStatus *status = opaque;
+    MigrationInfo *info;
+
+    info = qmp_query_migrate(NULL);
+    if (!info->has_status || strcmp(info->status, "active") == 0) {
+        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock) + 1000);
+    } else {
+        monitor_resume(status->mon);
+        qemu_del_timer(status->timer);
+        g_free(status);
+    }
+
+    qapi_free_MigrationInfo(info);
+}
+
+void hmp_migrate(Monitor *mon, const QDict *qdict)
+{
+    int detach = qdict_get_try_bool(qdict, "detach", 0);
+    int blk = qdict_get_try_bool(qdict, "blk", 0);
+    int inc = qdict_get_try_bool(qdict, "inc", 0);
+    const char *uri = qdict_get_str(qdict, "uri");
+    Error *err = NULL;
+
+    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
+    if (err) {
+        monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
+        error_free(err);
+        return;
+    }
+
+    if (!detach) {
+        MigrationStatus *status;
+
+        if (monitor_suspend(mon) < 0) {
+            monitor_printf(mon, "terminal does not allow synchronous "
+                           "migration, continuing detached\n");
+            return;
+        }
+
+        status = g_malloc0(sizeof(*status));
+        status->mon = mon;
+        status->timer = qemu_new_timer_ms(rt_clock, hmp_migrate_status_cb,
+                                          status);
+        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
+    }
+}
diff --git a/hmp.h b/hmp.h
index 5409464..8807853 100644
--- a/hmp.h
+++ b/hmp.h
@@ -59,5 +59,6 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
 void hmp_block_stream(Monitor *mon, const QDict *qdict);
 void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
 void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
+void hmp_migrate(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/migration-fd.c b/migration-fd.c
index 5a068c6..50138ed 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -75,7 +75,7 @@ static int fd_close(MigrationState *s)
 
 int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
 {
-    s->fd = monitor_get_fd(s->mon, fdname);
+    s->fd = monitor_get_fd(cur_mon, fdname);
     if (s->fd == -1) {
         DPRINTF("fd_migration: invalid file descriptor identifier\n");
         goto err_after_get_fd;
diff --git a/migration.c b/migration.c
index b21b2df..8c119ba 100644
--- a/migration.c
+++ b/migration.c
@@ -158,16 +158,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
 
 /* shared migration helpers */
 
-static void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
-{
-    if (monitor_suspend(mon) == 0) {
-        DPRINTF("suspending monitor\n");
-    } else {
-        monitor_printf(mon, "terminal does not allow synchronous "
-                       "migration, continuing detached\n");
-    }
-}
-
 static int migrate_fd_cleanup(MigrationState *s)
 {
     int ret = 0;
@@ -178,10 +168,6 @@ static int migrate_fd_cleanup(MigrationState *s)
         DPRINTF("closing file\n");
         ret = qemu_fclose(s->file);
         s->file = NULL;
-    } else {
-        if (s->mon) {
-            monitor_resume(s->mon);
-        }
     }
 
     if (s->fd != -1) {
@@ -321,9 +307,6 @@ static int migrate_fd_close(void *opaque)
 {
     MigrationState *s = opaque;
 
-    if (s->mon) {
-        monitor_resume(s->mon);
-    }
     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
     return s->close(s);
 }
@@ -376,7 +359,7 @@ void migrate_fd_connect(MigrationState *s)
     migrate_fd_put_ready(s);
 }
 
-static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
+static MigrationState *migrate_init(int blk, int inc)
 {
     MigrationState *s = migrate_get_current();
     int64_t bandwidth_limit = s->bandwidth_limit;
@@ -386,18 +369,9 @@ static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
     s->blk = blk;
     s->shared = inc;
 
-    /* s->mon is used for two things:
-       - pass fd in fd migration
-       - suspend/resume monitor for not detached migration
-    */
-    s->mon = mon;
     s->bandwidth_limit = bandwidth_limit;
     s->state = MIG_STATE_SETUP;
 
-    if (!detach) {
-        migrate_fd_monitor_suspend(s, mon);
-    }
-
     return s;
 }
 
@@ -413,32 +387,29 @@ void migrate_del_blocker(Error *reason)
     migration_blockers = g_slist_remove(migration_blockers, reason);
 }
 
-int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_migrate(const char *uri, bool has_blk, bool blk,
+                 bool has_inc, bool inc, bool has_detach, bool detach,
+                 Error **errp)
 {
     MigrationState *s = migrate_get_current();
     const char *p;
-    int detach = qdict_get_try_bool(qdict, "detach", 0);
-    int blk = qdict_get_try_bool(qdict, "blk", 0);
-    int inc = qdict_get_try_bool(qdict, "inc", 0);
-    const char *uri = qdict_get_str(qdict, "uri");
     int ret;
 
     if (s->state == MIG_STATE_ACTIVE) {
-        monitor_printf(mon, "migration already in progress\n");
-        return -1;
+        error_set(errp, QERR_MIGRATION_ACTIVE);
+        return;
     }
 
-    if (qemu_savevm_state_blocked(mon)) {
-        return -1;
+    if (qemu_savevm_state_blocked(errp)) {
+        return;
     }
 
     if (migration_blockers) {
-        Error *err = migration_blockers->data;
-        qerror_report_err(err);
-        return -1;
+        *errp = error_copy(migration_blockers->data);
+        return;
     }
 
-    s = migrate_init(mon, detach, blk, inc);
+    s = migrate_init(blk, inc);
 
     if (strstart(uri, "tcp:", &p)) {
         ret = tcp_start_outgoing_migration(s, p);
@@ -451,21 +422,18 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
         ret = fd_start_outgoing_migration(s, p);
 #endif
     } else {
-        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
-        ret  = -EINVAL;
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
+        return;
     }
 
     if (ret < 0) {
-        monitor_printf(mon, "migration failed: %s\n", strerror(-ret));
-        return ret;
-    }
-
-    if (detach) {
-        s->mon = NULL;
+        DPRINTF("migration failed: %s\n", strerror(-ret));
+        /* FIXME: we should return meaningful errors */
+        error_set(errp, QERR_UNDEFINED_ERROR);
+        return;
     }
 
     notifier_list_notify(&migration_state_notifiers, s);
-    return 0;
 }
 
 void qmp_migrate_cancel(Error **errp)
diff --git a/migration.h b/migration.h
index 0e44197..691b367 100644
--- a/migration.h
+++ b/migration.h
@@ -26,7 +26,6 @@ struct MigrationState
     int64_t bandwidth_limit;
     QEMUFile *file;
     int fd;
-    Monitor *mon;
     int state;
     int (*get_error)(MigrationState *s);
     int (*close)(MigrationState *s);
@@ -40,8 +39,6 @@ void process_incoming_migration(QEMUFile *f);
 
 int qemu_start_incoming_migration(const char *uri);
 
-int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
-
 uint64_t migrate_max_downtime(void);
 
 void do_info_migrate_print(Monitor *mon, const QObject *data);
diff --git a/qapi-schema.json b/qapi-schema.json
index 5f293c4..c0780ab 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1631,3 +1631,24 @@
 { 'command': 'qom-list-types',
   'data': { '*implements': 'str', '*abstract': 'bool' },
   'returns': [ 'ObjectTypeInfo' ] }
+
+##
+# @migrate
+#
+# Migrates the current running guest to another Virtual Machine.
+#
+# @uri: the Uniform Resource Identifier of the destination VM
+#
+# @blk: #optional do block migration (full disk copy)
+#
+# @inc: #optional incremental disk copy migration
+#
+# @detach: this argument exists only for compatibility reasons and should not
+#          be used.
+#
+# Returns: nothing on success
+#
+# Since: 0.14.0
+##
+{ 'command': 'migrate',
+  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 0c9bfac..f328bc1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -446,14 +446,7 @@ EQMP
     {
         .name       = "migrate",
         .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
-        .params     = "[-d] [-b] [-i] uri",
-        .help       = "migrate to URI (using -d to not wait for completion)"
-		      "\n\t\t\t -b for migration without shared storage with"
-		      " full copy of disk\n\t\t\t -i for migration without "
-		      "shared storage with incremental copy of disk "
-		      "(base image shared between src and destination)",
-        .user_print = monitor_user_noop,	
-	.mhandler.cmd_new = do_migrate,
+        .mhandler.cmd_new = qmp_marshal_input_migrate,
     },
 
 SQMP
diff --git a/savevm.c b/savevm.c
index 70f5c4f..5fdc3e1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1540,14 +1540,13 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
 #define QEMU_VM_SECTION_FULL         0x04
 #define QEMU_VM_SUBSECTION           0x05
 
-bool qemu_savevm_state_blocked(Monitor *mon)
+bool qemu_savevm_state_blocked(Error **errp)
 {
     SaveStateEntry *se;
 
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         if (se->no_migrate) {
-            monitor_printf(mon, "state blocked by non-migratable device '%s'\n",
-                           se->idstr);
+            error_set(errp, QERR_MIGRATION_NOT_SUPPORTED, se->idstr);
             return true;
         }
     }
@@ -1698,11 +1697,11 @@ void qemu_savevm_state_cancel(QEMUFile *f)
     }
 }
 
-static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
+static int qemu_savevm_state(QEMUFile *f)
 {
     int ret;
 
-    if (qemu_savevm_state_blocked(mon)) {
+    if (qemu_savevm_state_blocked(NULL)) {
         ret = -EINVAL;
         goto out;
     }
@@ -1836,7 +1835,7 @@ int qemu_loadvm_state(QEMUFile *f)
     unsigned int v;
     int ret;
 
-    if (qemu_savevm_state_blocked(default_mon)) {
+    if (qemu_savevm_state_blocked(NULL)) {
         return -EINVAL;
     }
 
@@ -2080,7 +2079,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "Could not open VM state file\n");
         goto the_end;
     }
-    ret = qemu_savevm_state(mon, f);
+    ret = qemu_savevm_state(f);
     vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
     if (ret < 0) {
diff --git a/sysemu.h b/sysemu.h
index 29b0e96..bc2c788 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -76,7 +76,7 @@ void do_info_snapshots(Monitor *mon);
 
 void qemu_announce_self(void);
 
-bool qemu_savevm_state_blocked(Monitor *mon);
+bool qemu_savevm_state_blocked(Error **errp);
 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);
-- 
1.7.9.2.384.g4a92a

Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]

On 03/09/2012 11:13 AM, Luiz Capitulino wrote:
> The migrate command is one of those commands where HMP and QMP completely
> mix up together. This made the conversion to the QAPI (which separates the
> command into QMP and HMP parts) a bit difficult.
> 
> The first important change to be noticed is that this commit completes the
> removal of the Monitor object from migration code, started by the previous
> commit.
> 
> Another important and tricky change is about supporting the non-detached
> mode. That's, if the user doesn't pass '-d' the migrate command will lock

As a native speaker, I can state that it is unusual or even wrong to see
a contraction immediately before a comma.  I would do:

s/That's,/That is,/

> the monitor and will only release it when migration is finished.
> 
> To support that in the new HMP command (hmp_migrate()), it is necessary
> to create a timer which runs every second and checks if the migration is
> still active. If it's, the timer callback will re-schedule itself to run

Another instance of the same grammar problem:

s/it's/it is/

> one second in the future. If the migration has already finished, the
> monitor lock is relased and the user can use it normally.

s/relased/released/

> 
> All these changes should be transparent to the user.
> 

> +++ b/qapi-schema.json
> @@ -1631,3 +1631,24 @@
>  { 'command': 'qom-list-types',
>    'data': { '*implements': 'str', '*abstract': 'bool' },
>    'returns': [ 'ObjectTypeInfo' ] }
> +
> +##
> +# @migrate
> +#
> +# Migrates the current running guest to another Virtual Machine.
> +#
> +# @uri: the Uniform Resource Identifier of the destination VM
> +#
> +# @blk: #optional do block migration (full disk copy)
> +#
> +# @inc: #optional incremental disk copy migration
> +#
> +# @detach: this argument exists only for compatibility reasons and should not
> +#          be used.

Libvirt is currently using it, but it appears that libvirt always sets
it to true.  Does this mean the argument is ignored no matter whether
you pass true or false?  Or does this value have a default if omitted,
and it is safe to explicitly pass the default (I assume the default is
true), and that it is wrong to pass an explicit value that contradicts
the default (that is, I'm assuming passing an explicit 'detach':'false'
is wrong)?

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Luiz Capitulino]

On Fri, 09 Mar 2012 11:26:53 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 03/09/2012 11:13 AM, Luiz Capitulino wrote:
> > The migrate command is one of those commands where HMP and QMP completely
> > mix up together. This made the conversion to the QAPI (which separates the
> > command into QMP and HMP parts) a bit difficult.
> > 
> > The first important change to be noticed is that this commit completes the
> > removal of the Monitor object from migration code, started by the previous
> > commit.
> > 
> > Another important and tricky change is about supporting the non-detached
> > mode. That's, if the user doesn't pass '-d' the migrate command will lock
> 
> As a native speaker, I can state that it is unusual or even wrong to see
> a contraction immediately before a comma.  I would do:

Oh, that rule is new to me. Thanks for letting me know!

> 
> s/That's,/That is,/
> 
> > the monitor and will only release it when migration is finished.
> > 
> > To support that in the new HMP command (hmp_migrate()), it is necessary
> > to create a timer which runs every second and checks if the migration is
> > still active. If it's, the timer callback will re-schedule itself to run
> 
> Another instance of the same grammar problem:
> 
> s/it's/it is/
> 
> > one second in the future. If the migration has already finished, the
> > monitor lock is relased and the user can use it normally.
> 
> s/relased/released/
> 
> > 
> > All these changes should be transparent to the user.
> > 
> 
> > +++ b/qapi-schema.json
> > @@ -1631,3 +1631,24 @@
> >  { 'command': 'qom-list-types',
> >    'data': { '*implements': 'str', '*abstract': 'bool' },
> >    'returns': [ 'ObjectTypeInfo' ] }
> > +
> > +##
> > +# @migrate
> > +#
> > +# Migrates the current running guest to another Virtual Machine.
> > +#
> > +# @uri: the Uniform Resource Identifier of the destination VM
> > +#
> > +# @blk: #optional do block migration (full disk copy)
> > +#
> > +# @inc: #optional incremental disk copy migration
> > +#
> > +# @detach: this argument exists only for compatibility reasons and should not
> > +#          be used.
> 
> Libvirt is currently using it, but it appears that libvirt always sets
> it to true.  Does this mean the argument is ignored no matter whether
> you pass true or false?  Or does this value have a default if omitted,
> and it is safe to explicitly pass the default (I assume the default is
> true), and that it is wrong to pass an explicit value that contradicts
> the default (that is, I'm assuming passing an explicit 'detach':'false'
> is wrong)?

'detach' is completely ignored in QMP, it's never read.

Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Anthony Liguori]

On 03/09/2012 12:26 PM, Eric Blake wrote:
> On 03/09/2012 11:13 AM, Luiz Capitulino wrote:
>> The migrate command is one of those commands where HMP and QMP completely
>> mix up together. This made the conversion to the QAPI (which separates the
>> command into QMP and HMP parts) a bit difficult.
>>
>> The first important change to be noticed is that this commit completes the
>> removal of the Monitor object from migration code, started by the previous
>> commit.
>>
>> Another important and tricky change is about supporting the non-detached
>> mode. That's, if the user doesn't pass '-d' the migrate command will lock
>
> As a native speaker, I can state that it is unusual or even wrong to see
> a contraction immediately before a comma.  I would do:
>
> s/That's,/That is,/
>
>> the monitor and will only release it when migration is finished.
>>
>> To support that in the new HMP command (hmp_migrate()), it is necessary
>> to create a timer which runs every second and checks if the migration is
>> still active. If it's, the timer callback will re-schedule itself to run
>
> Another instance of the same grammar problem:
>
> s/it's/it is/
>
>> one second in the future. If the migration has already finished, the
>> monitor lock is relased and the user can use it normally.
>
> s/relased/released/
>
>>
>> All these changes should be transparent to the user.
>>
>
>> +++ b/qapi-schema.json
>> @@ -1631,3 +1631,24 @@
>>   { 'command': 'qom-list-types',
>>     'data': { '*implements': 'str', '*abstract': 'bool' },
>>     'returns': [ 'ObjectTypeInfo' ] }
>> +
>> +##
>> +# @migrate
>> +#
>> +# Migrates the current running guest to another Virtual Machine.
>> +#
>> +# @uri: the Uniform Resource Identifier of the destination VM
>> +#
>> +# @blk: #optional do block migration (full disk copy)
>> +#
>> +# @inc: #optional incremental disk copy migration
>> +#
>> +# @detach: this argument exists only for compatibility reasons and should not
>> +#          be used.
>
> Libvirt is currently using it, but it appears that libvirt always sets
> it to true.  Does this mean the argument is ignored no matter whether
> you pass true or false?

Yes, and we should specify that.  The argument is ignored by QEMU.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Luiz Capitulino]

On Fri, 09 Mar 2012 12:35:32 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> >> +##
> >> +# @migrate
> >> +#
> >> +# Migrates the current running guest to another Virtual Machine.
> >> +#
> >> +# @uri: the Uniform Resource Identifier of the destination VM
> >> +#
> >> +# @blk: #optional do block migration (full disk copy)
> >> +#
> >> +# @inc: #optional incremental disk copy migration
> >> +#
> >> +# @detach: this argument exists only for compatibility reasons and should not
> >> +#          be used.
> >
> > Libvirt is currently using it, but it appears that libvirt always sets
> > it to true.  Does this mean the argument is ignored no matter whether
> > you pass true or false?
> 
> Yes, and we should specify that.  The argument is ignored by QEMU.

Will do for v3.

Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Alon Levy]

On Fri, Mar 09, 2012 at 03:13:06PM -0300, Luiz Capitulino wrote:
> The migrate command is one of those commands where HMP and QMP completely
> mix up together. This made the conversion to the QAPI (which separates the
> command into QMP and HMP parts) a bit difficult.
> 
> The first important change to be noticed is that this commit completes the
> removal of the Monitor object from migration code, started by the previous
> commit.
> 
> Another important and tricky change is about supporting the non-detached
> mode. That's, if the user doesn't pass '-d' the migrate command will lock
> the monitor and will only release it when migration is finished.
> 
> To support that in the new HMP command (hmp_migrate()), it is necessary
> to create a timer which runs every second and checks if the migration is
> still active. If it's, the timer callback will re-schedule itself to run
> one second in the future. If the migration has already finished, the
> monitor lock is relased and the user can use it normally.
> 
> All these changes should be transparent to the user.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hmp-commands.hx  |    3 +--
>  hmp.c            |   56 +++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h            |    1 +
>  migration-fd.c   |    2 +-
>  migration.c      |   66 ++++++++++++++----------------------------------------
>  migration.h      |    3 ---
>  qapi-schema.json |   21 +++++++++++++++++
>  qmp-commands.hx  |    9 +-------
>  savevm.c         |   13 +++++------
>  sysemu.h         |    2 +-
>  10 files changed, 105 insertions(+), 71 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index ed88877..a86f53f 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -806,8 +806,7 @@ ETEXI
>  		      " full copy of disk\n\t\t\t -i for migration without "
>  		      "shared storage with incremental copy of disk "
>  		      "(base image shared between src and destination)",
> -        .user_print = monitor_user_noop,	
> -	.mhandler.cmd_new = do_migrate,
> +        .mhandler.cmd = hmp_migrate,
>      },
>  
>  
> diff --git a/hmp.c b/hmp.c
> index 3a54455..0f62b3f 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include "hmp.h"
> +#include "qemu-timer.h"
>  #include "qmp-commands.h"
>  
>  static void hmp_handle_error(Monitor *mon, Error **errp)
> @@ -856,3 +857,58 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
>  
>      hmp_handle_error(mon, &error);
>  }
> +
> +typedef struct MigrationStatus
> +{
> +    QEMUTimer *timer;
> +    Monitor *mon;
> +} MigrationStatus;
> +
> +static void hmp_migrate_status_cb(void *opaque)
> +{
> +    MigrationStatus *status = opaque;
> +    MigrationInfo *info;
> +
> +    info = qmp_query_migrate(NULL);
> +    if (!info->has_status || strcmp(info->status, "active") == 0) {
> +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock) + 1000);
> +    } else {
> +        monitor_resume(status->mon);
> +        qemu_del_timer(status->timer);
> +        g_free(status);
> +    }
> +
> +    qapi_free_MigrationInfo(info);
> +}
> +
> +void hmp_migrate(Monitor *mon, const QDict *qdict)
> +{
> +    int detach = qdict_get_try_bool(qdict, "detach", 0);
> +    int blk = qdict_get_try_bool(qdict, "blk", 0);
> +    int inc = qdict_get_try_bool(qdict, "inc", 0);
> +    const char *uri = qdict_get_str(qdict, "uri");
> +    Error *err = NULL;
> +
> +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> +    if (err) {
> +        monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
> +        error_free(err);
> +        return;
> +    }
> +
> +    if (!detach) {
> +        MigrationStatus *status;
> +
> +        if (monitor_suspend(mon) < 0) {

So using monitor_suspend is ok? is that a possible solution for the
screendump issue? I could pass the Monitor through the vga callback. Tbh
I think it's ugly, but at least it doesn't introduce any new command.

> +            monitor_printf(mon, "terminal does not allow synchronous "
> +                           "migration, continuing detached\n");
> +            return;
> +        }
> +
> +        status = g_malloc0(sizeof(*status));
> +        status->mon = mon;
> +        status->timer = qemu_new_timer_ms(rt_clock, hmp_migrate_status_cb,
> +                                          status);
> +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
> +    }
> +}
> diff --git a/hmp.h b/hmp.h
> index 5409464..8807853 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -59,5 +59,6 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
>  void hmp_block_stream(Monitor *mon, const QDict *qdict);
>  void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
>  void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
> +void hmp_migrate(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/migration-fd.c b/migration-fd.c
> index 5a068c6..50138ed 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -75,7 +75,7 @@ static int fd_close(MigrationState *s)
>  
>  int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
>  {
> -    s->fd = monitor_get_fd(s->mon, fdname);
> +    s->fd = monitor_get_fd(cur_mon, fdname);
>      if (s->fd == -1) {
>          DPRINTF("fd_migration: invalid file descriptor identifier\n");
>          goto err_after_get_fd;
> diff --git a/migration.c b/migration.c
> index b21b2df..8c119ba 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -158,16 +158,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>  
>  /* shared migration helpers */
>  
> -static void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
> -{
> -    if (monitor_suspend(mon) == 0) {
> -        DPRINTF("suspending monitor\n");
> -    } else {
> -        monitor_printf(mon, "terminal does not allow synchronous "
> -                       "migration, continuing detached\n");
> -    }
> -}
> -
>  static int migrate_fd_cleanup(MigrationState *s)
>  {
>      int ret = 0;
> @@ -178,10 +168,6 @@ static int migrate_fd_cleanup(MigrationState *s)
>          DPRINTF("closing file\n");
>          ret = qemu_fclose(s->file);
>          s->file = NULL;
> -    } else {
> -        if (s->mon) {
> -            monitor_resume(s->mon);
> -        }
>      }
>  
>      if (s->fd != -1) {
> @@ -321,9 +307,6 @@ static int migrate_fd_close(void *opaque)
>  {
>      MigrationState *s = opaque;
>  
> -    if (s->mon) {
> -        monitor_resume(s->mon);
> -    }
>      qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>      return s->close(s);
>  }
> @@ -376,7 +359,7 @@ void migrate_fd_connect(MigrationState *s)
>      migrate_fd_put_ready(s);
>  }
>  
> -static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
> +static MigrationState *migrate_init(int blk, int inc)
>  {
>      MigrationState *s = migrate_get_current();
>      int64_t bandwidth_limit = s->bandwidth_limit;
> @@ -386,18 +369,9 @@ static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
>      s->blk = blk;
>      s->shared = inc;
>  
> -    /* s->mon is used for two things:
> -       - pass fd in fd migration
> -       - suspend/resume monitor for not detached migration
> -    */
> -    s->mon = mon;
>      s->bandwidth_limit = bandwidth_limit;
>      s->state = MIG_STATE_SETUP;
>  
> -    if (!detach) {
> -        migrate_fd_monitor_suspend(s, mon);
> -    }
> -
>      return s;
>  }
>  
> @@ -413,32 +387,29 @@ void migrate_del_blocker(Error *reason)
>      migration_blockers = g_slist_remove(migration_blockers, reason);
>  }
>  
> -int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void qmp_migrate(const char *uri, bool has_blk, bool blk,
> +                 bool has_inc, bool inc, bool has_detach, bool detach,
> +                 Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
>      const char *p;
> -    int detach = qdict_get_try_bool(qdict, "detach", 0);
> -    int blk = qdict_get_try_bool(qdict, "blk", 0);
> -    int inc = qdict_get_try_bool(qdict, "inc", 0);
> -    const char *uri = qdict_get_str(qdict, "uri");
>      int ret;
>  
>      if (s->state == MIG_STATE_ACTIVE) {
> -        monitor_printf(mon, "migration already in progress\n");
> -        return -1;
> +        error_set(errp, QERR_MIGRATION_ACTIVE);
> +        return;
>      }
>  
> -    if (qemu_savevm_state_blocked(mon)) {
> -        return -1;
> +    if (qemu_savevm_state_blocked(errp)) {
> +        return;
>      }
>  
>      if (migration_blockers) {
> -        Error *err = migration_blockers->data;
> -        qerror_report_err(err);
> -        return -1;
> +        *errp = error_copy(migration_blockers->data);
> +        return;
>      }
>  
> -    s = migrate_init(mon, detach, blk, inc);
> +    s = migrate_init(blk, inc);
>  
>      if (strstart(uri, "tcp:", &p)) {
>          ret = tcp_start_outgoing_migration(s, p);
> @@ -451,21 +422,18 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>          ret = fd_start_outgoing_migration(s, p);
>  #endif
>      } else {
> -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> -        ret  = -EINVAL;
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
> +        return;
>      }
>  
>      if (ret < 0) {
> -        monitor_printf(mon, "migration failed: %s\n", strerror(-ret));
> -        return ret;
> -    }
> -
> -    if (detach) {
> -        s->mon = NULL;
> +        DPRINTF("migration failed: %s\n", strerror(-ret));
> +        /* FIXME: we should return meaningful errors */
> +        error_set(errp, QERR_UNDEFINED_ERROR);
> +        return;
>      }
>  
>      notifier_list_notify(&migration_state_notifiers, s);
> -    return 0;
>  }
>  
>  void qmp_migrate_cancel(Error **errp)
> diff --git a/migration.h b/migration.h
> index 0e44197..691b367 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -26,7 +26,6 @@ struct MigrationState
>      int64_t bandwidth_limit;
>      QEMUFile *file;
>      int fd;
> -    Monitor *mon;
>      int state;
>      int (*get_error)(MigrationState *s);
>      int (*close)(MigrationState *s);
> @@ -40,8 +39,6 @@ void process_incoming_migration(QEMUFile *f);
>  
>  int qemu_start_incoming_migration(const char *uri);
>  
> -int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
> -
>  uint64_t migrate_max_downtime(void);
>  
>  void do_info_migrate_print(Monitor *mon, const QObject *data);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5f293c4..c0780ab 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1631,3 +1631,24 @@
>  { 'command': 'qom-list-types',
>    'data': { '*implements': 'str', '*abstract': 'bool' },
>    'returns': [ 'ObjectTypeInfo' ] }
> +
> +##
> +# @migrate
> +#
> +# Migrates the current running guest to another Virtual Machine.
> +#
> +# @uri: the Uniform Resource Identifier of the destination VM
> +#
> +# @blk: #optional do block migration (full disk copy)
> +#
> +# @inc: #optional incremental disk copy migration
> +#
> +# @detach: this argument exists only for compatibility reasons and should not
> +#          be used.
> +#
> +# Returns: nothing on success
> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'migrate',
> +  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 0c9bfac..f328bc1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -446,14 +446,7 @@ EQMP
>      {
>          .name       = "migrate",
>          .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
> -        .params     = "[-d] [-b] [-i] uri",
> -        .help       = "migrate to URI (using -d to not wait for completion)"
> -		      "\n\t\t\t -b for migration without shared storage with"
> -		      " full copy of disk\n\t\t\t -i for migration without "
> -		      "shared storage with incremental copy of disk "
> -		      "(base image shared between src and destination)",
> -        .user_print = monitor_user_noop,	
> -	.mhandler.cmd_new = do_migrate,
> +        .mhandler.cmd_new = qmp_marshal_input_migrate,
>      },
>  
>  SQMP
> diff --git a/savevm.c b/savevm.c
> index 70f5c4f..5fdc3e1 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1540,14 +1540,13 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
>  #define QEMU_VM_SECTION_FULL         0x04
>  #define QEMU_VM_SUBSECTION           0x05
>  
> -bool qemu_savevm_state_blocked(Monitor *mon)
> +bool qemu_savevm_state_blocked(Error **errp)
>  {
>      SaveStateEntry *se;
>  
>      QTAILQ_FOREACH(se, &savevm_handlers, entry) {
>          if (se->no_migrate) {
> -            monitor_printf(mon, "state blocked by non-migratable device '%s'\n",
> -                           se->idstr);
> +            error_set(errp, QERR_MIGRATION_NOT_SUPPORTED, se->idstr);
>              return true;
>          }
>      }
> @@ -1698,11 +1697,11 @@ void qemu_savevm_state_cancel(QEMUFile *f)
>      }
>  }
>  
> -static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
> +static int qemu_savevm_state(QEMUFile *f)
>  {
>      int ret;
>  
> -    if (qemu_savevm_state_blocked(mon)) {
> +    if (qemu_savevm_state_blocked(NULL)) {
>          ret = -EINVAL;
>          goto out;
>      }
> @@ -1836,7 +1835,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      unsigned int v;
>      int ret;
>  
> -    if (qemu_savevm_state_blocked(default_mon)) {
> +    if (qemu_savevm_state_blocked(NULL)) {
>          return -EINVAL;
>      }
>  
> @@ -2080,7 +2079,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "Could not open VM state file\n");
>          goto the_end;
>      }
> -    ret = qemu_savevm_state(mon, f);
> +    ret = qemu_savevm_state(f);
>      vm_state_size = qemu_ftell(f);
>      qemu_fclose(f);
>      if (ret < 0) {
> diff --git a/sysemu.h b/sysemu.h
> index 29b0e96..bc2c788 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -76,7 +76,7 @@ void do_info_snapshots(Monitor *mon);
>  
>  void qemu_announce_self(void);
>  
> -bool qemu_savevm_state_blocked(Monitor *mon);
> +bool qemu_savevm_state_blocked(Error **errp);
>  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);
> -- 
> 1.7.9.2.384.g4a92a
> 
> 

Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Luiz Capitulino]

On Sat, 10 Mar 2012 00:51:29 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Fri, Mar 09, 2012 at 03:13:06PM -0300, Luiz Capitulino wrote:
> > The migrate command is one of those commands where HMP and QMP completely
> > mix up together. This made the conversion to the QAPI (which separates the
> > command into QMP and HMP parts) a bit difficult.
> > 
> > The first important change to be noticed is that this commit completes the
> > removal of the Monitor object from migration code, started by the previous
> > commit.
> > 
> > Another important and tricky change is about supporting the non-detached
> > mode. That's, if the user doesn't pass '-d' the migrate command will lock
> > the monitor and will only release it when migration is finished.
> > 
> > To support that in the new HMP command (hmp_migrate()), it is necessary
> > to create a timer which runs every second and checks if the migration is
> > still active. If it's, the timer callback will re-schedule itself to run
> > one second in the future. If the migration has already finished, the
> > monitor lock is relased and the user can use it normally.
> > 
> > All these changes should be transparent to the user.
> > 
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hmp-commands.hx  |    3 +--
> >  hmp.c            |   56 +++++++++++++++++++++++++++++++++++++++++++++
> >  hmp.h            |    1 +
> >  migration-fd.c   |    2 +-
> >  migration.c      |   66 ++++++++++++++----------------------------------------
> >  migration.h      |    3 ---
> >  qapi-schema.json |   21 +++++++++++++++++
> >  qmp-commands.hx  |    9 +-------
> >  savevm.c         |   13 +++++------
> >  sysemu.h         |    2 +-
> >  10 files changed, 105 insertions(+), 71 deletions(-)
> > 
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index ed88877..a86f53f 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -806,8 +806,7 @@ ETEXI
> >  		      " full copy of disk\n\t\t\t -i for migration without "
> >  		      "shared storage with incremental copy of disk "
> >  		      "(base image shared between src and destination)",
> > -        .user_print = monitor_user_noop,	
> > -	.mhandler.cmd_new = do_migrate,
> > +        .mhandler.cmd = hmp_migrate,
> >      },
> >  
> >  
> > diff --git a/hmp.c b/hmp.c
> > index 3a54455..0f62b3f 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -14,6 +14,7 @@
> >   */
> >  
> >  #include "hmp.h"
> > +#include "qemu-timer.h"
> >  #include "qmp-commands.h"
> >  
> >  static void hmp_handle_error(Monitor *mon, Error **errp)
> > @@ -856,3 +857,58 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
> >  
> >      hmp_handle_error(mon, &error);
> >  }
> > +
> > +typedef struct MigrationStatus
> > +{
> > +    QEMUTimer *timer;
> > +    Monitor *mon;
> > +} MigrationStatus;
> > +
> > +static void hmp_migrate_status_cb(void *opaque)
> > +{
> > +    MigrationStatus *status = opaque;
> > +    MigrationInfo *info;
> > +
> > +    info = qmp_query_migrate(NULL);
> > +    if (!info->has_status || strcmp(info->status, "active") == 0) {
> > +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock) + 1000);
> > +    } else {
> > +        monitor_resume(status->mon);
> > +        qemu_del_timer(status->timer);
> > +        g_free(status);
> > +    }
> > +
> > +    qapi_free_MigrationInfo(info);
> > +}
> > +
> > +void hmp_migrate(Monitor *mon, const QDict *qdict)
> > +{
> > +    int detach = qdict_get_try_bool(qdict, "detach", 0);
> > +    int blk = qdict_get_try_bool(qdict, "blk", 0);
> > +    int inc = qdict_get_try_bool(qdict, "inc", 0);
> > +    const char *uri = qdict_get_str(qdict, "uri");
> > +    Error *err = NULL;
> > +
> > +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> > +    if (err) {
> > +        monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
> > +        error_free(err);
> > +        return;
> > +    }
> > +
> > +    if (!detach) {
> > +        MigrationStatus *status;
> > +
> > +        if (monitor_suspend(mon) < 0) {
> 
> So using monitor_suspend is ok? is that a possible solution for the
> screendump issue? I could pass the Monitor through the vga callback. Tbh
> I think it's ugly, but at least it doesn't introduce any new command.

This is only valid for HMP.

> 
> > +            monitor_printf(mon, "terminal does not allow synchronous "
> > +                           "migration, continuing detached\n");
> > +            return;
> > +        }
> > +
> > +        status = g_malloc0(sizeof(*status));
> > +        status->mon = mon;
> > +        status->timer = qemu_new_timer_ms(rt_clock, hmp_migrate_status_cb,
> > +                                          status);
> > +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
> > +    }
> > +}
> > diff --git a/hmp.h b/hmp.h
> > index 5409464..8807853 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -59,5 +59,6 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
> >  void hmp_block_stream(Monitor *mon, const QDict *qdict);
> >  void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
> >  void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
> > +void hmp_migrate(Monitor *mon, const QDict *qdict);
> >  
> >  #endif
> > diff --git a/migration-fd.c b/migration-fd.c
> > index 5a068c6..50138ed 100644
> > --- a/migration-fd.c
> > +++ b/migration-fd.c
> > @@ -75,7 +75,7 @@ static int fd_close(MigrationState *s)
> >  
> >  int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
> >  {
> > -    s->fd = monitor_get_fd(s->mon, fdname);
> > +    s->fd = monitor_get_fd(cur_mon, fdname);
> >      if (s->fd == -1) {
> >          DPRINTF("fd_migration: invalid file descriptor identifier\n");
> >          goto err_after_get_fd;
> > diff --git a/migration.c b/migration.c
> > index b21b2df..8c119ba 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -158,16 +158,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
> >  
> >  /* shared migration helpers */
> >  
> > -static void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
> > -{
> > -    if (monitor_suspend(mon) == 0) {
> > -        DPRINTF("suspending monitor\n");
> > -    } else {
> > -        monitor_printf(mon, "terminal does not allow synchronous "
> > -                       "migration, continuing detached\n");
> > -    }
> > -}
> > -
> >  static int migrate_fd_cleanup(MigrationState *s)
> >  {
> >      int ret = 0;
> > @@ -178,10 +168,6 @@ static int migrate_fd_cleanup(MigrationState *s)
> >          DPRINTF("closing file\n");
> >          ret = qemu_fclose(s->file);
> >          s->file = NULL;
> > -    } else {
> > -        if (s->mon) {
> > -            monitor_resume(s->mon);
> > -        }
> >      }
> >  
> >      if (s->fd != -1) {
> > @@ -321,9 +307,6 @@ static int migrate_fd_close(void *opaque)
> >  {
> >      MigrationState *s = opaque;
> >  
> > -    if (s->mon) {
> > -        monitor_resume(s->mon);
> > -    }
> >      qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> >      return s->close(s);
> >  }
> > @@ -376,7 +359,7 @@ void migrate_fd_connect(MigrationState *s)
> >      migrate_fd_put_ready(s);
> >  }
> >  
> > -static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
> > +static MigrationState *migrate_init(int blk, int inc)
> >  {
> >      MigrationState *s = migrate_get_current();
> >      int64_t bandwidth_limit = s->bandwidth_limit;
> > @@ -386,18 +369,9 @@ static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
> >      s->blk = blk;
> >      s->shared = inc;
> >  
> > -    /* s->mon is used for two things:
> > -       - pass fd in fd migration
> > -       - suspend/resume monitor for not detached migration
> > -    */
> > -    s->mon = mon;
> >      s->bandwidth_limit = bandwidth_limit;
> >      s->state = MIG_STATE_SETUP;
> >  
> > -    if (!detach) {
> > -        migrate_fd_monitor_suspend(s, mon);
> > -    }
> > -
> >      return s;
> >  }
> >  
> > @@ -413,32 +387,29 @@ void migrate_del_blocker(Error *reason)
> >      migration_blockers = g_slist_remove(migration_blockers, reason);
> >  }
> >  
> > -int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > +void qmp_migrate(const char *uri, bool has_blk, bool blk,
> > +                 bool has_inc, bool inc, bool has_detach, bool detach,
> > +                 Error **errp)
> >  {
> >      MigrationState *s = migrate_get_current();
> >      const char *p;
> > -    int detach = qdict_get_try_bool(qdict, "detach", 0);
> > -    int blk = qdict_get_try_bool(qdict, "blk", 0);
> > -    int inc = qdict_get_try_bool(qdict, "inc", 0);
> > -    const char *uri = qdict_get_str(qdict, "uri");
> >      int ret;
> >  
> >      if (s->state == MIG_STATE_ACTIVE) {
> > -        monitor_printf(mon, "migration already in progress\n");
> > -        return -1;
> > +        error_set(errp, QERR_MIGRATION_ACTIVE);
> > +        return;
> >      }
> >  
> > -    if (qemu_savevm_state_blocked(mon)) {
> > -        return -1;
> > +    if (qemu_savevm_state_blocked(errp)) {
> > +        return;
> >      }
> >  
> >      if (migration_blockers) {
> > -        Error *err = migration_blockers->data;
> > -        qerror_report_err(err);
> > -        return -1;
> > +        *errp = error_copy(migration_blockers->data);
> > +        return;
> >      }
> >  
> > -    s = migrate_init(mon, detach, blk, inc);
> > +    s = migrate_init(blk, inc);
> >  
> >      if (strstart(uri, "tcp:", &p)) {
> >          ret = tcp_start_outgoing_migration(s, p);
> > @@ -451,21 +422,18 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >          ret = fd_start_outgoing_migration(s, p);
> >  #endif
> >      } else {
> > -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> > -        ret  = -EINVAL;
> > +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
> > +        return;
> >      }
> >  
> >      if (ret < 0) {
> > -        monitor_printf(mon, "migration failed: %s\n", strerror(-ret));
> > -        return ret;
> > -    }
> > -
> > -    if (detach) {
> > -        s->mon = NULL;
> > +        DPRINTF("migration failed: %s\n", strerror(-ret));
> > +        /* FIXME: we should return meaningful errors */
> > +        error_set(errp, QERR_UNDEFINED_ERROR);
> > +        return;
> >      }
> >  
> >      notifier_list_notify(&migration_state_notifiers, s);
> > -    return 0;
> >  }
> >  
> >  void qmp_migrate_cancel(Error **errp)
> > diff --git a/migration.h b/migration.h
> > index 0e44197..691b367 100644
> > --- a/migration.h
> > +++ b/migration.h
> > @@ -26,7 +26,6 @@ struct MigrationState
> >      int64_t bandwidth_limit;
> >      QEMUFile *file;
> >      int fd;
> > -    Monitor *mon;
> >      int state;
> >      int (*get_error)(MigrationState *s);
> >      int (*close)(MigrationState *s);
> > @@ -40,8 +39,6 @@ void process_incoming_migration(QEMUFile *f);
> >  
> >  int qemu_start_incoming_migration(const char *uri);
> >  
> > -int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
> > -
> >  uint64_t migrate_max_downtime(void);
> >  
> >  void do_info_migrate_print(Monitor *mon, const QObject *data);
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 5f293c4..c0780ab 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1631,3 +1631,24 @@
> >  { 'command': 'qom-list-types',
> >    'data': { '*implements': 'str', '*abstract': 'bool' },
> >    'returns': [ 'ObjectTypeInfo' ] }
> > +
> > +##
> > +# @migrate
> > +#
> > +# Migrates the current running guest to another Virtual Machine.
> > +#
> > +# @uri: the Uniform Resource Identifier of the destination VM
> > +#
> > +# @blk: #optional do block migration (full disk copy)
> > +#
> > +# @inc: #optional incremental disk copy migration
> > +#
> > +# @detach: this argument exists only for compatibility reasons and should not
> > +#          be used.
> > +#
> > +# Returns: nothing on success
> > +#
> > +# Since: 0.14.0
> > +##
> > +{ 'command': 'migrate',
> > +  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 0c9bfac..f328bc1 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -446,14 +446,7 @@ EQMP
> >      {
> >          .name       = "migrate",
> >          .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
> > -        .params     = "[-d] [-b] [-i] uri",
> > -        .help       = "migrate to URI (using -d to not wait for completion)"
> > -		      "\n\t\t\t -b for migration without shared storage with"
> > -		      " full copy of disk\n\t\t\t -i for migration without "
> > -		      "shared storage with incremental copy of disk "
> > -		      "(base image shared between src and destination)",
> > -        .user_print = monitor_user_noop,	
> > -	.mhandler.cmd_new = do_migrate,
> > +        .mhandler.cmd_new = qmp_marshal_input_migrate,
> >      },
> >  
> >  SQMP
> > diff --git a/savevm.c b/savevm.c
> > index 70f5c4f..5fdc3e1 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -1540,14 +1540,13 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
> >  #define QEMU_VM_SECTION_FULL         0x04
> >  #define QEMU_VM_SUBSECTION           0x05
> >  
> > -bool qemu_savevm_state_blocked(Monitor *mon)
> > +bool qemu_savevm_state_blocked(Error **errp)
> >  {
> >      SaveStateEntry *se;
> >  
> >      QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> >          if (se->no_migrate) {
> > -            monitor_printf(mon, "state blocked by non-migratable device '%s'\n",
> > -                           se->idstr);
> > +            error_set(errp, QERR_MIGRATION_NOT_SUPPORTED, se->idstr);
> >              return true;
> >          }
> >      }
> > @@ -1698,11 +1697,11 @@ void qemu_savevm_state_cancel(QEMUFile *f)
> >      }
> >  }
> >  
> > -static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
> > +static int qemu_savevm_state(QEMUFile *f)
> >  {
> >      int ret;
> >  
> > -    if (qemu_savevm_state_blocked(mon)) {
> > +    if (qemu_savevm_state_blocked(NULL)) {
> >          ret = -EINVAL;
> >          goto out;
> >      }
> > @@ -1836,7 +1835,7 @@ int qemu_loadvm_state(QEMUFile *f)
> >      unsigned int v;
> >      int ret;
> >  
> > -    if (qemu_savevm_state_blocked(default_mon)) {
> > +    if (qemu_savevm_state_blocked(NULL)) {
> >          return -EINVAL;
> >      }
> >  
> > @@ -2080,7 +2079,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> >          monitor_printf(mon, "Could not open VM state file\n");
> >          goto the_end;
> >      }
> > -    ret = qemu_savevm_state(mon, f);
> > +    ret = qemu_savevm_state(f);
> >      vm_state_size = qemu_ftell(f);
> >      qemu_fclose(f);
> >      if (ret < 0) {
> > diff --git a/sysemu.h b/sysemu.h
> > index 29b0e96..bc2c788 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -76,7 +76,7 @@ void do_info_snapshots(Monitor *mon);
> >  
> >  void qemu_announce_self(void);
> >  
> > -bool qemu_savevm_state_blocked(Monitor *mon);
> > +bool qemu_savevm_state_blocked(Error **errp);
> >  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);
> > -- 
> > 1.7.9.2.384.g4a92a
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Alon Levy]

On Mon, Mar 12, 2012 at 10:21:20AM -0300, Luiz Capitulino wrote:
> On Sat, 10 Mar 2012 00:51:29 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Fri, Mar 09, 2012 at 03:13:06PM -0300, Luiz Capitulino wrote:
> > > The migrate command is one of those commands where HMP and QMP completely
> > > mix up together. This made the conversion to the QAPI (which separates the
> > > command into QMP and HMP parts) a bit difficult.
> > > 
> > > The first important change to be noticed is that this commit completes the
> > > removal of the Monitor object from migration code, started by the previous
> > > commit.
> > > 
> > > Another important and tricky change is about supporting the non-detached
> > > mode. That's, if the user doesn't pass '-d' the migrate command will lock
> > > the monitor and will only release it when migration is finished.
> > > 
> > > To support that in the new HMP command (hmp_migrate()), it is necessary
> > > to create a timer which runs every second and checks if the migration is
> > > still active. If it's, the timer callback will re-schedule itself to run
> > > one second in the future. If the migration has already finished, the
> > > monitor lock is relased and the user can use it normally.
> > > 
> > > All these changes should be transparent to the user.
> > > 
> > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > ---
> > >  hmp-commands.hx  |    3 +--
> > >  hmp.c            |   56 +++++++++++++++++++++++++++++++++++++++++++++
> > >  hmp.h            |    1 +
> > >  migration-fd.c   |    2 +-
> > >  migration.c      |   66 ++++++++++++++----------------------------------------
> > >  migration.h      |    3 ---
> > >  qapi-schema.json |   21 +++++++++++++++++
> > >  qmp-commands.hx  |    9 +-------
> > >  savevm.c         |   13 +++++------
> > >  sysemu.h         |    2 +-
> > >  10 files changed, 105 insertions(+), 71 deletions(-)
> > > 
> > > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > > index ed88877..a86f53f 100644
> > > --- a/hmp-commands.hx
> > > +++ b/hmp-commands.hx
> > > @@ -806,8 +806,7 @@ ETEXI
> > >  		      " full copy of disk\n\t\t\t -i for migration without "
> > >  		      "shared storage with incremental copy of disk "
> > >  		      "(base image shared between src and destination)",
> > > -        .user_print = monitor_user_noop,	
> > > -	.mhandler.cmd_new = do_migrate,
> > > +        .mhandler.cmd = hmp_migrate,
> > >      },
> > >  
> > >  
> > > diff --git a/hmp.c b/hmp.c
> > > index 3a54455..0f62b3f 100644
> > > --- a/hmp.c
> > > +++ b/hmp.c
> > > @@ -14,6 +14,7 @@
> > >   */
> > >  
> > >  #include "hmp.h"
> > > +#include "qemu-timer.h"
> > >  #include "qmp-commands.h"
> > >  
> > >  static void hmp_handle_error(Monitor *mon, Error **errp)
> > > @@ -856,3 +857,58 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
> > >  
> > >      hmp_handle_error(mon, &error);
> > >  }
> > > +
> > > +typedef struct MigrationStatus
> > > +{
> > > +    QEMUTimer *timer;
> > > +    Monitor *mon;
> > > +} MigrationStatus;
> > > +
> > > +static void hmp_migrate_status_cb(void *opaque)
> > > +{
> > > +    MigrationStatus *status = opaque;
> > > +    MigrationInfo *info;
> > > +
> > > +    info = qmp_query_migrate(NULL);
> > > +    if (!info->has_status || strcmp(info->status, "active") == 0) {
> > > +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock) + 1000);
> > > +    } else {
> > > +        monitor_resume(status->mon);
> > > +        qemu_del_timer(status->timer);
> > > +        g_free(status);
> > > +    }
> > > +
> > > +    qapi_free_MigrationInfo(info);
> > > +}
> > > +
> > > +void hmp_migrate(Monitor *mon, const QDict *qdict)
> > > +{
> > > +    int detach = qdict_get_try_bool(qdict, "detach", 0);
> > > +    int blk = qdict_get_try_bool(qdict, "blk", 0);
> > > +    int inc = qdict_get_try_bool(qdict, "inc", 0);
> > > +    const char *uri = qdict_get_str(qdict, "uri");
> > > +    Error *err = NULL;
> > > +
> > > +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> > > +    if (err) {
> > > +        monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
> > > +        error_free(err);
> > > +        return;
> > > +    }
> > > +
> > > +    if (!detach) {
> > > +        MigrationStatus *status;
> > > +
> > > +        if (monitor_suspend(mon) < 0) {
> > 
> > So using monitor_suspend is ok? is that a possible solution for the
> > screendump issue? I could pass the Monitor through the vga callback. Tbh
> > I think it's ugly, but at least it doesn't introduce any new command.
> 
> This is only valid for HMP.
> 

Yes, I'm thinking at this point for qmp to wait for the output file to
be written via repeated poll / inotify.

> > 
> > > +            monitor_printf(mon, "terminal does not allow synchronous "
> > > +                           "migration, continuing detached\n");
> > > +            return;
> > > +        }
> > > +
> > > +        status = g_malloc0(sizeof(*status));
> > > +        status->mon = mon;
> > > +        status->timer = qemu_new_timer_ms(rt_clock, hmp_migrate_status_cb,
> > > +                                          status);
> > > +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
> > > +    }
> > > +}
> > > diff --git a/hmp.h b/hmp.h
> > > index 5409464..8807853 100644
> > > --- a/hmp.h
> > > +++ b/hmp.h
> > > @@ -59,5 +59,6 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
> > >  void hmp_block_stream(Monitor *mon, const QDict *qdict);
> > >  void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
> > >  void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
> > > +void hmp_migrate(Monitor *mon, const QDict *qdict);
> > >  
> > >  #endif
> > > diff --git a/migration-fd.c b/migration-fd.c
> > > index 5a068c6..50138ed 100644
> > > --- a/migration-fd.c
> > > +++ b/migration-fd.c
> > > @@ -75,7 +75,7 @@ static int fd_close(MigrationState *s)
> > >  
> > >  int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
> > >  {
> > > -    s->fd = monitor_get_fd(s->mon, fdname);
> > > +    s->fd = monitor_get_fd(cur_mon, fdname);
> > >      if (s->fd == -1) {
> > >          DPRINTF("fd_migration: invalid file descriptor identifier\n");
> > >          goto err_after_get_fd;
> > > diff --git a/migration.c b/migration.c
> > > index b21b2df..8c119ba 100644
> > > --- a/migration.c
> > > +++ b/migration.c
> > > @@ -158,16 +158,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
> > >  
> > >  /* shared migration helpers */
> > >  
> > > -static void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
> > > -{
> > > -    if (monitor_suspend(mon) == 0) {
> > > -        DPRINTF("suspending monitor\n");
> > > -    } else {
> > > -        monitor_printf(mon, "terminal does not allow synchronous "
> > > -                       "migration, continuing detached\n");
> > > -    }
> > > -}
> > > -
> > >  static int migrate_fd_cleanup(MigrationState *s)
> > >  {
> > >      int ret = 0;
> > > @@ -178,10 +168,6 @@ static int migrate_fd_cleanup(MigrationState *s)
> > >          DPRINTF("closing file\n");
> > >          ret = qemu_fclose(s->file);
> > >          s->file = NULL;
> > > -    } else {
> > > -        if (s->mon) {
> > > -            monitor_resume(s->mon);
> > > -        }
> > >      }
> > >  
> > >      if (s->fd != -1) {
> > > @@ -321,9 +307,6 @@ static int migrate_fd_close(void *opaque)
> > >  {
> > >      MigrationState *s = opaque;
> > >  
> > > -    if (s->mon) {
> > > -        monitor_resume(s->mon);
> > > -    }
> > >      qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> > >      return s->close(s);
> > >  }
> > > @@ -376,7 +359,7 @@ void migrate_fd_connect(MigrationState *s)
> > >      migrate_fd_put_ready(s);
> > >  }
> > >  
> > > -static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
> > > +static MigrationState *migrate_init(int blk, int inc)
> > >  {
> > >      MigrationState *s = migrate_get_current();
> > >      int64_t bandwidth_limit = s->bandwidth_limit;
> > > @@ -386,18 +369,9 @@ static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
> > >      s->blk = blk;
> > >      s->shared = inc;
> > >  
> > > -    /* s->mon is used for two things:
> > > -       - pass fd in fd migration
> > > -       - suspend/resume monitor for not detached migration
> > > -    */
> > > -    s->mon = mon;
> > >      s->bandwidth_limit = bandwidth_limit;
> > >      s->state = MIG_STATE_SETUP;
> > >  
> > > -    if (!detach) {
> > > -        migrate_fd_monitor_suspend(s, mon);
> > > -    }
> > > -
> > >      return s;
> > >  }
> > >  
> > > @@ -413,32 +387,29 @@ void migrate_del_blocker(Error *reason)
> > >      migration_blockers = g_slist_remove(migration_blockers, reason);
> > >  }
> > >  
> > > -int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > > +void qmp_migrate(const char *uri, bool has_blk, bool blk,
> > > +                 bool has_inc, bool inc, bool has_detach, bool detach,
> > > +                 Error **errp)
> > >  {
> > >      MigrationState *s = migrate_get_current();
> > >      const char *p;
> > > -    int detach = qdict_get_try_bool(qdict, "detach", 0);
> > > -    int blk = qdict_get_try_bool(qdict, "blk", 0);
> > > -    int inc = qdict_get_try_bool(qdict, "inc", 0);
> > > -    const char *uri = qdict_get_str(qdict, "uri");
> > >      int ret;
> > >  
> > >      if (s->state == MIG_STATE_ACTIVE) {
> > > -        monitor_printf(mon, "migration already in progress\n");
> > > -        return -1;
> > > +        error_set(errp, QERR_MIGRATION_ACTIVE);
> > > +        return;
> > >      }
> > >  
> > > -    if (qemu_savevm_state_blocked(mon)) {
> > > -        return -1;
> > > +    if (qemu_savevm_state_blocked(errp)) {
> > > +        return;
> > >      }
> > >  
> > >      if (migration_blockers) {
> > > -        Error *err = migration_blockers->data;
> > > -        qerror_report_err(err);
> > > -        return -1;
> > > +        *errp = error_copy(migration_blockers->data);
> > > +        return;
> > >      }
> > >  
> > > -    s = migrate_init(mon, detach, blk, inc);
> > > +    s = migrate_init(blk, inc);
> > >  
> > >      if (strstart(uri, "tcp:", &p)) {
> > >          ret = tcp_start_outgoing_migration(s, p);
> > > @@ -451,21 +422,18 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > >          ret = fd_start_outgoing_migration(s, p);
> > >  #endif
> > >      } else {
> > > -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> > > -        ret  = -EINVAL;
> > > +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
> > > +        return;
> > >      }
> > >  
> > >      if (ret < 0) {
> > > -        monitor_printf(mon, "migration failed: %s\n", strerror(-ret));
> > > -        return ret;
> > > -    }
> > > -
> > > -    if (detach) {
> > > -        s->mon = NULL;
> > > +        DPRINTF("migration failed: %s\n", strerror(-ret));
> > > +        /* FIXME: we should return meaningful errors */
> > > +        error_set(errp, QERR_UNDEFINED_ERROR);
> > > +        return;
> > >      }
> > >  
> > >      notifier_list_notify(&migration_state_notifiers, s);
> > > -    return 0;
> > >  }
> > >  
> > >  void qmp_migrate_cancel(Error **errp)
> > > diff --git a/migration.h b/migration.h
> > > index 0e44197..691b367 100644
> > > --- a/migration.h
> > > +++ b/migration.h
> > > @@ -26,7 +26,6 @@ struct MigrationState
> > >      int64_t bandwidth_limit;
> > >      QEMUFile *file;
> > >      int fd;
> > > -    Monitor *mon;
> > >      int state;
> > >      int (*get_error)(MigrationState *s);
> > >      int (*close)(MigrationState *s);
> > > @@ -40,8 +39,6 @@ void process_incoming_migration(QEMUFile *f);
> > >  
> > >  int qemu_start_incoming_migration(const char *uri);
> > >  
> > > -int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
> > > -
> > >  uint64_t migrate_max_downtime(void);
> > >  
> > >  void do_info_migrate_print(Monitor *mon, const QObject *data);
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 5f293c4..c0780ab 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -1631,3 +1631,24 @@
> > >  { 'command': 'qom-list-types',
> > >    'data': { '*implements': 'str', '*abstract': 'bool' },
> > >    'returns': [ 'ObjectTypeInfo' ] }
> > > +
> > > +##
> > > +# @migrate
> > > +#
> > > +# Migrates the current running guest to another Virtual Machine.
> > > +#
> > > +# @uri: the Uniform Resource Identifier of the destination VM
> > > +#
> > > +# @blk: #optional do block migration (full disk copy)
> > > +#
> > > +# @inc: #optional incremental disk copy migration
> > > +#
> > > +# @detach: this argument exists only for compatibility reasons and should not
> > > +#          be used.
> > > +#
> > > +# Returns: nothing on success
> > > +#
> > > +# Since: 0.14.0
> > > +##
> > > +{ 'command': 'migrate',
> > > +  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
> > > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > > index 0c9bfac..f328bc1 100644
> > > --- a/qmp-commands.hx
> > > +++ b/qmp-commands.hx
> > > @@ -446,14 +446,7 @@ EQMP
> > >      {
> > >          .name       = "migrate",
> > >          .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
> > > -        .params     = "[-d] [-b] [-i] uri",
> > > -        .help       = "migrate to URI (using -d to not wait for completion)"
> > > -		      "\n\t\t\t -b for migration without shared storage with"
> > > -		      " full copy of disk\n\t\t\t -i for migration without "
> > > -		      "shared storage with incremental copy of disk "
> > > -		      "(base image shared between src and destination)",
> > > -        .user_print = monitor_user_noop,	
> > > -	.mhandler.cmd_new = do_migrate,
> > > +        .mhandler.cmd_new = qmp_marshal_input_migrate,
> > >      },
> > >  
> > >  SQMP
> > > diff --git a/savevm.c b/savevm.c
> > > index 70f5c4f..5fdc3e1 100644
> > > --- a/savevm.c
> > > +++ b/savevm.c
> > > @@ -1540,14 +1540,13 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
> > >  #define QEMU_VM_SECTION_FULL         0x04
> > >  #define QEMU_VM_SUBSECTION           0x05
> > >  
> > > -bool qemu_savevm_state_blocked(Monitor *mon)
> > > +bool qemu_savevm_state_blocked(Error **errp)
> > >  {
> > >      SaveStateEntry *se;
> > >  
> > >      QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> > >          if (se->no_migrate) {
> > > -            monitor_printf(mon, "state blocked by non-migratable device '%s'\n",
> > > -                           se->idstr);
> > > +            error_set(errp, QERR_MIGRATION_NOT_SUPPORTED, se->idstr);
> > >              return true;
> > >          }
> > >      }
> > > @@ -1698,11 +1697,11 @@ void qemu_savevm_state_cancel(QEMUFile *f)
> > >      }
> > >  }
> > >  
> > > -static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
> > > +static int qemu_savevm_state(QEMUFile *f)
> > >  {
> > >      int ret;
> > >  
> > > -    if (qemu_savevm_state_blocked(mon)) {
> > > +    if (qemu_savevm_state_blocked(NULL)) {
> > >          ret = -EINVAL;
> > >          goto out;
> > >      }
> > > @@ -1836,7 +1835,7 @@ int qemu_loadvm_state(QEMUFile *f)
> > >      unsigned int v;
> > >      int ret;
> > >  
> > > -    if (qemu_savevm_state_blocked(default_mon)) {
> > > +    if (qemu_savevm_state_blocked(NULL)) {
> > >          return -EINVAL;
> > >      }
> > >  
> > > @@ -2080,7 +2079,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> > >          monitor_printf(mon, "Could not open VM state file\n");
> > >          goto the_end;
> > >      }
> > > -    ret = qemu_savevm_state(mon, f);
> > > +    ret = qemu_savevm_state(f);
> > >      vm_state_size = qemu_ftell(f);
> > >      qemu_fclose(f);
> > >      if (ret < 0) {
> > > diff --git a/sysemu.h b/sysemu.h
> > > index 29b0e96..bc2c788 100644
> > > --- a/sysemu.h
> > > +++ b/sysemu.h
> > > @@ -76,7 +76,7 @@ void do_info_snapshots(Monitor *mon);
> > >  
> > >  void qemu_announce_self(void);
> > >  
> > > -bool qemu_savevm_state_blocked(Monitor *mon);
> > > +bool qemu_savevm_state_blocked(Error **errp);
> > >  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);
> > > -- 
> > > 1.7.9.2.384.g4a92a
> > > 
> > > 
> > 
> 

Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Luiz Capitulino]

On Mon, 12 Mar 2012 15:49:27 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Mon, Mar 12, 2012 at 10:21:20AM -0300, Luiz Capitulino wrote:
> > On Sat, 10 Mar 2012 00:51:29 +0200
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > On Fri, Mar 09, 2012 at 03:13:06PM -0300, Luiz Capitulino wrote:
> > > > The migrate command is one of those commands where HMP and QMP completely
> > > > mix up together. This made the conversion to the QAPI (which separates the
> > > > command into QMP and HMP parts) a bit difficult.
> > > > 
> > > > The first important change to be noticed is that this commit completes the
> > > > removal of the Monitor object from migration code, started by the previous
> > > > commit.
> > > > 
> > > > Another important and tricky change is about supporting the non-detached
> > > > mode. That's, if the user doesn't pass '-d' the migrate command will lock
> > > > the monitor and will only release it when migration is finished.
> > > > 
> > > > To support that in the new HMP command (hmp_migrate()), it is necessary
> > > > to create a timer which runs every second and checks if the migration is
> > > > still active. If it's, the timer callback will re-schedule itself to run
> > > > one second in the future. If the migration has already finished, the
> > > > monitor lock is relased and the user can use it normally.
> > > > 
> > > > All these changes should be transparent to the user.
> > > > 
> > > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > > ---
> > > >  hmp-commands.hx  |    3 +--
> > > >  hmp.c            |   56 +++++++++++++++++++++++++++++++++++++++++++++
> > > >  hmp.h            |    1 +
> > > >  migration-fd.c   |    2 +-
> > > >  migration.c      |   66 ++++++++++++++----------------------------------------
> > > >  migration.h      |    3 ---
> > > >  qapi-schema.json |   21 +++++++++++++++++
> > > >  qmp-commands.hx  |    9 +-------
> > > >  savevm.c         |   13 +++++------
> > > >  sysemu.h         |    2 +-
> > > >  10 files changed, 105 insertions(+), 71 deletions(-)
> > > > 
> > > > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > > > index ed88877..a86f53f 100644
> > > > --- a/hmp-commands.hx
> > > > +++ b/hmp-commands.hx
> > > > @@ -806,8 +806,7 @@ ETEXI
> > > >  		      " full copy of disk\n\t\t\t -i for migration without "
> > > >  		      "shared storage with incremental copy of disk "
> > > >  		      "(base image shared between src and destination)",
> > > > -        .user_print = monitor_user_noop,	
> > > > -	.mhandler.cmd_new = do_migrate,
> > > > +        .mhandler.cmd = hmp_migrate,
> > > >      },
> > > >  
> > > >  
> > > > diff --git a/hmp.c b/hmp.c
> > > > index 3a54455..0f62b3f 100644
> > > > --- a/hmp.c
> > > > +++ b/hmp.c
> > > > @@ -14,6 +14,7 @@
> > > >   */
> > > >  
> > > >  #include "hmp.h"
> > > > +#include "qemu-timer.h"
> > > >  #include "qmp-commands.h"
> > > >  
> > > >  static void hmp_handle_error(Monitor *mon, Error **errp)
> > > > @@ -856,3 +857,58 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
> > > >  
> > > >      hmp_handle_error(mon, &error);
> > > >  }
> > > > +
> > > > +typedef struct MigrationStatus
> > > > +{
> > > > +    QEMUTimer *timer;
> > > > +    Monitor *mon;
> > > > +} MigrationStatus;
> > > > +
> > > > +static void hmp_migrate_status_cb(void *opaque)
> > > > +{
> > > > +    MigrationStatus *status = opaque;
> > > > +    MigrationInfo *info;
> > > > +
> > > > +    info = qmp_query_migrate(NULL);
> > > > +    if (!info->has_status || strcmp(info->status, "active") == 0) {
> > > > +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock) + 1000);
> > > > +    } else {
> > > > +        monitor_resume(status->mon);
> > > > +        qemu_del_timer(status->timer);
> > > > +        g_free(status);
> > > > +    }
> > > > +
> > > > +    qapi_free_MigrationInfo(info);
> > > > +}
> > > > +
> > > > +void hmp_migrate(Monitor *mon, const QDict *qdict)
> > > > +{
> > > > +    int detach = qdict_get_try_bool(qdict, "detach", 0);
> > > > +    int blk = qdict_get_try_bool(qdict, "blk", 0);
> > > > +    int inc = qdict_get_try_bool(qdict, "inc", 0);
> > > > +    const char *uri = qdict_get_str(qdict, "uri");
> > > > +    Error *err = NULL;
> > > > +
> > > > +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> > > > +    if (err) {
> > > > +        monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
> > > > +        error_free(err);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (!detach) {
> > > > +        MigrationStatus *status;
> > > > +
> > > > +        if (monitor_suspend(mon) < 0) {
> > > 
> > > So using monitor_suspend is ok? is that a possible solution for the
> > > screendump issue? I could pass the Monitor through the vga callback. Tbh
> > > I think it's ugly, but at least it doesn't introduce any new command.
> > 
> > This is only valid for HMP.
> > 
> 
> Yes, I'm thinking at this point for qmp to wait for the output file to
> be written via repeated poll / inotify.

Making this synchronous you mean? This will hold the global mutex for too
long, won't it?

Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Alon Levy]

On Mon, Mar 12, 2012 at 11:01:54AM -0300, Luiz Capitulino wrote:
> On Mon, 12 Mar 2012 15:49:27 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Mon, Mar 12, 2012 at 10:21:20AM -0300, Luiz Capitulino wrote:
> > > On Sat, 10 Mar 2012 00:51:29 +0200
> > > Alon Levy <alevy@redhat.com> wrote:
> > > 
> > > > On Fri, Mar 09, 2012 at 03:13:06PM -0300, Luiz Capitulino wrote:
> > > > > The migrate command is one of those commands where HMP and QMP completely
> > > > > mix up together. This made the conversion to the QAPI (which separates the
> > > > > command into QMP and HMP parts) a bit difficult.
> > > > > 
> > > > > The first important change to be noticed is that this commit completes the
> > > > > removal of the Monitor object from migration code, started by the previous
> > > > > commit.
> > > > > 
> > > > > Another important and tricky change is about supporting the non-detached
> > > > > mode. That's, if the user doesn't pass '-d' the migrate command will lock
> > > > > the monitor and will only release it when migration is finished.
> > > > > 
> > > > > To support that in the new HMP command (hmp_migrate()), it is necessary
> > > > > to create a timer which runs every second and checks if the migration is
> > > > > still active. If it's, the timer callback will re-schedule itself to run
> > > > > one second in the future. If the migration has already finished, the
> > > > > monitor lock is relased and the user can use it normally.
> > > > > 
> > > > > All these changes should be transparent to the user.
> > > > > 
> > > > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > > > ---
> > > > >  hmp-commands.hx  |    3 +--
> > > > >  hmp.c            |   56 +++++++++++++++++++++++++++++++++++++++++++++
> > > > >  hmp.h            |    1 +
> > > > >  migration-fd.c   |    2 +-
> > > > >  migration.c      |   66 ++++++++++++++----------------------------------------
> > > > >  migration.h      |    3 ---
> > > > >  qapi-schema.json |   21 +++++++++++++++++
> > > > >  qmp-commands.hx  |    9 +-------
> > > > >  savevm.c         |   13 +++++------
> > > > >  sysemu.h         |    2 +-
> > > > >  10 files changed, 105 insertions(+), 71 deletions(-)
> > > > > 
> > > > > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > > > > index ed88877..a86f53f 100644
> > > > > --- a/hmp-commands.hx
> > > > > +++ b/hmp-commands.hx
> > > > > @@ -806,8 +806,7 @@ ETEXI
> > > > >  		      " full copy of disk\n\t\t\t -i for migration without "
> > > > >  		      "shared storage with incremental copy of disk "
> > > > >  		      "(base image shared between src and destination)",
> > > > > -        .user_print = monitor_user_noop,	
> > > > > -	.mhandler.cmd_new = do_migrate,
> > > > > +        .mhandler.cmd = hmp_migrate,
> > > > >      },
> > > > >  
> > > > >  
> > > > > diff --git a/hmp.c b/hmp.c
> > > > > index 3a54455..0f62b3f 100644
> > > > > --- a/hmp.c
> > > > > +++ b/hmp.c
> > > > > @@ -14,6 +14,7 @@
> > > > >   */
> > > > >  
> > > > >  #include "hmp.h"
> > > > > +#include "qemu-timer.h"
> > > > >  #include "qmp-commands.h"
> > > > >  
> > > > >  static void hmp_handle_error(Monitor *mon, Error **errp)
> > > > > @@ -856,3 +857,58 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
> > > > >  
> > > > >      hmp_handle_error(mon, &error);
> > > > >  }
> > > > > +
> > > > > +typedef struct MigrationStatus
> > > > > +{
> > > > > +    QEMUTimer *timer;
> > > > > +    Monitor *mon;
> > > > > +} MigrationStatus;
> > > > > +
> > > > > +static void hmp_migrate_status_cb(void *opaque)
> > > > > +{
> > > > > +    MigrationStatus *status = opaque;
> > > > > +    MigrationInfo *info;
> > > > > +
> > > > > +    info = qmp_query_migrate(NULL);
> > > > > +    if (!info->has_status || strcmp(info->status, "active") == 0) {
> > > > > +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock) + 1000);
> > > > > +    } else {
> > > > > +        monitor_resume(status->mon);
> > > > > +        qemu_del_timer(status->timer);
> > > > > +        g_free(status);
> > > > > +    }
> > > > > +
> > > > > +    qapi_free_MigrationInfo(info);
> > > > > +}
> > > > > +
> > > > > +void hmp_migrate(Monitor *mon, const QDict *qdict)
> > > > > +{
> > > > > +    int detach = qdict_get_try_bool(qdict, "detach", 0);
> > > > > +    int blk = qdict_get_try_bool(qdict, "blk", 0);
> > > > > +    int inc = qdict_get_try_bool(qdict, "inc", 0);
> > > > > +    const char *uri = qdict_get_str(qdict, "uri");
> > > > > +    Error *err = NULL;
> > > > > +
> > > > > +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> > > > > +    if (err) {
> > > > > +        monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
> > > > > +        error_free(err);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    if (!detach) {
> > > > > +        MigrationStatus *status;
> > > > > +
> > > > > +        if (monitor_suspend(mon) < 0) {
> > > > 
> > > > So using monitor_suspend is ok? is that a possible solution for the
> > > > screendump issue? I could pass the Monitor through the vga callback. Tbh
> > > > I think it's ugly, but at least it doesn't introduce any new command.
> > > 
> > > This is only valid for HMP.
> > > 
> > 
> > Yes, I'm thinking at this point for qmp to wait for the output file to
> > be written via repeated poll / inotify.
> 
> Making this synchronous you mean? This will hold the global mutex for too
> long, won't it?

No, not at all what I mean. I'm talking about a libvirt change. There is
nothing that we can do in qemu other than maybe implementing a
qmp_suspend (automatic nack), already suggested screendump-async (nack
by no update) and now waiting for comments on just the HMP fix for
screendump.

Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Luiz Capitulino]

On Mon, 12 Mar 2012 16:23:01 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Mon, Mar 12, 2012 at 11:01:54AM -0300, Luiz Capitulino wrote:
> > On Mon, 12 Mar 2012 15:49:27 +0200
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > On Mon, Mar 12, 2012 at 10:21:20AM -0300, Luiz Capitulino wrote:
> > > > On Sat, 10 Mar 2012 00:51:29 +0200
> > > > Alon Levy <alevy@redhat.com> wrote:
> > > > 
> > > > > On Fri, Mar 09, 2012 at 03:13:06PM -0300, Luiz Capitulino wrote:
> > > > > > The migrate command is one of those commands where HMP and QMP completely
> > > > > > mix up together. This made the conversion to the QAPI (which separates the
> > > > > > command into QMP and HMP parts) a bit difficult.
> > > > > > 
> > > > > > The first important change to be noticed is that this commit completes the
> > > > > > removal of the Monitor object from migration code, started by the previous
> > > > > > commit.
> > > > > > 
> > > > > > Another important and tricky change is about supporting the non-detached
> > > > > > mode. That's, if the user doesn't pass '-d' the migrate command will lock
> > > > > > the monitor and will only release it when migration is finished.
> > > > > > 
> > > > > > To support that in the new HMP command (hmp_migrate()), it is necessary
> > > > > > to create a timer which runs every second and checks if the migration is
> > > > > > still active. If it's, the timer callback will re-schedule itself to run
> > > > > > one second in the future. If the migration has already finished, the
> > > > > > monitor lock is relased and the user can use it normally.
> > > > > > 
> > > > > > All these changes should be transparent to the user.
> > > > > > 
> > > > > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > > > > ---
> > > > > >  hmp-commands.hx  |    3 +--
> > > > > >  hmp.c            |   56 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  hmp.h            |    1 +
> > > > > >  migration-fd.c   |    2 +-
> > > > > >  migration.c      |   66 ++++++++++++++----------------------------------------
> > > > > >  migration.h      |    3 ---
> > > > > >  qapi-schema.json |   21 +++++++++++++++++
> > > > > >  qmp-commands.hx  |    9 +-------
> > > > > >  savevm.c         |   13 +++++------
> > > > > >  sysemu.h         |    2 +-
> > > > > >  10 files changed, 105 insertions(+), 71 deletions(-)
> > > > > > 
> > > > > > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > > > > > index ed88877..a86f53f 100644
> > > > > > --- a/hmp-commands.hx
> > > > > > +++ b/hmp-commands.hx
> > > > > > @@ -806,8 +806,7 @@ ETEXI
> > > > > >  		      " full copy of disk\n\t\t\t -i for migration without "
> > > > > >  		      "shared storage with incremental copy of disk "
> > > > > >  		      "(base image shared between src and destination)",
> > > > > > -        .user_print = monitor_user_noop,	
> > > > > > -	.mhandler.cmd_new = do_migrate,
> > > > > > +        .mhandler.cmd = hmp_migrate,
> > > > > >      },
> > > > > >  
> > > > > >  
> > > > > > diff --git a/hmp.c b/hmp.c
> > > > > > index 3a54455..0f62b3f 100644
> > > > > > --- a/hmp.c
> > > > > > +++ b/hmp.c
> > > > > > @@ -14,6 +14,7 @@
> > > > > >   */
> > > > > >  
> > > > > >  #include "hmp.h"
> > > > > > +#include "qemu-timer.h"
> > > > > >  #include "qmp-commands.h"
> > > > > >  
> > > > > >  static void hmp_handle_error(Monitor *mon, Error **errp)
> > > > > > @@ -856,3 +857,58 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
> > > > > >  
> > > > > >      hmp_handle_error(mon, &error);
> > > > > >  }
> > > > > > +
> > > > > > +typedef struct MigrationStatus
> > > > > > +{
> > > > > > +    QEMUTimer *timer;
> > > > > > +    Monitor *mon;
> > > > > > +} MigrationStatus;
> > > > > > +
> > > > > > +static void hmp_migrate_status_cb(void *opaque)
> > > > > > +{
> > > > > > +    MigrationStatus *status = opaque;
> > > > > > +    MigrationInfo *info;
> > > > > > +
> > > > > > +    info = qmp_query_migrate(NULL);
> > > > > > +    if (!info->has_status || strcmp(info->status, "active") == 0) {
> > > > > > +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock) + 1000);
> > > > > > +    } else {
> > > > > > +        monitor_resume(status->mon);
> > > > > > +        qemu_del_timer(status->timer);
> > > > > > +        g_free(status);
> > > > > > +    }
> > > > > > +
> > > > > > +    qapi_free_MigrationInfo(info);
> > > > > > +}
> > > > > > +
> > > > > > +void hmp_migrate(Monitor *mon, const QDict *qdict)
> > > > > > +{
> > > > > > +    int detach = qdict_get_try_bool(qdict, "detach", 0);
> > > > > > +    int blk = qdict_get_try_bool(qdict, "blk", 0);
> > > > > > +    int inc = qdict_get_try_bool(qdict, "inc", 0);
> > > > > > +    const char *uri = qdict_get_str(qdict, "uri");
> > > > > > +    Error *err = NULL;
> > > > > > +
> > > > > > +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> > > > > > +    if (err) {
> > > > > > +        monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
> > > > > > +        error_free(err);
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (!detach) {
> > > > > > +        MigrationStatus *status;
> > > > > > +
> > > > > > +        if (monitor_suspend(mon) < 0) {
> > > > > 
> > > > > So using monitor_suspend is ok? is that a possible solution for the
> > > > > screendump issue? I could pass the Monitor through the vga callback. Tbh
> > > > > I think it's ugly, but at least it doesn't introduce any new command.
> > > > 
> > > > This is only valid for HMP.
> > > > 
> > > 
> > > Yes, I'm thinking at this point for qmp to wait for the output file to
> > > be written via repeated poll / inotify.
> > 
> > Making this synchronous you mean? This will hold the global mutex for too
> > long, won't it?
> 
> No, not at all what I mean. I'm talking about a libvirt change.

Oh, good idea, that's a valid workaround.

> There is
> nothing that we can do in qemu other than maybe implementing a
> qmp_suspend (automatic nack), already suggested screendump-async (nack
> by no update) and now waiting for comments on just the HMP fix for
> screendump.

Will review it shortly.