[Qemu-devel] [PATCH] migration: fix applying wrong capabilities

[Qemu-devel] [PATCH] migration: fix applying wrong capabilities

[Peter Xu]

When setting migration capabilities via QMP/HMP, we'll apply them even
if the capability check failed.  Fix it.

Fixes: 4a84214ebe ("migration: provide migrate_caps_check()", 2017-07-18)
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0aa596f867..88ed9375aa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -747,13 +747,15 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
 {
     MigrationState *s = migrate_get_current();
     MigrationCapabilityStatusList *cap;
+    bool cap_list[MIGRATION_CAPABILITY__MAX];
 
     if (migration_is_setup_or_active(s->state)) {
         error_setg(errp, QERR_MIGRATION_ACTIVE);
         return;
     }
 
-    if (!migrate_caps_check(s->enabled_capabilities, params, errp)) {
+    memcpy(cap_list, s->enabled_capabilities, sizeof(cap_list));
+    if (!migrate_caps_check(cap_list, params, errp)) {
         return;
     }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH] migration: fix applying wrong capabilities

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> When setting migration capabilities via QMP/HMP, we'll apply them even
> if the capability check failed.  Fix it.
> 
> Fixes: 4a84214ebe ("migration: provide migrate_caps_check()", 2017-07-18)
> Signed-off-by: Peter Xu <peterx@redhat.com>

OK, yes, that works, so:


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


It is a little odd in a way; 'caps_check' you might expect only checked
and didn't change anything.   migrate_params is organised a bit
differently; and somewhat more confusingly.

Dave

> ---
>  migration/migration.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0aa596f867..88ed9375aa 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -747,13 +747,15 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>  {
>      MigrationState *s = migrate_get_current();
>      MigrationCapabilityStatusList *cap;
> +    bool cap_list[MIGRATION_CAPABILITY__MAX];
>  
>      if (migration_is_setup_or_active(s->state)) {
>          error_setg(errp, QERR_MIGRATION_ACTIVE);
>          return;
>      }
>  
> -    if (!migrate_caps_check(s->enabled_capabilities, params, errp)) {
> +    memcpy(cap_list, s->enabled_capabilities, sizeof(cap_list));
> +    if (!migrate_caps_check(cap_list, params, errp)) {
>          return;
>      }
>  
> -- 
> 2.14.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: fix applying wrong capabilities

[Peter Xu]

On Tue, Mar 06, 2018 at 08:08:37PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > When setting migration capabilities via QMP/HMP, we'll apply them even
> > if the capability check failed.  Fix it.
> > 
> > Fixes: 4a84214ebe ("migration: provide migrate_caps_check()", 2017-07-18)
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> OK, yes, that works, so:
> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks.

> 
> 
> It is a little odd in a way; 'caps_check' you might expect only checked
> and didn't change anything.   migrate_params is organised a bit
> differently; and somewhat more confusingly.

Indeed.  Maybe the cap_list copy should be within the function, and
then define the function as:

static bool migrate_caps_check(MigrationCapabilityStatusList *params,
                               Error **errp);

Then it at least looks more like the param_check one.

Let me know if you think it's good; I can post another one after
all, and this one would be easy. :)

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] migration: fix applying wrong capabilities

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> When setting migration capabilities via QMP/HMP, we'll apply them even
> if the capability check failed.  Fix it.
> 
> Fixes: 4a84214ebe ("migration: provide migrate_caps_check()", 2017-07-18)
> Signed-off-by: Peter Xu <peterx@redhat.com>

Queued

> ---
>  migration/migration.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0aa596f867..88ed9375aa 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -747,13 +747,15 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>  {
>      MigrationState *s = migrate_get_current();
>      MigrationCapabilityStatusList *cap;
> +    bool cap_list[MIGRATION_CAPABILITY__MAX];
>  
>      if (migration_is_setup_or_active(s->state)) {
>          error_setg(errp, QERR_MIGRATION_ACTIVE);
>          return;
>      }
>  
> -    if (!migrate_caps_check(s->enabled_capabilities, params, errp)) {
> +    memcpy(cap_list, s->enabled_capabilities, sizeof(cap_list));
> +    if (!migrate_caps_check(cap_list, params, errp)) {
>          return;
>      }
>  
> -- 
> 2.14.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK