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

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

[Luiz Capitulino]

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 5/6 purges the 'mon' object from migration code. One of the
    consequences is that we lose 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

Besides, this is missing testing with libvirt. I plan to do it shortly, but
wanted to get some review in parallel.

 arch_init.c       |    2 +-
 block-migration.c |   58 ++++++++++++++++++-----------------------
 check-qdict.c     |   29 ++++++++++++++++++++
 error.c           |   12 ++++++++
 error.h           |    5 +++
 hmp-commands.hx   |    3 +-
 hmp.c             |   51 ++++++++++++++++++++++++++++++++++++
 hmp.h             |    1 +
 migration-fd.c    |    2 +-
 migration.c       |   74 +++++++++++++++--------------------------------------
 migration.h       |    5 +---
 monitor.c         |    5 +++
 monitor.h         |    1 +
 qapi-schema.json  |   21 +++++++++++++++
 qdict.c           |   18 +++++++++++++
 qdict.h           |    1 +
 qerror.c          |    8 +++++
 qerror.h          |    6 ++++
 qmp-commands.hx   |    9 +-----
 savevm.c          |   42 ++++++++++++++---------------
 sysemu.h          |   11 +++----
 vmstate.h         |    3 +-
 22 files changed, 235 insertions(+), 132 deletions(-)

[Qemu-devel] [PATCH 1/6] 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(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 8e6efaf..ce9aa05 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 e8718bf..557d7a0 100644
--- a/qerror.h
+++ b/qerror.h
@@ -165,6 +165,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.111.gf3fb0.dirty

[Qemu-devel] [PATCH 2/6] monitor: Introduce qemu_get_fd()

[Luiz Capitulino]

Get the file-descriptor from 'cur_mon', will be used by the QAPI
converted migration command.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    5 +++++
 monitor.h |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index aadbdcb..11639b1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2250,6 +2250,11 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
     }
 }
 
+int qemu_get_fd(const char *fdname)
+{
+    return monitor_get_fd(cur_mon, fdname);
+}
+
 int monitor_get_fd(Monitor *mon, const char *fdname)
 {
     mon_fd_t *monfd;
diff --git a/monitor.h b/monitor.h
index b72ea07..58109af 100644
--- a/monitor.h
+++ b/monitor.h
@@ -56,6 +56,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device,
                                   BlockDriverCompletionFunc *completion_cb,
                                   void *opaque);
 
+int qemu_get_fd(const char *fdname);
 int monitor_get_fd(Monitor *mon, const char *fdname);
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
-- 
1.7.9.111.gf3fb0.dirty

[Qemu-devel] [PATCH 3/6] QDict: Introduce qdict_copy()

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 check-qdict.c |   29 +++++++++++++++++++++++++++++
 qdict.c       |   18 ++++++++++++++++++
 qdict.h       |    1 +
 3 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/check-qdict.c b/check-qdict.c
index fc0d276..12c73ab 100644
--- a/check-qdict.c
+++ b/check-qdict.c
@@ -227,6 +227,34 @@ static void qdict_iterapi_test(void)
     QDECREF(tests_dict);
 }
 
+static void qdict_copy_test(void)
+{
+    QDict *tests_dict = qdict_new();
+    const QDictEntry *ent;
+    QDict *new;
+    int i;
+
+    for (i = 0; i < 1024; i++) {
+        char key[32];
+
+        snprintf(key, sizeof(key), "key%d", i);
+        qdict_put(tests_dict, key, qint_from_int(i));
+    }
+
+    new = qdict_copy(tests_dict);
+
+    g_assert(qdict_size(new) == qdict_size(tests_dict));
+    for (ent = qdict_first(tests_dict); ent; ent = qdict_next(tests_dict, ent)){
+        const char *key = qdict_entry_key(ent);
+
+        g_assert(qdict_haskey(new, key) == 1);
+        g_assert(qdict_get_int(new, key) == qdict_get_int(tests_dict, key));
+    }
+
+    QDECREF(new);
+    QDECREF(tests_dict);
+}
+
 /*
  * Errors test-cases
  */
@@ -365,6 +393,7 @@ int main(int argc, char **argv)
     g_test_add_func("/public/del", qdict_del_test);
     g_test_add_func("/public/to_qdict", qobject_to_qdict_test);
     g_test_add_func("/public/iterapi", qdict_iterapi_test);
+    g_test_add_func("/public/copy", qdict_copy_test);
 
     g_test_add_func("/errors/put_exists", qdict_put_exists_test);
     g_test_add_func("/errors/get_not_exists", qdict_get_not_exists_test);
