[email protected] writes: > From: Nguyen Dinh Phi <[email protected]> >
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 <[email protected]> > --- > 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);
