[PATCH v4 0/6] Fix multifd + cancel + multifd

[PATCH v4 0/6] Fix multifd + cancel + multifd

[Juan Quintela]

Hi

In the v4 series:
* Updated to latest pull request
* redo the check for active migration at the end of the iteration stage
* testing/testing/testing it now passes the test for multifd + cancel
  hundred of times in a row
* Use g_free consistently

Please review, Juan.

In the v3 series:
- Add back the ->shutdown fix
  Dave found the problem, we need to setup an error if we do shutdown.
- Make iotests to work back (we need to setup ->active for savevm)
- So postcopy/recovery is fixed again.

Please review, if there are not outstanding issues, I plan to push it.

Thanks, Juan

In the v2 series:
- get the multifd test patch
- drop the ->shutdown fix
  it break postcopy recovery test.  Still trying to determine if the
  problem is inside the recover test or the recover code.
- upgrade the migrate_cancel test

Please review.

[v1]
This series:
- create a test that does:
  launch multifd on target
  migrate to target
  cancel on source
  create another target
  migrate again

- And fixes the cases that made it fail:
* Make sure that we don't try ever IO after shutdown/error

Please, review.

Juan Quintela (6):
  migration-test: Use g_free() instead of free()
  multifd: Make sure that we don't do any IO after an error
  qemu-file: Don't do IO after shutdown
  migration-test: Make sure that multifd and cancel works
  migration: Create migration_is_running()
  migration: Don't send data if we have stopped

 migration/migration.c        |  29 +++++++--
 migration/migration.h        |   1 +
 migration/qemu-file.c        |  22 ++++++-
 migration/ram.c              |  23 ++++---
 migration/savevm.c           |   4 +-
 tests/qtest/migration-test.c | 114 ++++++++++++++++++++++++++++++++++-
 6 files changed, 173 insertions(+), 20 deletions(-)

-- 
2.24.1

[PATCH v4 1/6] migration-test: Use g_free() instead of free()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 tests/qtest/migration-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 26e2e77289..b6a74a05ce 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1291,7 +1291,7 @@ static void test_multifd_tcp(void)
     wait_for_serial("dest_serial");
     wait_for_migration_complete(from);
     test_migrate_end(from, to, true);
-    free(uri);
+    g_free(uri);
 }
 
 int main(int argc, char **argv)
-- 
2.24.1

[PATCH v4 2/6] multifd: Make sure that we don't do any IO after an error

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index d2208b5534..f95d656c26 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3445,7 +3445,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 {
     RAMState **temp = opaque;
     RAMState *rs = *temp;
-    int ret;
+    int ret = 0;
     int i;
     int64_t t0;
     int done = 0;
@@ -3524,12 +3524,14 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     ram_control_after_iterate(f, RAM_CONTROL_ROUND);
 
 out:
-    multifd_send_sync_main(rs);
-    qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
-    qemu_fflush(f);
-    ram_counters.transferred += 8;
+    if (ret >= 0) {
+        multifd_send_sync_main(rs);
+        qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
+        qemu_fflush(f);
+        ram_counters.transferred += 8;
 
-    ret = qemu_file_get_error(f);
+        ret = qemu_file_get_error(f);
+    }
     if (ret < 0) {
         return ret;
     }
@@ -3581,9 +3583,11 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
         ram_control_after_iterate(f, RAM_CONTROL_FINISH);
     }
 
-    multifd_send_sync_main(rs);
-    qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
-    qemu_fflush(f);
+    if (ret >= 0) {
+        multifd_send_sync_main(rs);
+        qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
+        qemu_fflush(f);
+    }
 
     return ret;
 }
-- 
2.24.1

[PATCH v4 3/6] qemu-file: Don't do IO after shutdown

[Juan Quintela]

Be sure that we are not doing neither read/write after shutdown of the
QEMUFile.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>

---