diff --git a/qdict.c b/qdict.c
index 4bf308b..dc9141f 100644
--- a/qdict.c
+++ b/qdict.c
@@ -401,6 +401,24 @@ const QDictEntry *qdict_next(const QDict *qdict, const QDictEntry *entry)
 }
 
 /**
+ * qdict_copy(): Build a new dictionary from an existing one.
+ */
+QDict *qdict_copy(const QDict *from)
+{
+    const QDictEntry *ent;
+    QDict *new;
+
+    new = qdict_new();
+
+    for (ent = qdict_first(from); ent; ent = qdict_next(from, ent)) {
+        qdict_put_obj(new, qdict_entry_key(ent), qdict_entry_value(ent));
+        qobject_incref(qdict_entry_value(ent));
+    }
+
+    return new;
+}
+
+/**
  * qentry_destroy(): Free all the memory allocated by a QDictEntry
  */
 static void qentry_destroy(QDictEntry *e)
diff --git a/qdict.h b/qdict.h
index 929d8d2..9976a18 100644
--- a/qdict.h
+++ b/qdict.h
@@ -36,6 +36,7 @@ typedef struct QDict {
 QDict *qdict_new(void);
 const char *qdict_entry_key(const QDictEntry *entry);
 QObject *qdict_entry_value(const QDictEntry *entry);
+QDict *qdict_copy(const QDict *from);
 size_t qdict_size(const QDict *qdict);
 void qdict_put_obj(QDict *qdict, const char *key, QObject *value);
 void qdict_del(QDict *qdict, const char *key);
-- 
1.7.9.111.gf3fb0.dirty

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

[Luiz Capitulino]

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

diff --git a/error.c b/error.c
index 990050f..d85e136 100644
--- a/error.c
+++ b/error.c
@@ -43,6 +43,18 @@ 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->obj = qdict_copy(err->obj);
+    err_new->msg = g_strdup(err->msg);
+    err_new->fmt = err->fmt;
+
+    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.111.gf3fb0.dirty

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

[Qemu-devel] [PATCH 6/6] 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 was 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            |   51 +++++++++++++++++++++++++++++++++++++++++
 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, 100 insertions(+), 71 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 573b823..10d3f1b 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -792,8 +792,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 8ff8c94..70bd574 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)
@@ -851,3 +852,53 @@ 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);
+        goto out;
+    }
+
+    monitor_resume(status->mon);
+    qemu_del_timer(status->timer);
+    g_free(status);
+
+out:
+    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 = g_malloc0(sizeof(*status));
+        status->timer = qemu_new_timer_ms(rt_clock, hmp_migrate_status_cb,
+                                          status);
+        status->mon = mon;
+        monitor_suspend(mon);
+        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
+    }
+}
diff --git a/hmp.h b/hmp.h
index 18eecbd..989a613 100644
--- a/hmp.h
+++ b/hmp.h
@@ -58,5 +58,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..99f192d 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 = qemu_get_fd(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 5c5c94c..6f148f9 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 d02ee86..0cdc3ce 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1582,3 +1582,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 b5e2ab8..84b348a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -425,14 +425,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 3a0f683..18c07ca 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -65,7 +65,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.111.gf3fb0.dirty

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

[Jan Kiszka]

On 2012-02-10 20:31, Luiz Capitulino wrote:
> 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 5/6 purges the 'mon' object from migration code. One of the
>     consequences is that we lose the ability to print progress status to
>     the HMP user (esp. in block migration)

This smells extremely fishy. You have some common "monitor" context in
both cases, means something that decides where suspend/resume takes
effect or where to pick up file descriptors from. If the exiting Monitor
object is not generic enough, introduce some super-class and use that in
common services. Or make sure that a variant of Monitor is also valid
over QMP. But don't remove the dependency from the API, while
reintroducing it via the backdoor of cur_mon.

Jan

> 
>  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
> 
> Besides, this is missing testing with libvirt. I plan to do it shortly, but
> wanted to get some review in parallel.
> 
>  arch_init.c       |    2 +-
>  block-migration.c |   58 ++++++++++++++++++-----------------------
>  check-qdict.c     |   29 ++++++++++++++++++++
>  error.c           |   12 ++++++++
>  error.h           |    5 +++
>  hmp-commands.hx   |    3 +-
>  hmp.c             |   51 ++++++++++++++++++++++++++++++++++++
>  hmp.h             |    1 +
>  migration-fd.c    |    2 +-
>  migration.c       |   74 +++++++++++++++--------------------------------------
>  migration.h       |    5 +---
>  monitor.c         |    5 +++
>  monitor.h         |    1 +
>  qapi-schema.json  |   21 +++++++++++++++
>  qdict.c           |   18 +++++++++++++
>  qdict.h           |    1 +
>  qerror.c          |    8 +++++
>  qerror.h          |    6 ++++
>  qmp-commands.hx   |    9 +-----
>  savevm.c          |   42 ++++++++++++++---------------
>  sysemu.h          |   11 +++----
>  vmstate.h         |    3 +-
>  22 files changed, 235 insertions(+), 132 deletions(-)
> 
> 

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

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

[Jan Kiszka]

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

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

Jan

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

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

[Paolo Bonzini]

On 02/10/2012 08:31 PM, Luiz Capitulino wrote:
> +    err_new = g_malloc0(sizeof(*err));
> +    err_new->obj = qdict_copy(err->obj);
> +    err_new->msg = g_strdup(err->msg);
> +    err_new->fmt = err->fmt;
> +
> +    return err_new;
> +}

