[Qemu-devel] [PATCH v2 00/11] migration: export cap/params to qdev props

[Qemu-devel] [PATCH v2 00/11] migration: export cap/params to qdev props

[Peter Xu]

v2:
- extended the series from 3 -> 11 patches
- renamed all the properties with "x-" prefix
- handled the cap/param check for these new properties (mostly patch
  4-11, but it contains lots of refactorings in general)

We have the MigrationState as QDev now (which seems crazy). Let's
continue to benefit.

This series is exporting all migration capabilities/params as global
parameters. Then we can do something like this:

  qemu -global migration.postcopy-ram=true \
       -global migration.max-bandwidth=4096

The values will be inited just like we typed these values into HMP
monitor. It'll simplify lots of migration scripts.

The changes are fairly straightforward. One tiny loss is that we still
don't support:

  -global migration.max-bandwidth=1g

...just like what we did in HMP:

  migrate_set_speed 1g

...while we need to use:

  -global migration.max-bandwidth=1073741824

However that should only be used in scripts, and that's good enough
imho.

These properties should only be used for debugging/testing purpose,
and we should not guarantee any interface compatibility for them (just
like HMP).

Please review. Thanks.

Peter Xu (11):
  qdev: provide DEFINE_PROP_INT64()
  migration: export parameters to props
  migration: export capabilities to props
  qom: call parent first on post_init()
  migration: introduce migrate_params_check()
  migration: provide migrate_params_apply()
  migration: check global params for validity
  migration: remove check against colo support
  migration: provide migrate_caps_check()
  migration: provide migrate_cap_add()
  migration: check global caps for validity

 hw/core/qdev-properties.c    |  32 +++++
 include/hw/qdev-properties.h |   3 +
 include/migration/colo.h     |   1 -
 migration/colo.c             |   5 -
 migration/migration.c        | 272 ++++++++++++++++++++++++++++++++++---------
 qom/object.c                 |   8 +-
 6 files changed, 253 insertions(+), 68 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 01/11] qdev: provide DEFINE_PROP_INT64()

[Peter Xu]

We have merely all the stuff, but this one is missing. Add it in.

Am going to use this new helper for MigrationParameters fields, since
most of them are int64_t.

CC: Markus Armbruster <armbru@redhat.com>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: Marc-André Lureau <marcandre.lureau@redhat.com>
CC: Peter Xu <peterx@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/qdev-properties.c    | 32 ++++++++++++++++++++++++++++++++
 include/hw/qdev-properties.h |  3 +++
 2 files changed, 35 insertions(+)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index dcecdf0..c1d4e54 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -404,6 +404,31 @@ static void set_uint64(Object *obj, Visitor *v, const char *name,
     visit_type_uint64(v, name, ptr, errp);
 }
 
+static void get_int64(Object *obj, Visitor *v, const char *name,
+                      void *opaque, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
+
+    visit_type_int64(v, name, ptr, errp);
+}
+
+static void set_int64(Object *obj, Visitor *v, const char *name,
+                      void *opaque, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
+
+    if (dev->realized) {
+        qdev_prop_set_after_realize(dev, name, errp);
+        return;
+    }
+
+    visit_type_int64(v, name, ptr, errp);
+}
+
 const PropertyInfo qdev_prop_uint64 = {
     .name  = "uint64",
     .get   = get_uint64,
@@ -411,6 +436,13 @@ const PropertyInfo qdev_prop_uint64 = {
     .set_default_value = set_default_value_uint,
 };
 
+const PropertyInfo qdev_prop_int64 = {
+    .name  = "int64",
+    .get   = get_int64,
+    .set   = set_int64,
+    .set_default_value = set_default_value_int,
+};
+
 /* --- string --- */
 
 static void release_string(Object *obj, const char *name, void *opaque)
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index f6692d5..30af33b 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -13,6 +13,7 @@ extern const PropertyInfo qdev_prop_uint16;
 extern const PropertyInfo qdev_prop_uint32;
 extern const PropertyInfo qdev_prop_int32;
 extern const PropertyInfo qdev_prop_uint64;
+extern const PropertyInfo qdev_prop_int64;
 extern const PropertyInfo qdev_prop_size;
 extern const PropertyInfo qdev_prop_string;
 extern const PropertyInfo qdev_prop_chr;
@@ -136,6 +137,8 @@ extern const PropertyInfo qdev_prop_link;
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_int32, int32_t)
 #define DEFINE_PROP_UINT64(_n, _s, _f, _d)                      \
     DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
+#define DEFINE_PROP_INT64(_n, _s, _f, _d)                      \
+    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_int64, int64_t)
 #define DEFINE_PROP_SIZE(_n, _s, _f, _d)                       \
     DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_size, uint64_t)
 #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 01/11] qdev: provide DEFINE_PROP_INT64()

[Marcel Apfelbaum]

On 17/07/2017 11:26, Peter Xu wrote:
> We have merely all the stuff, but this one is missing. Add it in.
> 
> Am going to use this new helper for MigrationParameters fields, since
> most of them are int64_t.
> 
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: Marc-André Lureau <marcandre.lureau@redhat.com>
> CC: Peter Xu <peterx@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/core/qdev-properties.c    | 32 ++++++++++++++++++++++++++++++++
>   include/hw/qdev-properties.h |  3 +++
>   2 files changed, 35 insertions(+)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index dcecdf0..c1d4e54 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -404,6 +404,31 @@ static void set_uint64(Object *obj, Visitor *v, const char *name,
>       visit_type_uint64(v, name, ptr, errp);
>   }
>   
> +static void get_int64(Object *obj, Visitor *v, const char *name,
> +                      void *opaque, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    visit_type_int64(v, name, ptr, errp);
> +}
> +
> +static void set_int64(Object *obj, Visitor *v, const char *name,
> +                      void *opaque, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    if (dev->realized) {
> +        qdev_prop_set_after_realize(dev, name, errp);
> +        return;
> +    }
> +
> +    visit_type_int64(v, name, ptr, errp);
> +}
> +
>   const PropertyInfo qdev_prop_uint64 = {
>       .name  = "uint64",
>       .get   = get_uint64,
> @@ -411,6 +436,13 @@ const PropertyInfo qdev_prop_uint64 = {
>       .set_default_value = set_default_value_uint,
>   };
>   
> +const PropertyInfo qdev_prop_int64 = {
> +    .name  = "int64",
> +    .get   = get_int64,
> +    .set   = set_int64,
> +    .set_default_value = set_default_value_int,
> +};
> +
>   /* --- string --- */
>   
>   static void release_string(Object *obj, const char *name, void *opaque)
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index f6692d5..30af33b 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -13,6 +13,7 @@ extern const PropertyInfo qdev_prop_uint16;
>   extern const PropertyInfo qdev_prop_uint32;
>   extern const PropertyInfo qdev_prop_int32;
>   extern const PropertyInfo qdev_prop_uint64;
> +extern const PropertyInfo qdev_prop_int64;
>   extern const PropertyInfo qdev_prop_size;
>   extern const PropertyInfo qdev_prop_string;
>   extern const PropertyInfo qdev_prop_chr;
> @@ -136,6 +137,8 @@ extern const PropertyInfo qdev_prop_link;
>       DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_int32, int32_t)
>   #define DEFINE_PROP_UINT64(_n, _s, _f, _d)                      \
>       DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
> +#define DEFINE_PROP_INT64(_n, _s, _f, _d)                      \
> +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_int64, int64_t)
>   #define DEFINE_PROP_SIZE(_n, _s, _f, _d)                       \
>       DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_size, uint64_t)
>   #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
> 


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2 01/11] qdev: provide DEFINE_PROP_INT64()

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> We have merely all the stuff, but this one is missing. Add it in.
>
> Am going to use this new helper for MigrationParameters fields, since
> most of them are int64_t.
>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: Marc-André Lureau <marcandre.lureau@redhat.com>
> CC: Peter Xu <peterx@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 01/11] qdev: provide DEFINE_PROP_INT64()