Set error in case that there is none (dave)
---
 migration/qemu-file.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 26fb25ddc1..bbb2b63927 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -53,6 +53,8 @@ struct QEMUFile {
 
     int last_error;
     Error *last_error_obj;
+    /* has the file has been shutdown */
+    bool shutdown;
 };
 
 /*
@@ -61,10 +63,18 @@ struct QEMUFile {
  */
 int qemu_file_shutdown(QEMUFile *f)
 {
+    int ret;
+
+    f->shutdown = true;
     if (!f->ops->shut_down) {
         return -ENOSYS;
     }
-    return f->ops->shut_down(f->opaque, true, true, NULL);
+    ret = f->ops->shut_down(f->opaque, true, true, NULL);
+
+    if (!f->last_error) {
+        qemu_file_set_error(f, -EIO);
+    }
+    return ret;
 }
 
 /*
@@ -214,6 +224,9 @@ void qemu_fflush(QEMUFile *f)
         return;
     }
 
+    if (f->shutdown) {
+        return;
+    }
     if (f->iovcnt > 0) {
         expect = iov_size(f->iov, f->iovcnt);
         ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
@@ -328,6 +341,10 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
     f->buf_index = 0;
     f->buf_size = pending;
 
+    if (f->shutdown) {
+        return 0;
+    }
+
     len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
                              IO_BUF_SIZE - pending, &local_error);
     if (len > 0) {
@@ -642,6 +659,9 @@ int64_t qemu_ftell(QEMUFile *f)
 
 int qemu_file_rate_limit(QEMUFile *f)
 {
+    if (f->shutdown) {
+        return 1;
+    }
     if (qemu_file_get_error(f)) {
         return 1;
     }
-- 
2.24.1

[PATCH v4 4/6] migration-test: Make sure that multifd and cancel works

[Juan Quintela]

Test that this sequerce works:

- launch source
- launch target
- start migration
- cancel migration
- relaunch target
- do migration again

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>

---

- Wait for 1st trhead to move to cancelled before launching second
  migration
- Add 'to2' parameter to diferentiate 1st and second target.
- Use g_free() instead of free()
---
 tests/qtest/migration-test.c | 112 ++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b6a74a05ce..cf27ebbc9d 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -424,6 +424,14 @@ static void migrate_recover(QTestState *who, const char *uri)
     qobject_unref(rsp);
 }
 
+static void migrate_cancel(QTestState *who)
+{
+    QDict *rsp;
+
+    rsp = wait_command(who, "{ 'execute': 'migrate_cancel' }");
+    qobject_unref(rsp);
+}
+
 static void migrate_set_capability(QTestState *who, const char *capability,
                                    bool value)
 {
@@ -456,6 +464,8 @@ static void migrate_postcopy_start(QTestState *from, QTestState *to)
 typedef struct {
     bool hide_stderr;
     bool use_shmem;
+    /* only launch the target process */
+    bool only_target;
     char *opts_source;
     char *opts_target;
 } MigrateStart;
@@ -571,7 +581,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  arch_source, shmem_opts, args->opts_source,
                                  ignore_stderr);
     g_free(arch_source);
-    *from = qtest_init(cmd_source);
+    if (!args->only_target) {
+        *from = qtest_init(cmd_source);
+    }
     g_free(cmd_source);
 
     cmd_target = g_strdup_printf("-accel kvm -accel tcg%s%s "
@@ -1294,6 +1306,103 @@ static void test_multifd_tcp(void)
     g_free(uri);
 }
 
