[Qemu-devel] [PATCH 0/3] Remove old MigrationParams

[Qemu-devel] [PATCH 0/3] Remove old MigrationParams

[Juan Quintela]

Hi

Upon a time there were MigrationParms (only used for block migration)
and then MigrationParams used for everything else.  This series:

- create migration capabilities for block parameters
- make the migrate command line parameters to use capabilities
- remove MigrationParams completely

Please, review.


Juan Quintela (3):
  migration: Create block capabilities for shared and enable
  migration: Remove use of old MigrationParams
  migration: Remove old MigrationParams

 include/migration/migration.h | 14 +++++-------
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h       |  1 -
 include/sysemu/sysemu.h       |  3 +--
 migration/block.c             | 17 ++------------
 migration/colo.c              |  5 +----
 migration/migration.c         | 52 ++++++++++++++++++++++++++++++++++++-------
 migration/savevm.c            | 18 +++------------
 qapi-schema.json              |  7 +++++-
 9 files changed, 62 insertions(+), 56 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Those two capabilities were added through the command line.  Notice that
we just created them.  This is just the boilerplate.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/migration/migration.h |  3 +++
 migration/migration.c         | 36 ++++++++++++++++++++++++++++++++++++
 qapi-schema.json              |  7 ++++++-
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index dfeca38..618ab0e 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -307,6 +307,9 @@ bool migrate_colo_enabled(void);
 
 int64_t xbzrle_cache_resize(int64_t new_size);
 
+bool migrate_use_block_enabled(void);
+bool migrate_use_block_shared(void);
+
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
diff --git a/migration/migration.c b/migration/migration.c
index 5447cab..775b24c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1195,6 +1195,16 @@ bool migration_is_blocked(Error **errp)
     return false;
 }
 
+static void migrate_set_block_shared(MigrationState *s)
+{
+    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = true;
+}
+
+static void migrate_set_block_enabled(MigrationState *s)
+{
+    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
+}
+
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  Error **errp)
@@ -1224,6 +1234,14 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 
     s = migrate_init(&params);
 
+    if (has_blk && blk) {
+        migrate_set_block_enabled(s);
+    }
+
+    if (has_inc && inc) {
+        migrate_set_block_shared(s);
+    }
+
     if (strstart(uri, "tcp:", &p)) {
         tcp_start_outgoing_migration(s, p, &local_err);
 #ifdef CONFIG_RDMA
@@ -1419,6 +1437,24 @@ int64_t migrate_xbzrle_cache_size(void)
     return s->xbzrle_cache_size;
 }
 
+bool migrate_use_block_enabled(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED];
+}
+
+bool migrate_use_block_shared(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED];
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/qapi-schema.json b/qapi-schema.json
index 01b087f..e963bb3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -894,11 +894,16 @@
 # @release-ram: if enabled, qemu will free the migrated ram pages on the source
 #        during postcopy-ram migration. (since 2.9)
 #
+# @block-enabled: enable block migration (Since 2.10)
+#
+# @block-shared: enable block shared migration (Since 2.10)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
+           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
+           'block-enabled', 'block-shared' ] }
 
 ##
 # @MigrationCapabilityStatus:
-- 
2.9.3

[Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

We have change in the previous patch to use migration capabilities for
it.  Notice that we continue using the old command line flags from
migrate command from the time being.  Remove the set_params method as
now it is empty.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/migration.h |  3 +--
 migration/block.c             | 17 ++---------------
 migration/colo.c              |  3 ---
 migration/migration.c         |  8 +++++---
 migration/savevm.c            |  2 --
 5 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 618ab0e..2917baa 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -42,8 +42,7 @@
 extern int only_migratable;
 
 struct MigrationParams {
-    bool blk;
-    bool shared;
+    bool unused; /* C don't allow empty structs */
 };
 
 /* Messages sent on the return path from destination to source */
diff --git a/migration/block.c b/migration/block.c
index 060087f..fcfa823 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -94,9 +94,6 @@ typedef struct BlkMigBlock {
 } BlkMigBlock;
 
 typedef struct BlkMigState {
-    /* Written during setup phase.  Can be read without a lock.  */
-    int blk_enable;
-    int shared_base;
     QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
     int64_t total_sector_sum;
     bool zero_blocks;
@@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f)
         bmds->bulk_completed = 0;
         bmds->total_sectors = sectors;
         bmds->completed_sectors = 0;
-        bmds->shared_base = block_mig_state.shared_base;
+        bmds->shared_base = migrate_use_block_shared();
 
         assert(i < num_bs);
         bmds_bs[i].bmds = bmds;
@@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static void block_set_params(const MigrationParams *params, void *opaque)
-{
-    block_mig_state.blk_enable = params->blk;
-    block_mig_state.shared_base = params->shared;
-
-    /* shared base means that blk_enable = 1 */
-    block_mig_state.blk_enable |= params->shared;
-}
-
 static bool block_is_active(void *opaque)
 {
-    return block_mig_state.blk_enable == 1;
+    return migrate_use_block_enabled();
 }
 
 static SaveVMHandlers savevm_block_handlers = {
-    .set_params = block_set_params,
     .save_live_setup = block_save_setup,
     .save_live_iterate = block_save_iterate,
     .save_live_complete_precopy = block_save_complete,
diff --git a/migration/colo.c b/migration/colo.c
index c19eb3f..5c6c2f0 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -332,9 +332,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
         goto out;
     }
 
-    /* Disable block migration */
-    s->params.blk = 0;
-    s->params.shared = 0;
     qemu_savevm_state_header(fb);
     qemu_savevm_state_begin(fb, &s->params);
     qemu_mutex_lock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index 775b24c..9b96f1a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -787,6 +787,10 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
         s->enabled_capabilities[cap->value->capability] = cap->value->state;
     }
 