[Eric Blake]

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

On 07/17/2017 03:26 AM, Peter Xu wrote:
> We have merely all the stuff, but this one is missing. Add it in.

s/merely/nearly/

> 
> Am going to use this new helper for MigrationParameters fields, since
> most of them are int64_t.
> 
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: Marc-André Lureau <marcandre.lureau@redhat.com>
> CC: Peter Xu <peterx@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/core/qdev-properties.c    | 32 ++++++++++++++++++++++++++++++++
>  include/hw/qdev-properties.h |  3 +++
>  2 files changed, 35 insertions(+)
> 

-- 
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: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 01/11] qdev: provide DEFINE_PROP_INT64()

[Peter Xu]

On Mon, Jul 17, 2017 at 11:30:47AM -0500, Eric Blake wrote:
> On 07/17/2017 03:26 AM, Peter Xu wrote:
> > We have merely all the stuff, but this one is missing. Add it in.
> 
> s/merely/nearly/

Will fix this without removing r-bs.  Thanks,

> 
> > 
> > Am going to use this new helper for MigrationParameters fields, since
> > most of them are int64_t.
> > 
> > CC: Markus Armbruster <armbru@redhat.com>
> > CC: Eduardo Habkost <ehabkost@redhat.com>
> > CC: Marc-André Lureau <marcandre.lureau@redhat.com>
> > CC: Peter Xu <peterx@redhat.com>
> > CC: Juan Quintela <quintela@redhat.com>
> > CC: Marcel Apfelbaum <marcel@redhat.com>
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/core/qdev-properties.c    | 32 ++++++++++++++++++++++++++++++++
> >  include/hw/qdev-properties.h |  3 +++
> >  2 files changed, 35 insertions(+)
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 




-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 01/11] qdev: provide DEFINE_PROP_INT64()

[Eduardo Habkost]

On Mon, Jul 17, 2017 at 04:26:01PM +0800, Peter Xu wrote:
> We have merely all the stuff, but this one is missing. Add it in.
> 
> Am going to use this new helper for MigrationParameters fields, since
> most of them are int64_t.
> 
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: Marc-André Lureau <marcandre.lureau@redhat.com>
> CC: Peter Xu <peterx@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Assuming that this will get merged through the migration tree:

Acked-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v2 02/11] migration: export parameters to props

[Peter Xu]

Export migration parameters to qdev properties. Then we can use, for
example:

  -global migration.x-cpu-throttle-initial=xxx

To specify migration parameters during init.

Prefix "x-" is appended for each parameter exported to show that this is
not a stable interface, and only for debugging/testing purpose.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index a0db40d..ad2505c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2009,6 +2009,31 @@ static Property migration_properties[] = {
                      send_configuration, true),
     DEFINE_PROP_BOOL("send-section-footer", MigrationState,
                      send_section_footer, true),
+
+    /* Migration parameters */
+    DEFINE_PROP_INT64("x-compress-level", MigrationState,
+                      parameters.compress_level,
+                      DEFAULT_MIGRATE_COMPRESS_LEVEL),
+    DEFINE_PROP_INT64("x-compress-threads", MigrationState,
+                      parameters.compress_threads,
+                      DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT),
+    DEFINE_PROP_INT64("x-decompress-threads", MigrationState,
+                      parameters.decompress_threads,
+                      DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
+    DEFINE_PROP_INT64("x-cpu-throttle-initial", MigrationState,
+                      parameters.cpu_throttle_initial,
+                      DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL),
+    DEFINE_PROP_INT64("x-cpu-throttle-increment", MigrationState,
+                      parameters.cpu_throttle_increment,
+                      DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT),
+    DEFINE_PROP_INT64("x-max-bandwidth", MigrationState,
+                      parameters.max_bandwidth, MAX_THROTTLE),
+    DEFINE_PROP_INT64("x-downtime-limit", MigrationState,
+                      parameters.downtime_limit,
+                      DEFAULT_MIGRATE_SET_DOWNTIME),
+    DEFINE_PROP_INT64("x-checkpoint-delay", MigrationState,
+                      parameters.x_checkpoint_delay,
+                      DEFAULT_MIGRATE_X_CHECKPOINT_DELAY),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2027,16 +2052,6 @@ static void migration_instance_init(Object *obj)
     ms->state = MIGRATION_STATUS_NONE;
     ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
     ms->mbps = -1;
-    ms->parameters = (MigrationParameters) {
-        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
-        .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
-        .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
-        .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
-        .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
-        .max_bandwidth = MAX_THROTTLE,
-        .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
-        .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
-    };
     ms->parameters.tls_creds = g_strdup("");
     ms->parameters.tls_hostname = g_strdup("");
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 02/11] migration: export parameters to props

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Export migration parameters to qdev properties. Then we can use, for
> example:
>
>   -global migration.x-cpu-throttle-initial=xxx
>
> To specify migration parameters during init.
>
> Prefix "x-" is appended for each parameter exported to show that this is
> not a stable interface, and only for debugging/testing purpose.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

YESS!!!!!!


The number of types that I have typed that on the command line .....

[Qemu-devel] [PATCH v2 03/11] migration: export capabilities to props

[Peter Xu]

Do the same thing to migration capabilities, just like what we did in
previous patch for migration parameters.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index ad2505c..3208162 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2001,6 +2001,9 @@ void migration_global_dump(Monitor *mon)
                    ms->send_configuration, ms->send_section_footer);
 }
 
+#define DEFINE_PROP_MIG_CAP(name, x)             \
+    DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false)
+
 static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
@@ -2034,6 +2037,20 @@ static Property migration_properties[] = {
     DEFINE_PROP_INT64("x-checkpoint-delay", MigrationState,
                       parameters.x_checkpoint_delay,
                       DEFAULT_MIGRATE_X_CHECKPOINT_DELAY),
+
+    /* Migration capabilities */
+    DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
+    DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
+    DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
+    DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
+    DEFINE_PROP_MIG_CAP("x-compress", MIGRATION_CAPABILITY_COMPRESS),
+    DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS),
+    DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
+    DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
+    DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
+    DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
+    DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
+
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 03/11] migration: export capabilities to props

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Do the same thing to migration capabilities, just like what we did in
> previous patch for migration parameters.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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


A pitty that the preprocessor is not able to pass to uppercase ...

> +#define DEFINE_PROP_MIG_CAP(name, x)             \
> +    DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false)

#define DEFINE_PROP_MIG_CAP(name)             \
    DEFINE_PROP_BOOL(#name, MigrationState,
    enabled_capabilities[MIGRATION_CAPABILITY_##name], false)

Re: [Qemu-devel] [PATCH v2 03/11] migration: export capabilities to props

[Peter Xu]

On Mon, Jul 17, 2017 at 06:58:44PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Do the same thing to migration capabilities, just like what we did in
> > previous patch for migration parameters.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks!

> 
> 
> A pitty that the preprocessor is not able to pass to uppercase ...
> 
> > +#define DEFINE_PROP_MIG_CAP(name, x)             \
> > +    DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false)
> 
> #define DEFINE_PROP_MIG_CAP(name)             \
>     DEFINE_PROP_BOOL(#name, MigrationState,
>     enabled_capabilities[MIGRATION_CAPABILITY_##name], false)

Yes a pity, this is more beautiful (though this may let the grepping
of specific MIGRATION_CAPABILITY_* slightly harder since it breaks the
macro).

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 03/11] migration: export capabilities to props

[Eduardo Habkost]

On Mon, Jul 17, 2017 at 04:26:03PM +0800, Peter Xu wrote:
> Do the same thing to migration capabilities, just like what we did in
> previous patch for migration parameters.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index ad2505c..3208162 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2001,6 +2001,9 @@ void migration_global_dump(Monitor *mon)
>                     ms->send_configuration, ms->send_section_footer);
>  }
>  
> +#define DEFINE_PROP_MIG_CAP(name, x)             \
> +    DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false)
> +

Maybe for the future: have you considered replacing the
enabled_capabilities array with a uint32_t and using
DEFINE_PROP_BIT?

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


>  static Property migration_properties[] = {
>      DEFINE_PROP_BOOL("store-global-state", MigrationState,
>                       store_global_state, true),
> @@ -2034,6 +2037,20 @@ static Property migration_properties[] = {
>      DEFINE_PROP_INT64("x-checkpoint-delay", MigrationState,
>                        parameters.x_checkpoint_delay,
>                        DEFAULT_MIGRATE_X_CHECKPOINT_DELAY),
> +
> +    /* Migration capabilities */
> +    DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> +    DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
> +    DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
> +    DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
> +    DEFINE_PROP_MIG_CAP("x-compress", MIGRATION_CAPABILITY_COMPRESS),
> +    DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS),
> +    DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
> +    DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
> +    DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
> +    DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
> +    DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
> +
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 03/11] migration: export capabilities to props

[Peter Xu]

On Mon, Jul 17, 2017 at 02:52:31PM -0300, Eduardo Habkost wrote:
> On Mon, Jul 17, 2017 at 04:26:03PM +0800, Peter Xu wrote:
> > Do the same thing to migration capabilities, just like what we did in
> > previous patch for migration parameters.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index ad2505c..3208162 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2001,6 +2001,9 @@ void migration_global_dump(Monitor *mon)
> >                     ms->send_configuration, ms->send_section_footer);
> >  }
> >  
> > +#define DEFINE_PROP_MIG_CAP(name, x)             \
> > +    DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false)
> > +
> 
> Maybe for the future: have you considered replacing the
> enabled_capabilities array with a uint32_t and using
> DEFINE_PROP_BIT?

Yes, this sounds reasonable.  Noted.

> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Thanks!

-- 
Peter Xu

[Qemu-devel] [PATCH v2 04/11] qom: call parent first on post_init()

[Peter Xu]

It makes more sense to call the post_init() hook of the parent first
then the child, just like what we do in the rest of the hooks.

CC: Andreas Färber <afaerber@suse.de>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qom/object.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index dfdbd50..e2c9c4a 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -347,13 +347,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
 
 static void object_post_init_with_type(Object *obj, TypeImpl *ti)
 {
-    if (ti->instance_post_init) {
-        ti->instance_post_init(obj);
-    }
-
     if (type_has_parent(ti)) {
         object_post_init_with_type(obj, type_get_parent(ti));
     }
+
+    if (ti->instance_post_init) {
+        ti->instance_post_init(obj);
+    }
 }
 
 static void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 04/11] qom: call parent first on post_init()

[Eduardo Habkost]

On Mon, Jul 17, 2017 at 04:26:04PM +0800, Peter Xu wrote:
> It makes more sense to call the post_init() hook of the parent first
> then the child, just like what we do in the rest of the hooks.

I suggest documenting the existing instance_post_init users in
the commit message (TYPE_DEVICE and TYPE_ARM_CPU, as far as I can
see), and explain why they won't be affected (or if they are
affected, why the new behavior is better).

Also, can you please update the @instance_post_init documentation
in qom/object.h to mention the new ordering?

Maybe something like:

- * @instance_post_init: This function is called to finish initialization of
- *   an object, after all @instance_init functions were called.
+ * @instance_post_init: Finish initialization of an object.  This
+ *   function is called after the @instance_init functions for
+ *   all subclasses were called, but before the @instance_post_init
+ *   function for the subclasses are called.


> 
> CC: Andreas Färber <afaerber@suse.de>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qom/object.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index dfdbd50..e2c9c4a 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -347,13 +347,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
>  
>  static void object_post_init_with_type(Object *obj, TypeImpl *ti)
>  {
> -    if (ti->instance_post_init) {
> -        ti->instance_post_init(obj);
> -    }
> -
>      if (type_has_parent(ti)) {
>          object_post_init_with_type(obj, type_get_parent(ti));
>      }
> +
> +    if (ti->instance_post_init) {
> +        ti->instance_post_init(obj);
> +    }
>  }
>  
>  static void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
> -- 
> 2.7.4
> 

-- 
Eduardo

[Qemu-devel] [PATCH v2 05/11] migration: introduce migrate_params_check()

[Peter Xu]

Helper to check the parameters. Abstracted from
qmp_migrate_set_parameters().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3208162..2821f8a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -643,64 +643,86 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     }
 }
 
-void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
+/*
+ * Check whether the parameters are valid. Error will be put into errp
+ * (if provided). Return true if valid, otherwise false.
+ */
+static bool migrate_params_check(MigrationParameters *params, Error **errp)
 {
-    MigrationState *s = migrate_get_current();
-
     if (params->has_compress_level &&
         (params->compress_level < 0 || params->compress_level > 9)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
                    "is invalid, it should be in the range of 0 to 9");
-        return;
+        return false;
     }