+/*
+ * This test does:
+ *  source               target
+ *                       migrate_incoming
+ *     migrate
+ *     migrate_cancel
+ *                       launch another target
+ *     migrate
+ *
+ *  And see that it works
+ */
+
+static void test_multifd_tcp_cancel(void)
+{
+    MigrateStart *args = migrate_start_new();
+    QTestState *from, *to, *to2;
+    QDict *rsp;
+    char *uri;
+
+    args->hide_stderr = true;
+
+    if (test_migrate_start(&from, &to, "defer", args)) {
+        return;
+    }
+
+    /*
+     * We want to pick a speed slow enough that the test completes
+     * quickly, but that it doesn't complete precopy even on a slow
+     * machine, so also set the downtime.
+     */
+    /* 1 ms should make it not converge*/
+    migrate_set_parameter_int(from, "downtime-limit", 1);
+    /* 300MB/s */
+    migrate_set_parameter_int(from, "max-bandwidth", 30000000);
+
+    migrate_set_parameter_int(from, "multifd-channels", 16);
+    migrate_set_parameter_int(to, "multifd-channels", 16);
+
+    migrate_set_capability(from, "multifd", "true");
+    migrate_set_capability(to, "multifd", "true");
+
+    /* Start incoming migration from the 1st socket */
+    rsp = wait_command(to, "{ 'execute': 'migrate-incoming',"
+                           "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+    qobject_unref(rsp);
+
+    /* Wait for the first serial output from the source */
+    wait_for_serial("src_serial");
+
+    uri = migrate_get_socket_address(to, "socket-address");
+
+    migrate_qmp(from, uri, "{}");
+
+    wait_for_migration_pass(from);
+
+    migrate_cancel(from);
+
+    args = migrate_start_new();
+    args->only_target = true;
+
+    if (test_migrate_start(&from, &to2, "defer", args)) {
+        return;
+    }
+
+    migrate_set_parameter_int(to2, "multifd-channels", 16);
+
+    migrate_set_capability(to2, "multifd", "true");
+
+    /* Start incoming migration from the 1st socket */
+    rsp = wait_command(to2, "{ 'execute': 'migrate-incoming',"
+                            "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+    qobject_unref(rsp);
+
+    uri = migrate_get_socket_address(to2, "socket-address");
+
+    wait_for_migration_status(from, "cancelled", NULL);
+
+    /* 300ms it should converge */
+    migrate_set_parameter_int(from, "downtime-limit", 300);
+    /* 1GB/s */
+    migrate_set_parameter_int(from, "max-bandwidth", 1000000000);
+
+    migrate_qmp(from, uri, "{}");
+
+    wait_for_migration_pass(from);
+
+    if (!got_stop) {
+        qtest_qmp_eventwait(from, "STOP");
+    }
+    qtest_qmp_eventwait(to2, "RESUME");
+
+    wait_for_serial("dest_serial");
+    wait_for_migration_complete(from);
+    test_migrate_end(from, to2, true);
+    g_free(uri);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -1359,6 +1468,7 @@ int main(int argc, char **argv)
 
     qtest_add_func("/migration/auto_converge", test_migrate_auto_converge);
     qtest_add_func("/migration/multifd/tcp", test_multifd_tcp);
+    qtest_add_func("/migration/multifd/tcp/cancel", test_multifd_tcp_cancel);
 
     ret = g_test_run();
 
-- 
2.24.1

[PATCH v4 5/6] migration: Create migration_is_running()

[Juan Quintela]

This function returns if we are in the middle of a migration.
It is like migration_is_setup_or_active() with CANCELLING and COLO.
Adapt all calers that are needed.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 29 ++++++++++++++++++++++++-----
 migration/migration.h |  1 +
 migration/savevm.c    |  4 +---
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 990bff00c0..1fb0aab44d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -829,6 +829,27 @@ bool migration_is_setup_or_active(int state)
     }
 }
 
+bool migration_is_running(int state)
+{
+    switch (state) {
+    case MIGRATION_STATUS_ACTIVE:
+    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
+    case MIGRATION_STATUS_POSTCOPY_PAUSED:
+    case MIGRATION_STATUS_POSTCOPY_RECOVER:
+    case MIGRATION_STATUS_SETUP:
+    case MIGRATION_STATUS_PRE_SWITCHOVER:
+    case MIGRATION_STATUS_DEVICE:
+    case MIGRATION_STATUS_WAIT_UNPLUG:
+    case MIGRATION_STATUS_CANCELLING:
+    case MIGRATION_STATUS_COLO:
+        return true;
+
+    default:
+        return false;
+
+    }
+}
+
 static void populate_time_info(MigrationInfo *info, MigrationState *s)
 {
     info->has_status = true;
@@ -1077,7 +1098,7 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     MigrationCapabilityStatusList *cap;
     bool cap_list[MIGRATION_CAPABILITY__MAX];
 
-    if (migration_is_setup_or_active(s->state)) {
+    if (migration_is_running(s->state)) {
         error_setg(errp, QERR_MIGRATION_ACTIVE);
         return;
     }
@@ -1590,7 +1611,7 @@ static void migrate_fd_cancel(MigrationState *s)
 
     do {
         old_state = s->state;
-        if (!migration_is_setup_or_active(old_state)) {
+        if (!migration_is_running(old_state)) {
             break;
         }
         /* If the migration is paused, kick it out of the pause */
@@ -1888,9 +1909,7 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
         return true;
     }
 
-    if (migration_is_setup_or_active(s->state) ||
-        s->state == MIGRATION_STATUS_CANCELLING ||
-        s->state == MIGRATION_STATUS_COLO) {
+    if (migration_is_running(s->state)) {
         error_setg(errp, QERR_MIGRATION_ACTIVE);
         return false;
     }
diff --git a/migration/migration.h b/migration/migration.h
index aa9ff6f27b..44b1d56929 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -279,6 +279,7 @@ void migrate_fd_error(MigrationState *s, const Error *error);
 void migrate_fd_connect(MigrationState *s, Error *error_in);
 
 bool migration_is_setup_or_active(int state);
+bool migration_is_running(int state);
 
 void migrate_init(MigrationState *s);
 bool migration_is_blocked(Error **errp);
diff --git a/migration/savevm.c b/migration/savevm.c
index adfdca26ac..f19cb9ec7a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1531,9 +1531,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
     MigrationState *ms = migrate_get_current();
     MigrationStatus status;
 
-    if (migration_is_setup_or_active(ms->state) ||
-        ms->state == MIGRATION_STATUS_CANCELLING ||
-        ms->state == MIGRATION_STATUS_COLO) {
+    if (migration_is_running(ms->state)) {
         error_setg(errp, QERR_MIGRATION_ACTIVE);
         return -EINVAL;
     }
-- 
2.24.1

[PATCH v4 6/6] migration: Don't send data if we have stopped

[Juan Quintela]

If we do a cancel, we got out without one error, but we can't do the
rest of the output as in a normal situation.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index f95d656c26..3fd7fdffcf 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3524,7 +3524,8 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     ram_control_after_iterate(f, RAM_CONTROL_ROUND);
 
 out:
-    if (ret >= 0) {
+    if (ret >= 0
+        && migration_is_setup_or_active(migrate_get_current()->state)) {
         multifd_send_sync_main(rs);
         qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
         qemu_fflush(f);
-- 
2.24.1

Re: [PATCH v4 1/6] migration-test: Use g_free() instead of free()

[Thomas Huth]

On 22/01/2020 12.15, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  tests/qtest/migration-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 26e2e77289..b6a74a05ce 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1291,7 +1291,7 @@ static void test_multifd_tcp(void)
>      wait_for_serial("dest_serial");
>      wait_for_migration_complete(from);
>      test_migrate_end(from, to, true);
> -    free(uri);
> +    g_free(uri);
>  }
>  
>  int main(int argc, char **argv)
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 1/6] migration-test: Use g_free() instead of free()

[Philippe Mathieu-Daudé]

On 1/22/20 12:15 PM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Fixes: b99784ef6c3
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   tests/qtest/migration-test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 26e2e77289..b6a74a05ce 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1291,7 +1291,7 @@ static void test_multifd_tcp(void)
>       wait_for_serial("dest_serial");
>       wait_for_migration_complete(from);
>       test_migrate_end(from, to, true);
> -    free(uri);
> +    g_free(uri);
>   }
>   
>   int main(int argc, char **argv)
> 

Re: [PATCH v4 5/6] migration: Create migration_is_running()

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> This function returns if we are in the middle of a migration.
> It is like migration_is_setup_or_active() with CANCELLING and COLO.
> Adapt all calers that are needed.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Yeh the changes do change behaviour, but they're probably more correct.


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

> ---
>  migration/migration.c | 29 ++++++++++++++++++++++++-----
>  migration/migration.h |  1 +
>  migration/savevm.c    |  4 +---
>  3 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 990bff00c0..1fb0aab44d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -829,6 +829,27 @@ bool migration_is_setup_or_active(int state)
>      }
>  }
>  
> +bool migration_is_running(int state)
> +{
> +    switch (state) {
> +    case MIGRATION_STATUS_ACTIVE:
> +    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> +    case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER:
> +    case MIGRATION_STATUS_SETUP:
> +    case MIGRATION_STATUS_PRE_SWITCHOVER:
> +    case MIGRATION_STATUS_DEVICE:
> +    case MIGRATION_STATUS_WAIT_UNPLUG:
> +    case MIGRATION_STATUS_CANCELLING:
> +    case MIGRATION_STATUS_COLO:
> +        return true;
> +
> +    default:
> +        return false;
> +
> +    }
> +}
> +
>  static void populate_time_info(MigrationInfo *info, MigrationState *s)
>  {
>      info->has_status = true;
> @@ -1077,7 +1098,7 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>      MigrationCapabilityStatusList *cap;
>      bool cap_list[MIGRATION_CAPABILITY__MAX];
>  
> -    if (migration_is_setup_or_active(s->state)) {
> +    if (migration_is_running(s->state)) {
>          error_setg(errp, QERR_MIGRATION_ACTIVE);
>          return;
>      }
> @@ -1590,7 +1611,7 @@ static void migrate_fd_cancel(MigrationState *s)
>  
>      do {
>          old_state = s->state;
> -        if (!migration_is_setup_or_active(old_state)) {
> +        if (!migration_is_running(old_state)) {
>              break;
>          }
>          /* If the migration is paused, kick it out of the pause */
> @@ -1888,9 +1909,7 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>          return true;
>      }
>  
> -    if (migration_is_setup_or_active(s->state) ||
> -        s->state == MIGRATION_STATUS_CANCELLING ||
> -        s->state == MIGRATION_STATUS_COLO) {
> +    if (migration_is_running(s->state)) {
>          error_setg(errp, QERR_MIGRATION_ACTIVE);
>          return false;
>      }
> diff --git a/migration/migration.h b/migration/migration.h
> index aa9ff6f27b..44b1d56929 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -279,6 +279,7 @@ void migrate_fd_error(MigrationState *s, const Error *error);
>  void migrate_fd_connect(MigrationState *s, Error *error_in);
>  
>  bool migration_is_setup_or_active(int state);
> +bool migration_is_running(int state);
>  
>  void migrate_init(MigrationState *s);
>  bool migration_is_blocked(Error **errp);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index adfdca26ac..f19cb9ec7a 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1531,9 +1531,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>      MigrationState *ms = migrate_get_current();
>      MigrationStatus status;
>  
> -    if (migration_is_setup_or_active(ms->state) ||
> -        ms->state == MIGRATION_STATUS_CANCELLING ||
> -        ms->state == MIGRATION_STATUS_COLO) {
> +    if (migration_is_running(ms->state)) {
>          error_setg(errp, QERR_MIGRATION_ACTIVE);
>          return -EINVAL;
>      }
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v4 6/6] migration: Don't send data if we have stopped

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> If we do a cancel, we got out without one error, but we can't do the
> rest of the output as in a normal situation.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

OK.


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

> ---
>  migration/ram.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f95d656c26..3fd7fdffcf 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3524,7 +3524,8 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      ram_control_after_iterate(f, RAM_CONTROL_ROUND);
>  
>  out:
> -    if (ret >= 0) {
> +    if (ret >= 0
> +        && migration_is_setup_or_active(migrate_get_current()->state)) {
>          multifd_send_sync_main(rs);
>          qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>          qemu_fflush(f);
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v4 4/6] migration-test: Make sure that multifd and cancel works

[Peter Maydell]

On Wed, 22 Jan 2020 at 11:20, Juan Quintela <quintela@redhat.com> wrote:
>
> Test that this sequerce works:
>
> - launch source
> - launch target
> - start migration
> - cancel migration
> - relaunch target
> - do migration again
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

A year-old commit, but we've just got around to running Coverity
on the tests/ directory, and it spotted this one:

>  static void migrate_set_capability(QTestState *who, const char *capability,
>                                     bool value)

The 3rd argument to migrate_set_capability() is a bool...


> +static void test_multifd_tcp_cancel(void)
> +{

> +    migrate_set_parameter_int(from, "downtime-limit", 1);
> +    /* 300MB/s */
> +    migrate_set_parameter_int(from, "max-bandwidth", 30000000);
> +
> +    migrate_set_parameter_int(from, "multifd-channels", 16);
> +    migrate_set_parameter_int(to, "multifd-channels", 16);
> +
> +    migrate_set_capability(from, "multifd", "true");
> +    migrate_set_capability(to, "multifd", "true");

...but here you pass it the character string '"true"' rather than
the boolean value 'true'.

This works by fluke since the implicit comparison of the literal string
against NULL will evaluate to true, but it isn't really right :-)

CID 1432373, 1432292, 1432288.

There seem to be 7 uses of the string "true" when the boolean
was intended; I don't know why Coverity only found 3 issues.

thanks
-- PMM

Re: [PATCH v4 4/6] migration-test: Make sure that multifd and cancel works

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Wed, 22 Jan 2020 at 11:20, Juan Quintela <quintela@redhat.com> wrote:
> >
> > Test that this sequerce works:
> >
> > - launch source
> > - launch target
> > - start migration
> > - cancel migration
> > - relaunch target
> > - do migration again
> >
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> A year-old commit, but we've just got around to running Coverity
> on the tests/ directory, and it spotted this one:
> 
> >  static void migrate_set_capability(QTestState *who, const char *capability,
> >                                     bool value)
> 
> The 3rd argument to migrate_set_capability() is a bool...

oops

> 
> > +static void test_multifd_tcp_cancel(void)
> > +{
> 
> > +    migrate_set_parameter_int(from, "downtime-limit", 1);
> > +    /* 300MB/s */
> > +    migrate_set_parameter_int(from, "max-bandwidth", 30000000);
> > +
> > +    migrate_set_parameter_int(from, "multifd-channels", 16);
> > +    migrate_set_parameter_int(to, "multifd-channels", 16);
> > +
> > +    migrate_set_capability(from, "multifd", "true");
> > +    migrate_set_capability(to, "multifd", "true");
> 
> ...but here you pass it the character string '"true"' rather than
> the boolean value 'true'.
> 
> This works by fluke since the implicit comparison of the literal string
> against NULL will evaluate to true, but it isn't really right :-)
> 
> CID 1432373, 1432292, 1432288.
> 
> There seem to be 7 uses of the string "true" when the boolean
> was intended; I don't know why Coverity only found 3 issues.

I'll send a patch.

Dave

> thanks
> -- PMM
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK