On 03/22/2017 12:26 PM, Jiri Denemark wrote: > On Fri, Mar 17, 2017 at 14:39:01 -0400, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1300769 >> >> If the migration flags indicate this migration will be using TLS, >> then while we have connection in the Begin phase check and setup the >> TLS environment that will be used by virMigrationRun during the Perform >> phase for the source to configure TLS. >> >> This creates at least an "-object tls-creds-x509,endpoint=client,..." >> and potentially an "-object secret,..." to handle the passphrase response >> to access the TLS credentials. The alias/id used for the TLS objects >> will contain "libvirt_migrate" as a mechanism to signify that libvirt >> started the migration on the source (reaping benefits possibly). >> >> Once the objects are created, the code will set the "tls-creds" and >> "tls-hostname" migration parameters to signify usage of TLS. > > Looks like a copy&paste from the previous patch. Is it necessary? > I can reword >> >> Since qemuProcessRecoverMigrationOut will cancel outgoing migrations >> that are still in the QEMU_MIGRATION_PHASE_PERFORM{2|3} stages, there's >> no need to do anything special as the Perform cleanup and Cancel phases >> will reset the environment. > > Sure, TLS will be reset because you added a call to > qemuMigrationResetTLS() in qemuMigrationCancel(). Just drop this > paragraph which contradicts what you actually did. > OK - so as I figured out things the RecoverMigrationOut only mattered when certain migration phase requirements were met - that wouldn't happen because the setting and clearing of the parameters is done in the PrepareAny and Run phases as well as Cancel. >> >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> >> --- >> src/qemu/qemu_migration.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 53 insertions(+) >> >> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c >> index 42074f0..5acae6e 100644 >> --- a/src/qemu/qemu_migration.c >> +++ b/src/qemu/qemu_migration.c > ... >> @@ -5075,6 +5086,38 @@ qemuMigrationRun(virQEMUDriverPtr driver, >> if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig, graphicsuri) < 0) >> VIR_WARN("unable to provide data for graphics client relocation"); >> >> + if (flags & VIR_MIGRATE_TLS) { >> + cfg = virQEMUDriverGetConfig(driver); >> + >> + /* Begin/CheckSetupTLS already set up migTLSAlias, the following >> + * assumes that and adds the TLS objects to the domain. */ >> + if (qemuMigrationAddTLSObjects(driver, vm, cfg, false, >> + QEMU_ASYNC_JOB_MIGRATION_OUT, >> + &tlsAlias, &secAlias, migParams) < 0) >> + goto cleanup; >> + >> + /* We need to add the tls-hostname only for special circumstances, >> + * e.g. for a fd: or exec: based migration. As it turns out the >> + * CONNECT_HOST turns into an FD migration (see below). */ > > Specifically, we need to add tls-hostname whenever QEMU itself does not > connect directly to the destination, which means > MIGRATION_DEST_CONNECT_HOST and MIGRATION_DEST_FD. > Sure - something that wasn't obvious on my first pass through the code. In fact having CONNECT_HOST change into FD later on wasn't as obvious to me until I dug through the code. I can change the comment to: /* We need to add tls-hostname whenever QEMU itself does not * connect directly to the destination. NB: The CONNECT_HOST * turns into a FD migration below in qemuMigrationConnect */ >> + if (spec->destType == MIGRATION_DEST_CONNECT_HOST || >> + spec->destType == MIGRATION_DEST_FD) { >> + if (VIR_STRDUP(migParams->migrateTLSHostname, >> + spec->dest.host.name) < 0) >> + goto cleanup; >> + } else { >> + /* Be sure there's nothing from a previous migration */ >> + if (VIR_STRDUP(migParams->migrateTLSHostname, "") < 0) >> + goto cleanup; >> + } >> + } else { >> + /* If we support setting the tls-creds, be sure to always reset >> + * the migration parameters when this migration isn't using TLS */ >> + if ((qemuMigrationCheckTLSCreds(driver, vm, >> + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) || >> + (qemuMigrationSetEmptyTLSParams(priv, migParams) < 0)) and I've fixed the extra () here as well John >> + goto cleanup; >> + } >> + >> if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK | >> QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) { >> if (mig->nbd) { > > Jirka > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