+
     if (params->has_compress_threads &&
         (params->compress_threads < 1 || params->compress_threads > 255)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "compress_threads",
                    "is invalid, it should be in the range of 1 to 255");
-        return;
+        return false;
     }
+
     if (params->has_decompress_threads &&
         (params->decompress_threads < 1 || params->decompress_threads > 255)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "decompress_threads",
                    "is invalid, it should be in the range of 1 to 255");
-        return;
+        return false;
     }
+
     if (params->has_cpu_throttle_initial &&
         (params->cpu_throttle_initial < 1 ||
          params->cpu_throttle_initial > 99)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "cpu_throttle_initial",
                    "an integer in the range of 1 to 99");
-        return;
+        return false;
     }
+
     if (params->has_cpu_throttle_increment &&
         (params->cpu_throttle_increment < 1 ||
          params->cpu_throttle_increment > 99)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "cpu_throttle_increment",
                    "an integer in the range of 1 to 99");
-        return;
+        return false;
     }
+
     if (params->has_max_bandwidth &&
         (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
         error_setg(errp, "Parameter 'max_bandwidth' expects an integer in the"
                          " range of 0 to %zu bytes/second", SIZE_MAX);
-        return;
+        return false;
     }
+
     if (params->has_downtime_limit &&
         (params->downtime_limit < 0 ||
          params->downtime_limit > MAX_MIGRATE_DOWNTIME)) {
         error_setg(errp, "Parameter 'downtime_limit' expects an integer in "
                          "the range of 0 to %d milliseconds",
                          MAX_MIGRATE_DOWNTIME);
-        return;
+        return false;
     }
+
     if (params->has_x_checkpoint_delay && (params->x_checkpoint_delay < 0)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                     "x_checkpoint_delay",
                     "is invalid, it should be positive");
+        return false;
+    }
+
+    return true;
+}
+
+void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
+{
+    MigrationState *s = migrate_get_current();
+
+    if (!migrate_params_check(params, errp)) {
+        /* Invalid parameter */
+        return;
     }
 
     if (params->has_compress_level) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 05/11] migration: introduce migrate_params_check()

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Helper to check the parameters. Abstracted from
> qmp_migrate_set_parameters().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

[Qemu-devel] [PATCH v2 06/11] migration: provide migrate_params_apply()

[Peter Xu]

Abstracted from qmp_migrate_set_parameters().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 2821f8a..8c65054 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -716,38 +716,40 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
     return true;
 }
 
-void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
+static void migrate_params_apply(MigrationParameters *params)
 {
     MigrationState *s = migrate_get_current();
 
-    if (!migrate_params_check(params, errp)) {
-        /* Invalid parameter */
-        return;
-    }
-
     if (params->has_compress_level) {
         s->parameters.compress_level = params->compress_level;
     }
+
     if (params->has_compress_threads) {
         s->parameters.compress_threads = params->compress_threads;
     }
+
     if (params->has_decompress_threads) {
         s->parameters.decompress_threads = params->decompress_threads;
     }
+
     if (params->has_cpu_throttle_initial) {
         s->parameters.cpu_throttle_initial = params->cpu_throttle_initial;
     }
+
     if (params->has_cpu_throttle_increment) {
         s->parameters.cpu_throttle_increment = params->cpu_throttle_increment;
     }
+
     if (params->has_tls_creds) {
         g_free(s->parameters.tls_creds);
         s->parameters.tls_creds = g_strdup(params->tls_creds);
     }
+
     if (params->has_tls_hostname) {
         g_free(s->parameters.tls_hostname);
         s->parameters.tls_hostname = g_strdup(params->tls_hostname);
     }
+
     if (params->has_max_bandwidth) {
         s->parameters.max_bandwidth = params->max_bandwidth;
         if (s->to_dst_file) {
@@ -755,6 +757,7 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
                                 s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
         }
     }
+
     if (params->has_downtime_limit) {
         s->parameters.downtime_limit = params->downtime_limit;
     }
@@ -765,11 +768,22 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
             colo_checkpoint_notify(s);
         }
     }
+
     if (params->has_block_incremental) {
         s->parameters.block_incremental = params->block_incremental;
     }
 }
 
+void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
+{
+    if (!migrate_params_check(params, errp)) {
+        /* Invalid parameter */
+        return;
+    }
+
+    migrate_params_apply(params);
+}
+
 
 void qmp_migrate_start_postcopy(Error **errp)
 {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 06/11] migration: provide migrate_params_apply()

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Abstracted from qmp_migrate_set_parameters().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

[Qemu-devel] [PATCH v2 07/11] migration: check global params for validity

[Peter Xu]

Adding validity check for the migration parameters passed in via global
properties.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 8c65054..5a7f22c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2109,6 +2109,39 @@ static void migration_instance_init(Object *obj)
     ms->parameters.tls_hostname = g_strdup("");
 }
 
