Re: [PATCH 1/3] tests/migration: Factor out per-process launch and setup helpers

[Fabiano Rosas <farosas@suse.de>]

phind.uet@gmail.com writes:

> From: Nguyen Dinh Phi <phind.uet@gmail.com>
>

Hi, sorry for the delay, this series conflicts with some ongoing work
I'm doing so I had to find time to double check.

> migrate_start() always spawned source and destination together, and
> migrate_start_set_capabilities() required both QTestState pointers at
> once. This coupling prevents launching a third QEMU instance or
> re-configuring an existing destination as a new source.
>

What about migrate_end? Don't you need to allow instances to be freed
individually as well?

> Split the logic into three public helpers:
>
>   migrate_launch_source() / migrate_launch_dest()
>     Spawn a single QEMU process and record its serial file path on
>     the returned QTestState.
>
>   migrate_setup_instance()
>     Register the event callback and apply migration capabilities to
>     one already-running process.  Can be called again to re-wire the
>     same process for a different role.
>
> migrate_start() is kept for backward compatibility and now delegates
> to these helpers internally.
>
> Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
> ---
>  tests/qtest/migration/framework.c | 253 +++++++++++++++++-------------
>  tests/qtest/migration/framework.h |  18 ++-
>  2 files changed, 165 insertions(+), 106 deletions(-)
>
> diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
> index a830b96f41..305b8968ff 100644
> --- a/tests/qtest/migration/framework.c
> +++ b/tests/qtest/migration/framework.c
> @@ -208,9 +208,11 @@ static QList *migrate_start_get_qmp_capabilities(const MigrateStart *args)
>      return capabilities;
>  }
>  
> -static void migrate_start_set_capabilities(QTestState *from, QTestState *to,
> -                                           MigrateStart *args)
> +void migrate_setup_instance(QTestState *who, QTestMigrationState *state,
> +                   MigrateStart *args)
>  {
> +    qtest_qmp_set_event_callback(who, migrate_watch_for_events, state);

You could pass in a defer_connect boolean in here and exit at this
point, no need to set the callback operation in the caller as well.

> +    *state = (QTestMigrationState) { .suspend_me = args->suspend_me };
>      /*
>       * MigrationCapability_lookup and MIGRATION_CAPABILITY_ constants
>       * are from qapi-types-migration.h.
> @@ -220,25 +222,13 @@ static void migrate_start_set_capabilities(QTestState *from, QTestState *to,
>       * Enable return path first, since other features depend on it.
>       */
>      if (args->caps[MIGRATION_CAPABILITY_RETURN_PATH]) {
> -        if (from) {
> -            migrate_set_capability(from, "return-path", true);
> -        }
> -        if (to) {
> -            migrate_set_capability(to, "return-path", true);
> -        }
> +        migrate_set_capability(who, "return-path", true);
>      }
>  
>      for (uint8_t i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
> -        if (!args->caps[i]) {
> -            continue;
> -        }
> -        if (from) {
> -            migrate_set_capability(from,
> -                            MigrationCapability_lookup.array[i], true);
> -        }
> -        if (to) {
> -            migrate_set_capability(to,
> -                            MigrationCapability_lookup.array[i], true);
> +        if (args->caps[i]) {
> +            migrate_set_capability(who, MigrationCapability_lookup.array[i],
> +                                   true);
>          }
>      }
>  
> @@ -246,26 +236,17 @@ static void migrate_start_set_capabilities(QTestState *from, QTestState *to,
>       * Always enable migration events.  Libvirt always uses it, let's try
>       * to mimic as closer as that.
>       */
> -    migrate_set_capability(from, "events", true);
> -    if (!args->defer_target_connect && to) {
> -        migrate_set_capability(to, "events", true);
> -    }
> +    migrate_set_capability(who, "events", true);
>  
> -    /*
> +     /*

Please keep the open comment mark in the correct place.

>       * Default number of channels should be fine for most
>       * tests. Individual tests can override by calling
>       * migrate_set_parameter() directly.
>       */
>      if (args->caps[MIGRATION_CAPABILITY_MULTIFD]) {
> -        migrate_set_parameter_int(from, "multifd-channels",
> +        migrate_set_parameter_int(who, "multifd-channels",
>                                    MULTIFD_TEST_CHANNELS);
> -        if (to) {
> -            migrate_set_parameter_int(to, "multifd-channels",
> -                                      MULTIFD_TEST_CHANNELS);
> -        }
>      }
> -
> -    return;
>  }
>  
>  static char *test_shmem_path(void)
> @@ -308,12 +289,28 @@ static char *migrate_mem_type_get_opts(MemType type, const char *memory_size)
>      return opts;
>  }
>  
> -int migrate_args(char **from, char **to, MigrateStart *args)
> +/*
> + * Build a QEMU command line for one migration participant.
> + *
> + * @serial_name  : serial output file basename under tmpfs
> + * @is_target    : set to true if the new qemu process is destination side

If we now have launch_source|dest, we shouldn't need to pass this flag in.

> + * @extra_opts   : extra arguments
> + * @defer_events : when true and is_target is set, use the command-line
> + *                 event flag instead of the QMP capability (for deferred
> + *                 monitor connections)

Same here.

> + * @args         : common process settings
> + *
> + * Returns 0 and sets *cmd_out on success.  Returns -1 and calls
> + * g_test_skip() if the machine type is not available.
> + */
> +static int migrate_build_cmd(const char *serial_name,
> +                             bool is_target,
> +                             const char *extra_opts,
> +                             bool defer_events,
> +                             MigrateStart *args,
> +                             char **cmd_out)
>  {
> -    /* options for source and target */
>      g_autofree gchar *arch_opts = NULL;
> -    gchar *cmd_source = NULL;
> -    gchar *cmd_target = NULL;
>      const gchar *ignore_stderr;
>      g_autofree char *mem_object = NULL;
>      const char *kvm_opts = NULL;
> @@ -323,16 +320,10 @@ int migrate_args(char **from, char **to, MigrateStart *args)
>      g_autofree char *machine = NULL;
>      const char *bootpath = bootfile_get();
>      g_autofree char *memory_backend = NULL;
> -    const char *events;
>  
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>          memory_size = "150M";
> -
> -        if (g_str_equal(arch, "i386")) {
> -            machine_alias = "pc";
> -        } else {
> -            machine_alias = "q35";
> -        }
> +        machine_alias = g_str_equal(arch, "i386") ? "pc" : "q35";

We usually wouldn't include changes of this kind among other
things. This could be an individual patch if you think it's worth it.
(I think we should just leave it alone)

>          arch_opts = g_strdup_printf(
>              "-drive if=none,id=d0,file=%s,format=raw "
>              "-device ide-hd,drive=d0,secs=1,cyls=1,heads=1", bootpath);
> @@ -346,13 +337,13 @@ int migrate_args(char **from, char **to, MigrateStart *args)
>          end_address = S390_TEST_MEM_END;
>      } else if (strcmp(arch, "ppc64") == 0) {
>          memory_size = "256M";
> -        start_address = PPC_TEST_MEM_START;
> -        end_address = PPC_TEST_MEM_END;
>          machine_alias = "pseries";
>          machine_opts = "vsmt=8";
>          arch_opts = g_strdup_printf(
>              "-nodefaults -machine " PSERIES_DEFAULT_CAPABILITIES " "
>              "-bios %s", bootpath);
> +        start_address = PPC_TEST_MEM_START;
> +        end_address = PPC_TEST_MEM_END;

This moved for no reason...

>      } else if (strcmp(arch, "aarch64") == 0) {
>          memory_size = "150M";
>          machine_alias = "virt";
> @@ -394,53 +385,103 @@ int migrate_args(char **from, char **to, MigrateStart *args)
>      }
>  
>      if (!qtest_has_machine(machine_alias)) {
> -        g_autofree char *msg = g_strdup_printf("machine %s not supported", machine_alias);
> +        g_autofree char *msg = g_strdup_printf("machine %s not supported",
> +                                               machine_alias);

Same here, this can stay as it is.

>          g_test_skip(msg);
>          return -1;
>      }
>  
>      machine = resolve_machine_version(machine_alias, QEMU_ENV_SRC,
>                                        QEMU_ENV_DST);
> -
>      g_test_message("Using machine type: %s", machine);
>  
> -    cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
> -                                 "-machine %s,%s "
> -                                 "-name source,debug-threads=on "
> -                                 "%s "
> -                                 "-serial file:%s/src_serial "
> -                                 "%s %s %s",
> -                                 kvm_opts ? kvm_opts : "",
> -                                 machine, machine_opts,
> -                                 memory_backend, tmpfs,
> -                                 arch_opts ? arch_opts : "",
> -                                 args->opts_source ? args->opts_source : "",
> -                                 ignore_stderr);
> +    if (is_target) {
> +        /*
> +         * If the monitor connection is deferred, enable events on the command
> +         * line so none are missed. This is for testing only, do not set
> +         * migration options like this in general.
> +         */
> +        const char *events = defer_events ? "-global migration.x-events=on"
> +                                          : "";
> +        *cmd_out = g_strdup_printf(
> +            "-accel kvm%s -accel tcg "
> +            "-machine %s,%s "
> +            "-name target,debug-threads=on "
> +            "%s "
> +            "-serial file:%s/%s "
> +            "-incoming defer "
> +            "%s %s %s %s",
> +            kvm_opts ? kvm_opts : "",
> +            machine, machine_opts,
> +            memory_backend, tmpfs, serial_name,
> +            events,
> +            arch_opts ? arch_opts : "",
> +            extra_opts ? extra_opts : "",
> +            ignore_stderr);
> +    } else {
> +        *cmd_out = g_strdup_printf(
> +            "-accel kvm%s -accel tcg "
> +            "-machine %s,%s "
> +            "-name source,debug-threads=on "
> +            "%s "
> +            "-serial file:%s/%s "
> +            "%s %s %s",
> +            kvm_opts ? kvm_opts : "",
> +            machine, machine_opts,
> +            memory_backend, tmpfs, serial_name,
> +            arch_opts ? arch_opts : "",
> +            extra_opts ? extra_opts : "",
> +            ignore_stderr);
> +    }

Let's move all the instance-specific information into the upper levels,
this conditional can go away, all information comes in via extra_opts:

    *cmd_out = g_strdup_printf(
        "-accel kvm%s -accel tcg "
        "-machine %s,%s "
        "%s "
        "-serial file:%s/%s "
        "%s %s %s",
        kvm_opts ? kvm_opts : "",
        machine, machine_opts,
        memory_backend, tmpfs, serial_name,
        arch_opts ? arch_opts : "",
        extra_opts ? extra_opts : "",
        ignore_stderr);

>  
> -    /*
> -     * If the monitor connection is deferred, enable events on the command line
> -     * so none are missed.  This is for testing only, do not set migration
> -     * options like this in general.
> -     */
> -    events = args->defer_target_connect ? "-global migration.x-events=on" : "";
> -
> -    cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
> -                                 "-machine %s,%s "
> -                                 "-name target,debug-threads=on "
> -                                 "%s "
> -                                 "-serial file:%s/dest_serial "
> -                                 "-incoming defer "
> -                                 "%s %s %s %s",
> -                                 kvm_opts ? kvm_opts : "",
> -                                 machine, machine_opts,
> -                                 memory_backend, tmpfs,
> -                                 events,
> -                                 arch_opts ? arch_opts : "",
> -                                 args->opts_target ? args->opts_target : "",
> -                                 ignore_stderr);
> -
> -    *from = cmd_source;
> -    *to = cmd_target;
> +    return 0;
> +}
> +
> +/*
> + * Launch a QEMU process as a migration source.
> + * Returns the QTestState on success, or NULL otherwise
> + */
> +QTestState *migrate_launch_source(const char *serial_name, MigrateStart *args)