+    if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
+        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
+    }
+
     if (migrate_postcopy_ram()) {
         if (migrate_use_compression()) {
             /* The decompression threads asynchronously write into RAM
@@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationParams params;
     const char *p;
 
-    params.blk = has_blk && blk;
-    params.shared = has_inc && inc;
-
     if (migration_is_setup_or_active(s->state) ||
         s->state == MIGRATION_STATUS_CANCELLING ||
         s->state == MIGRATION_STATUS_COLO) {
@@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     }
 
     if (has_inc && inc) {
+        migrate_set_block_enabled(s);
         migrate_set_block_shared(s);
     }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 0c01988..102b11d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
     MigrationParams params = {
-        .blk = 0,
-        .shared = 0
     };
     MigrationState *ms = migrate_init(&params);
     MigrationStatus status;
-- 
2.9.3

[Qemu-devel] [PATCH 3/3] migration: Remove old MigrationParams

[Juan Quintela]

Not used anymore after moving block migration to use capabilities.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/migration.h | 10 ++--------
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h       |  1 -
 include/sysemu/sysemu.h       |  3 +--
 migration/colo.c              |  2 +-
 migration/migration.c         |  8 +++-----
 migration/savevm.c            | 16 +++-------------
 7 files changed, 10 insertions(+), 31 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 2917baa..3495162 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -41,10 +41,6 @@
 /* for vl.c */
 extern int only_migratable;
 
-struct MigrationParams {
-    bool unused; /* C don't allow empty structs */
-};
-
 /* Messages sent on the return path from destination to source */
 enum mig_rp_message_type {
     MIG_RP_MSG_INVALID = 0,  /* Must be 0 */
@@ -134,12 +130,10 @@ struct MigrationState
     QEMUBH *cleanup_bh;
     QEMUFile *to_dst_file;
 
-    /* New style params from 'migrate-set-parameters' */
+    /* params from 'migrate-set-parameters' */
     MigrationParameters parameters;
 
     int state;
-    /* Old style params from 'migrate' command */
-    MigrationParams params;
 
     /* State related to return path */
     struct {
@@ -229,7 +223,7 @@ void migrate_fd_connect(MigrationState *s);
 
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
-MigrationState *migrate_init(const MigrationParams *params);
+MigrationState *migrate_init(void);
 bool migration_is_blocked(Error **errp);
 bool migration_in_setup(MigrationState *);
 bool migration_is_idle(void);
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9452dec..4396d7e 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -37,7 +37,6 @@ typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
 
 typedef struct SaveVMHandlers {
     /* This runs inside the iothread lock.  */
-    void (*set_params)(const MigrationParams *params, void * opaque);
     SaveStateHandler *save_state;
 
     void (*cleanup)(void *opaque);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index f08d327..a388243 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -49,7 +49,6 @@ typedef struct MemoryRegion MemoryRegion;
 typedef struct MemoryRegionCache MemoryRegionCache;
 typedef struct MemoryRegionSection MemoryRegionSection;
 typedef struct MigrationIncomingState MigrationIncomingState;
-typedef struct MigrationParams MigrationParams;
 typedef struct MigrationState MigrationState;
 typedef struct Monitor Monitor;
 typedef struct MonitorDef MonitorDef;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 058d5eb..3340202 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -102,8 +102,7 @@ enum qemu_vm_cmd {
 #define MAX_VM_CMD_PACKAGED_SIZE (1ul << 24)
 
 bool qemu_savevm_state_blocked(Error **errp);
-void qemu_savevm_state_begin(QEMUFile *f,
-                             const MigrationParams *params);
+void qemu_savevm_state_begin(QEMUFile *f);
 void qemu_savevm_state_header(QEMUFile *f);
 int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
 void qemu_savevm_state_cleanup(void);
diff --git a/migration/colo.c b/migration/colo.c
index 5c6c2f0..75e8807 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -333,7 +333,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     qemu_savevm_state_header(fb);
-    qemu_savevm_state_begin(fb, &s->params);
+    qemu_savevm_state_begin(fb);
     qemu_mutex_lock_iothread();
     qemu_savevm_state_complete_precopy(fb, false);
     qemu_mutex_unlock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index 9b96f1a..f094079 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1102,7 +1102,7 @@ bool migration_is_idle(void)
     return false;
 }
 
-MigrationState *migrate_init(const MigrationParams *params)
+MigrationState *migrate_init(void)
 {
     MigrationState *s = migrate_get_current();
 
@@ -1116,7 +1116,6 @@ MigrationState *migrate_init(const MigrationParams *params)
     s->cleanup_bh = 0;
     s->to_dst_file = NULL;
     s->state = MIGRATION_STATUS_NONE;
-    s->params = *params;
     s->rp_state.from_dst_file = NULL;
     s->rp_state.error = false;
     s->mbps = 0.0;
@@ -1215,7 +1214,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    MigrationParams params;
     const char *p;
 
     if (migration_is_setup_or_active(s->state) ||
@@ -1233,7 +1231,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         return;
     }
 
-    s = migrate_init(&params);
+    s = migrate_init();
 
     if (has_blk && blk) {
         migrate_set_block_enabled(s);
@@ -1966,7 +1964,7 @@ static void *migration_thread(void *opaque)
         qemu_savevm_send_postcopy_advise(s->to_dst_file);
     }
 
-    qemu_savevm_state_begin(s->to_dst_file, &s->params);
+    qemu_savevm_state_begin(s->to_dst_file);
 
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
     migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
diff --git a/migration/savevm.c b/migration/savevm.c
index 102b11d..97c6908 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -966,21 +966,13 @@ void qemu_savevm_state_header(QEMUFile *f)
 
 }
 
-void qemu_savevm_state_begin(QEMUFile *f,
-                             const MigrationParams *params)
+void qemu_savevm_state_begin(QEMUFile *f)
 {
     SaveStateEntry *se;
     int ret;
 
     trace_savevm_state_begin();
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
-        if (!se->ops || !se->ops->set_params) {
-            continue;
-        }
-        se->ops->set_params(params, se->opaque);
-    }
-
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (!se->ops || !se->ops->save_live_setup) {
             continue;
         }
@@ -1232,9 +1224,7 @@ void qemu_savevm_state_cleanup(void)
 static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
-    MigrationParams params = {
-    };
-    MigrationState *ms = migrate_init(&params);
+    MigrationState *ms = migrate_init();
     MigrationStatus status;
     ms->to_dst_file = f;
 
@@ -1245,7 +1235,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 
     qemu_mutex_unlock_iothread();
     qemu_savevm_state_header(f);
-    qemu_savevm_state_begin(f, &params);
+    qemu_savevm_state_begin(f);
     qemu_mutex_lock_iothread();
 
     while (qemu_file_get_error(f) == 0) {
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> We have change in the previous patch to use migration capabilities for
> it.  Notice that we continue using the old command line flags from
> migrate command from the time being.  Remove the set_params method as
> now it is empty.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  include/migration/migration.h |  3 +--
>  migration/block.c             | 17 ++---------------
>  migration/colo.c              |  3 ---
>  migration/migration.c         |  8 +++++---
>  migration/savevm.c            |  2 --
>  5 files changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index c19eb3f..5c6c2f0 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -332,9 +332,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>          goto out;
>      }
>  
> -    /* Disable block migration */
> -    s->params.blk = 0;
> -    s->params.shared = 0;

Hmm you don't seem to have replaced this with anything.
I think that's a behavioural change; the trick COLO did (I'm not sure if this
is still the way it works) is that they initiate the first migration
with block migration enabled so that the two hosts (with non-shared storage)
get sync'd storage, and then at the completion of that first migration
they then switch into the checkpointing mode where they're only
doing updates - that's why it gets switched off at this point
prior to the 1st checkpoint.

Dave

>      qemu_savevm_state_header(fb);
>      qemu_savevm_state_begin(fb, &s->params);
>      qemu_mutex_lock_iothread();
> diff --git a/migration/migration.c b/migration/migration.c
> index 775b24c..9b96f1a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -787,6 +787,10 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>          s->enabled_capabilities[cap->value->capability] = cap->value->state;
>      }
>  
> +    if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
> +        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
> +    }
> +
>      if (migrate_postcopy_ram()) {
>          if (migrate_use_compression()) {
>              /* The decompression threads asynchronously write into RAM
> @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationParams params;
>      const char *p;
>  
> -    params.blk = has_blk && blk;
> -    params.shared = has_inc && inc;
> -
>      if (migration_is_setup_or_active(s->state) ||
>          s->state == MIGRATION_STATUS_CANCELLING ||
>          s->state == MIGRATION_STATUS_COLO) {
> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      }
>  
>      if (has_inc && inc) {
> +        migrate_set_block_enabled(s);
>          migrate_set_block_shared(s);
>      }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0c01988..102b11d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  {
>      int ret;
>      MigrationParams params = {
> -        .blk = 0,
> -        .shared = 0
>      };
>      MigrationState *ms = migrate_init(&params);
>      MigrationStatus status;
> -- 
> 2.9.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/3] migration: Remove old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Not used anymore after moving block migration to use capabilities.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  include/migration/migration.h | 10 ++--------
>  include/migration/vmstate.h   |  1 -
>  include/qemu/typedefs.h       |  1 -
>  include/sysemu/sysemu.h       |  3 +--
>  migration/colo.c              |  2 +-
>  migration/migration.c         |  8 +++-----
>  migration/savevm.c            | 16 +++-------------
>  7 files changed, 10 insertions(+), 31 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 2917baa..3495162 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -41,10 +41,6 @@
>  /* for vl.c */
>  extern int only_migratable;
>  
> -struct MigrationParams {
> -    bool unused; /* C don't allow empty structs */
> -};
> -
>  /* Messages sent on the return path from destination to source */
>  enum mig_rp_message_type {
>      MIG_RP_MSG_INVALID = 0,  /* Must be 0 */
> @@ -134,12 +130,10 @@ struct MigrationState
>      QEMUBH *cleanup_bh;
>      QEMUFile *to_dst_file;
>  
> -    /* New style params from 'migrate-set-parameters' */
> +    /* params from 'migrate-set-parameters' */
>      MigrationParameters parameters;
>  
>      int state;
> -    /* Old style params from 'migrate' command */
> -    MigrationParams params;
>  
>      /* State related to return path */
>      struct {
> @@ -229,7 +223,7 @@ void migrate_fd_connect(MigrationState *s);
>  
>  void add_migration_state_change_notifier(Notifier *notify);
>  void remove_migration_state_change_notifier(Notifier *notify);
> -MigrationState *migrate_init(const MigrationParams *params);
> +MigrationState *migrate_init(void);
>  bool migration_is_blocked(Error **errp);
>  bool migration_in_setup(MigrationState *);
>  bool migration_is_idle(void);
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 9452dec..4396d7e 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -37,7 +37,6 @@ typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
>  
>  typedef struct SaveVMHandlers {
>      /* This runs inside the iothread lock.  */
> -    void (*set_params)(const MigrationParams *params, void * opaque);
>      SaveStateHandler *save_state;
>  
>      void (*cleanup)(void *opaque);
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index f08d327..a388243 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -49,7 +49,6 @@ typedef struct MemoryRegion MemoryRegion;
>  typedef struct MemoryRegionCache MemoryRegionCache;
>  typedef struct MemoryRegionSection MemoryRegionSection;
>  typedef struct MigrationIncomingState MigrationIncomingState;
> -typedef struct MigrationParams MigrationParams;
>  typedef struct MigrationState MigrationState;
>  typedef struct Monitor Monitor;
>  typedef struct MonitorDef MonitorDef;
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 058d5eb..3340202 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -102,8 +102,7 @@ enum qemu_vm_cmd {
>  #define MAX_VM_CMD_PACKAGED_SIZE (1ul << 24)
>  
>  bool qemu_savevm_state_blocked(Error **errp);
> -void qemu_savevm_state_begin(QEMUFile *f,
> -                             const MigrationParams *params);
> +void qemu_savevm_state_begin(QEMUFile *f);
>  void qemu_savevm_state_header(QEMUFile *f);
>  int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
>  void qemu_savevm_state_cleanup(void);
> diff --git a/migration/colo.c b/migration/colo.c
> index 5c6c2f0..75e8807 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -333,7 +333,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>      }
>  
>      qemu_savevm_state_header(fb);
> -    qemu_savevm_state_begin(fb, &s->params);
> +    qemu_savevm_state_begin(fb);
>      qemu_mutex_lock_iothread();
>      qemu_savevm_state_complete_precopy(fb, false);
>      qemu_mutex_unlock_iothread();
> diff --git a/migration/migration.c b/migration/migration.c
> index 9b96f1a..f094079 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1102,7 +1102,7 @@ bool migration_is_idle(void)
>      return false;
>  }
>  
> -MigrationState *migrate_init(const MigrationParams *params)
> +MigrationState *migrate_init(void)
>  {
>      MigrationState *s = migrate_get_current();
>  
> @@ -1116,7 +1116,6 @@ MigrationState *migrate_init(const MigrationParams *params)
>      s->cleanup_bh = 0;
>      s->to_dst_file = NULL;
>      s->state = MIGRATION_STATUS_NONE;
> -    s->params = *params;
>      s->rp_state.from_dst_file = NULL;
>      s->rp_state.error = false;
>      s->mbps = 0.0;
> @@ -1215,7 +1214,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>  {
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
> -    MigrationParams params;
>      const char *p;
>  
>      if (migration_is_setup_or_active(s->state) ||
> @@ -1233,7 +1231,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          return;
>      }
>  
> -    s = migrate_init(&params);
> +    s = migrate_init();
>  
>      if (has_blk && blk) {
>          migrate_set_block_enabled(s);
> @@ -1966,7 +1964,7 @@ static void *migration_thread(void *opaque)
>          qemu_savevm_send_postcopy_advise(s->to_dst_file);
>      }
>  
> -    qemu_savevm_state_begin(s->to_dst_file, &s->params);
> +    qemu_savevm_state_begin(s->to_dst_file);
>  
>      s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
>      migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 102b11d..97c6908 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -966,21 +966,13 @@ void qemu_savevm_state_header(QEMUFile *f)
>  
>  }
>  
> -void qemu_savevm_state_begin(QEMUFile *f,
> -                             const MigrationParams *params)
> +void qemu_savevm_state_begin(QEMUFile *f)
>  {
>      SaveStateEntry *se;
>      int ret;
>  
>      trace_savevm_state_begin();
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> -        if (!se->ops || !se->ops->set_params) {
> -            continue;
> -        }
> -        se->ops->set_params(params, se->opaque);
> -    }
> -
> -    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (!se->ops || !se->ops->save_live_setup) {
>              continue;
>          }
> @@ -1232,9 +1224,7 @@ void qemu_savevm_state_cleanup(void)
>  static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  {
>      int ret;
> -    MigrationParams params = {
> -    };
> -    MigrationState *ms = migrate_init(&params);
> +    MigrationState *ms = migrate_init();
>      MigrationStatus status;
>      ms->to_dst_file = f;
>  
> @@ -1245,7 +1235,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  
>      qemu_mutex_unlock_iothread();
>      qemu_savevm_state_header(f);
> -    qemu_savevm_state_begin(f, &params);
> +    qemu_savevm_state_begin(f);
>      qemu_mutex_lock_iothread();
>  
>      while (qemu_file_get_error(f) == 0) {
> -- 
> 2.9.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Eric Blake]

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

On 04/25/2017 05:30 AM, Juan Quintela wrote:
> We have change in the previous patch to use migration capabilities for
> it.  Notice that we continue using the old command line flags from
> migrate command from the time being.  Remove the set_params method as
> now it is empty.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  include/migration/migration.h |  3 +--
>  migration/block.c             | 17 ++---------------
>  migration/colo.c              |  3 ---
>  migration/migration.c         |  8 +++++---
>  migration/savevm.c            |  2 --
>  5 files changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 618ab0e..2917baa 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -42,8 +42,7 @@
>  extern int only_migratable;
>  
>  struct MigrationParams {
> -    bool blk;
> -    bool shared;
> +    bool unused; /* C don't allow empty structs */

s/don't/doesn't/

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> We have change in the previous patch to use migration capabilities for
>> it.  Notice that we continue using the old command line flags from
>> migrate command from the time being.  Remove the set_params method as
>> now it is empty.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  include/migration/migration.h |  3 +--
>>  migration/block.c             | 17 ++---------------
>>  migration/colo.c              |  3 ---
>>  migration/migration.c         |  8 +++++---
>>  migration/savevm.c            |  2 --
>>  5 files changed, 8 insertions(+), 25 deletions(-)
>> 
>> diff --git a/migration/colo.c b/migration/colo.c
>> index c19eb3f..5c6c2f0 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -332,9 +332,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>>          goto out;
>>      }
>>  
>> -    /* Disable block migration */
>> -    s->params.blk = 0;
>> -    s->params.shared = 0;
>
> Hmm you don't seem to have replaced this with anything.
> I think that's a behavioural change; the trick COLO did (I'm not sure if this
> is still the way it works) is that they initiate the first migration
> with block migration enabled so that the two hosts (with non-shared storage)
> get sync'd storage, and then at the completion of that first migration
> they then switch into the checkpointing mode where they're only
> doing updates - that's why it gets switched off at this point
> prior to the 1st checkpoint.


Weird, really.

I did't catch that.

Will investigate.

Thanks.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Hailiang Zhang]

Hi,

On 2017/5/4 16:51, Juan Quintela wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>> * Juan Quintela (quintela@redhat.com) wrote:
>>> We have change in the previous patch to use migration capabilities for
>>> it.  Notice that we continue using the old command line flags from
>>> migrate command from the time being.  Remove the set_params method as
>>> now it is empty.
>>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>   include/migration/migration.h |  3 +--
>>>   migration/block.c             | 17 ++---------------
>>>   migration/colo.c              |  3 ---
>>>   migration/migration.c         |  8 +++++---
>>>   migration/savevm.c            |  2 --
>>>   5 files changed, 8 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/migration/colo.c b/migration/colo.c
>>> index c19eb3f..5c6c2f0 100644
>>> --- a/migration/colo.c
>>> +++ b/migration/colo.c
>>> @@ -332,9 +332,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>>>           goto out;
>>>       }
>>>   
>>> -    /* Disable block migration */
>>> -    s->params.blk = 0;
>>> -    s->params.shared = 0;
>> Hmm you don't seem to have replaced this with anything.
>> I think that's a behavioural change; the trick COLO did (I'm not sure if this
>> is still the way it works) is that they initiate the first migration
>> with block migration enabled so that the two hosts (with non-shared storage)
>> get sync'd storage, and then at the completion of that first migration
>> they then switch into the checkpointing mode where they're only
>> doing updates - that's why it gets switched off at this point
>> prior to the 1st checkpoint.
>
> Weird, really.
>
> I did't catch that.
>
> Will investigate.

Yes, Dave is right, for non-shared disk, we need to enable block migration for first cycle,
to sync the disks of two sides. After that, qemu will go into COLO state which we need to
disable block migration.

Thanks,
Hailiang


> Thanks.
>
> .
>

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Hailiang Zhang <zhang.zhanghailiang@huawei.com> wrote:
> Hi,
>
>>> Hmm you don't seem to have replaced this with anything.
>>> I think that's a behavioural change; the trick COLO did (I'm not sure if this
>>> is still the way it works) is that they initiate the first migration
>>> with block migration enabled so that the two hosts (with non-shared storage)
>>> get sync'd storage, and then at the completion of that first migration
>>> they then switch into the checkpointing mode where they're only
>>> doing updates - that's why it gets switched off at this point
>>> prior to the 1st checkpoint.
>>
>> Weird, really.
>>
>> I did't catch that.
>>
>> Will investigate.
>
> Yes, Dave is right, for non-shared disk, we need to enable block
> migration for first cycle,
> to sync the disks of two sides. After that, qemu will go into COLO
> state which we need to
> disable block migration.

v2 posted.

My understanding is that it maintains the sematic, please test/comment.

Thanks, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Hailiang Zhang]

On 2017/5/12 0:33, Juan Quintela wrote:
> Hailiang Zhang <zhang.zhanghailiang@huawei.com> wrote:
>> Hi,
>>
>>>> Hmm you don't seem to have replaced this with anything.
>>>> I think that's a behavioural change; the trick COLO did (I'm not sure if this
>>>> is still the way it works) is that they initiate the first migration
>>>> with block migration enabled so that the two hosts (with non-shared storage)
>>>> get sync'd storage, and then at the completion of that first migration
>>>> they then switch into the checkpointing mode where they're only
>>>> doing updates - that's why it gets switched off at this point
>>>> prior to the 1st checkpoint.
>>> Weird, really.
>>>
>>> I did't catch that.
>>>
>>> Will investigate.
>> Yes, Dave is right, for non-shared disk, we need to enable block
>> migration for first cycle,
>> to sync the disks of two sides. After that, qemu will go into COLO
>> state which we need to
>> disable block migration.
> v2 posted.
>
> My understanding is that it maintains the sematic, please test/comment.

Yes, it is right now, i have reviewed it, thanks.

> Thanks, Juan.
>
> .
>

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:

...

>> As qmp command is asynchronous, you can think that -d is *always* on in
>> QMP O:-)
>
> Yes.  The existence of "detach" in QMP is owed to limitations of early
> QMP infrastructure.  It's flagged as "invalid" and "should not be
> used" since 2010.
>
> Perhaps we should start a section on QMP in
> <http://wiki.qemu.org/Features/LegacyRemoval>.  But I'd like to first
> have a way to communicate "you're using a deprecated feature" warnings
> via QMP.