+static void migration_instance_post_init(Object *obj)
+{
+    MigrationState *ms = (MigrationState *)obj;
+    Error *err = NULL;
+    MigrationParameters params = {
+        .has_compress_level = true,
+        .compress_level = ms->parameters.compress_level,
+        .has_compress_threads = true,
+        .compress_threads = ms->parameters.compress_threads,
+        .has_decompress_threads = true,
+        .decompress_threads = ms->parameters.decompress_threads,
+        .has_cpu_throttle_initial = true,
+        .cpu_throttle_initial = ms->parameters.cpu_throttle_initial,
+        .has_cpu_throttle_increment = true,
+        .cpu_throttle_increment = ms->parameters.cpu_throttle_increment,
+        .has_max_bandwidth = true,
+        .max_bandwidth = ms->parameters.max_bandwidth,
+        .has_downtime_limit = true,
+        .downtime_limit = ms->parameters.downtime_limit,
+        .has_x_checkpoint_delay = true,
+        .x_checkpoint_delay = ms->parameters.x_checkpoint_delay,
+        .has_block_incremental = true,
+        .block_incremental = ms->parameters.block_incremental,
+    };
+
+    /* We have applied all the migration properties... */
+
+    if (!migrate_params_check(&params, &err)) {
+        error_report_err(err);
+        exit(1);
+    }
+}
+
 static const TypeInfo migration_type = {
     .name = TYPE_MIGRATION,
     /*
@@ -2124,6 +2157,7 @@ static const TypeInfo migration_type = {
     .class_size = sizeof(MigrationClass),
     .instance_size = sizeof(MigrationState),
     .instance_init = migration_instance_init,
+    .instance_post_init = migration_instance_post_init,
 };
 
 static void register_migration_types(void)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 07/11] migration: check global params for validity

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Adding validity check for the migration parameters passed in via global
> properties.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 07/11] migration: check global params for validity

[Eduardo Habkost]

On Mon, Jul 17, 2017 at 04:26:07PM +0800, Peter Xu wrote:
> Adding validity check for the migration parameters passed in via global
> properties.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 8c65054..5a7f22c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2109,6 +2109,39 @@ static void migration_instance_init(Object *obj)
>      ms->parameters.tls_hostname = g_strdup("");
>  }
>  
> +static void migration_instance_post_init(Object *obj)
> +{
> +    MigrationState *ms = (MigrationState *)obj;
> +    Error *err = NULL;
> +    MigrationParameters params = {
> +        .has_compress_level = true,
> +        .compress_level = ms->parameters.compress_level,
> +        .has_compress_threads = true,
> +        .compress_threads = ms->parameters.compress_threads,
> +        .has_decompress_threads = true,
> +        .decompress_threads = ms->parameters.decompress_threads,
> +        .has_cpu_throttle_initial = true,
> +        .cpu_throttle_initial = ms->parameters.cpu_throttle_initial,
> +        .has_cpu_throttle_increment = true,
> +        .cpu_throttle_increment = ms->parameters.cpu_throttle_increment,
> +        .has_max_bandwidth = true,
> +        .max_bandwidth = ms->parameters.max_bandwidth,
> +        .has_downtime_limit = true,
> +        .downtime_limit = ms->parameters.downtime_limit,
> +        .has_x_checkpoint_delay = true,
> +        .x_checkpoint_delay = ms->parameters.x_checkpoint_delay,
> +        .has_block_incremental = true,
> +        .block_incremental = ms->parameters.block_incremental,
> +    };
> +
> +    /* We have applied all the migration properties... */
> +
> +    if (!migrate_params_check(&params, &err)) {

Why not just:
  if (!migrate_params_check(&ms->parameters, &err))
?

If the ms->parameters.has_* fields are not set to true anywhere,
we can set them to true in instance_init.

(This would also also us to use QAPI_CLONE in
qmp_query_migrate_parameters() instead of manually copying each
field.


> +        error_report_err(err);
> +        exit(1);
> +    }
> +}

On real devices, this is normally done on realize (which has
proper error reporting).  We never realize the TYPE_MIGRATION
object because it's not a real device, though.

Doing error checks on post_init feels fragile, as the only way
errors can be handled is triggering exit(1) in the middle of an
object_new() call.

As we have only one place where the TYPE_MIGRATION object is
created, I suggest calling migrate_params_check() inside
migration_object_init().

> +
>  static const TypeInfo migration_type = {
>      .name = TYPE_MIGRATION,
>      /*
> @@ -2124,6 +2157,7 @@ static const TypeInfo migration_type = {
>      .class_size = sizeof(MigrationClass),
>      .instance_size = sizeof(MigrationState),
>      .instance_init = migration_instance_init,
> +    .instance_post_init = migration_instance_post_init,
>  };
>  
>  static void register_migration_types(void)
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 07/11] migration: check global params for validity

[Peter Xu]

On Mon, Jul 17, 2017 at 03:25:05PM -0300, Eduardo Habkost wrote:
> On Mon, Jul 17, 2017 at 04:26:07PM +0800, Peter Xu wrote:
> > Adding validity check for the migration parameters passed in via global
> > properties.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 8c65054..5a7f22c 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2109,6 +2109,39 @@ static void migration_instance_init(Object *obj)
> >      ms->parameters.tls_hostname = g_strdup("");
> >  }
> >  
> > +static void migration_instance_post_init(Object *obj)
> > +{
> > +    MigrationState *ms = (MigrationState *)obj;
> > +    Error *err = NULL;
> > +    MigrationParameters params = {
> > +        .has_compress_level = true,
> > +        .compress_level = ms->parameters.compress_level,
> > +        .has_compress_threads = true,
> > +        .compress_threads = ms->parameters.compress_threads,
> > +        .has_decompress_threads = true,
> > +        .decompress_threads = ms->parameters.decompress_threads,
> > +        .has_cpu_throttle_initial = true,
> > +        .cpu_throttle_initial = ms->parameters.cpu_throttle_initial,
> > +        .has_cpu_throttle_increment = true,
> > +        .cpu_throttle_increment = ms->parameters.cpu_throttle_increment,
> > +        .has_max_bandwidth = true,
> > +        .max_bandwidth = ms->parameters.max_bandwidth,
> > +        .has_downtime_limit = true,
> > +        .downtime_limit = ms->parameters.downtime_limit,
> > +        .has_x_checkpoint_delay = true,
> > +        .x_checkpoint_delay = ms->parameters.x_checkpoint_delay,
> > +        .has_block_incremental = true,
> > +        .block_incremental = ms->parameters.block_incremental,
> > +    };
> > +
> > +    /* We have applied all the migration properties... */
> > +
> > +    if (!migrate_params_check(&params, &err)) {
> 
> Why not just:
>   if (!migrate_params_check(&ms->parameters, &err))
> ?
> 
> If the ms->parameters.has_* fields are not set to true anywhere,
> we can set them to true in instance_init.

Sure.

> 
> (This would also also us to use QAPI_CLONE in
> qmp_query_migrate_parameters() instead of manually copying each
> field.
> 
> 
> > +        error_report_err(err);
> > +        exit(1);
> > +    }
> > +}
> 
> On real devices, this is normally done on realize (which has
> proper error reporting).  We never realize the TYPE_MIGRATION
> object because it's not a real device, though.

Hmm...

> 
> Doing error checks on post_init feels fragile, as the only way
> errors can be handled is triggering exit(1) in the middle of an
> object_new() call.
> 
> As we have only one place where the TYPE_MIGRATION object is
> created, I suggest calling migrate_params_check() inside
> migration_object_init().

... How about I just provide a realize() for it? Then I can drop the
QOM patch 4, also, I can keep the checks along with the object itself.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 07/11] migration: check global params for validity

[Eduardo Habkost]

On Tue, Jul 18, 2017 at 09:56:06AM +0800, Peter Xu wrote:
> On Mon, Jul 17, 2017 at 03:25:05PM -0300, Eduardo Habkost wrote:
> > On Mon, Jul 17, 2017 at 04:26:07PM +0800, Peter Xu wrote:
> > > Adding validity check for the migration parameters passed in via global
> > > properties.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  migration/migration.c | 34 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 8c65054..5a7f22c 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -2109,6 +2109,39 @@ static void migration_instance_init(Object *obj)
> > >      ms->parameters.tls_hostname = g_strdup("");
> > >  }
> > >  
> > > +static void migration_instance_post_init(Object *obj)
> > > +{
> > > +    MigrationState *ms = (MigrationState *)obj;
> > > +    Error *err = NULL;
> > > +    MigrationParameters params = {
> > > +        .has_compress_level = true,
> > > +        .compress_level = ms->parameters.compress_level,
> > > +        .has_compress_threads = true,
> > > +        .compress_threads = ms->parameters.compress_threads,
> > > +        .has_decompress_threads = true,
> > > +        .decompress_threads = ms->parameters.decompress_threads,
> > > +        .has_cpu_throttle_initial = true,
> > > +        .cpu_throttle_initial = ms->parameters.cpu_throttle_initial,
> > > +        .has_cpu_throttle_increment = true,
> > > +        .cpu_throttle_increment = ms->parameters.cpu_throttle_increment,
> > > +        .has_max_bandwidth = true,
> > > +        .max_bandwidth = ms->parameters.max_bandwidth,
> > > +        .has_downtime_limit = true,
> > > +        .downtime_limit = ms->parameters.downtime_limit,
> > > +        .has_x_checkpoint_delay = true,
> > > +        .x_checkpoint_delay = ms->parameters.x_checkpoint_delay,
> > > +        .has_block_incremental = true,
> > > +        .block_incremental = ms->parameters.block_incremental,
> > > +    };
> > > +
> > > +    /* We have applied all the migration properties... */
> > > +
> > > +    if (!migrate_params_check(&params, &err)) {
> > 
> > Why not just:
> >   if (!migrate_params_check(&ms->parameters, &err))
> > ?
> > 
> > If the ms->parameters.has_* fields are not set to true anywhere,
> > we can set them to true in instance_init.
> 
> Sure.
> 
> > 
> > (This would also also us to use QAPI_CLONE in
> > qmp_query_migrate_parameters() instead of manually copying each
> > field.
> > 
> > 
> > > +        error_report_err(err);
> > > +        exit(1);
> > > +    }
> > > +}
> > 
> > On real devices, this is normally done on realize (which has
> > proper error reporting).  We never realize the TYPE_MIGRATION
> > object because it's not a real device, though.
> 
> Hmm...
> 
> > 
> > Doing error checks on post_init feels fragile, as the only way
> > errors can be handled is triggering exit(1) in the middle of an
> > object_new() call.
> > 
> > As we have only one place where the TYPE_MIGRATION object is
> > created, I suggest calling migrate_params_check() inside
> > migration_object_init().
> 
> ... How about I just provide a realize() for it? Then I can drop the
> QOM patch 4, also, I can keep the checks along with the object itself.

qdev does lots of other stuff at realize time (e.g. it adds the
device to the device tree), and I don't think we want to trigger
that.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 07/11] migration: check global params for validity