Why isn't an incref sufficient?  QDicts should be constant once they've
been built.  (Also, I would refcount Errors rather than copy them, but
that's a personal preference and I do not really object to error_copy).

Paolo

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

[Wen Congyang]

At 02/11/2012 03:31 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
> the monitor and will only release it when migration is finished.
> 
> To support that in the new HMP command (hmp_migrate()), it was 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            |   51 +++++++++++++++++++++++++++++++++++++++++
>  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, 100 insertions(+), 71 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 573b823..10d3f1b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -792,8 +792,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 8ff8c94..70bd574 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)
> @@ -851,3 +852,53 @@ 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);
> +        goto out;
> +    }
> +
> +    monitor_resume(status->mon);
> +    qemu_del_timer(status->timer);
> +    g_free(status);
> +
> +out:
> +    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 = g_malloc0(sizeof(*status));
> +        status->timer = qemu_new_timer_ms(rt_clock, hmp_migrate_status_cb,
> +                                          status);
> +        status->mon = mon;
> +        monitor_suspend(mon);
> +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
> +    }
> +}
> diff --git a/hmp.h b/hmp.h
> index 18eecbd..989a613 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -58,5 +58,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..99f192d 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 = qemu_get_fd(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 5c5c94c..6f148f9 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);

It seems that the variable detach is not used now.
Does it mean we always run migration at the background?

Thanks
Wen Congyang
> +    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 d02ee86..0cdc3ce 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1582,3 +1582,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 b5e2ab8..84b348a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -425,14 +425,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 3a0f683..18c07ca 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -65,7 +65,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);

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

[Luiz Capitulino]

On Wed, 15 Feb 2012 09:59:07 +0100
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2012-02-10 20:31, Luiz Capitulino wrote:
> > 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 5/6 purges the 'mon' object from migration code. One of the
> >     consequences is that we lose the ability to print progress status to
> >     the HMP user (esp. in block migration)
> 
> This smells extremely fishy. You have some common "monitor" context in
> both cases, means something that decides where suspend/resume takes
> effect or where to pick up file descriptors from. If the exiting Monitor
> object is not generic enough, introduce some super-class and use that in
> common services. Or make sure that a variant of Monitor is also valid
> over QMP. But don't remove the dependency from the API, while
> reintroducing it via the backdoor of cur_mon.

What we really want to do here is to untangle HMP and QMP. Unfortunately,
the migrate command is one of those commands where the two are deeply
tangled and the split won't be perfect.

However, the two cases you mention above are solvable:

 1. suspend/resume: this is *really* a HMP feature and shouldn't be in any
    QMP code path. This is correctly addressed in this series by moving it
    to hmp_migrate()

 2. file descriptor passing: the new QMP server will support sessions and
    we'll move statefull commands (like getfd) to it. When we do it, we'll
    introduce a new API to get fds that won't depend on the monitor. However,
    this requires all commands to be converted to the qapi first. Meanwhile
    we use the qemu_get_fd() API.

    Note: qemu_get_fd() is temporary, it shouldn't be a problem to use it
    (if it's not incorrect, of course, I honestly haven't fully tested it yet).

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

[Luiz Capitulino]

On Wed, 15 Feb 2012 10:02:54 +0100
Jan Kiszka <jan.kiszka@siemens.com> wrote:

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

I will consider dropping it :)

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

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

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

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

[Luiz Capitulino]

On Wed, 15 Feb 2012 10:04:50 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 02/10/2012 08:31 PM, Luiz Capitulino wrote:
> > +    err_new = g_malloc0(sizeof(*err));
> > +    err_new->obj = qdict_copy(err->obj);
> > +    err_new->msg = g_strdup(err->msg);
> > +    err_new->fmt = err->fmt;
> > +
> > +    return err_new;
> > +}
> 
> Why isn't an incref sufficient?

You know, I'm looking at the code right now and can't answer myself that
question :-) I think what wanted to do was to duplicate the Error object
and just ended up duplicating everything...

>  QDicts should be constant once they've
> been built. 

I think you meant QErrors?