+1

>> Tristates will complicate it.  I still think that:
>>
>> - capability: block_migration
>> - parameter: block_shared
>>
>> Makes more sense, no?
>>
>> If block_migration is not enabled, we ignore the shared parameter.  We
>> already do that for other parameters.
>
> My impression as a superficial reader is that migration configuration is
> a historically grown mess.  Perhaps we shouldn't try to interpret too
> much intent into it :)
>
> If we redo migration as an instance of the "job" abstraction once we
> have it, then migration configuration & control should become more less
> messy.  Of course, the old messes will stay with us for a while in the
> form of backward compatibility messes.
>
> I'm not too particular on how we do the tri-state now, as long as it
> reasonably fits what we have, and is documented clearly.

>>> If the new interface isn't used, the old one still needs to work.  If it
>>> is used, the old one either has to do "the right thing", or fail
>>> cleanly.
>>>
>>> We approximate "new interface isn't used" by "block migration is off in
>>> global state".  When it is off, the migration command needs to honor its
>>> two flags for compatibility.  It must leave block migration off in
>>> global state.  Yes, this will complicate the implementation until we
>>> actually remove the deprecated flags.  Par for the backward compatility
>>> course.
>>>
>>> When block migration isn't off in global state, we can either
>>>
>>> * let the flags take precedence over the global state (one
>>>   interpretation of "do the right thing"), or
>>>
>>> * reject flags that conflict with global state (another interpretation),
>>>   or
>>>
>>> * reject *all* flags (fail cleanly).
>>>
>>> The last one looks perfectly servicable to me.
>>
>> Yeap,  I think that makes sense.  If you use capabilities, parameters,
>> old interface don't work at all.
>>
>> We still have a problem that is what happens if the user does:
>>
>> migrate -b <foo>
>> migrate_cancel (or error)
>> migrate <bar> (without -b)
>>
>> With current patches, it will still use -b.  Fixing it requires still
>> anding more code.  But I think that this use case is so weird what we
>> should not even care about it.
>
> It's a compatibility break.  Whether it's tolerable is a judgement call,
> and not for me to make.
>
> Compatibility breaks need documentation, including release notes.
>
> Say you run migrate with -b by accident (say by recalling a prior
> command from persistent command history, such as qmp-shell's or rlwrap's
> or socat READLINE's), immediately realize what you've done and cancel
> the migration.  Are you then stuck with -b forever?

migrate_set_capability block off

and you are done.


But I think that adding documentation would be longer that just adding
the code to clean it.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Juan Quintela <quintela@redhat.com> writes:
>>
>>> Eric Blake <eblake@redhat.com> wrote:
>
>
>>>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>>>> command to get rid of crufty parameters?
>>>
>>> I didn't read it that way, but I would not oppose O:-)
>>>
>>> Later, Juan.
>>
>> I'm not too familiar with this stuff, so please correct my
>> misunderstandings.
>>
>> "Normal" migration configuration is global state, i.e. it applies to all
>> future migrations.
>>
>> Except the "migrate" command's flags apply to just the migration kicked
>> off by that command.
>>
>> QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
>> !blk && inc makes no sense and is silently treated like !blk && !inc.
>>
>> There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
>
> As qmp command is asynchronous, you can think that -d is *always* on in
> QMP O:-)

Yes.  The existence of "detach" in QMP is owed to limitations of early
QMP infrastructure.  It's flagged as "invalid" and "should not be
used" since 2010.

Perhaps we should start a section on QMP in
<http://wiki.qemu.org/Features/LegacyRemoval>.  But I'd like to first
have a way to communicate "you're using a deprecated feature" warnings
via QMP.

>> You'd like to deprecate these flags in favour of "normal" configuration.
>> However, we need to maintain QMP backward compatibility at least for a
>> while.  HMP backward compatibility is nice to have, but not required.
>>
>> First step is to design the new interface you want.  Second step is to
>> figure out backward compatibility.
>>
>> The new interface adds a block migration tri-state (off,
>> non-incremental, incremental) to global state, default off.  Whether
>> it's done as two bools or an enum of three values doesn't matter here.
>
> Tristates will complicate it.  I still think that:
>
> - capability: block_migration
> - parameter: block_shared
>
> Makes more sense, no?
>
> If block_migration is not enabled, we ignore the shared parameter.  We
> already do that for other parameters.

My impression as a superficial reader is that migration configuration is
a historically grown mess.  Perhaps we shouldn't try to interpret too
much intent into it :)

If we redo migration as an instance of the "job" abstraction once we
have it, then migration configuration & control should become more less
messy.  Of course, the old messes will stay with us for a while in the
form of backward compatibility messes.

I'm not too particular on how we do the tri-state now, as long as it
reasonably fits what we have, and is documented clearly.

>> If the new interface isn't used, the old one still needs to work.  If it
>> is used, the old one either has to do "the right thing", or fail
>> cleanly.
>>
>> We approximate "new interface isn't used" by "block migration is off in
>> global state".  When it is off, the migration command needs to honor its
>> two flags for compatibility.  It must leave block migration off in
>> global state.  Yes, this will complicate the implementation until we
>> actually remove the deprecated flags.  Par for the backward compatility
>> course.
>>
>> When block migration isn't off in global state, we can either
>>
>> * let the flags take precedence over the global state (one
>>   interpretation of "do the right thing"), or
>>
>> * reject flags that conflict with global state (another interpretation),
>>   or
>>
>> * reject *all* flags (fail cleanly).
>>
>> The last one looks perfectly servicable to me.
>
> Yeap,  I think that makes sense.  If you use capabilities, parameters,
> old interface don't work at all.
>
> We still have a problem that is what happens if the user does:
>
> migrate -b <foo>
> migrate_cancel (or error)
> migrate <bar> (without -b)
>
> With current patches, it will still use -b.  Fixing it requires still
> anding more code.  But I think that this use case is so weird what we
> should not even care about it.

It's a compatibility break.  Whether it's tolerable is a judgement call,
and not for me to make.

Compatibility breaks need documentation, including release notes.

Say you run migrate with -b by accident (say by recalling a prior
command from persistent command history, such as qmp-shell's or rlwrap's
or socat READLINE's), immediately realize what you've done and cancel
the migration.  Are you then stuck with -b forever?

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> >> Forget -b/-i.
> >> 
> >> migration_set_parameter compression_threads 8
> >> 
> >> migrate <foo>
> >> 
> >> We don't use compression_threads at all
> >> 
> >> migrate_set_capability compress
> >> 
> >> migrate <foo>
> >> 
> >> Now, we use compression threads.
> >> 
> >> So, compression_threads parameter is a parameter that is only used when
> >> compress capability is enabled.
> >> 
> >> Same for block migration.  Block_incremental parameter is used only when
> >> block migration capability is setup.  No dependency check needed at all.
> >> 
> >> Or I am losing something obvious here?
> >
> > Ah, you've made up a new rule
> 
> Oops, I thought that was a rule on how things worked O:-)
> 
> >- I don't think it's a bad rule
> > but is it true? Do we always enable a capability before we use a
> > parameter? I don't think so - I think the tls parameters don't have
> > a capability.
> 
> To use tls paramater, we need to use an url with tls, no?
> 
> > My previous rule was just that if it was a bool it was a capability
> > and you can have whatever dependencies you like there - or none.
> 
> Dunno.
> 
> If you think that we can document it (one way or another), I am for it.
> 
> It is really weird that we both thought (reading) the interface that the
> rules are different O:-)

Well I think that's because no one ever defined any rules!  We just
created parameters because we needed something that could take
non-boolean values, but I don't think there's anything more
about the structure of their use that's actually defined.
Similarly, I don't think there's anything defined about the
structure of capabilities.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

>> Forget -b/-i.
>> 
>> migration_set_parameter compression_threads 8
>> 
>> migrate <foo>
>> 
>> We don't use compression_threads at all
>> 
>> migrate_set_capability compress
>> 
>> migrate <foo>
>> 
>> Now, we use compression threads.
>> 
>> So, compression_threads parameter is a parameter that is only used when
>> compress capability is enabled.
>> 
>> Same for block migration.  Block_incremental parameter is used only when
>> block migration capability is setup.  No dependency check needed at all.
>> 
>> Or I am losing something obvious here?
>
> Ah, you've made up a new rule

Oops, I thought that was a rule on how things worked O:-)

>- I don't think it's a bad rule
> but is it true? Do we always enable a capability before we use a
> parameter? I don't think so - I think the tls parameters don't have
> a capability.

To use tls paramater, we need to use an url with tls, no?

> My previous rule was just that if it was a bool it was a capability
> and you can have whatever dependencies you like there - or none.

Dunno.

If you think that we can document it (one way or another), I am for it.

It is really weird that we both thought (reading) the interface that the
rules are different O:-)

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >> > * Juan Quintela (quintela@redhat.com) wrote:
> >> >> Markus Armbruster <armbru@redhat.com> wrote:
> >> >> > Juan Quintela <quintela@redhat.com> writes:
> >> >> >
> >> >> >> Eric Blake <eblake@redhat.com> wrote:
> >> >> 
> >> >> 
> >> >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
> >> >> >>> command to get rid of crufty parameters?
> >> >> >>
> >> >> >> I didn't read it that way, but I would not oppose O:-)
> >> >> >>
> >> >> >> Later, Juan.
> >> >> >
> >> >> > I'm not too familiar with this stuff, so please correct my
> >> >> > misunderstandings.
> >> >> >
> >> >> > "Normal" migration configuration is global state, i.e. it applies to all
> >> >> > future migrations.
> >> >> >
> >> >> > Except the "migrate" command's flags apply to just the migration kicked
> >> >> > off by that command.
> >> >> >
> >> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> >> >> > !blk && inc makes no sense and is silently treated like !blk && !inc.
> >> >> >
> >> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
> >> >> 
> >> >> As qmp command is asynchronous, you can think that -d is *always* on in
> >> >> QMP O:-)
> >> >> 
> >> >> > You'd like to deprecate these flags in favour of "normal" configuration.
> >> >> > However, we need to maintain QMP backward compatibility at least for a
> >> >> > while.  HMP backward compatibility is nice to have, but not required.
> >> >> >
> >> >> > First step is to design the new interface you want.  Second step is to
> >> >> > figure out backward compatibility.
> >> >> >
> >> >> > The new interface adds a block migration tri-state (off,
> >> >> > non-incremental, incremental) to global state, default off.  Whether
> >> >> > it's done as two bools or an enum of three values doesn't matter here.
> >> >> 
> >> >> Tristates will complicate it.  I still think that:
> >> >> 
> >> >> - capability: block_migration
> >> >> - parameter: block_shared
> >> >> 
> >> >> Makes more sense, no?
> >> >
> >> > I don't understand what making block_shared a parameter gives you as
> >> > opposed to simply having two capabilities.
> >> >
> >> > (And how did we get 'shared'? We started off with block & incremental)
> >> 
> >> The variables on MigrationParams:
> >> 
> >> struct MigrationParams {
> >>     bool blk;
> >>     bool shared;
> >> };
> >> 
> >> 
> >> I can move to incremental.  I am not sure which one is clearer.
> >> 
> >> The advantage of having shared as a parameter is that we forget about
> >> all this dependency bussiness.  Is the same than compression_threads
> >> paramter, you setup to whichever value that you want.  But you don't get
> >> compression_threads until you set the compress capability.
> >> 
> >> So, in this case we will have:
> >> 
> >> block capability: Are we using block migration or not
> >> block-incremental parameter: If we are using block migration, are we
> >>       using incremental copying of the block layer?
> >
> > If it's still a boolean why does having it as a parameter remove the
> > dependency?
> 
> Forget -b/-i.
> 
> migration_set_parameter compression_threads 8
> 
> migrate <foo>
> 
> We don't use compression_threads at all
> 
> migrate_set_capability compress
> 
> migrate <foo>
> 
> Now, we use compression threads.
> 
> So, compression_threads parameter is a parameter that is only used when
> compress capability is enabled.
> 
> Same for block migration.  Block_incremental parameter is used only when
> block migration capability is setup.  No dependency check needed at all.
> 
> Or I am losing something obvious here?

Ah, you've made up a new rule - I don't think it's a bad rule
but is it true? Do we always enable a capability before we use a
parameter? I don't think so - I think the tls parameters don't have
a capability.
My previous rule was just that if it was a bool it was a capability
and you can have whatever dependencies you like there - or none.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>> > * Juan Quintela (quintela@redhat.com) wrote:
>> >> Markus Armbruster <armbru@redhat.com> wrote:
>> >> > Juan Quintela <quintela@redhat.com> writes:
>> >> >
>> >> >> Eric Blake <eblake@redhat.com> wrote:
>> >> 
>> >> 
>> >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>> >> >>> command to get rid of crufty parameters?
>> >> >>
>> >> >> I didn't read it that way, but I would not oppose O:-)
>> >> >>
>> >> >> Later, Juan.
>> >> >
>> >> > I'm not too familiar with this stuff, so please correct my
>> >> > misunderstandings.
>> >> >
>> >> > "Normal" migration configuration is global state, i.e. it applies to all
>> >> > future migrations.
>> >> >
>> >> > Except the "migrate" command's flags apply to just the migration kicked
>> >> > off by that command.
>> >> >
>> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
>> >> > !blk && inc makes no sense and is silently treated like !blk && !inc.
>> >> >
>> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
>> >> 
>> >> As qmp command is asynchronous, you can think that -d is *always* on in
>> >> QMP O:-)
>> >> 
>> >> > You'd like to deprecate these flags in favour of "normal" configuration.
>> >> > However, we need to maintain QMP backward compatibility at least for a
>> >> > while.  HMP backward compatibility is nice to have, but not required.
>> >> >
>> >> > First step is to design the new interface you want.  Second step is to
>> >> > figure out backward compatibility.
>> >> >
>> >> > The new interface adds a block migration tri-state (off,
>> >> > non-incremental, incremental) to global state, default off.  Whether
>> >> > it's done as two bools or an enum of three values doesn't matter here.
>> >> 
>> >> Tristates will complicate it.  I still think that:
>> >> 
>> >> - capability: block_migration
>> >> - parameter: block_shared
>> >> 
>> >> Makes more sense, no?
>> >
>> > I don't understand what making block_shared a parameter gives you as
>> > opposed to simply having two capabilities.
>> >
>> > (And how did we get 'shared'? We started off with block & incremental)
>> 
>> The variables on MigrationParams:
>> 
>> struct MigrationParams {
>>     bool blk;
>>     bool shared;
>> };
>> 
>> 
>> I can move to incremental.  I am not sure which one is clearer.
>> 
>> The advantage of having shared as a parameter is that we forget about
>> all this dependency bussiness.  Is the same than compression_threads
>> paramter, you setup to whichever value that you want.  But you don't get
>> compression_threads until you set the compress capability.
>> 
>> So, in this case we will have:
>> 
>> block capability: Are we using block migration or not
>> block-incremental parameter: If we are using block migration, are we
>>       using incremental copying of the block layer?
>
> If it's still a boolean why does having it as a parameter remove the
> dependency?

Forget -b/-i.

migration_set_parameter compression_threads 8

migrate <foo>

We don't use compression_threads at all

migrate_set_capability compress

migrate <foo>

Now, we use compression threads.

So, compression_threads parameter is a parameter that is only used when
compress capability is enabled.

Same for block migration.  Block_incremental parameter is used only when
block migration capability is setup.  No dependency check needed at all.

Or I am losing something obvious here?

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> Markus Armbruster <armbru@redhat.com> wrote:
> >> > Juan Quintela <quintela@redhat.com> writes:
> >> >
> >> >> Eric Blake <eblake@redhat.com> wrote:
> >> 
> >> 
> >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
> >> >>> command to get rid of crufty parameters?
> >> >>
> >> >> I didn't read it that way, but I would not oppose O:-)
> >> >>
> >> >> Later, Juan.
> >> >
> >> > I'm not too familiar with this stuff, so please correct my
> >> > misunderstandings.
> >> >
> >> > "Normal" migration configuration is global state, i.e. it applies to all
> >> > future migrations.
> >> >
> >> > Except the "migrate" command's flags apply to just the migration kicked
> >> > off by that command.
> >> >
> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> >> > !blk && inc makes no sense and is silently treated like !blk && !inc.
> >> >
> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
> >> 
> >> As qmp command is asynchronous, you can think that -d is *always* on in
> >> QMP O:-)
> >> 
> >> > You'd like to deprecate these flags in favour of "normal" configuration.
> >> > However, we need to maintain QMP backward compatibility at least for a
> >> > while.  HMP backward compatibility is nice to have, but not required.
> >> >
> >> > First step is to design the new interface you want.  Second step is to
> >> > figure out backward compatibility.
> >> >
> >> > The new interface adds a block migration tri-state (off,
> >> > non-incremental, incremental) to global state, default off.  Whether
> >> > it's done as two bools or an enum of three values doesn't matter here.
> >> 
> >> Tristates will complicate it.  I still think that:
> >> 
> >> - capability: block_migration
> >> - parameter: block_shared
> >> 
> >> Makes more sense, no?
> >
> > I don't understand what making block_shared a parameter gives you as
> > opposed to simply having two capabilities.
> >
> > (And how did we get 'shared'? We started off with block & incremental)
> 
> The variables on MigrationParams:
> 
> struct MigrationParams {
>     bool blk;
>     bool shared;
> };
> 
> 
> I can move to incremental.  I am not sure which one is clearer.
> 
> The advantage of having shared as a parameter is that we forget about
> all this dependency bussiness.  Is the same than compression_threads
> paramter, you setup to whichever value that you want.  But you don't get
> compression_threads until you set the compress capability.
> 
> So, in this case we will have:
> 
> block capability: Are we using block migration or not
> block-incremental parameter: If we are using block migration, are we
>       using incremental copying of the block layer?