[Peter Xu]

On Mon, Jul 17, 2017 at 11:33:55PM -0300, Eduardo Habkost wrote:
> On Tue, Jul 18, 2017 at 09:56:06AM +0800, Peter Xu wrote:
> > On Mon, Jul 17, 2017 at 03:25:05PM -0300, Eduardo Habkost wrote:
> > > On Mon, Jul 17, 2017 at 04:26:07PM +0800, Peter Xu wrote:
> > > > Adding validity check for the migration parameters passed in via global
> > > > properties.
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  migration/migration.c | 34 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > > 
> > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > index 8c65054..5a7f22c 100644
> > > > --- a/migration/migration.c
> > > > +++ b/migration/migration.c
> > > > @@ -2109,6 +2109,39 @@ static void migration_instance_init(Object *obj)
> > > >      ms->parameters.tls_hostname = g_strdup("");
> > > >  }
> > > >  
> > > > +static void migration_instance_post_init(Object *obj)
> > > > +{
> > > > +    MigrationState *ms = (MigrationState *)obj;
> > > > +    Error *err = NULL;
> > > > +    MigrationParameters params = {
> > > > +        .has_compress_level = true,
> > > > +        .compress_level = ms->parameters.compress_level,
> > > > +        .has_compress_threads = true,
> > > > +        .compress_threads = ms->parameters.compress_threads,
> > > > +        .has_decompress_threads = true,
> > > > +        .decompress_threads = ms->parameters.decompress_threads,
> > > > +        .has_cpu_throttle_initial = true,
> > > > +        .cpu_throttle_initial = ms->parameters.cpu_throttle_initial,
> > > > +        .has_cpu_throttle_increment = true,
> > > > +        .cpu_throttle_increment = ms->parameters.cpu_throttle_increment,
> > > > +        .has_max_bandwidth = true,
> > > > +        .max_bandwidth = ms->parameters.max_bandwidth,
> > > > +        .has_downtime_limit = true,
> > > > +        .downtime_limit = ms->parameters.downtime_limit,
> > > > +        .has_x_checkpoint_delay = true,
> > > > +        .x_checkpoint_delay = ms->parameters.x_checkpoint_delay,
> > > > +        .has_block_incremental = true,
> > > > +        .block_incremental = ms->parameters.block_incremental,
> > > > +    };
> > > > +
> > > > +    /* We have applied all the migration properties... */
> > > > +
> > > > +    if (!migrate_params_check(&params, &err)) {
> > > 
> > > Why not just:
> > >   if (!migrate_params_check(&ms->parameters, &err))
> > > ?
> > > 
> > > If the ms->parameters.has_* fields are not set to true anywhere,
> > > we can set them to true in instance_init.
> > 
> > Sure.
> > 
> > > 
> > > (This would also also us to use QAPI_CLONE in
> > > qmp_query_migrate_parameters() instead of manually copying each
> > > field.
> > > 
> > > 
> > > > +        error_report_err(err);
> > > > +        exit(1);
> > > > +    }
> > > > +}
> > > 
> > > On real devices, this is normally done on realize (which has
> > > proper error reporting).  We never realize the TYPE_MIGRATION
> > > object because it's not a real device, though.
> > 
> > Hmm...
> > 
> > > 
> > > Doing error checks on post_init feels fragile, as the only way
> > > errors can be handled is triggering exit(1) in the middle of an
> > > object_new() call.
> > > 
> > > As we have only one place where the TYPE_MIGRATION object is
> > > created, I suggest calling migrate_params_check() inside
> > > migration_object_init().
> > 
> > ... How about I just provide a realize() for it? Then I can drop the
> > QOM patch 4, also, I can keep the checks along with the object itself.
> 
> qdev does lots of other stuff at realize time (e.g. it adds the
> device to the device tree), and I don't think we want to trigger
> that.

Ok. Then let me take your suggestion to move this to
migration_object_init().  Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v2 08/11] migration: remove check against colo support

[Peter Xu]

Since commit a15215f3 ("build: remove --enable-colo/--disable-colo"),
colo is always supported. We don't need any colo_supported() now since
it is always true. Removing any extra code that depends on it.

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/colo.h |  1 -
 migration/colo.c         |  5 -----
 migration/migration.c    | 11 -----------
 3 files changed, 17 deletions(-)