If this is "launch_source" we already know the serial name is
"src_serial", no need to pass in.

The launch_* functions could return bool and receive the **qts as first
argument.

> +{
> +    g_autofree char *cmd = NULL;
> +    g_autoptr(QList) capabilities = migrate_start_get_qmp_capabilities(args);
> +
> +    if (migrate_build_cmd(serial_name, false, args->opts_source,
> +                          false, args, &cmd)) {

Here's what I'm thinking to remove the instance-specific information
from migrate_build_cmd:

static bool migrate_launch_instance(QTestState **qts, MigrateStart *args,
                                    QTestMigrationState *state, const char *env,
                                    bool defer_connect, char *cmd)
{
    g_autoptr(QList) capabilities = migrate_start_get_qmp_capabilities(args);

    *qts = qtest_init_ext(env, cmd, capabilities, !defer_connect);
    if (*qts) {
        migrate_setup_instance(*qts, state, args, defer_connect);
    }

    return !!*qts;
}

static bool migrate_build_dest(MigrateStart *args, char **cmd)
{
    g_autofree char *opts = NULL;
    int r;

    if (args->only_source) {
        return true;
    }

    /*
     * If the monitor connection is deferred, enable events on the command
     * line so none are missed. This is for testing only, do not set
     * migration options like this in general.
     */

    opts = g_strdup_printf(
        "-name target,debug-threads=on "
        "-incoming defer "
        "%s %s",
        args->defer_target_connect ? "-global migration.x-events=on" : "",
        args->opts_target ?: "");

    r = migrate_build_cmd("dest_serial", opts, args, cmd);
    return r == 0;
}

static bool migrate_launch_dest(QTestState **qts, MigrateStart *args)
{
    g_autofree char *cmd = NULL;

    if (!migrate_build_dest(args, &cmd)){
        return false;
    }

    if (cmd && !migrate_launch_instance(qts, args, &dst_state, QEMU_ENV_DST,
                                        args->defer_target_connect, cmd)) {
        return false;
    }

    return true;
}

migrate_args -> migrate_build_dest
migrate_start -> migrate_launch_dest -> migrate_build_dest

And similarly for the source.

What do you think?

> +        return NULL;
> +    }
> +    return qtest_init_ext(QEMU_ENV_SRC, cmd, capabilities, true);
> +}
> +
> +/*
> + * Launch a QEMU process as a migration destination.
> + * Returns the QTestState on success, or NULL otherwise
> + */
> +QTestState *migrate_launch_dest(const char *serial_name, MigrateStart *args)
> +{
> +    g_autofree char *cmd = NULL;
> +    g_autoptr(QList) capabilities = migrate_start_get_qmp_capabilities(args);
> +
> +    if (migrate_build_cmd(serial_name, true, args->opts_target,
> +                          args->defer_target_connect, args, &cmd)) {
> +        return NULL;
> +    }
> +    return qtest_init_ext(QEMU_ENV_DST, cmd, capabilities,
> +                          !args->defer_target_connect);
> +}
> +
> +int migrate_args(char **from, char **to, MigrateStart *args)
> +{
> +    if (migrate_build_cmd("src_serial", false, args->opts_source,
> +                          false, args, from)) {
> +        return -1;
> +    }
> +    if (migrate_build_cmd("dest_serial", true, args->opts_target,
> +                          args->defer_target_connect, args, to)) {
> +        g_free(*from);
> +        *from = NULL;
> +        return -1;
> +    }
>      return 0;
>  }
>  
> @@ -482,43 +523,45 @@ static void migrate_mem_type_cleanup(MemType type)
>  
>  int migrate_start(QTestState **from, QTestState **to, MigrateStart *args)
>  {
> -    g_autofree gchar *cmd_source = NULL;
> -    g_autofree gchar *cmd_target = NULL;
> -    g_autoptr(QList) capabilities = migrate_start_get_qmp_capabilities(args);
> -
>      if (!migrate_mem_type_prepare(args->mem_type)) {
>          return -1;
>      }
>  
> -    dst_state = (QTestMigrationState) { };
> -    src_state = (QTestMigrationState) { };
> +    src_state = (QTestMigrationState){};
> +    dst_state = (QTestMigrationState){};
>      bootfile_create(qtest_get_arch(), tmpfs, args->suspend_me);
> -    src_state.suspend_me = args->suspend_me;
> -
> -    if (migrate_args(&cmd_source, &cmd_target, args)) {
> -        return -1;
> -    }
>  
>      if (!args->only_target) {

The only_* checks can go into the _launch_source level, keeps this
function cleaner.

> -        *from = qtest_init_ext(QEMU_ENV_SRC, cmd_source, capabilities, true);
> -        qtest_qmp_set_event_callback(*from,
> -                                     migrate_watch_for_events,
> -                                     &src_state);
> +        *from = migrate_launch_source("src_serial", args);
> +        if (!*from) {
> +            return -1;
> +        }
> +        migrate_setup_instance(*from, &src_state, args);
>      }
>  
>      if (!args->only_source) {
> -        *to = qtest_init_ext(QEMU_ENV_DST, cmd_target, capabilities,
> -                             !args->defer_target_connect);
> -        qtest_qmp_set_event_callback(*to,
> -                                     migrate_watch_for_events,
> -                                     &dst_state);
> +        *to = migrate_launch_dest("dest_serial", args);
> +        if (!*to) {
> +            if (!args->only_target) {
> +                qtest_quit(*from);
> +            }
> +            return -1;
> +        }
> +        if (!args->defer_target_connect) {

This check would move into migrate_setup_instance.

> +            migrate_setup_instance(*to, &dst_state, args);
> +        } else {
> +            /*
> +             * The monitor is not connected yet; only register the event
> +             * callback so no events are missed.
> +             * The caller must call migrate_setup_instance() after
> +             * qtest_connect() if it wants to setup any capability
> +             */
> +            qtest_qmp_set_event_callback(*to, migrate_watch_for_events,
> +                                         &dst_state);
> +        }
>      }
>  
>      migrate_mem_type_cleanup(args->mem_type);
> -    migrate_start_set_capabilities(*from,
> -                                   args->only_source ? NULL : *to,
> -                                   args);
> -
>      return 0;
>  }
>  
> diff --git a/tests/qtest/migration/framework.h b/tests/qtest/migration/framework.h
> index 941cbd7102..f054ef8f4d 100644
> --- a/tests/qtest/migration/framework.h
> +++ b/tests/qtest/migration/framework.h
> @@ -225,10 +225,27 @@ typedef struct {
>      bool live;
>  } MigrateCommon;
>  
> +typedef struct QTestMigrationState QTestMigrationState;
> +
>  void wait_for_serial(const char *side);
>  void migrate_prepare_for_dirty_mem(QTestState *from);
>  void migrate_wait_for_dirty_mem(QTestState *from, QTestState *to);
>  
> +/*
> + * Start a QEMU process which has serial set to @serial_name
> + */
> +QTestState *migrate_launch_source(const char *serial_name, MigrateStart *args);
> +/*
> + * Start a QEMU process which has serial set to @serial_name
> + */
> +QTestState *migrate_launch_dest(const char *serial_name, MigrateStart *args);
> +/*
> + * Registers the event callback and sets migration capabilities on an
> + * already-running process
> + */
> +void migrate_setup_instance(QTestState *who, QTestMigrationState *state,
> +                   MigrateStart *args);

These don't need to be exported, keep them static.

> +
>  int migrate_args(char **from, char **to, MigrateStart *args);
>  int migrate_start(QTestState **from, QTestState **to, MigrateStart *args);
>  void migrate_end(QTestState *from, QTestState *to, bool test_dest);
> @@ -240,7 +257,6 @@ int test_precopy_common(MigrateCommon *args);
>  void test_precopy_unix_common(MigrateCommon *args);
>  void test_file_common(MigrateCommon *args, bool stop_src);
>  
> -typedef struct QTestMigrationState QTestMigrationState;
>  QTestMigrationState *get_src(void);
>  QTestMigrationState *get_dst(void);