If it's still a boolean why does having it as a parameter remove the
dependency?

Dave

> 
> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Markus Armbruster <armbru@redhat.com> wrote:
>> > Juan Quintela <quintela@redhat.com> writes:
>> >
>> >> Eric Blake <eblake@redhat.com> wrote:
>> 
>> 
>> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>> >>> command to get rid of crufty parameters?
>> >>
>> >> I didn't read it that way, but I would not oppose O:-)
>> >>
>> >> Later, Juan.
>> >
>> > I'm not too familiar with this stuff, so please correct my
>> > misunderstandings.
>> >
>> > "Normal" migration configuration is global state, i.e. it applies to all
>> > future migrations.
>> >
>> > Except the "migrate" command's flags apply to just the migration kicked
>> > off by that command.
>> >
>> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
>> > !blk && inc makes no sense and is silently treated like !blk && !inc.
>> >
>> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
>> 
>> As qmp command is asynchronous, you can think that -d is *always* on in
>> QMP O:-)
>> 
>> > You'd like to deprecate these flags in favour of "normal" configuration.
>> > However, we need to maintain QMP backward compatibility at least for a
>> > while.  HMP backward compatibility is nice to have, but not required.
>> >
>> > First step is to design the new interface you want.  Second step is to
>> > figure out backward compatibility.
>> >
>> > The new interface adds a block migration tri-state (off,
>> > non-incremental, incremental) to global state, default off.  Whether
>> > it's done as two bools or an enum of three values doesn't matter here.
>> 
>> Tristates will complicate it.  I still think that:
>> 
>> - capability: block_migration
>> - parameter: block_shared
>> 
>> Makes more sense, no?
>
> I don't understand what making block_shared a parameter gives you as
> opposed to simply having two capabilities.
>
> (And how did we get 'shared'? We started off with block & incremental)

The variables on MigrationParams:

struct MigrationParams {
    bool blk;
    bool shared;
};


I can move to incremental.  I am not sure which one is clearer.

The advantage of having shared as a parameter is that we forget about
all this dependency bussiness.  Is the same than compression_threads
paramter, you setup to whichever value that you want.  But you don't get
compression_threads until you set the compress capability.

So, in this case we will have:

block capability: Are we using block migration or not
block-incremental parameter: If we are using block migration, are we
      using incremental copying of the block layer?


Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Markus Armbruster <armbru@redhat.com> wrote:
> > Juan Quintela <quintela@redhat.com> writes:
> >
> >> Eric Blake <eblake@redhat.com> wrote:
> 
> 
> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
> >>> command to get rid of crufty parameters?
> >>
> >> I didn't read it that way, but I would not oppose O:-)
> >>
> >> Later, Juan.
> >
> > I'm not too familiar with this stuff, so please correct my
> > misunderstandings.
> >
> > "Normal" migration configuration is global state, i.e. it applies to all
> > future migrations.
> >
> > Except the "migrate" command's flags apply to just the migration kicked
> > off by that command.
> >
> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> > !blk && inc makes no sense and is silently treated like !blk && !inc.
> >
> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
> 
> As qmp command is asynchronous, you can think that -d is *always* on in
> QMP O:-)
> 
> > You'd like to deprecate these flags in favour of "normal" configuration.
> > However, we need to maintain QMP backward compatibility at least for a
> > while.  HMP backward compatibility is nice to have, but not required.
> >
> > First step is to design the new interface you want.  Second step is to
> > figure out backward compatibility.
> >
> > The new interface adds a block migration tri-state (off,
> > non-incremental, incremental) to global state, default off.  Whether
> > it's done as two bools or an enum of three values doesn't matter here.
> 
> Tristates will complicate it.  I still think that:
> 
> - capability: block_migration
> - parameter: block_shared
> 
> Makes more sense, no?

I don't understand what making block_shared a parameter gives you as
opposed to simply having two capabilities.

(And how did we get 'shared'? We started off with block & incremental)

Dave

> If block_migration is not enabled, we ignore the shared parameter.  We
> already do that for other parameters.
> 
> > If the new interface isn't used, the old one still needs to work.  If it
> > is used, the old one either has to do "the right thing", or fail
> > cleanly.
> >
> > We approximate "new interface isn't used" by "block migration is off in
> > global state".  When it is off, the migration command needs to honor its
> > two flags for compatibility.  It must leave block migration off in
> > global state.  Yes, this will complicate the implementation until we
> > actually remove the deprecated flags.  Par for the backward compatility
> > course.
> >
> > When block migration isn't off in global state, we can either
> >
> > * let the flags take precedence over the global state (one
> >   interpretation of "do the right thing"), or
> >
> > * reject flags that conflict with global state (another interpretation),
> >   or
> >
> > * reject *all* flags (fail cleanly).
> >
> > The last one looks perfectly servicable to me.
> 
> Yeap,  I think that makes sense.  If you use capabilities, parameters,
> old interface don't work at all.
> 
> We still have a problem that is what happens if the user does:
> 
> migrate -b <foo>
> migrate_cancel (or error)
> migrate <bar> (without -b)
> 
> With current patches, it will still use -b.  Fixing it requires still
> anding more code.  But I think that this use case is so weird what we
> should not even care about it.
> 
> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Eric Blake <eblake@redhat.com> wrote:


>>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>>> command to get rid of crufty parameters?
>>
>> I didn't read it that way, but I would not oppose O:-)
>>
>> Later, Juan.
>
> I'm not too familiar with this stuff, so please correct my
> misunderstandings.
>
> "Normal" migration configuration is global state, i.e. it applies to all
> future migrations.
>
> Except the "migrate" command's flags apply to just the migration kicked
> off by that command.
>
> QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> !blk && inc makes no sense and is silently treated like !blk && !inc.
>
> There's a third flag "detach" (HMP: -d), but it does nothing in QMP.

As qmp command is asynchronous, you can think that -d is *always* on in
QMP O:-)

> You'd like to deprecate these flags in favour of "normal" configuration.
> However, we need to maintain QMP backward compatibility at least for a
> while.  HMP backward compatibility is nice to have, but not required.
>
> First step is to design the new interface you want.  Second step is to
> figure out backward compatibility.
>
> The new interface adds a block migration tri-state (off,
> non-incremental, incremental) to global state, default off.  Whether
> it's done as two bools or an enum of three values doesn't matter here.

Tristates will complicate it.  I still think that:

- capability: block_migration
- parameter: block_shared

Makes more sense, no?

If block_migration is not enabled, we ignore the shared parameter.  We
already do that for other parameters.

> If the new interface isn't used, the old one still needs to work.  If it
> is used, the old one either has to do "the right thing", or fail
> cleanly.
>
> We approximate "new interface isn't used" by "block migration is off in
> global state".  When it is off, the migration command needs to honor its
> two flags for compatibility.  It must leave block migration off in
> global state.  Yes, this will complicate the implementation until we
> actually remove the deprecated flags.  Par for the backward compatility
> course.
>
> When block migration isn't off in global state, we can either
>
> * let the flags take precedence over the global state (one
>   interpretation of "do the right thing"), or
>
> * reject flags that conflict with global state (another interpretation),
>   or
>
> * reject *all* flags (fail cleanly).
>
> The last one looks perfectly servicable to me.

Yeap,  I think that makes sense.  If you use capabilities, parameters,
old interface don't work at all.

We still have a problem that is what happens if the user does:

migrate -b <foo>
migrate_cancel (or error)
migrate <bar> (without -b)