diff --git a/include/migration/colo.h b/include/migration/colo.h
index be6beba..ff9874e 100644
--- a/include/migration/colo.h
+++ b/include/migration/colo.h
@@ -15,7 +15,6 @@
 
 #include "qemu-common.h"
 
-bool colo_supported(void);
 void colo_info_init(void);
 
 void migrate_start_colo_process(MigrationState *s);
diff --git a/migration/colo.c b/migration/colo.c
index ef35f00..a425543 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -29,11 +29,6 @@ static bool vmstate_loading;
 
 #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
 
-bool colo_supported(void)
-{
-    return true;
-}
-
 bool migration_in_colo_state(void)
 {
     MigrationState *s = migrate_get_current();
diff --git a/migration/migration.c b/migration/migration.c
index 5a7f22c..eb750c5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -403,9 +403,6 @@ MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
             continue;
         }
 #endif
-        if (i == MIGRATION_CAPABILITY_X_COLO && !colo_supported()) {
-            continue;
-        }
         if (head == NULL) {
             head = g_malloc0(sizeof(*caps));
             caps = head;
@@ -604,14 +601,6 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
             continue;
         }
 #endif
-        if (cap->value->capability == MIGRATION_CAPABILITY_X_COLO) {
-            if (!colo_supported()) {
-                error_setg(errp, "COLO is not currently supported, please"
-                             " configure with --enable-colo option in order to"
-                             " support COLO feature");
-                continue;
-            }
-        }
         s->enabled_capabilities[cap->value->capability] = cap->value->state;
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 08/11] migration: remove check against colo support

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Since commit a15215f3 ("build: remove --enable-colo/--disable-colo"),
> colo is always supported. We don't need any colo_supported() now since
> it is always true. Removing any extra code that depends on it.
>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 08/11] migration: remove check against colo support

[Zhang Chen]

On 07/17/2017 04:26 PM, Peter Xu wrote:
> Since commit a15215f3 ("build: remove --enable-colo/--disable-colo"),
> colo is always supported. We don't need any colo_supported() now since
> it is always true. Removing any extra code that depends on it.
>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

COLO running well.

Reviewed-by: Zhang Chen<zhangchen.fnst@cn.fujitsu.com>


Thanks
Zhang Chen

> ---
>   include/migration/colo.h |  1 -
>   migration/colo.c         |  5 -----
>   migration/migration.c    | 11 -----------
>   3 files changed, 17 deletions(-)
>
> diff --git a/include/migration/colo.h b/include/migration/colo.h
> index be6beba..ff9874e 100644
> --- a/include/migration/colo.h
> +++ b/include/migration/colo.h
> @@ -15,7 +15,6 @@
>   
>   #include "qemu-common.h"
>   
> -bool colo_supported(void);
>   void colo_info_init(void);
>   
>   void migrate_start_colo_process(MigrationState *s);
> diff --git a/migration/colo.c b/migration/colo.c
> index ef35f00..a425543 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -29,11 +29,6 @@ static bool vmstate_loading;
>   
>   #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
>   
> -bool colo_supported(void)
> -{
> -    return true;
> -}
> -
>   bool migration_in_colo_state(void)
>   {
>       MigrationState *s = migrate_get_current();
> diff --git a/migration/migration.c b/migration/migration.c
> index 5a7f22c..eb750c5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -403,9 +403,6 @@ MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
>               continue;
>           }
>   #endif
> -        if (i == MIGRATION_CAPABILITY_X_COLO && !colo_supported()) {
> -            continue;
> -        }
>           if (head == NULL) {
>               head = g_malloc0(sizeof(*caps));
>               caps = head;
> @@ -604,14 +601,6 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>               continue;
>           }
>   #endif
> -        if (cap->value->capability == MIGRATION_CAPABILITY_X_COLO) {
> -            if (!colo_supported()) {
> -                error_setg(errp, "COLO is not currently supported, please"
> -                             " configure with --enable-colo option in order to"
> -                             " support COLO feature");
> -                continue;
> -            }
> -        }
>           s->enabled_capabilities[cap->value->capability] = cap->value->state;
>       }
>   

-- 
Thanks
Zhang Chen

[Qemu-devel] [PATCH v2 09/11] migration: provide migrate_caps_check()

[Peter Xu]

Abstract helper function to check migration capabilities (from the old
qmp_migrate_set_capabilities).  Prepare to be used somewhere else.

There is side effect on the change: when applying the capabilities, we
were skipping the invalid ones, but still applying the valid ones (if
they are provided in the same QMP request). After this refactoring,
we'll ignore all the capabilities if we detected invalid setup along the
way. However, I don't think it is a problem since general users should
not provide anything invalid after all.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 79 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 26 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index eb750c5..52db5e7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -579,43 +579,49 @@ MigrationInfo *qmp_query_migrate(Error **errp)
     return info;
 }
 
-void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
-                                  Error **errp)
+/**
+ * @migration_caps_check - check capability validity
+ *
+ * @cap_list: old capability list, array of bool
+ * @params: new capabilities to be applied soon
+ * @errp: set *errp if the check failed, with reason
+ *
+ * Returns true if check passed, otherwise false.
+ */
+static bool migrate_caps_check(bool *cap_list,
+                               MigrationCapabilityStatusList *params,
+                               Error **errp)
 {
-    MigrationState *s = migrate_get_current();
     MigrationCapabilityStatusList *cap;
-    bool old_postcopy_cap = migrate_postcopy_ram();
+    bool old_postcopy_cap;
 
-    if (migration_is_setup_or_active(s->state)) {
-        error_setg(errp, QERR_MIGRATION_ACTIVE);
-        return;
-    }
+    old_postcopy_cap = cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM];
 
     for (cap = params; cap; cap = cap->next) {
+        cap_list[cap->value->capability] = cap->value->state;
+    }
+
 #ifndef CONFIG_LIVE_BLOCK_MIGRATION
-        if (cap->value->capability == MIGRATION_CAPABILITY_BLOCK
-            && cap->value->state) {
-            error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
-                       "block migration");
-            error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
-            continue;
-        }
-#endif
-        s->enabled_capabilities[cap->value->capability] = cap->value->state;
+    if (cap_list[MIGRATION_CAPABILITY_BLOCK]) {
+        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
 
-    if (migrate_postcopy_ram()) {
-        if (migrate_use_compression()) {
+    if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
+        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
             /* The decompression threads asynchronously write into RAM
              * rather than use the atomic copies needed to avoid
              * userfaulting.  It should be possible to fix the decompression
              * threads for compatibility in future.
              */
-            error_report("Postcopy is not currently compatible with "
-                         "compression");
-            s->enabled_capabilities[MIGRATION_CAPABILITY_POSTCOPY_RAM] =
-                false;
+            error_setg(errp, "Postcopy is not currently compatible "
+                       "with compression");
+            return false;
         }
+
         /* This check is reasonably expensive, so only when it's being
          * set the first time, also it's only the destination that needs
          * special support.
@@ -625,11 +631,32 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
             /* postcopy_ram_supported_by_host will have emitted a more
              * detailed message
              */
-            error_report("Postcopy is not supported");
-            s->enabled_capabilities[MIGRATION_CAPABILITY_POSTCOPY_RAM] =
-                false;
+            error_setg(errp, "Postcopy is not supported");
+            return false;
         }
     }