> (Also, I would refcount Errors rather than copy them, but
> that's a personal preference and I do not really object to error_copy).

The Error object doesn't have refcounts and I'd prefer to just duplicate it
for now (instead of adding it).

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

[Luiz Capitulino]

On Wed, 15 Feb 2012 17:07:16 +0800
Wen Congyang <wency@cn.fujitsu.com> wrote:

> At 02/11/2012 03:31 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
> > the monitor and will only release it when migration is finished.
> > 
> > To support that in the new HMP command (hmp_migrate()), it was 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            |   51 +++++++++++++++++++++++++++++++++++++++++
> >  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, 100 insertions(+), 71 deletions(-)
> > 
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 573b823..10d3f1b 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -792,8 +792,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 8ff8c94..70bd574 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)
> > @@ -851,3 +852,53 @@ 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);
> > +        goto out;
> > +    }
> > +
> > +    monitor_resume(status->mon);
> > +    qemu_del_timer(status->timer);
> > +    g_free(status);
> > +
> > +out:
> > +    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 = g_malloc0(sizeof(*status));
> > +        status->timer = qemu_new_timer_ms(rt_clock, hmp_migrate_status_cb,
> > +                                          status);
> > +        status->mon = mon;
> > +        monitor_suspend(mon);
> > +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
> > +    }
> > +}
> > diff --git a/hmp.h b/hmp.h
> > index 18eecbd..989a613 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -58,5 +58,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..99f192d 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 = qemu_get_fd(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 5c5c94c..6f148f9 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);
> 
> It seems that the variable detach is not used now.
> Does it mean we always run migration at the background?

In QMP, yes.

> 
> Thanks
> Wen Congyang
> > +    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 d02ee86..0cdc3ce 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1582,3 +1582,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 b5e2ab8..84b348a 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -425,14 +425,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 3a0f683..18c07ca 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -65,7 +65,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);
> 

Re: [Qemu-devel] [PATCH 3/6] QDict: Introduce qdict_copy()

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>


>  /**
> + * qdict_copy(): Build a new dictionary from an existing one.
> + */
> +QDict *qdict_copy(const QDict *from)
> +{
> +    const QDictEntry *ent;
> +    QDict *new;
> +
> +    new = qdict_new();
> +
> +    for (ent = qdict_first(from); ent; ent = qdict_next(from, ent)) {
> +        qdict_put_obj(new, qdict_entry_key(ent), qdict_entry_value(ent));
> +        qobject_incref(qdict_entry_value(ent));
> +    }


Without having any clue about how qobject refcounting works,  it looks
suspicious that we first insert an object in a dict, and increase the
ref counter after.  Shouldn't we do it the other way around?

Later, Juan.

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

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 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>


Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH 2/6] monitor: Introduce qemu_get_fd()

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> Get the file-descriptor from 'cur_mon', will be used by the QAPI
> converted migration command.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

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

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> The Monitor object is passed back and forth within the migration/savevm
> code so that it can print errors and progress to the user.
>
> However, that approach assumes a HMP monitor, being completely invalid
> in QMP.
>
> This commit drops almost every single usage of the Monitor object, all
> monitor_printf() calls have been converted into DPRINTF() ones.
>
> There are a few remaining Monitor objects, those are going to be dropped
> by the next commit.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

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

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

Later, Juan.

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

[Paolo Bonzini]

On 02/15/2012 02:05 PM, Luiz Capitulino wrote:
>> >  QDicts should be constant once they've
>> > been built. 
> I think you meant QErrors?

I meant QDicts included in Errors.

> > (Also, I would refcount Errors rather than copy them, but
> > that's a personal preference and I do not really object to error_copy).
> The Error object doesn't have refcounts and I'd prefer to just duplicate it
> for now (instead of adding it).

Yes, that's fine.

Paolo

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

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> 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 was 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.

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

1 second is one eternity.  I will ask for something smaller (100ms or
200ms range).

> +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);

This is the only user of qmp_migrate, why do we need to pass both
has_blk & blk (and inc and !!inc?)  Both parameters are bool, I am
really confused here.

Later, Juan.

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

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Didn't care if we copy/incref the dictionaries as Paolo suggests.

Both work for me.

Later, Juan.

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

[Jan Kiszka]

On 2012-02-10 20:31, 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 was 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            |   51 +++++++++++++++++++++++++++++++++++++++++
>  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, 100 insertions(+), 71 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 573b823..10d3f1b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -792,8 +792,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 8ff8c94..70bd574 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)
> @@ -851,3 +852,53 @@ 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);
> +        goto out;
> +    }
> +
> +    monitor_resume(status->mon);

status->mon can be NULL if suspend is not supported, see below.

> +    qemu_del_timer(status->timer);
> +    g_free(status);
> +
> +out:
> +    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 = g_malloc0(sizeof(*status));
> +        status->timer = qemu_new_timer_ms(rt_clock, hmp_migrate_status_cb,
> +                                          status);
> +        status->mon = mon;
> +        monitor_suspend(mon);

Lacks return code check and handling, see original version.

> +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
> +    }
> +}

Jan

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

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

[Jan Kiszka]

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

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

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

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

Jan

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

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

[Jan Kiszka]

On 2012-02-15 13:49, Luiz Capitulino wrote:
> On Wed, 15 Feb 2012 09:59:07 +0100
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> On 2012-02-10 20:31, Luiz Capitulino wrote:
>>> 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 5/6 purges the 'mon' object from migration code. One of the
>>>     consequences is that we lose the ability to print progress status to
>>>     the HMP user (esp. in block migration)
>>
>> This smells extremely fishy. You have some common "monitor" context in
>> both cases, means something that decides where suspend/resume takes
>> effect or where to pick up file descriptors from. If the exiting Monitor
>> object is not generic enough, introduce some super-class and use that in
>> common services. Or make sure that a variant of Monitor is also valid
>> over QMP. But don't remove the dependency from the API, while
>> reintroducing it via the backdoor of cur_mon.
> 
> What we really want to do here is to untangle HMP and QMP. Unfortunately,
> the migrate command is one of those commands where the two are deeply
> tangled and the split won't be perfect.
> 
> However, the two cases you mention above are solvable:
> 
>  1. suspend/resume: this is *really* a HMP feature and shouldn't be in any
>     QMP code path. This is correctly addressed in this series by moving it
>     to hmp_migrate()

Almost correctly. ;)

> 
>  2. file descriptor passing: the new QMP server will support sessions and
>     we'll move statefull commands (like getfd) to it. When we do it, we'll
>     introduce a new API to get fds that won't depend on the monitor. However,
>     this requires all commands to be converted to the qapi first. Meanwhile
>     we use the qemu_get_fd() API.
> 
>     Note: qemu_get_fd() is temporary, it shouldn't be a problem to use it
>     (if it's not incorrect, of course, I honestly haven't fully tested it yet).

So there will be a common super-class of Monitor and that new QMP
session that also manages the file descriptors? That would make sense.
Still, there would be monitor_get_fd and qmp_get_fd then not
qemu_get_fd. I think that should be done already. BTW, where do you get
the FDs from now in QMP mode? Is there currently a Monitor instance
associated?

Jan

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

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

[Jan Kiszka]

On 2012-02-15 14:31, Jan Kiszka wrote:
> On 2012-02-10 20:31, 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 was 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            |   51 +++++++++++++++++++++++++++++++++++++++++
>>  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, 100 insertions(+), 71 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 573b823..10d3f1b 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -792,8 +792,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 8ff8c94..70bd574 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)
>> @@ -851,3 +852,53 @@ 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);
>> +        goto out;
>> +    }
>> +
>> +    monitor_resume(status->mon);
> 
> status->mon can be NULL if suspend is not supported, see below.
> 
>> +    qemu_del_timer(status->timer);
>> +    g_free(status);
>> +
>> +out:

Ah, and "else" please, no need for goto here.

Jan

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

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

[Michael Roth]

On Wed, Feb 15, 2012 at 02:25:49PM +0100, Juan Quintela wrote:
> Luiz Capitulino <lcapitulino@redhat.com> 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 was 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.
> 
> > +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);
> 
> 1 second is one eternity.  I will ask for something smaller (100ms or
> 200ms range).
> 
> > +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> 
> This is the only user of qmp_migrate, why do we need to pass both
> has_blk & blk (and inc and !!inc?)  Both parameters are bool, I am
> really confused here.

QAPI-generated qmp_marshal_input_migrate() is another user, and we don't
want to decide the defaults for incoming/blk there, so we need some way
to tell qmp_migrate() whether or not the bools we pass should be
ignored, or whether they were explicitly defined by the qmp user.

It looks weird in this particular case, but other than having multiple
variants of the qmp_migrate() command it's the only way to avoid having
randomly/un- initialized values clobber the defaults expected by the user.

> 
> Later, Juan.
> 

Re: [Qemu-devel] [PATCH 3/6] QDict: Introduce qdict_copy()

[Luiz Capitulino]

On Wed, 15 Feb 2012 14:10:32 +0100
Juan Quintela <quintela@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> 
> >  /**
> > + * qdict_copy(): Build a new dictionary from an existing one.
> > + */
> > +QDict *qdict_copy(const QDict *from)
> > +{
> > +    const QDictEntry *ent;
> > +    QDict *new;
> > +
> > +    new = qdict_new();
> > +
> > +    for (ent = qdict_first(from); ent; ent = qdict_next(from, ent)) {
> > +        qdict_put_obj(new, qdict_entry_key(ent), qdict_entry_value(ent));
> > +        qobject_incref(qdict_entry_value(ent));
> > +    }
> 
> 
> Without having any clue about how qobject refcounting works,  it looks
> suspicious that we first insert an object in a dict, and increase the
> ref counter after.  Shouldn't we do it the other way around?

I think this only matters for threaded applications. QDicts are not thread
safe, so this is not an issue but I'll drop this patch anyway (in favor of
increfing the error object).

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

[Luiz Capitulino]

On Wed, 15 Feb 2012 10:01:22 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Wed, Feb 15, 2012 at 02:25:49PM +0100, Juan Quintela wrote:
> > Luiz Capitulino <lcapitulino@redhat.com> 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 was 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.
> > 
> > > +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);
> > 
> > 1 second is one eternity.  I will ask for something smaller (100ms or
> > 200ms range).