With current patches, it will still use -b.  Fixing it requires still
anding more code.  But I think that this use case is so weird what we
should not even care about it.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Eric Blake <eblake@redhat.com> wrote:
>> On 05/12/2017 05:55 AM, Juan Quintela wrote:
>>>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>>      }
>>>>>  
>>>>>      if (has_inc && inc) {
>>>>> +        migrate_set_block_enabled(s, true);
>>>>>          migrate_set_block_shared(s, true);
>>>>
>>>> [2]
>>>>
>>>> IIUC for [1] & [2] we are solving the same problem that "shared"
>>>> depends on "enabled" bit. Would it be good to unitfy this dependency
>>>> somewhere? E.g., by changing migrate_set_block_shared() into:
>>>>
>>>> void migrate_set_block_shared(MigrationState *s, bool value)
>>>> {
>>>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>>>>     if (value) {
>>>>         migrate_set_block_enabled(s, true);
>>>>     }
>>>> }
>>> 
>>> ok with this.
>>
>> Or, as I commented on 1/3, maybe having a single property that is a
>> tri-state enum value, instead of 2 separate boolean properties, might be
>> nicer (but certainly a bit more complex to code up).
>
> If you teach me how to do the qapi/qmp part, I will do the other bits.
> I don't really care if we do it one way or the other.
>
>>> I will add once here that when we disable block enabled, we also disable
>>> shared, or just let it that way?
>>> 
>>>> Another thing to mention: after switching to the capability interface,
>>>> we'll cache the "enabled" and "shared" bits now while we don't cache
>>>> it before, right? IIUC it'll affect behavior of such sequence:
>>>>
>>>> - 1st migrate with enabled=1, shared=1, then
>>>> - 2nd migrate with enabled=0, shared=0
>>>>
>>>> Before the series, the 2nd migrate will use enabled=shared=0, but
>>>> after the series it should be using enabled=shared=1. Not sure whether
>>>> this would be a problem (or I missed anything?).
>>> 
>>> We can't be consistent with both old/new way.
>>> 
>>> Old way: we always setup the capabilities on command line (that should
>>> have been deprecated long, long ago)
>>
>> Well, the easy way out is to have the HMP migrate command (I assume
>> that's what you mean by "on command line") explicitly clear the
>> parameters if it is called without the -b/-i flag.  So the start of each
>> migration is what changes the properties, so long as you are still using
>> HMP to start the migration.  Or, on the QMP side, since 'migrate' has
>> optional 'blk' and 'inc' booleans, basically leave the settings alone if
>> the parameters were omitted, and explicitly update the property to the
>> value of those parameters if they were present.
>
> We are going to have trouble whatever way that we do it, or we start
> doing lots of strange things.
>
> Forget about qmp, we are going to assume that it is consistent with hmp.
>
> migrate_set_capabilities block_enabled on
> migrate -b .....
>
> Should migrate disable the block_enabled capability?  Give one
> warning/error?
>
> And notice that this only matter if we do a migration, we cancel/get one
> error, and then we migrate again.
>
> What I tried to do is assume that -b/-i arguments don't exist.  And if
> the user use them, we implement its behaviour with the minimal fuss
> possibly.
>
> Only way that I can think of being consistent and bug compatible will be
> to store:
> - old block_shared/enanbeld capability value
> - if we set -b/-i on the command line
>
> In migration cleanup do the right thing depending on this four
> variables.  I think that it is adding lots of complexity for very few
> improvement.
>
>
>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>> command to get rid of crufty parameters?
>
> I didn't read it that way, but I would not oppose O:-)
>
> Later, Juan.

I'm not too familiar with this stuff, so please correct my
misunderstandings.

"Normal" migration configuration is global state, i.e. it applies to all
future migrations.

Except the "migrate" command's flags apply to just the migration kicked
off by that command.

QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
!blk && inc makes no sense and is silently treated like !blk && !inc.

There's a third flag "detach" (HMP: -d), but it does nothing in QMP.

You'd like to deprecate these flags in favour of "normal" configuration.
However, we need to maintain QMP backward compatibility at least for a
while.  HMP backward compatibility is nice to have, but not required.

First step is to design the new interface you want.  Second step is to
figure out backward compatibility.

The new interface adds a block migration tri-state (off,
non-incremental, incremental) to global state, default off.  Whether
it's done as two bools or an enum of three values doesn't matter here.

If the new interface isn't used, the old one still needs to work.  If it
is used, the old one either has to do "the right thing", or fail
cleanly.

We approximate "new interface isn't used" by "block migration is off in
global state".  When it is off, the migration command needs to honor its
two flags for compatibility.  It must leave block migration off in
global state.  Yes, this will complicate the implementation until we
actually remove the deprecated flags.  Par for the backward compatility
course.

When block migration isn't off in global state, we can either

* let the flags take precedence over the global state (one
  interpretation of "do the right thing"), or

* reject flags that conflict with global state (another interpretation),
  or

* reject *all* flags (fail cleanly).

The last one looks perfectly servicable to me.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> [adding Markus]
>
> On 05/15/2017 04:48 AM, Juan Quintela wrote:
>> Eric Blake <eblake@redhat.com> wrote:
>>> On 05/12/2017 05:55 AM, Juan Quintela wrote:
>>>>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>>>      }
>>>>>>  
>>>>>>      if (has_inc && inc) {
>>>>>> +        migrate_set_block_enabled(s, true);
>>>>>>          migrate_set_block_shared(s, true);
>>>>>
>>>>> [2]
>>>>>
>>>>> IIUC for [1] & [2] we are solving the same problem that "shared"
>>>>> depends on "enabled" bit. Would it be good to unitfy this dependency
>>>>> somewhere? E.g., by changing migrate_set_block_shared() into:
>>>>>
>>>>> void migrate_set_block_shared(MigrationState *s, bool value)
>>>>> {
>>>>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>>>>>     if (value) {
>>>>>         migrate_set_block_enabled(s, true);
>>>>>     }
>>>>> }
>>>>
>>>> ok with this.
>>>
>>> Or, as I commented on 1/3, maybe having a single property that is a
>>> tri-state enum value, instead of 2 separate boolean properties, might be
>>> nicer (but certainly a bit more complex to code up).
>> 
>> If you teach me how to do the qapi/qmp part, I will do the other bits.
>> I don't really care if we do it one way or the other.
>
> Adding Markus in, as I value his opinion on matters of UI design.


This is my "working" version of Dave patch on top of my block layer
changes.  Still using the two capabilities.

I am open to suggestions


commit 242852b71ec2b8f08754afa6eafce6fa3fe69139
Author: Dr. David Alan Gilbert <dgilbert@redhat.com>
Date:   Mon May 15 15:05:29 2017 +0100

    block migration: Allow compile time disable
    
    Many users now prefer to use drive_mirror over NBD as an
    alternative to the older migrate -b option; drive_mirror is
    more complex to setup but gives you more options (e.g. only
    migrating some of the disks if some of them are shared).
    
    Allow the large chunk of block migration code to be compiled
    out for those who don't use it.
    
    Based on a downstream-patch we've had for a while by Jeff Cody.
    
    Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

diff --git a/configure b/configure
index 7c020c0..7aba6c5 100755
--- a/configure
+++ b/configure
@@ -316,6 +316,7 @@ vte=""
 virglrenderer=""
 tpm="yes"
 libssh2=""
+live_block_migration="yes"
 numa=""
 tcmalloc="no"
 jemalloc="no"
@@ -1168,6 +1169,10 @@ for opt do
   ;;
   --enable-libssh2) libssh2="yes"
   ;;
+  --disable-live-block-migration) live_block_migration="no"
+  ;;
+  --enable-live-block-migration) live_block_migration="yes"
+  ;;
   --disable-numa) numa="no"
   ;;
   --enable-numa) numa="yes"
@@ -1400,6 +1405,7 @@ disabled with --disable-FEATURE, default is enabled if available:
   libnfs          nfs support
   smartcard       smartcard support (libcacard)
   libusb          libusb (for usb passthrough)
+  live-block-migration   Block migration in the main migration stream
   usb-redir       usb network redirection support
   lzo             support of lzo compression library
   snappy          support of snappy compression library
@@ -5224,6 +5230,7 @@ echo "TPM support       $tpm"
 echo "libssh2 support   $libssh2"
 echo "TPM passthrough   $tpm_passthrough"
 echo "QOM debugging     $qom_cast_debug"
+echo "Live block migration $live_block_migration"
 echo "lzo support       $lzo"
 echo "snappy support    $snappy"
 echo "bzip2 support     $bzip2"
@@ -5790,6 +5797,10 @@ if test "$libssh2" = "yes" ; then
   echo "LIBSSH2_LIBS=$libssh2_libs" >> $config_host_mak
 fi
 
+if test "$live_block_migration" = "yes" ; then
+  echo "CONFIG_LIVE_BLOCK_MIGRATION=y" >> $config_host_mak
+fi
+
 # USB host support
 if test "$libusb" = "yes"; then
   echo "HOST_USB=libusb legacy" >> $config_host_mak
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 5c62e92..99b5795 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -22,7 +22,12 @@ void ram_mig_init(void);
 
 /* migration/block.c */
 
+#ifdef CONFIG_LIVE_BLOCK_MIGRATION
 void blk_mig_init(void);
+#else
+static inline void blk_mig_init(void) { };
+#endif
+
 
 #define SELF_ANNOUNCE_ROUNDS 5
 
diff --git a/migration/Makefile.objs b/migration/Makefile.objs
index 4277c88..1c7770d 100644
--- a/migration/Makefile.objs
+++ b/migration/Makefile.objs
@@ -9,5 +9,5 @@ common-obj-y += qjson.o
 
 common-obj-$(CONFIG_RDMA) += rdma.o
 
-common-obj-y += block.o
+common-obj-$(CONFIG_LIVE_BLOCK_MIGRATION) += block.o
 
diff --git a/migration/block.h b/migration/block.h
index a4d4974..1884fd0 100644
--- a/migration/block.h
+++ b/migration/block.h
@@ -14,12 +14,33 @@
 #ifndef MIGRATION_BLOCK_H
 #define MIGRATION_BLOCK_H
 
+#ifdef CONFIG_LIVE_BLOCK_MIGRATION
 int blk_mig_active(void);
 uint64_t blk_mig_bytes_transferred(void);
 uint64_t blk_mig_bytes_remaining(void);
 uint64_t blk_mig_bytes_total(void);
 
-void migrate_set_block_shared(MigrationState *s, bool value);
-void migrate_set_block_enabled(MigrationState *s, bool value);
+#else
+static inline int blk_mig_active(void)
+{
+    return false;
+}
+static inline uint64_t blk_mig_bytes_transferred(void)
+{
+    return 0;
+}
 
+static inline uint64_t blk_mig_bytes_remaining(void)
+{
+    return 0;
+}
+
+static inline uint64_t blk_mig_bytes_total(void)
+{
+    return 0;
+}
+#endif /* CONFIG_LIVE_BLOCK_MIGRATION */
+
+bool migrate_set_block_shared(MigrationState *s, bool value, Error **errp);
+bool migrate_set_block_enabled(MigrationState *s, bool value, Error **errp);
 #endif /* MIGRATION_BLOCK_H */
diff --git a/migration/colo.c b/migration/colo.c
index 10ee97c..4c652ba 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -348,8 +348,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     /* Disable block migration */
-    migrate_set_block_enabled(s, false);
-    migrate_set_block_shared(s, false);
+    migrate_set_block_enabled(s, false, NULL);
+    migrate_set_block_shared(s, false, NULL);
     qemu_savevm_state_header(fb);
     qemu_savevm_state_begin(fb);
     qemu_mutex_lock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index fd0b11f..a525de5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -595,11 +595,20 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
         }
         s->enabled_capabilities[cap->value->capability] = cap->value->state;
     }
-
+#ifdef CONFIG_LIVE_BLOCK_MIGRATION
     if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
         s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
     }
