Re: [libvirt] [PATCH 2/3] qemu: RDMA migration support using 'x-rdma' URI

On Fri, Jul 26, 2013 at 13:47:44 -0400, mrhines(a)linux.vnet.ibm.com wrote: > From: "Michael R. Hines" <mrhines(a)us.ibm.com&gt; > > QEMU has in tree now for version 1.6 support for RDMA Live migration. > Full documenation of the feature: http://wiki.qemu.org/Features/RDMALiveMigration > > This patch includes mainly making all the locations in libvirt where > the 'tcp' string was hard-coded to be more flexible to use more than > one protocol. > > While the RDMA protocol has been extensively tested (from multiple > companies as well as virt-test), the protocol 'x-rdma' will later be > renamed to 'rdma' after the community has allowed the feature more cooking. This does not prevent us from calling the protocol "rdma" right away and possibly translating it to "x-rdma". However, I don't think we actually want to commit patches for rdma migration before QEMU changes the name to "rdma".

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 5dc3c9e..94d17c6 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -234,6 +234,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, > > "vnc-share-policy", /* 150 */ > "device-del-event", > + > + "x-rdma", /* 152 */ > ); > > struct _virQEMUCaps { > @@ -1101,6 +1103,7 @@ virQEMUCapsComputeCmdFlags(const char *help, > * -incoming unix (qemu >= 0.12.0) > * -incoming fd (qemu >= 0.12.0) > * -incoming stdio (all earlier kvm) > + * -incoming x-rdma (qemu >= 1.6.0) > * > * NB, there was a pre-kvm-79 'tcp' support, but it > * was broken, because it blocked the monitor console > @@ -2437,6 +2440,10 @@ virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps, > char *archstr = NULL; > int ret = -1; > > + if (qemuCaps->version >= MIN_X_RDMA_VERSION) { > + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_X_RDMA); > + } > + > if (!(archstr = qemuMonitorGetTargetArch(mon))) > return -1; > This is wrong. First, you're adding this into a totally wrong place and second, we need a better detection which is not based on qemu version.

> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 19001b9..de20d23 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -2169,7 +2169,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, > virDomainDefPtr *def, > virStreamPtr st, > unsigned int port, > - unsigned long flags) > + unsigned long flags, > + const char *protocol) > { > virDomainObjPtr vm = NULL; > virDomainEventPtr event = NULL; > @@ -2280,7 +2281,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, > * and there is at least one IPv6 address configured > */ > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_IPV6_MIGRATION) && > - getaddrinfo("::", NULL, &hints, &info) == 0) { > + getaddrinfo("::", NULL, &hints, &info) == 0 && > + !strstr(protocol, "rdma")) { > freeaddrinfo(info); > listenAddr = "[::]"; > } else { Is there any reason why RDMA migration does not work over IPv6?

> @@ -2291,7 +2293,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, > /* QEMU will be started with -incoming [::]:port > * or -incoming 0.0.0.0:port > */ > - if (virAsprintf(&migrateFrom, "tcp:%s:%d", listenAddr, port) < 0) > + if (virAsprintf(&migrateFrom, "%s:%s:%d", protocol, > + listenAddr, port) < 0) > goto cleanup; > } > > @@ -2482,7 +2485,7 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver, > > ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, > cookieout, cookieoutlen, def, > - st, 0, flags); > + st, 0, flags, "tcp"); > return ret; > } > > @@ -2502,6 +2505,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, > static int port = 0; > int this_port; > char *hostname = NULL; > + const char *protocol = NULL; > + char *well_formed_protocol = NULL; > const char *p; > char *uri_str = NULL; > int ret = -1; > @@ -2550,20 +2555,29 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, > if (virAsprintf(uri_out, "tcp:%s:%d", hostname, this_port) < 0) > goto cleanup; > } else { > - /* Check the URI starts with "tcp:". We will escape the > + /* Check the URI starts with a valid prefix. We will escape the > * URI when passing it to the qemu monitor, so bad > * characters in hostname part don't matter. > */ > - if (!(p = STRSKIP(uri_in, "tcp:"))) { > - virReportError(VIR_ERR_INVALID_ARG, "%s", > - _("only tcp URIs are supported for KVM/QEMU" > - " migrations")); > + > + protocol = strtok(strdup(uri_in), ":"); > + if (protocol) { > + if (virAsprintf(&well_formed_protocol, "%s://", protocol) < 0) > + goto cleanup; > + } > + > + /* Make sure it's a valid protocol */ > + if (!(p = STRSKIP(uri_in, "tcp:")) && > + !(p = STRSKIP(uri_in, "x-rdma:"))) { > + virReportError(VIR_ERR_INVALID_ARG, _("URI %s (%s) not supported" > + " for KVM/QEMU migrations"), protocol, uri_in); > goto cleanup; > } > > - /* Convert uri_in to well-formed URI with // after tcp: */ > - if (!(STRPREFIX(uri_in, "tcp://"))) { > - if (virAsprintf(&uri_str, "tcp://%s", p) < 0) > + > + /* Convert uri_in to well-formed URI with // after colon */ > + if (!(STRPREFIX(uri_in, well_formed_protocol))) { > + if (virAsprintf(&uri_str, "%s://%s", protocol, p) < 0) > goto cleanup; > } Just because we did the mistake with tcp protocol we don't have to repeat it with rdma. Just change the rdma protocol to always be well-formed, i.e., rdma://host

> > @@ -2602,10 +2616,20 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, > > ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, > cookieout, cookieoutlen, def, > - NULL, this_port, flags); > + NULL, this_port, flags, > + protocol ? protocol : "tcp"); > cleanup: > virURIFree(uri); > VIR_FREE(hostname); > + > + if (protocol) { > + VIR_FREE(protocol); > + } > + > + if (well_formed_protocol) { > + VIR_FREE(well_formed_protocol); > + } > + You're not supposed to check if a variable you're about to free is non-NULL. Running make syntax-check would have warned you.