Well, I see two issues in reducing the timeout. The first one is that, we'll
be polling more aggressively for something that doesn't finish quickly. The
one is that, this is HMP, so I'm not sure humans will perceive the difference
most of the time.

> > > +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> > 
> > This is the only user of qmp_migrate, why do we need to pass both
> > has_blk & blk (and inc and !!inc?)  Both parameters are bool, I am
> > really confused here.
> 
> QAPI-generated qmp_marshal_input_migrate() is another user, and we don't
> want to decide the defaults for incoming/blk there, so we need some way
> to tell qmp_migrate() whether or not the bools we pass should be
> ignored, or whether they were explicitly defined by the qmp user.
> 
> It looks weird in this particular case,

For all qapi functions that take optionals parameters, yes.

> but other than having multiple
> variants of the qmp_migrate() command it's the only way to avoid having
> randomly/un- initialized values clobber the defaults expected by the user.

This question always comes from time to time, I'm wondering if we could
generate a wrapper that assumes optionals are always passed or have good
default values (like checking if a pointer argument is NULL, and if it's
setting the has_ bool to false).

I don't think this is a must have, but is a nice feature.

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

[Luiz Capitulino]

On Wed, 15 Feb 2012 14:34:52 +0100
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2012-02-15 13:49, Luiz Capitulino wrote:
> > On Wed, 15 Feb 2012 09:59:07 +0100
> > Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > 
> >> On 2012-02-10 20:31, Luiz Capitulino wrote:
> >>> 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 5/6 purges the 'mon' object from migration code. One of the
> >>>     consequences is that we lose the ability to print progress status to
> >>>     the HMP user (esp. in block migration)
> >>
> >> This smells extremely fishy. You have some common "monitor" context in
> >> both cases, means something that decides where suspend/resume takes
> >> effect or where to pick up file descriptors from. If the exiting Monitor
> >> object is not generic enough, introduce some super-class and use that in
> >> common services. Or make sure that a variant of Monitor is also valid
> >> over QMP. But don't remove the dependency from the API, while
> >> reintroducing it via the backdoor of cur_mon.
> > 
> > What we really want to do here is to untangle HMP and QMP. Unfortunately,
> > the migrate command is one of those commands where the two are deeply
> > tangled and the split won't be perfect.
> > 
> > However, the two cases you mention above are solvable:
> > 
> >  1. suspend/resume: this is *really* a HMP feature and shouldn't be in any
> >     QMP code path. This is correctly addressed in this series by moving it
> >     to hmp_migrate()
> 
> Almost correctly. ;)

Well, it was moved to the right place :)

> > 
> >  2. file descriptor passing: the new QMP server will support sessions and
> >     we'll move statefull commands (like getfd) to it. When we do it, we'll
> >     introduce a new API to get fds that won't depend on the monitor. However,
> >     this requires all commands to be converted to the qapi first. Meanwhile
> >     we use the qemu_get_fd() API.
> > 
> >     Note: qemu_get_fd() is temporary, it shouldn't be a problem to use it
> >     (if it's not incorrect, of course, I honestly haven't fully tested it yet).
> 
> So there will be a common super-class of Monitor and that new QMP
> session that also manages the file descriptors? That would make sense.

Oh, yes. Now I see that you said exactly that earlier. Sorry for more or less
re-stating it.

> Still, there would be monitor_get_fd and qmp_get_fd then not
> qemu_get_fd. I think that should be done already.

The problem is that monitor_get_fd() already exists and qmp_get_fd()
doesn't make much sense (as this is not related to QMP right now). So,
I could call it monitor_get_fd_cur() or something like this.

> BTW, where do you get
> the FDs from now in QMP mode? Is there currently a Monitor instance
> associated?

Yes. The current QMP server is associated with a Monitor instance and
it supports the getfd command.

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

[Jan Kiszka]

On 2012-02-15 18:23, Luiz Capitulino wrote:
> On Wed, 15 Feb 2012 14:34:52 +0100
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> On 2012-02-15 13:49, Luiz Capitulino wrote:
>>> On Wed, 15 Feb 2012 09:59:07 +0100
>>> Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>
>>>> On 2012-02-10 20:31, Luiz Capitulino wrote:
>>>>> 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 5/6 purges the 'mon' object from migration code. One of the
>>>>>     consequences is that we lose the ability to print progress status to
>>>>>     the HMP user (esp. in block migration)
>>>>
>>>> This smells extremely fishy. You have some common "monitor" context in
>>>> both cases, means something that decides where suspend/resume takes
>>>> effect or where to pick up file descriptors from. If the exiting Monitor
>>>> object is not generic enough, introduce some super-class and use that in
>>>> common services. Or make sure that a variant of Monitor is also valid
>>>> over QMP. But don't remove the dependency from the API, while
>>>> reintroducing it via the backdoor of cur_mon.
>>>
>>> What we really want to do here is to untangle HMP and QMP. Unfortunately,
>>> the migrate command is one of those commands where the two are deeply
>>> tangled and the split won't be perfect.
>>>
>>> However, the two cases you mention above are solvable:
>>>
>>>  1. suspend/resume: this is *really* a HMP feature and shouldn't be in any
>>>     QMP code path. This is correctly addressed in this series by moving it
>>>     to hmp_migrate()
>>
>> Almost correctly. ;)
> 
> Well, it was moved to the right place :)