-
+#else
+    if ((s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED])
+        ||(s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED])) {
+        error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
+                         "block migration");
+        error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
+        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = false;
+        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = false;
+    }
+#endif
     if (migrate_postcopy_ram()) {
         if (migrate_use_compression()) {
             /* The decompression threads asynchronously write into RAM
@@ -1004,20 +1013,38 @@ bool migration_is_blocked(Error **errp)
     return false;
 }
 
-void migrate_set_block_shared(MigrationState *s, bool value)
+bool migrate_set_block_shared(MigrationState *s, bool value, Error **errp)
 {
+#ifndef CONFIG_LIVE_BLOCK_MIGRATION
+    if (value) {
+        error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
+                         "block migration");
+        error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
+        return false;
+    }
+#endif
     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
     if (value) {
         s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
     }
+    return true;
 }
 
-void migrate_set_block_enabled(MigrationState *s, bool value)
+bool migrate_set_block_enabled(MigrationState *s, bool value, Error **errp)
 {
+#ifndef CONFIG_LIVE_BLOCK_MIGRATION
+    if (value) {
+        error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
+                         "block migration");
+        error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
+        return false;
+    }
+#endif
     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = value;
     if (!value) {
         s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = false;
     }
+    return true;
 }
 
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
@@ -1046,11 +1073,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     s = migrate_init();
 
     if (has_blk && blk) {
-        migrate_set_block_enabled(s, true);
+        if(!migrate_set_block_enabled(s, true, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     if (has_inc && inc) {
-        migrate_set_block_shared(s, true);
+        if(!migrate_set_block_shared(s, true, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     if (strstart(uri, "tcp:", &p)) {

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Eric Blake]

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

[adding Markus]

On 05/15/2017 04:48 AM, Juan Quintela wrote:
> Eric Blake <eblake@redhat.com> wrote:
>> On 05/12/2017 05:55 AM, Juan Quintela wrote:
>>>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>>      }
>>>>>  
>>>>>      if (has_inc && inc) {
>>>>> +        migrate_set_block_enabled(s, true);
>>>>>          migrate_set_block_shared(s, true);
>>>>
>>>> [2]
>>>>
>>>> IIUC for [1] & [2] we are solving the same problem that "shared"
>>>> depends on "enabled" bit. Would it be good to unitfy this dependency
>>>> somewhere? E.g., by changing migrate_set_block_shared() into:
>>>>
>>>> void migrate_set_block_shared(MigrationState *s, bool value)
>>>> {
>>>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>>>>     if (value) {
>>>>         migrate_set_block_enabled(s, true);
>>>>     }
>>>> }
>>>
>>> ok with this.
>>
>> Or, as I commented on 1/3, maybe having a single property that is a
>> tri-state enum value, instead of 2 separate boolean properties, might be
>> nicer (but certainly a bit more complex to code up).
> 
> If you teach me how to do the qapi/qmp part, I will do the other bits.
> I don't really care if we do it one way or the other.

Adding Markus in, as I value his opinion on matters of UI design.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Eric Blake <eblake@redhat.com> wrote:
> > On 05/12/2017 05:55 AM, Juan Quintela wrote:
> >>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >>>>      }
> >>>>  
> >>>>      if (has_inc && inc) {
> >>>> +        migrate_set_block_enabled(s, true);
> >>>>          migrate_set_block_shared(s, true);
> >>>
> >>> [2]
> >>>
> >>> IIUC for [1] & [2] we are solving the same problem that "shared"
> >>> depends on "enabled" bit. Would it be good to unitfy this dependency
> >>> somewhere? E.g., by changing migrate_set_block_shared() into:
> >>>
> >>> void migrate_set_block_shared(MigrationState *s, bool value)
> >>> {
> >>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
> >>>     if (value) {
> >>>         migrate_set_block_enabled(s, true);
> >>>     }
> >>> }
> >> 
> >> ok with this.
> >
> > Or, as I commented on 1/3, maybe having a single property that is a
> > tri-state enum value, instead of 2 separate boolean properties, might be
> > nicer (but certainly a bit more complex to code up).
> 
> If you teach me how to do the qapi/qmp part, I will do the other bits.
> I don't really care if we do it one way or the other.
> 
> >> I will add once here that when we disable block enabled, we also disable
> >> shared, or just let it that way?
> >> 
> >>> Another thing to mention: after switching to the capability interface,
> >>> we'll cache the "enabled" and "shared" bits now while we don't cache
> >>> it before, right? IIUC it'll affect behavior of such sequence:
> >>>
> >>> - 1st migrate with enabled=1, shared=1, then
> >>> - 2nd migrate with enabled=0, shared=0
> >>>
> >>> Before the series, the 2nd migrate will use enabled=shared=0, but
> >>> after the series it should be using enabled=shared=1. Not sure whether
> >>> this would be a problem (or I missed anything?).
> >> 
> >> We can't be consistent with both old/new way.
> >> 
> >> Old way: we always setup the capabilities on command line (that should
> >> have been deprecated long, long ago)
> >
> > Well, the easy way out is to have the HMP migrate command (I assume
> > that's what you mean by "on command line") explicitly clear the
> > parameters if it is called without the -b/-i flag.  So the start of each
> > migration is what changes the properties, so long as you are still using
> > HMP to start the migration.  Or, on the QMP side, since 'migrate' has
> > optional 'blk' and 'inc' booleans, basically leave the settings alone if
> > the parameters were omitted, and explicitly update the property to the
> > value of those parameters if they were present.
> 
> We are going to have trouble whatever way that we do it, or we start
> doing lots of strange things.
> 
> Forget about qmp, we are going to assume that it is consistent with hmp.
> 
> migrate_set_capabilities block_enabled on
> migrate -b .....
> 
> Should migrate disable the block_enabled capability?  Give one
> warning/error?
> 
> And notice that this only matter if we do a migration, we cancel/get one
> error, and then we migrate again.
> 
> What I tried to do is assume that -b/-i arguments don't exist.  And if
> the user use them, we implement its behaviour with the minimal fuss
> possibly.
> 
> Only way that I can think of being consistent and bug compatible will be
> to store:
> - old block_shared/enanbeld capability value
> - if we set -b/-i on the command line
> 
> In migration cleanup do the right thing depending on this four
> variables.  I think that it is adding lots of complexity for very few
> improvement.
> 
> 
> > Or is the proposal that we are also going to simplify the QMP 'migrate'
> > command to get rid of crufty parameters?
> 
> I didn't read it that way, but I would not oppose O:-)
> 

Ewww this is messy; you end up with almost as much code as the old
flags you're trying to remove.
For HMP you could gently deprecate the flags over time and give
a warning telling people to use the capabilities; but doing that
in one go is a bit nasty, and you still have to do something
cleverer for the QMP which is similar.
 
I think I agree though that migrate, cancel, migrate should work
sensibly and it's hard to get it right.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Peter Xu]

On Fri, May 12, 2017 at 12:55:22PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Thu, May 11, 2017 at 06:32:27PM +0200, Juan Quintela wrote:
> 
> >> @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >>      MigrationParams params;
> >>      const char *p;
> >>  
> >> -    params.blk = has_blk && blk;
> >> -    params.shared = has_inc && inc;
> >> -
> >>      if (migration_is_setup_or_active(s->state) ||
> >>          s->state == MIGRATION_STATUS_CANCELLING ||
> >>          s->state == MIGRATION_STATUS_COLO) {
> >> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >>      }
> >>  
> >>      if (has_inc && inc) {
> >> +        migrate_set_block_enabled(s, true);
> >>          migrate_set_block_shared(s, true);
> >
> > [2]
> >
> > IIUC for [1] & [2] we are solving the same problem that "shared"
> > depends on "enabled" bit. Would it be good to unitfy this dependency
> > somewhere? E.g., by changing migrate_set_block_shared() into:
> >
> > void migrate_set_block_shared(MigrationState *s, bool value)
> > {
> >     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
> >     if (value) {
> >         migrate_set_block_enabled(s, true);
> >     }
> > }
> 
> ok with this.
> I will add once here that when we disable block enabled, we also disable
> shared, or just let it that way?

I should mark "nitpick" in my above comment. Any way works for me. :)

> 
> > Another thing to mention: after switching to the capability interface,
> > we'll cache the "enabled" and "shared" bits now while we don't cache
> > it before, right? IIUC it'll affect behavior of such sequence:
> >
> > - 1st migrate with enabled=1, shared=1, then
> > - 2nd migrate with enabled=0, shared=0
> >
> > Before the series, the 2nd migrate will use enabled=shared=0, but
> > after the series it should be using enabled=shared=1. Not sure whether
> > this would be a problem (or I missed anything?).
> 
> We can't be consistent with both old/new way.
> 
> Old way: we always setup the capabilities on command line (that should
> have been deprecated long, long ago)
> 
> New way: Once set, they stay set.
> 
> So, alternatives are:
> - If we are going to deprecate the old way, just let things as they are
>   on this patch (easier for me O:-)
> 
> - Always disable this features at the end of migration: Compatible with
>   old migration semantics, bet we are inconsistent with all other
>   capabilities.
> 
> - Add yet more code to only disable them when we are setting them
>   through the command line.  More code to maintain.
> 
> My idea would be to deprecate the migrate command line parameter, that
> is the reason why I did the 1st option.  I hope that in due curse, we
> would be able to remove the code.  But if anyone strongly think that any
> of the other options is better, please let me know.

I assume that sending continuous "migrate" is very rare, right (after
all, normally source VM is useless after that...)? I was just trying
to post this question up, and so... If you/Dave/others won't worry
about its compatibility, I won't either. ;)

Thanks!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/12/2017 05:55 AM, Juan Quintela wrote:
>>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>      }
>>>>  
>>>>      if (has_inc && inc) {
>>>> +        migrate_set_block_enabled(s, true);
>>>>          migrate_set_block_shared(s, true);
>>>
>>> [2]
>>>
>>> IIUC for [1] & [2] we are solving the same problem that "shared"
>>> depends on "enabled" bit. Would it be good to unitfy this dependency
>>> somewhere? E.g., by changing migrate_set_block_shared() into:
>>>
>>> void migrate_set_block_shared(MigrationState *s, bool value)
>>> {
>>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>>>     if (value) {
>>>         migrate_set_block_enabled(s, true);
>>>     }
>>> }
>> 
>> ok with this.
>
> Or, as I commented on 1/3, maybe having a single property that is a
> tri-state enum value, instead of 2 separate boolean properties, might be
> nicer (but certainly a bit more complex to code up).

If you teach me how to do the qapi/qmp part, I will do the other bits.
I don't really care if we do it one way or the other.

>> I will add once here that when we disable block enabled, we also disable
>> shared, or just let it that way?
>> 
>>> Another thing to mention: after switching to the capability interface,
>>> we'll cache the "enabled" and "shared" bits now while we don't cache
>>> it before, right? IIUC it'll affect behavior of such sequence:
>>>
>>> - 1st migrate with enabled=1, shared=1, then
>>> - 2nd migrate with enabled=0, shared=0
>>>
>>> Before the series, the 2nd migrate will use enabled=shared=0, but
>>> after the series it should be using enabled=shared=1. Not sure whether
>>> this would be a problem (or I missed anything?).
>> 
>> We can't be consistent with both old/new way.
>> 
>> Old way: we always setup the capabilities on command line (that should
>> have been deprecated long, long ago)
>
> Well, the easy way out is to have the HMP migrate command (I assume
> that's what you mean by "on command line") explicitly clear the
> parameters if it is called without the -b/-i flag.  So the start of each
> migration is what changes the properties, so long as you are still using
> HMP to start the migration.  Or, on the QMP side, since 'migrate' has
> optional 'blk' and 'inc' booleans, basically leave the settings alone if
> the parameters were omitted, and explicitly update the property to the
> value of those parameters if they were present.

We are going to have trouble whatever way that we do it, or we start
doing lots of strange things.

Forget about qmp, we are going to assume that it is consistent with hmp.

migrate_set_capabilities block_enabled on
migrate -b .....

Should migrate disable the block_enabled capability?  Give one
warning/error?

And notice that this only matter if we do a migration, we cancel/get one
error, and then we migrate again.

What I tried to do is assume that -b/-i arguments don't exist.  And if
the user use them, we implement its behaviour with the minimal fuss
possibly.

Only way that I can think of being consistent and bug compatible will be
to store:
- old block_shared/enanbeld capability value
- if we set -b/-i on the command line

In migration cleanup do the right thing depending on this four
variables.  I think that it is adding lots of complexity for very few
improvement.


> Or is the proposal that we are also going to simplify the QMP 'migrate'
> command to get rid of crufty parameters?

I didn't read it that way, but I would not oppose O:-)

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Eric Blake]

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

On 05/12/2017 05:55 AM, Juan Quintela wrote:
>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>      }
>>>  
>>>      if (has_inc && inc) {
>>> +        migrate_set_block_enabled(s, true);
>>>          migrate_set_block_shared(s, true);
>>
>> [2]
>>
>> IIUC for [1] & [2] we are solving the same problem that "shared"
>> depends on "enabled" bit. Would it be good to unitfy this dependency
>> somewhere? E.g., by changing migrate_set_block_shared() into:
>>
>> void migrate_set_block_shared(MigrationState *s, bool value)
>> {
>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>>     if (value) {
>>         migrate_set_block_enabled(s, true);
>>     }
>> }
> 
> ok with this.

Or, as I commented on 1/3, maybe having a single property that is a
tri-state enum value, instead of 2 separate boolean properties, might be
nicer (but certainly a bit more complex to code up).

> I will add once here that when we disable block enabled, we also disable
> shared, or just let it that way?
> 
>> Another thing to mention: after switching to the capability interface,
>> we'll cache the "enabled" and "shared" bits now while we don't cache
>> it before, right? IIUC it'll affect behavior of such sequence:
>>
>> - 1st migrate with enabled=1, shared=1, then
>> - 2nd migrate with enabled=0, shared=0
>>
>> Before the series, the 2nd migrate will use enabled=shared=0, but
>> after the series it should be using enabled=shared=1. Not sure whether
>> this would be a problem (or I missed anything?).
> 
> We can't be consistent with both old/new way.
> 
> Old way: we always setup the capabilities on command line (that should
> have been deprecated long, long ago)

Well, the easy way out is to have the HMP migrate command (I assume
that's what you mean by "on command line") explicitly clear the
parameters if it is called without the -b/-i flag.  So the start of each
migration is what changes the properties, so long as you are still using
HMP to start the migration.  Or, on the QMP side, since 'migrate' has
optional 'blk' and 'inc' booleans, basically leave the settings alone if
the parameters were omitted, and explicitly update the property to the
value of those parameters if they were present.

Or is the proposal that we are also going to simplify the QMP 'migrate'
command to get rid of crufty parameters?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Thu, May 11, 2017 at 06:32:27PM +0200, Juan Quintela wrote:

>> @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>      MigrationParams params;
>>      const char *p;
>>  
>> -    params.blk = has_blk && blk;
>> -    params.shared = has_inc && inc;
>> -
>>      if (migration_is_setup_or_active(s->state) ||
>>          s->state == MIGRATION_STATUS_CANCELLING ||
>>          s->state == MIGRATION_STATUS_COLO) {
>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>      }
>>  
>>      if (has_inc && inc) {
>> +        migrate_set_block_enabled(s, true);
>>          migrate_set_block_shared(s, true);
>
> [2]
>
> IIUC for [1] & [2] we are solving the same problem that "shared"
> depends on "enabled" bit. Would it be good to unitfy this dependency
> somewhere? E.g., by changing migrate_set_block_shared() into:
>
> void migrate_set_block_shared(MigrationState *s, bool value)
> {
>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>     if (value) {
>         migrate_set_block_enabled(s, true);
>     }
> }

ok with this.
I will add once here that when we disable block enabled, we also disable
shared, or just let it that way?

> Another thing to mention: after switching to the capability interface,
> we'll cache the "enabled" and "shared" bits now while we don't cache
> it before, right? IIUC it'll affect behavior of such sequence:
>
> - 1st migrate with enabled=1, shared=1, then
> - 2nd migrate with enabled=0, shared=0
>
> Before the series, the 2nd migrate will use enabled=shared=0, but
> after the series it should be using enabled=shared=1. Not sure whether
> this would be a problem (or I missed anything?).

We can't be consistent with both old/new way.

Old way: we always setup the capabilities on command line (that should
have been deprecated long, long ago)

New way: Once set, they stay set.

So, alternatives are:
- If we are going to deprecate the old way, just let things as they are
  on this patch (easier for me O:-)

- Always disable this features at the end of migration: Compatible with
  old migration semantics, bet we are inconsistent with all other
  capabilities.

- Add yet more code to only disable them when we are setting them
  through the command line.  More code to maintain.

My idea would be to deprecate the migrate command line parameter, that
is the reason why I did the 1st option.  I hope that in due curse, we
would be able to remove the code.  But if anyone strongly think that any
of the other options is better, please let me know.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Peter Xu]

On Thu, May 11, 2017 at 06:32:27PM +0200, Juan Quintela wrote:
> We have change in the previous patch to use migration capabilities for
> it.  Notice that we continue using the old command line flags from
> migrate command from the time being.  Remove the set_params method as
> now it is empty.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  include/migration/migration.h |  3 +--
>  migration/block.c             | 17 ++---------------
>  migration/colo.c              |  5 +++--
>  migration/migration.c         |  8 +++++---
>  migration/savevm.c            |  2 --
>  5 files changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 30c2913..2d5525c 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -39,8 +39,7 @@
>  #define QEMU_VM_SECTION_FOOTER       0x7e
>  
>  struct MigrationParams {
> -    bool blk;
> -    bool shared;
> +    bool unused; /* C doesn't allow empty structs */
>  };
>  
>  /* Messages sent on the return path from destination to source */
> diff --git a/migration/block.c b/migration/block.c
> index 060087f..fcfa823 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -94,9 +94,6 @@ typedef struct BlkMigBlock {
>  } BlkMigBlock;
>  
>  typedef struct BlkMigState {
> -    /* Written during setup phase.  Can be read without a lock.  */
> -    int blk_enable;
> -    int shared_base;
>      QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
>      int64_t total_sector_sum;
>      bool zero_blocks;
> @@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f)
>          bmds->bulk_completed = 0;
>          bmds->total_sectors = sectors;
>          bmds->completed_sectors = 0;
> -        bmds->shared_base = block_mig_state.shared_base;
> +        bmds->shared_base = migrate_use_block_shared();
>  
>          assert(i < num_bs);
>          bmds_bs[i].bmds = bmds;
> @@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> -static void block_set_params(const MigrationParams *params, void *opaque)
> -{
> -    block_mig_state.blk_enable = params->blk;
> -    block_mig_state.shared_base = params->shared;
> -
> -    /* shared base means that blk_enable = 1 */
> -    block_mig_state.blk_enable |= params->shared;
> -}
> -
>  static bool block_is_active(void *opaque)
>  {
> -    return block_mig_state.blk_enable == 1;
> +    return migrate_use_block_enabled();
>  }
>  
>  static SaveVMHandlers savevm_block_handlers = {
> -    .set_params = block_set_params,
>      .save_live_setup = block_save_setup,
>      .save_live_iterate = block_save_iterate,
>      .save_live_complete_precopy = block_save_complete,
> diff --git a/migration/colo.c b/migration/colo.c
> index 963c802..e772384 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -14,6 +14,7 @@
>  #include "qemu/timer.h"
>  #include "sysemu/sysemu.h"
>  #include "migration/colo.h"
> +#include "migration/block.h"
>  #include "io/channel-buffer.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
> @@ -345,8 +346,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>      }
>  
>      /* Disable block migration */
> -    s->params.blk = 0;
> -    s->params.shared = 0;
> +    migrate_set_block_enabled(s, false);
> +    migrate_set_block_shared(s, false);
>      qemu_savevm_state_header(fb);
>      qemu_savevm_state_begin(fb, &s->params);
>      qemu_mutex_lock_iothread();
> diff --git a/migration/migration.c b/migration/migration.c
> index 2f981aa..8a3bf89 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -787,6 +787,10 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>          s->enabled_capabilities[cap->value->capability] = cap->value->state;
>      }
>  
> +    if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
> +        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
> +    }
> +