+
+    return true;
+}
+
+void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
+                                  Error **errp)
+{
+    MigrationState *s = migrate_get_current();
+    MigrationCapabilityStatusList *cap;
+
+    if (migration_is_setup_or_active(s->state)) {
+        error_setg(errp, QERR_MIGRATION_ACTIVE);
+        return;
+    }
+
+    if (!migrate_caps_check(s->enabled_capabilities, params, errp)) {
+        return;
+    }
+
+    for (cap = params; cap; cap = cap->next) {
+        s->enabled_capabilities[cap->value->capability] = cap->value->state;
+    }
 }
 
 /*
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 09/11] migration: provide migrate_caps_check()

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Abstract helper function to check migration capabilities (from the old
> qmp_migrate_set_capabilities).  Prepare to be used somewhere else.
>
> There is side effect on the change: when applying the capabilities, we
> were skipping the invalid ones, but still applying the valid ones (if
> they are provided in the same QMP request). After this refactoring,
> we'll ignore all the capabilities if we detected invalid setup along the
> way. However, I don't think it is a problem since general users should
> not provide anything invalid after all.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

I think that the "caveat" that we used to set the valid capabilities in
one message is just undefined behaviour.  I preffer the new semmantics
anyways.

[Qemu-devel] [PATCH v2 10/11] migration: provide migrate_cap_add()

[Peter Xu]

Abstracted from migrate_set_block_enabled() to allocate
MigrationCapabilityStatusList properly.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 52db5e7..300f84d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -833,14 +833,27 @@ void migrate_set_state(int *state, int old_state, int new_state)
     }
 }
 
-void migrate_set_block_enabled(bool value, Error **errp)
+static MigrationCapabilityStatusList *migrate_cap_add(
+    MigrationCapabilityStatusList *head,
+    MigrationCapability index,
+    bool state)
 {
     MigrationCapabilityStatusList *cap;
 
     cap = g_new0(MigrationCapabilityStatusList, 1);
     cap->value = g_new0(MigrationCapabilityStatus, 1);
-    cap->value->capability = MIGRATION_CAPABILITY_BLOCK;
-    cap->value->state = value;
+    cap->value->capability = index;
+    cap->value->state = state;
+    cap->next = head;
+
+    return cap;
+}
+
+void migrate_set_block_enabled(bool value, Error **errp)
+{
+    MigrationCapabilityStatusList *cap;
+
+    cap = migrate_cap_add(NULL, MIGRATION_CAPABILITY_BLOCK, value);
     qmp_migrate_set_capabilities(cap, errp);
     qapi_free_MigrationCapabilityStatusList(cap);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 10/11] migration: provide migrate_cap_add()

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Abstracted from migrate_set_block_enabled() to allocate
> MigrationCapabilityStatusList properly.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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


Nitpick

> -void migrate_set_block_enabled(bool value, Error **errp)
> +static MigrationCapabilityStatusList *migrate_cap_add(
> +    MigrationCapabilityStatusList *head,

We have a parameter called head

> +    MigrationCapability index,
> +    bool state)
>  {
>      MigrationCapabilityStatusList *cap;
>  
>      cap = g_new0(MigrationCapabilityStatusList, 1);
>      cap->value = g_new0(MigrationCapabilityStatus, 1);
> -    cap->value->capability = MIGRATION_CAPABILITY_BLOCK;
> -    cap->value->state = value;
> +    cap->value->capability = index;
> +    cap->value->state = state;
> +    cap->next = head;
> +
> +    return cap;


But we don't do the *head = cap?

Pelhaps is better to call it "next" or "list"?

Re: [Qemu-devel] [PATCH v2 10/11] migration: provide migrate_cap_add()

[Peter Xu]

On Mon, Jul 17, 2017 at 07:14:44PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Abstracted from migrate_set_block_enabled() to allocate
> > MigrationCapabilityStatusList properly.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> 
> Nitpick
> 
> > -void migrate_set_block_enabled(bool value, Error **errp)
> > +static MigrationCapabilityStatusList *migrate_cap_add(
> > +    MigrationCapabilityStatusList *head,
> 
> We have a parameter called head
> 
> > +    MigrationCapability index,
> > +    bool state)
> >  {
> >      MigrationCapabilityStatusList *cap;
> >  
> >      cap = g_new0(MigrationCapabilityStatusList, 1);
> >      cap->value = g_new0(MigrationCapabilityStatus, 1);
> > -    cap->value->capability = MIGRATION_CAPABILITY_BLOCK;
> > -    cap->value->state = value;
> > +    cap->value->capability = index;
> > +    cap->value->state = state;
> > +    cap->next = head;
> > +
> > +    return cap;
> 
> 
> But we don't do the *head = cap?
> 
> Pelhaps is better to call it "next" or "list"?

It's the (old) head, but sure. :)

Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v2 11/11] migration: check global caps for validity

[Peter Xu]

Checks validity for all the capabilities that we enabled with command
line.  Stop the VM if detected anything wrong.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 300f84d..b2667d0 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2142,6 +2142,9 @@ static void migration_instance_post_init(Object *obj)
 {
     MigrationState *ms = (MigrationState *)obj;
     Error *err = NULL;
+    MigrationCapabilityStatusList *head = NULL;
+    /* Assuming all off */
+    bool cap_list[MIGRATION_CAPABILITY__MAX] = { 0 };
     MigrationParameters params = {
         .has_compress_level = true,
         .compress_level = ms->parameters.compress_level,
@@ -2162,13 +2165,35 @@ static void migration_instance_post_init(Object *obj)
         .has_block_incremental = true,
         .block_incremental = ms->parameters.block_incremental,
     };
+    int i;
 
     /* We have applied all the migration properties... */
 
     if (!migrate_params_check(&params, &err)) {
+        goto error;
+    }
+
+    for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
+        if (ms->enabled_capabilities[i]) {
+            head = migrate_cap_add(head, i, true);
+        }
+    }
+
+    if (!migrate_caps_check(cap_list, head, &err)) {
+        goto error;
+    }
+
+    qapi_free_MigrationCapabilityStatusList(head);
+    return;
+
+error:
+    if (head) {
+        qapi_free_MigrationCapabilityStatusList(head);
+    }
+    if (err) {
         error_report_err(err);
-        exit(1);
     }
+    exit(1);
 }
 
 static const TypeInfo migration_type = {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 11/11] migration: check global caps for validity

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Checks validity for all the capabilities that we enabled with command
> line.  Stop the VM if detected anything wrong.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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