(see the other thread)

> 
>>>
>>>  2. file descriptor passing: the new QMP server will support sessions and
>>>     we'll move statefull commands (like getfd) to it. When we do it, we'll
>>>     introduce a new API to get fds that won't depend on the monitor. However,
>>>     this requires all commands to be converted to the qapi first. Meanwhile
>>>     we use the qemu_get_fd() API.
>>>
>>>     Note: qemu_get_fd() is temporary, it shouldn't be a problem to use it
>>>     (if it's not incorrect, of course, I honestly haven't fully tested it yet).
>>
>> So there will be a common super-class of Monitor and that new QMP
>> session that also manages the file descriptors? That would make sense.
> 
> Oh, yes. Now I see that you said exactly that earlier. Sorry for more or less
> re-stating it.
> 
>> Still, there would be monitor_get_fd and qmp_get_fd then not
>> qemu_get_fd. I think that should be done already.
> 
> The problem is that monitor_get_fd() already exists and qmp_get_fd()
> doesn't make much sense (as this is not related to QMP right now). So,
> I could call it monitor_get_fd_cur() or something like this.

What object represent a QMP session now? That object will once hold the
reference to the FDs. So some qmp_get_fd will take that session and
return the requested fd - so, it does make sense, long-term at least.

In any case, as long as everyone can mess with cur_mon, you don't need
to introduce wrappers that just link a normal monitor service with that
variable.

Jan

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

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

[Luiz Capitulino]

On Wed, 15 Feb 2012 18:39:31 +0100
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2012-02-15 18:23, Luiz Capitulino wrote:
> > On Wed, 15 Feb 2012 14:34:52 +0100
> > Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > 
> >> On 2012-02-15 13:49, Luiz Capitulino wrote:
> >>> On Wed, 15 Feb 2012 09:59:07 +0100
> >>> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>
> >>>> On 2012-02-10 20:31, Luiz Capitulino wrote:
> >>>>> 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 5/6 purges the 'mon' object from migration code. One of the
> >>>>>     consequences is that we lose the ability to print progress status to
> >>>>>     the HMP user (esp. in block migration)
> >>>>
> >>>> This smells extremely fishy. You have some common "monitor" context in
> >>>> both cases, means something that decides where suspend/resume takes
> >>>> effect or where to pick up file descriptors from. If the exiting Monitor
> >>>> object is not generic enough, introduce some super-class and use that in
> >>>> common services. Or make sure that a variant of Monitor is also valid
> >>>> over QMP. But don't remove the dependency from the API, while
> >>>> reintroducing it via the backdoor of cur_mon.
> >>>
> >>> What we really want to do here is to untangle HMP and QMP. Unfortunately,
> >>> the migrate command is one of those commands where the two are deeply
> >>> tangled and the split won't be perfect.
> >>>
> >>> However, the two cases you mention above are solvable:
> >>>
> >>>  1. suspend/resume: this is *really* a HMP feature and shouldn't be in any
> >>>     QMP code path. This is correctly addressed in this series by moving it
> >>>     to hmp_migrate()
> >>
> >> Almost correctly. ;)
> > 
> > Well, it was moved to the right place :)
> 
> (see the other thread)

Yeah, I saw it and will fix the problems you've pointed out.

> >>>  2. file descriptor passing: the new QMP server will support sessions and
> >>>     we'll move statefull commands (like getfd) to it. When we do it, we'll
> >>>     introduce a new API to get fds that won't depend on the monitor. However,
> >>>     this requires all commands to be converted to the qapi first. Meanwhile
> >>>     we use the qemu_get_fd() API.
> >>>
> >>>     Note: qemu_get_fd() is temporary, it shouldn't be a problem to use it
> >>>     (if it's not incorrect, of course, I honestly haven't fully tested it yet).
> >>
> >> So there will be a common super-class of Monitor and that new QMP
> >> session that also manages the file descriptors? That would make sense.
> > 
> > Oh, yes. Now I see that you said exactly that earlier. Sorry for more or less
> > re-stating it.
> > 
> >> Still, there would be monitor_get_fd and qmp_get_fd then not
> >> qemu_get_fd. I think that should be done already.
> > 
> > The problem is that monitor_get_fd() already exists and qmp_get_fd()
> > doesn't make much sense (as this is not related to QMP right now). So,
> > I could call it monitor_get_fd_cur() or something like this.
> 
> What object represent a QMP session now? 