[1]

>      if (migrate_postcopy_ram()) {
>          if (migrate_use_compression()) {
>              /* The decompression threads asynchronously write into RAM
> @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationParams params;
>      const char *p;
>  
> -    params.blk = has_blk && blk;
> -    params.shared = has_inc && inc;
> -
>      if (migration_is_setup_or_active(s->state) ||
>          s->state == MIGRATION_STATUS_CANCELLING ||
>          s->state == MIGRATION_STATUS_COLO) {
> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      }
>  
>      if (has_inc && inc) {
> +        migrate_set_block_enabled(s, true);
>          migrate_set_block_shared(s, true);

[2]

IIUC for [1] & [2] we are solving the same problem that "shared"
depends on "enabled" bit. Would it be good to unitfy this dependency
somewhere? E.g., by changing migrate_set_block_shared() into:

void migrate_set_block_shared(MigrationState *s, bool value)
{
    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
    if (value) {
        migrate_set_block_enabled(s, true);
    }
}

?

Another thing to mention: after switching to the capability interface,
we'll cache the "enabled" and "shared" bits now while we don't cache
it before, right? IIUC it'll affect behavior of such sequence:

- 1st migrate with enabled=1, shared=1, then
- 2nd migrate with enabled=0, shared=0

Before the series, the 2nd migrate will use enabled=shared=0, but
after the series it should be using enabled=shared=1. Not sure whether
this would be a problem (or I missed anything?).

Thanks,

>      }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f8d1e8b..221fb4b 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  {
>      int ret;
>      MigrationParams params = {
> -        .blk = 0,
> -        .shared = 0
>      };
>      MigrationState *ms = migrate_init(&params);
>      MigrationStatus status;
> -- 
> 2.9.3
> 

-- 
Peter Xu

[Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

We have change in the previous patch to use migration capabilities for
it.  Notice that we continue using the old command line flags from
migrate command from the time being.  Remove the set_params method as
now it is empty.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/migration.h |  3 +--
 migration/block.c             | 17 ++---------------
 migration/colo.c              |  5 +++--
 migration/migration.c         |  8 +++++---
 migration/savevm.c            |  2 --
 5 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 30c2913..2d5525c 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -39,8 +39,7 @@
 #define QEMU_VM_SECTION_FOOTER       0x7e
 
 struct MigrationParams {
-    bool blk;
-    bool shared;
+    bool unused; /* C doesn't allow empty structs */
 };
 
 /* Messages sent on the return path from destination to source */
diff --git a/migration/block.c b/migration/block.c
index 060087f..fcfa823 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -94,9 +94,6 @@ typedef struct BlkMigBlock {
 } BlkMigBlock;
 
 typedef struct BlkMigState {
-    /* Written during setup phase.  Can be read without a lock.  */
-    int blk_enable;
-    int shared_base;
     QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
     int64_t total_sector_sum;
     bool zero_blocks;
@@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f)
         bmds->bulk_completed = 0;
         bmds->total_sectors = sectors;
         bmds->completed_sectors = 0;
-        bmds->shared_base = block_mig_state.shared_base;
+        bmds->shared_base = migrate_use_block_shared();
 
         assert(i < num_bs);
         bmds_bs[i].bmds = bmds;
@@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static void block_set_params(const MigrationParams *params, void *opaque)
-{
-    block_mig_state.blk_enable = params->blk;
-    block_mig_state.shared_base = params->shared;
-
-    /* shared base means that blk_enable = 1 */
-    block_mig_state.blk_enable |= params->shared;
-}
-
 static bool block_is_active(void *opaque)
 {
-    return block_mig_state.blk_enable == 1;
+    return migrate_use_block_enabled();
 }
 
 static SaveVMHandlers savevm_block_handlers = {
-    .set_params = block_set_params,
     .save_live_setup = block_save_setup,
     .save_live_iterate = block_save_iterate,
     .save_live_complete_precopy = block_save_complete,
diff --git a/migration/colo.c b/migration/colo.c
index 963c802..e772384 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -14,6 +14,7 @@
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
 #include "migration/colo.h"
+#include "migration/block.h"
 #include "io/channel-buffer.h"
 #include "trace.h"
 #include "qemu/error-report.h"
@@ -345,8 +346,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     /* Disable block migration */
-    s->params.blk = 0;
-    s->params.shared = 0;
+    migrate_set_block_enabled(s, false);
+    migrate_set_block_shared(s, false);
     qemu_savevm_state_header(fb);
     qemu_savevm_state_begin(fb, &s->params);
     qemu_mutex_lock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index 2f981aa..8a3bf89 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -787,6 +787,10 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
         s->enabled_capabilities[cap->value->capability] = cap->value->state;
     }
 
+    if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
+        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
+    }
+
     if (migrate_postcopy_ram()) {
         if (migrate_use_compression()) {
             /* The decompression threads asynchronously write into RAM
@@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationParams params;
     const char *p;
 
-    params.blk = has_blk && blk;
-    params.shared = has_inc && inc;
-
     if (migration_is_setup_or_active(s->state) ||
         s->state == MIGRATION_STATUS_CANCELLING ||
         s->state == MIGRATION_STATUS_COLO) {
@@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     }
 
     if (has_inc && inc) {
+        migrate_set_block_enabled(s, true);
         migrate_set_block_shared(s, true);
     }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index f8d1e8b..221fb4b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
     MigrationParams params = {
-        .blk = 0,
-        .shared = 0
     };
     MigrationState *ms = migrate_init(&params);
     MigrationStatus status;
-- 
2.9.3