We don't exactly have the notion of a QMP session today, but all QMP state
is currently stored in the Monitor object.

> That object will once hold the
> reference to the FDs. So some qmp_get_fd will take that session and
> return the requested fd - so, it does make sense, long-term at least.

Yes. Actually most of the code has already been written by Anthony:

 git://repo.or.cz/qemu/aliguori.git glib

 (look at qmp-core.c)

What I'm doing is to rebase it, do some integration work & fix ups.

> In any case, as long as everyone can mess with cur_mon, you don't need
> to introduce wrappers that just link a normal monitor service with that
> variable.

So, you're suggestion to just use monitor_get_fd(), right?

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

[Jan Kiszka]

On 2012-02-15 18:49, Luiz Capitulino wrote:
> On Wed, 15 Feb 2012 18:39:31 +0100
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> On 2012-02-15 18:23, Luiz Capitulino wrote:
>>> On Wed, 15 Feb 2012 14:34:52 +0100
>>> Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>
>>>> On 2012-02-15 13:49, Luiz Capitulino wrote:
>>>>> On Wed, 15 Feb 2012 09:59:07 +0100
>>>>> Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>
>>>>>> On 2012-02-10 20:31, Luiz Capitulino wrote:
>>>>>>> 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 5/6 purges the 'mon' object from migration code. One of the
>>>>>>>     consequences is that we lose the ability to print progress status to
>>>>>>>     the HMP user (esp. in block migration)
>>>>>>
>>>>>> This smells extremely fishy. You have some common "monitor" context in
>>>>>> both cases, means something that decides where suspend/resume takes
>>>>>> effect or where to pick up file descriptors from. If the exiting Monitor
>>>>>> object is not generic enough, introduce some super-class and use that in
>>>>>> common services. Or make sure that a variant of Monitor is also valid
>>>>>> over QMP. But don't remove the dependency from the API, while
>>>>>> reintroducing it via the backdoor of cur_mon.
>>>>>
>>>>> What we really want to do here is to untangle HMP and QMP. Unfortunately,
>>>>> the migrate command is one of those commands where the two are deeply
>>>>> tangled and the split won't be perfect.
>>>>>
>>>>> However, the two cases you mention above are solvable:
>>>>>
>>>>>  1. suspend/resume: this is *really* a HMP feature and shouldn't be in any
>>>>>     QMP code path. This is correctly addressed in this series by moving it
>>>>>     to hmp_migrate()
>>>>
>>>> Almost correctly. ;)
>>>
>>> Well, it was moved to the right place :)
>>
>> (see the other thread)
> 
> Yeah, I saw it and will fix the problems you've pointed out.
> 
>>>>>  2. file descriptor passing: the new QMP server will support sessions and
>>>>>     we'll move statefull commands (like getfd) to it. When we do it, we'll
>>>>>     introduce a new API to get fds that won't depend on the monitor. However,
>>>>>     this requires all commands to be converted to the qapi first. Meanwhile
>>>>>     we use the qemu_get_fd() API.
>>>>>
>>>>>     Note: qemu_get_fd() is temporary, it shouldn't be a problem to use it
>>>>>     (if it's not incorrect, of course, I honestly haven't fully tested it yet).
>>>>
>>>> So there will be a common super-class of Monitor and that new QMP
>>>> session that also manages the file descriptors? That would make sense.
>>>
>>> Oh, yes. Now I see that you said exactly that earlier. Sorry for more or less
>>> re-stating it.
>>>
>>>> Still, there would be monitor_get_fd and qmp_get_fd then not
>>>> qemu_get_fd. I think that should be done already.
>>>
>>> The problem is that monitor_get_fd() already exists and qmp_get_fd()
>>> doesn't make much sense (as this is not related to QMP right now). So,
>>> I could call it monitor_get_fd_cur() or something like this.
>>
>> What object represent a QMP session now? 
> 
> We don't exactly have the notion of a QMP session today, but all QMP state
> is currently stored in the Monitor object.
> 
>> That object will once hold the
>> reference to the FDs. So some qmp_get_fd will take that session and
>> return the requested fd - so, it does make sense, long-term at least.
> 
> Yes. Actually most of the code has already been written by Anthony:
> 
>  git://repo.or.cz/qemu/aliguori.git glib
> 
>  (look at qmp-core.c)
> 
> What I'm doing is to rebase it, do some integration work & fix ups.
> 
>> In any case, as long as everyone can mess with cur_mon, you don't need
>> to introduce wrappers that just link a normal monitor service with that
>> variable.
> 
> So, you're suggestion to just use monitor_get_fd(), right?

As qemu_get_fd is obviously not to stay, yes. That reduces the risk of
proliferation (or pattern replication like qemu_monitor_suspend/resume).

Jan

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