5:17 a.m.
Hey!
Presented herewith is take 4 from tunnelled migration addressing all previous
comments. No functional changes from v2, only revert to v1 on the top level
migration functions and removing unnecessary usage of virReportError.
Thanks,
Joao
Bob Liu (1):
libxl: add tunnelled migration support
Joao Martins (1):
libxl: refactor libxlDomainMigrationPrepare
src/libxl/libxl_driver.c | 58 ++++++-
src/libxl/libxl_migration.c | 367 +++++++++++++++++++++++++++++++++++++-------
src/libxl/libxl_migration.h | 9 ++
3 files changed, 381 insertions(+), 53 deletions(-)
--
2.1.4
5:17 a.m.
New subject: [libvirt] [PATCH v3 1/2] libxl: refactor libxlDomainMigrationPrepare
The newly introduced function libxlDomainMigrationPrepareAny
will be shared between P2P and tunnelled variations.
Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com>
---
src/libxl/libxl_migration.c | 92 +++++++++++++++++++++++++++------------------
1 file changed, 56 insertions(+), 36 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index a471d2a..7beabd2 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -483,41 +483,26 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr driver,
return def;
}
-int
-libxlDomainMigrationPrepare(virConnectPtr dconn,
- virDomainDefPtr *def,
- const char *uri_in,
- char **uri_out,
- const char *cookiein,
- int cookieinlen,
- unsigned int flags)
+static int
+libxlDomainMigrationPrepareAny(virConnectPtr dconn,
+ virDomainDefPtr *def,
+ const char *cookiein,
+ int cookieinlen,
+ libxlMigrationCookiePtr *mig,
+ char **xmlout,
+ bool *taint_hook)
{
libxlDriverPrivatePtr driver = dconn->privateData;
libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
- libxlMigrationCookiePtr mig = NULL;
- virDomainObjPtr vm = NULL;
- char *hostname = NULL;
- char *xmlout = NULL;
- unsigned short port;
- char portstr[100];
- virURIPtr uri = NULL;
- virNetSocketPtr *socks = NULL;
- size_t nsocks = 0;
- int nsocks_listen = 0;
- libxlMigrationDstArgs *args = NULL;
- bool taint_hook = false;
- libxlDomainObjPrivatePtr priv = NULL;
- size_t i;
- int ret = -1;
- if (libxlMigrationEatCookie(cookiein, cookieinlen, &mig) < 0)
- goto error;
+ if (libxlMigrationEatCookie(cookiein, cookieinlen, mig) < 0)
+ return -1;
- if (mig->xenMigStreamVer > LIBXL_SAVE_VERSION) {
+ if ((*mig)->xenMigStreamVer > LIBXL_SAVE_VERSION) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("Xen migration stream version '%d' is not supported
on this host"),
- mig->xenMigStreamVer);
- goto error;
+ (*mig)->xenMigStreamVer);
+ return -1;
}
/* Let migration hook filter domain XML */
@@ -528,29 +513,29 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
if (!(xml = virDomainDefFormat(*def, cfg->caps,
VIR_DOMAIN_XML_SECURE |
VIR_DOMAIN_XML_MIGRATABLE)))
- goto error;
+ return -1;
hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, (*def)->name,
VIR_HOOK_LIBXL_OP_MIGRATE, VIR_HOOK_SUBOP_BEGIN,
- NULL, xml, &xmlout);
+ NULL, xml, xmlout);
VIR_FREE(xml);
if (hookret < 0) {
- goto error;
+ return -1;
} else if (hookret == 0) {
- if (virStringIsEmpty(xmlout)) {
+ if (virStringIsEmpty(*xmlout)) {
VIR_DEBUG("Migrate hook filter returned nothing; using the"
" original XML");
} else {
virDomainDefPtr newdef;
- VIR_DEBUG("Using hook-filtered domain XML: %s", xmlout);
- newdef = virDomainDefParseString(xmlout, cfg->caps,
driver->xmlopt,
+ VIR_DEBUG("Using hook-filtered domain XML: %s", *xmlout);
+ newdef = virDomainDefParseString(*xmlout, cfg->caps,
driver->xmlopt,
NULL,
VIR_DOMAIN_DEF_PARSE_INACTIVE |
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE);
if (!newdef)
- goto error;
+ return -1;
/* TODO At some stage we will want to have some check of what the user
* did in the hook. */
@@ -560,17 +545,52 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
/* We should taint the domain here. However, @vm and therefore
* privateData too are still NULL, so just notice the fact and
* taint it later. */
- taint_hook = true;
+ *taint_hook = true;
}
}
}
+ return 0;
+}
+
+int
+libxlDomainMigrationPrepare(virConnectPtr dconn,
+ virDomainDefPtr *def,
+ const char *uri_in,
+ char **uri_out,
+ const char *cookiein,
+ int cookieinlen,
+ unsigned int flags)
+{
+ libxlDriverPrivatePtr driver = dconn->privateData;
+ libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
+ libxlMigrationCookiePtr mig = NULL;
+ virDomainObjPtr vm = NULL;
+ char *hostname = NULL;
+ char *xmlout = NULL;
+ unsigned short port;
+ char portstr[100];
+ virURIPtr uri = NULL;
+ virNetSocketPtr *socks = NULL;
+ size_t nsocks = 0;
+ int nsocks_listen = 0;
+ libxlMigrationDstArgs *args = NULL;
+ bool taint_hook = false;
+ libxlDomainObjPrivatePtr priv = NULL;
+ size_t i;
+ int ret = -1;
+
+ if (libxlDomainMigrationPrepareAny(dconn, def, cookiein, cookieinlen,
+ &mig, &xmlout, &taint_hook) < 0)
+ goto error;
+
if (!(vm = virDomainObjListAdd(driver->domains, *def,
driver->xmlopt,
VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
NULL)))
goto error;
+
*def = NULL;
priv = vm->privateData;
--
2.1.4
5:17 a.m.
New subject: [libvirt] [PATCH v3 2/2] libxl: add tunnelled migration support
From: Bob Liu <bob.liu(a)oracle.com>
Tunnelled migration doesn't require any extra network connections
beside the libvirt daemon. It's capable of strong encryption and the
default option of openstack-nova.
This patch adds the tunnelled migration(Tunnel3params) support to
libxl. On the source side, the data flow is:
* libxlDoMigrateSend() -> pipe libxlTunnel3MigrationFunc() polls pipe
* out and then write to dest stream.
While on the destination side:
* Stream -> pipe -> 'recvfd of libxlDomainStartRestore'
The usage is the same as p2p migration, execpt adding one extra
'--tunnelled' to the libvirt p2p migration command.
Signed-off-by: Bob Liu <bob.liu(a)oracle.com>
Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com>
---
v3:
* virStream{Send,Finish} and VIR_ALLOC_N already report errors,
hence removing the virReportError calls from its error paths.
* Revert top-level migration APIs to initial v1 approach.
v2:
( Comments from Jim )
* Adjust common preparetunnel function reflecting v1 suggestions
- Using common top-level Prepare function by adding is_tunnel variable
- Using libxl_migration PrepareAny added in patch 1
- virHook support in PrepareTunnel3
* Move virThreadPtr from libxlTunnelMigrationThread into libxlTunnelControl
* Rename function local variable from tmThreadPtr to arg
* Remove redundant assignment "tmThreadPtr = &tc->tmThread;" always
being not
null.
* Adjust debug message to "poll returned 0" as opposed to "poll got
timeout" as the latter might not always be the case.
---
src/libxl/libxl_driver.c | 58 +++++++++-
src/libxl/libxl_migration.c | 275 +++++++++++++++++++++++++++++++++++++++++---
src/libxl/libxl_migration.h | 9 ++
3 files changed, 325 insertions(+), 17 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 74cb05a..e5b8f48 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5934,6 +5934,61 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
}
static int
+libxlDomainMigratePrepareTunnel3Params(virConnectPtr dconn,
+ virStreamPtr st,
+ virTypedParameterPtr params,
+ int nparams,
+ const char *cookiein,
+ int cookieinlen,
+ char **cookieout ATTRIBUTE_UNUSED,
+ int *cookieoutlen ATTRIBUTE_UNUSED,
+ unsigned int flags)
+{
+ libxlDriverPrivatePtr driver = dconn->privateData;
+ virDomainDefPtr def = NULL;
+ const char *dom_xml = NULL;
+ const char *dname = NULL;
+ const char *uri_in = NULL;
+
+#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
+ virReportUnsupportedError();
+ return -1;
+#endif
+
+ virCheckFlags(LIBXL_MIGRATION_FLAGS, -1);
+ if (virTypedParamsValidate(params, nparams, LIBXL_MIGRATION_PARAMETERS) < 0)
+ goto error;
+
+ if (virTypedParamsGetString(params, nparams,
+ VIR_MIGRATE_PARAM_DEST_XML,
+ &dom_xml) < 0 ||
+ virTypedParamsGetString(params, nparams,
+ VIR_MIGRATE_PARAM_DEST_NAME,
+ &dname) < 0 ||
+ virTypedParamsGetString(params, nparams,
+ VIR_MIGRATE_PARAM_URI,
+ &uri_in) < 0)
+
+ goto error;
+
+ if (!(def = libxlDomainMigrationPrepareDef(driver, dom_xml, dname)))
+ goto error;
+
+ if (virDomainMigratePrepareTunnel3ParamsEnsureACL(dconn, def) < 0)
+ goto error;
+
+ if (libxlDomainMigrationPrepareTunnel3(dconn, st, &def, cookiein,
+ cookieinlen, flags) < 0)
+ goto error;
+
+ return 0;
+
+ error:
+ virDomainDefFree(def);
+ return -1;
+}
+
+static int
libxlDomainMigratePrepare3Params(virConnectPtr dconn,
virTypedParameterPtr params,
int nparams,
@@ -6033,7 +6088,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
if (virDomainMigratePerform3ParamsEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- if (flags & VIR_MIGRATE_PEER2PEER) {
+ if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) {
if (libxlDomainMigrationPerformP2P(driver, vm, dom->conn, dom_xml,
dconnuri, uri, dname, flags) < 0)
goto cleanup;
@@ -6518,6 +6573,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
.nodeDeviceReset = libxlNodeDeviceReset, /* 1.2.3 */
.domainMigrateBegin3Params = libxlDomainMigrateBegin3Params, /* 1.2.6 */
.domainMigratePrepare3Params = libxlDomainMigratePrepare3Params, /* 1.2.6 */
+ .domainMigratePrepareTunnel3Params = libxlDomainMigratePrepareTunnel3Params, /* 3.1.0
*/
.domainMigratePerform3Params = libxlDomainMigratePerform3Params, /* 1.2.6 */
.domainMigrateFinish3Params = libxlDomainMigrateFinish3Params, /* 1.2.6 */
.domainMigrateConfirm3Params = libxlDomainMigrateConfirm3Params, /* 1.2.6 */
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 7beabd2..6bd8983 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -44,6 +44,7 @@
#include "libxl_migration.h"
#include "locking/domain_lock.h"
#include "virtypedparam.h"
+#include "fdstream.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -554,6 +555,95 @@ libxlDomainMigrationPrepareAny(virConnectPtr dconn,
}
int
+libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
+ virStreamPtr st,
+ virDomainDefPtr *def,
+ const char *cookiein,
+ int cookieinlen,
+ unsigned int flags)
+{
+ libxlMigrationCookiePtr mig = NULL;
+ libxlDriverPrivatePtr driver = dconn->privateData;
+ virDomainObjPtr vm = NULL;
+ libxlMigrationDstArgs *args = NULL;
+ virThread thread;
+ bool taint_hook = false;
+ libxlDomainObjPrivatePtr priv = NULL;
+ char *xmlout = NULL;
+ int dataFD[2] = { -1, -1 };
+ int ret = -1;
+
+ if (libxlDomainMigrationPrepareAny(dconn, def, cookiein, cookieinlen,
+ &mig, &xmlout, &taint_hook) < 0)
+ goto error;
+
+ if (!(vm = virDomainObjListAdd(driver->domains, *def,
+ driver->xmlopt,
+ VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
+ NULL)))
+ goto error;
+
+ *def = NULL;
+ priv = vm->privateData;
+
+ if (taint_hook) {
+ /* Domain XML has been altered by a hook script. */
+ priv->hookRun = true;
+ }
+
+ /*
+ * The data flow of tunnel3 migration in the dest side:
+ * stream -> pipe -> recvfd of libxlDomainStartRestore
+ */
+ if (pipe(dataFD) < 0)
+ goto error;
+
+ /* Stream data will be written to pipeIn */
+ if (virFDStreamOpen(st, dataFD[1]) < 0)
+ goto error;
+ dataFD[1] = -1; /* 'st' owns the FD now & will close it */
+
+ if (libxlMigrationDstArgsInitialize() < 0)
+ goto error;
+
+ if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
+ goto error;
+
+ args->conn = dconn;
+ args->vm = vm;
+ args->flags = flags;
+ args->migcookie = mig;
+ /* Receive from pipeOut */
+ args->recvfd = dataFD[0];
+ args->nsocks = 0;
+ if (virThreadCreate(&thread, false, libxlDoMigrateReceive, args) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("Failed to create thread for receiving migration
data"));
+ goto error;
+ }
+
+ ret = 0;
+ goto done;
+
+ error:
+ VIR_FORCE_CLOSE(dataFD[1]);
+ VIR_FORCE_CLOSE(dataFD[0]);
+ virObjectUnref(args);
+ /* Remove virDomainObj from domain list */
+ if (vm) {
+ virDomainObjListRemove(driver->domains, vm);
+ vm = NULL;
+ }
+
+ done:
+ if (vm)
+ virObjectUnlock(vm);
+
+ return ret;
+}
+
+int
libxlDomainMigrationPrepare(virConnectPtr dconn,
virDomainDefPtr *def,
const char *uri_in,
@@ -734,9 +824,143 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
return ret;
}
-/* This function is a simplification of virDomainMigrateVersion3Full
- * excluding tunnel support and restricting it to migration v3
- * with params since it was the first to be introduced in libxl.
+typedef struct _libxlTunnelMigrationThread libxlTunnelMigrationThread;
+struct _libxlTunnelMigrationThread {
+ virStreamPtr st;
+ int srcFD;
+};
+#define TUNNEL_SEND_BUF_SIZE 65536
+
+/*
+ * The data flow of tunnel3 migration in the src side:
+ * libxlDoMigrateSend() -> pipe
+ * libxlTunnel3MigrationFunc() polls pipe out and then write to dest stream.
+ */
+static void libxlTunnel3MigrationFunc(void *arg)
+{
+ libxlTunnelMigrationThread *data = (libxlTunnelMigrationThread *)arg;
+ char *buffer = NULL;
+ struct pollfd fds[1];
+ int timeout = -1;
+
+ if (VIR_ALLOC_N(buffer, TUNNEL_SEND_BUF_SIZE) < 0)
+ return;
+
+ fds[0].fd = data->srcFD;
+ for (;;) {
+ int ret;
+
+ fds[0].events = POLLIN;
+ fds[0].revents = 0;
+ ret = poll(fds, ARRAY_CARDINALITY(fds), timeout);
+ if (ret < 0) {
+ if (errno == EAGAIN || errno == EINTR)
+ continue;
+ virReportError(errno, "%s",
+ _("poll failed in libxlTunnel3MigrationFunc"));
+ goto cleanup;
+ }
+
+ if (ret == 0) {
+ VIR_DEBUG("poll returned 0");
+ break;
+ }
+
+ if (fds[0].revents & (POLLIN | POLLERR | POLLHUP)) {
+ int nbytes;
+
+ nbytes = read(data->srcFD, buffer, TUNNEL_SEND_BUF_SIZE);
+ if (nbytes > 0) {
+ /* Write to dest stream */
+ if (virStreamSend(data->st, buffer, nbytes) < 0) {
+ virStreamAbort(data->st);
+ goto cleanup;
+ }
+ } else if (nbytes < 0) {
+ virReportError(errno, "%s",
+ _("tunnelled migration failed to read from xen
side"));
+ virStreamAbort(data->st);
+ goto cleanup;
+ } else {
+ /* EOF; transferred all data */
+ break;
+ }
+ }
+ }
+
+ ignore_value(virStreamFinish(data->st));
+
+ cleanup:
+ VIR_FREE(buffer);
+
+ return;
+}
+
+struct libxlTunnelControl {
+ libxlTunnelMigrationThread tmThread;
+ virThread thread;
+ int dataFD[2];
+};
+
+static int
+libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
+ virDomainObjPtr vm,
+ unsigned long flags,
+ virStreamPtr st,
+ struct libxlTunnelControl *tc)
+{
+ libxlTunnelMigrationThread *arg = NULL;
+ int ret = -1;
+
+ if (VIR_ALLOC(tc) < 0)
+ goto out;
+
+ tc->dataFD[0] = -1;
+ tc->dataFD[1] = -1;
+ if (pipe(tc->dataFD) < 0) {
+ virReportError(errno, "%s", _("Unable to make pipes"));
+ goto out;
+ }
+
+ arg = &tc->tmThread;
+ /* Read from pipe */
+ arg->srcFD = tc->dataFD[0];
+ /* Write to dest stream */
+ arg->st = st;
+ if (virThreadCreate(&tc->thread, true,
+ libxlTunnel3MigrationFunc, arg) < 0) {
+ virReportError(errno, "%s",
+ _("Unable to create tunnel migration thread"));
+ goto out;
+ }
+
+ virObjectUnlock(vm);
+ /* Send data to pipe */
+ ret = libxlDoMigrateSend(driver, vm, flags, tc->dataFD[1]);
+ virObjectLock(vm);
+
+ out:
+ /* libxlMigrationStopTunnel will be called in libxlDoMigrateP2P to free
+ * all resources for us. */
+ return ret;
+}
+
+static void libxlMigrationStopTunnel(struct libxlTunnelControl *tc)
+{
+ if (!tc)
+ return;
+
+ virThreadCancel(&tc->thread);
+ virThreadJoin(&tc->thread);
+
+ VIR_FORCE_CLOSE(tc->dataFD[0]);
+ VIR_FORCE_CLOSE(tc->dataFD[1]);
+ VIR_FREE(tc);
+}
+
+/* This function is a simplification of virDomainMigrateVersion3Full and
+ * restricting it to migration v3 with params since it was the first to be
+ * introduced in libxl.
*/
static int
libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
@@ -761,6 +985,9 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
bool cancelled = true;
virErrorPtr orig_err = NULL;
int ret = -1;
+ /* For tunnel migration */
+ virStreamPtr st = NULL;
+ struct libxlTunnelControl *tc = NULL;
dom_xml = libxlDomainMigrationBegin(sconn, vm, xmlin,
&cookieout, &cookieoutlen);
@@ -788,29 +1015,40 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
VIR_DEBUG("Prepare3");
virObjectUnlock(vm);
- ret = dconn->driver->domainMigratePrepare3Params
- (dconn, params, nparams, cookieout, cookieoutlen, NULL, NULL, &uri_out,
destflags);
+ if (flags & VIR_MIGRATE_TUNNELLED) {
+ if (!(st = virStreamNew(dconn, 0)))
+ goto cleanup;
+ ret = dconn->driver->domainMigratePrepareTunnel3Params
+ (dconn, st, params, nparams, cookieout, cookieoutlen, NULL, NULL,
destflags);
+ } else {
+ ret = dconn->driver->domainMigratePrepare3Params
+ (dconn, params, nparams, cookieout, cookieoutlen, NULL, NULL, &uri_out,
destflags);
+ }
virObjectLock(vm);
if (ret == -1)
goto cleanup;
- if (uri_out) {
- if (virTypedParamsReplaceString(¶ms, &nparams,
- VIR_MIGRATE_PARAM_URI, uri_out) < 0) {
- orig_err = virSaveLastError();
+ if (!(flags & VIR_MIGRATE_TUNNELLED)) {
+ if (uri_out) {
+ if (virTypedParamsReplaceString(¶ms, &nparams,
+ VIR_MIGRATE_PARAM_URI, uri_out) < 0) {
+ orig_err = virSaveLastError();
+ goto finish;
+ }
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("domainMigratePrepare3 did not set uri"));
goto finish;
}
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domainMigratePrepare3 did not set uri"));
- goto finish;
}
VIR_DEBUG("Perform3 uri=%s", NULLSTR(uri_out));
- ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
- uri_out, NULL, flags);
-
+ if (flags & VIR_MIGRATE_TUNNELLED)
+ ret = libxlMigrationStartTunnel(driver, vm, flags, st, tc);
+ else
+ ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
+ uri_out, NULL, flags);
if (ret < 0)
orig_err = virSaveLastError();
@@ -848,6 +1086,11 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
vm->def->name);
cleanup:
+ if (flags & VIR_MIGRATE_TUNNELLED) {
+ libxlMigrationStopTunnel(tc);
+ virObjectUnref(st);
+ }
+
if (ddomain) {
virObjectUnref(ddomain);
ret = 0;
diff --git a/src/libxl/libxl_migration.h b/src/libxl/libxl_migration.h
index 8a074a0..fcea558 100644
--- a/src/libxl/libxl_migration.h
+++ b/src/libxl/libxl_migration.h
@@ -29,6 +29,7 @@
# define LIBXL_MIGRATION_FLAGS \
(VIR_MIGRATE_LIVE | \
VIR_MIGRATE_PEER2PEER | \
+ VIR_MIGRATE_TUNNELLED | \
VIR_MIGRATE_PERSIST_DEST | \
VIR_MIGRATE_UNDEFINE_SOURCE | \
VIR_MIGRATE_PAUSED)
@@ -53,6 +54,14 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr driver,
const char *dname);
int
+libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
+ virStreamPtr st,
+ virDomainDefPtr *def,
+ const char *cookiein,
+ int cookieinlen,
+ unsigned int flags);
+
+int
libxlDomainMigrationPrepare(virConnectPtr dconn,
virDomainDefPtr *def,
const char *uri_in,
--
2.1.4
5:42 a.m.
New subject: [libvirt] [PATCH v3 2/2] libxl: add tunnelled migration support
On 02/15/2017 11:17 AM, Joao Martins wrote:
From: Bob Liu <bob.liu(a)oracle.com>
Tunnelled migration doesn't require any extra network connections
beside the libvirt daemon. It's capable of strong encryption and the
default option of openstack-nova.
This patch adds the tunnelled migration(Tunnel3params) support to
libxl. On the source side, the data flow is:
* libxlDoMigrateSend() -> pipe libxlTunnel3MigrationFunc() polls pipe
* out and then write to dest stream.
While on the destination side:
* Stream -> pipe -> 'recvfd of libxlDomainStartRestore'
The usage is the same as p2p migration, execpt adding one extra
'--tunnelled' to the libvirt p2p migration command.
Signed-off-by: Bob Liu <bob.liu(a)oracle.com>
Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com>
---
v3:
* virStream{Send,Finish} and VIR_ALLOC_N already report errors,
hence removing the virReportError calls from its error paths.
* Revert top-level migration APIs to initial v1 approach.
v2:
( Comments from Jim )
* Adjust common preparetunnel function reflecting v1 suggestions
- Using common top-level Prepare function by adding is_tunnel variable
- Using libxl_migration PrepareAny added in patch 1
- virHook support in PrepareTunnel3
* Move virThreadPtr from libxlTunnelMigrationThread into libxlTunnelControl
* Rename function local variable from tmThreadPtr to arg
* Remove redundant assignment "tmThreadPtr = &tc->tmThread;" always
being not
null.
* Adjust debug message to "poll returned 0" as opposed to "poll got
timeout" as the latter might not always be the case.
---
src/libxl/libxl_driver.c | 58 +++++++++-
src/libxl/libxl_migration.c | 275 +++++++++++++++++++++++++++++++++++++++++---
src/libxl/libxl_migration.h | 9 ++
3 files changed, 325 insertions(+), 17 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 74cb05a..e5b8f48 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5934,6 +5934,61 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
}
static int
+libxlDomainMigratePrepareTunnel3Params(virConnectPtr dconn,
+ virStreamPtr st,
+ virTypedParameterPtr params,
+ int nparams,
+ const char *cookiein,
+ int cookieinlen,
+ char **cookieout ATTRIBUTE_UNUSED,
+ int *cookieoutlen ATTRIBUTE_UNUSED,
+ unsigned int flags)
+{
+ libxlDriverPrivatePtr driver = dconn->privateData;
+ virDomainDefPtr def = NULL;
+ const char *dom_xml = NULL;
+ const char *dname = NULL;
+ const char *uri_in = NULL;
+
+#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
+ virReportUnsupportedError();
+ return -1;
+#endif
+
+ virCheckFlags(LIBXL_MIGRATION_FLAGS, -1);
+ if (virTypedParamsValidate(params, nparams, LIBXL_MIGRATION_PARAMETERS) < 0)
+ goto error;
+
+ if (virTypedParamsGetString(params, nparams,
+ VIR_MIGRATE_PARAM_DEST_XML,
+ &dom_xml) < 0 ||
+ virTypedParamsGetString(params, nparams,
+ VIR_MIGRATE_PARAM_DEST_NAME,
+ &dname) < 0 ||
+ virTypedParamsGetString(params, nparams,
+ VIR_MIGRATE_PARAM_URI,
+ &uri_in) < 0)
+
+ goto error;
+
+ if (!(def = libxlDomainMigrationPrepareDef(driver, dom_xml, dname)))
+ goto error;
+
+ if (virDomainMigratePrepareTunnel3ParamsEnsureACL(dconn, def) < 0)
+ goto error;
+
+ if (libxlDomainMigrationPrepareTunnel3(dconn, st, &def, cookiein,
+ cookieinlen, flags) < 0)
+ goto error;
+
+ return 0;
+
+ error:
+ virDomainDefFree(def);
+ return -1;
+}
+
+static int
libxlDomainMigratePrepare3Params(virConnectPtr dconn,
virTypedParameterPtr params,
int nparams,
@@ -6033,7 +6088,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
if (virDomainMigratePerform3ParamsEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- if (flags & VIR_MIGRATE_PEER2PEER) {
+ if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) {
if (libxlDomainMigrationPerformP2P(driver, vm, dom->conn, dom_xml,
dconnuri, uri, dname, flags) < 0)
goto cleanup;
@@ -6518,6 +6573,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
.nodeDeviceReset = libxlNodeDeviceReset, /* 1.2.3 */
.domainMigrateBegin3Params = libxlDomainMigrateBegin3Params, /* 1.2.6 */
.domainMigratePrepare3Params = libxlDomainMigratePrepare3Params, /* 1.2.6 */
+ .domainMigratePrepareTunnel3Params = libxlDomainMigratePrepareTunnel3Params, /*
3.1.0 */
.domainMigratePerform3Params = libxlDomainMigratePerform3Params, /* 1.2.6 */
.domainMigrateFinish3Params = libxlDomainMigrateFinish3Params, /* 1.2.6 */
.domainMigrateConfirm3Params = libxlDomainMigrateConfirm3Params, /* 1.2.6 */
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 7beabd2..6bd8983 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -44,6 +44,7 @@
#include "libxl_migration.h"
#include "locking/domain_lock.h"
#include "virtypedparam.h"
+#include "fdstream.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -554,6 +555,95 @@ libxlDomainMigrationPrepareAny(virConnectPtr dconn,
}
int
+libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
+ virStreamPtr st,
+ virDomainDefPtr *def,
+ const char *cookiein,
+ int cookieinlen,
+ unsigned int flags)
+{
+ libxlMigrationCookiePtr mig = NULL;
+ libxlDriverPrivatePtr driver = dconn->privateData;
+ virDomainObjPtr vm = NULL;
+ libxlMigrationDstArgs *args = NULL;
+ virThread thread;
+ bool taint_hook = false;
+ libxlDomainObjPrivatePtr priv = NULL;
+ char *xmlout = NULL;
+ int dataFD[2] = { -1, -1 };
+ int ret = -1;
+
+ if (libxlDomainMigrationPrepareAny(dconn, def, cookiein, cookieinlen,
+ &mig, &xmlout, &taint_hook) < 0)
+ goto error;
+
+ if (!(vm = virDomainObjListAdd(driver->domains, *def,
+ driver->xmlopt,
+ VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
+ NULL)))
+ goto error;
+
+ *def = NULL;
+ priv = vm->privateData;
+
+ if (taint_hook) {
+ /* Domain XML has been altered by a hook script. */
+ priv->hookRun = true;
+ }
+
+ /*
+ * The data flow of tunnel3 migration in the dest side:
+ * stream -> pipe -> recvfd of libxlDomainStartRestore
+ */
+ if (pipe(dataFD) < 0)
+ goto error;
+
+ /* Stream data will be written to pipeIn */
+ if (virFDStreamOpen(st, dataFD[1]) < 0)
+ goto error;
+ dataFD[1] = -1; /* 'st' owns the FD now & will close it */
+
+ if (libxlMigrationDstArgsInitialize() < 0)
+ goto error;
+
+ if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
+ goto error;
+
+ args->conn = dconn;
+ args->vm = vm;
+ args->flags = flags;
+ args->migcookie = mig;
+ /* Receive from pipeOut */
+ args->recvfd = dataFD[0];
+ args->nsocks = 0;
+ if (virThreadCreate(&thread, false, libxlDoMigrateReceive, args) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("Failed to create thread for receiving migration
data"));
+ goto error;
+ }
+
+ ret = 0;
+ goto done;
+
+ error:
+ VIR_FORCE_CLOSE(dataFD[1]);
+ VIR_FORCE_CLOSE(dataFD[0]);
+ virObjectUnref(args);
+ /* Remove virDomainObj from domain list */
+ if (vm) {
+ virDomainObjListRemove(driver->domains, vm);
+ vm = NULL;
+ }
+
+ done:
+ if (vm)
+ virObjectUnlock(vm);
+
+ return ret;
+}
+
+int
libxlDomainMigrationPrepare(virConnectPtr dconn,
virDomainDefPtr *def,
const char *uri_in,
@@ -734,9 +824,143 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
return ret;
}
-/* This function is a simplification of virDomainMigrateVersion3Full
- * excluding tunnel support and restricting it to migration v3
- * with params since it was the first to be introduced in libxl.
+typedef struct _libxlTunnelMigrationThread libxlTunnelMigrationThread;
+struct _libxlTunnelMigrationThread {
+ virStreamPtr st;
+ int srcFD;
+};
+#define TUNNEL_SEND_BUF_SIZE 65536
+
+/*
+ * The data flow of tunnel3 migration in the src side:
+ * libxlDoMigrateSend() -> pipe
+ * libxlTunnel3MigrationFunc() polls pipe out and then write to dest stream.
+ */
+static void libxlTunnel3MigrationFunc(void *arg)
+{
+ libxlTunnelMigrationThread *data = (libxlTunnelMigrationThread *)arg;
+ char *buffer = NULL;
+ struct pollfd fds[1];
+ int timeout = -1;
+
+ if (VIR_ALLOC_N(buffer, TUNNEL_SEND_BUF_SIZE) < 0)
+ return;
+
+ fds[0].fd = data->srcFD;
+ for (;;) {
+ int ret;
+
+ fds[0].events = POLLIN;
+ fds[0].revents = 0;
+ ret = poll(fds, ARRAY_CARDINALITY(fds), timeout);
+ if (ret < 0) {
+ if (errno == EAGAIN || errno == EINTR)
+ continue;
+ virReportError(errno, "%s",
+ _("poll failed in libxlTunnel3MigrationFunc"));
+ goto cleanup;
+ }
+
+ if (ret == 0) {
+ VIR_DEBUG("poll returned 0");
+ break;
+ }
+
+ if (fds[0].revents & (POLLIN | POLLERR | POLLHUP)) {
+ int nbytes;
+
+ nbytes = read(data->srcFD, buffer, TUNNEL_SEND_BUF_SIZE);
+ if (nbytes > 0) {
+ /* Write to dest stream */
+ if (virStreamSend(data->st, buffer, nbytes) < 0) {
+ virStreamAbort(data->st);
+ goto cleanup;
+ }
+ } else if (nbytes < 0) {
+ virReportError(errno, "%s",
+ _("tunnelled migration failed to read from xen
side"));
+ virStreamAbort(data->st);
+ goto cleanup;
This is here since v1 and went unnoticed (in
make syntax-check too), but the
indentation is off here. It should be 4 spaces to the left. But hopefully that
could be adjusted upon commit if no more comments are brought up?
+ } else {
+ /* EOF; transferred all data */
+ break;
+ }
+ }
+ }
+
+ ignore_value(virStreamFinish(data->st));
+
+ cleanup:
+ VIR_FREE(buffer);
+
+ return;
+}
+
+struct libxlTunnelControl {
+ libxlTunnelMigrationThread tmThread;
+ virThread thread;
+ int dataFD[2];
+};
+
+static int
+libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
+ virDomainObjPtr vm,
+ unsigned long flags,
+ virStreamPtr st,
+ struct libxlTunnelControl *tc)
+{
+ libxlTunnelMigrationThread *arg = NULL;
+ int ret = -1;
+
+ if (VIR_ALLOC(tc) < 0)
+ goto out;
+
+ tc->dataFD[0] = -1;
+ tc->dataFD[1] = -1;
+ if (pipe(tc->dataFD) < 0) {
+ virReportError(errno, "%s", _("Unable to make pipes"));
+ goto out;
+ }
+
+ arg = &tc->tmThread;
+ /* Read from pipe */
+ arg->srcFD = tc->dataFD[0];
+ /* Write to dest stream */
+ arg->st = st;
+ if (virThreadCreate(&tc->thread, true,
+ libxlTunnel3MigrationFunc, arg) < 0) {
+ virReportError(errno, "%s",
+ _("Unable to create tunnel migration thread"));
+ goto out;
+ }
+
+ virObjectUnlock(vm);
+ /* Send data to pipe */
+ ret = libxlDoMigrateSend(driver, vm, flags, tc->dataFD[1]);
+ virObjectLock(vm);
+
+ out:
+ /* libxlMigrationStopTunnel will be called in libxlDoMigrateP2P to free
+ * all resources for us. */
+ return ret;
+}
+
+static void libxlMigrationStopTunnel(struct libxlTunnelControl *tc)
+{
+ if (!tc)
+ return;
+
+ virThreadCancel(&tc->thread);
+ virThreadJoin(&tc->thread);
+
+ VIR_FORCE_CLOSE(tc->dataFD[0]);
+ VIR_FORCE_CLOSE(tc->dataFD[1]);
+ VIR_FREE(tc);
+}
+
+/* This function is a simplification of virDomainMigrateVersion3Full and
+ * restricting it to migration v3 with params since it was the first to be
+ * introduced in libxl.
*/
static int
libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
@@ -761,6 +985,9 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
bool cancelled = true;
virErrorPtr orig_err = NULL;
int ret = -1;
+ /* For tunnel migration */
+ virStreamPtr st = NULL;
+ struct libxlTunnelControl *tc = NULL;
dom_xml = libxlDomainMigrationBegin(sconn, vm, xmlin,
&cookieout, &cookieoutlen);
@@ -788,29 +1015,40 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
VIR_DEBUG("Prepare3");
virObjectUnlock(vm);
- ret = dconn->driver->domainMigratePrepare3Params
- (dconn, params, nparams, cookieout, cookieoutlen, NULL, NULL, &uri_out,
destflags);
+ if (flags & VIR_MIGRATE_TUNNELLED) {
+ if (!(st = virStreamNew(dconn, 0)))
+ goto cleanup;
+ ret = dconn->driver->domainMigratePrepareTunnel3Params
+ (dconn, st, params, nparams, cookieout, cookieoutlen, NULL, NULL,
destflags);
+ } else {
+ ret = dconn->driver->domainMigratePrepare3Params
+ (dconn, params, nparams, cookieout, cookieoutlen, NULL, NULL, &uri_out,
destflags);
+ }
virObjectLock(vm);
if (ret == -1)
goto cleanup;
- if (uri_out) {
- if (virTypedParamsReplaceString(¶ms, &nparams,
- VIR_MIGRATE_PARAM_URI, uri_out) < 0) {
- orig_err = virSaveLastError();
+ if (!(flags & VIR_MIGRATE_TUNNELLED)) {
+ if (uri_out) {
+ if (virTypedParamsReplaceString(¶ms, &nparams,
+ VIR_MIGRATE_PARAM_URI, uri_out) < 0) {
+ orig_err = virSaveLastError();
+ goto finish;
+ }
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("domainMigratePrepare3 did not set uri"));
goto finish;
}
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domainMigratePrepare3 did not set uri"));
- goto finish;
}
VIR_DEBUG("Perform3 uri=%s", NULLSTR(uri_out));
- ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
- uri_out, NULL, flags);
-
+ if (flags & VIR_MIGRATE_TUNNELLED)
+ ret = libxlMigrationStartTunnel(driver, vm, flags, st, tc);
+ else
+ ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
+ uri_out, NULL, flags);
if (ret < 0)
orig_err = virSaveLastError();
@@ -848,6 +1086,11 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
vm->def->name);
cleanup:
+ if (flags & VIR_MIGRATE_TUNNELLED) {
+ libxlMigrationStopTunnel(tc);
+ virObjectUnref(st);
+ }
+
if (ddomain) {
virObjectUnref(ddomain);
ret = 0;
diff --git a/src/libxl/libxl_migration.h b/src/libxl/libxl_migration.h
index 8a074a0..fcea558 100644
--- a/src/libxl/libxl_migration.h
+++ b/src/libxl/libxl_migration.h
@@ -29,6 +29,7 @@
# define LIBXL_MIGRATION_FLAGS \
(VIR_MIGRATE_LIVE | \
VIR_MIGRATE_PEER2PEER | \
+ VIR_MIGRATE_TUNNELLED | \
VIR_MIGRATE_PERSIST_DEST | \
VIR_MIGRATE_UNDEFINE_SOURCE | \
VIR_MIGRATE_PAUSED)
@@ -53,6 +54,14 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr driver,
const char *dname);
int
+libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
+ virStreamPtr st,
+ virDomainDefPtr *def,
+ const char *cookiein,
+ int cookieinlen,
+ unsigned int flags);
+
+int
libxlDomainMigrationPrepare(virConnectPtr dconn,
virDomainDefPtr *def,
const char *uri_in,
5:47 a.m.
New subject: [libvirt] [PATCH v3 2/2] libxl: add tunnelled migration support
FYI: Couple of Coverity issues resulted from these patches.
int
+libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
+ virStreamPtr st,
+ virDomainDefPtr *def,
+ const char *cookiein,
+ int cookieinlen,
+ unsigned int flags)
+{
+ libxlMigrationCookiePtr mig = NULL;
+ libxlDriverPrivatePtr driver = dconn->privateData;
+ virDomainObjPtr vm = NULL;
+ libxlMigrationDstArgs *args = NULL;
+ virThread thread;
+ bool taint_hook = false;
+ libxlDomainObjPrivatePtr priv = NULL;
+ char *xmlout = NULL;
+ int dataFD[2] = { -1, -1 };
+ int ret = -1;
+
+ if (libxlDomainMigrationPrepareAny(dconn, def, cookiein, cookieinlen,
+ &mig, &xmlout, &taint_hook) < 0)
Coverity notes that '@mig' will be leaked in this case when "if
(!cookiein || !cookieinlen) {" is true in libxlMigrationEatCookie and
failures from libxlDomainMigrationPrepareAny don't free it.
+ goto error;
+
+ if (!(vm = virDomainObjListAdd(driver->domains, *def,
+ driver->xmlopt,
+ VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
+ NULL)))
+ goto error;
+
+ *def = NULL;
+ priv = vm->privateData;
+
+ if (taint_hook) {
+ /* Domain XML has been altered by a hook script. */
+ priv->hookRun = true;
+ }
+
+ /*
+ * The data flow of tunnel3 migration in the dest side:
+ * stream -> pipe -> recvfd of libxlDomainStartRestore
+ */
+ if (pipe(dataFD) < 0)
+ goto error;
+
+ /* Stream data will be written to pipeIn */
+ if (virFDStreamOpen(st, dataFD[1]) < 0)
+ goto error;
+ dataFD[1] = -1; /* 'st' owns the FD now & will close it */
+
+ if (libxlMigrationDstArgsInitialize() < 0)
+ goto error;
+
+ if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
+ goto error;
+
+ args->conn = dconn;
+ args->vm = vm;
+ args->flags = flags;
+ args->migcookie = mig;
+ /* Receive from pipeOut */
+ args->recvfd = dataFD[0];
+ args->nsocks = 0;
+ if (virThreadCreate(&thread, false, libxlDoMigrateReceive, args) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("Failed to create thread for receiving migration
data"));
+ goto error;
+ }
+
+ ret = 0;
+ goto done;
+
+ error:
+ VIR_FORCE_CLOSE(dataFD[1]);
+ VIR_FORCE_CLOSE(dataFD[0]);
+ virObjectUnref(args);
+ /* Remove virDomainObj from domain list */
+ if (vm) {
+ virDomainObjListRemove(driver->domains, vm);
+ vm = NULL;
+ }
+
+ done:
+ if (vm)
+ virObjectUnlock(vm);
+
+ return ret;
+}
[...]
+
+static int
+libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
+ virDomainObjPtr vm,
+ unsigned long flags,
+ virStreamPtr st,
+ struct libxlTunnelControl *tc)
+{
+ libxlTunnelMigrationThread *arg = NULL;
+ int ret = -1;
+
+ if (VIR_ALLOC(tc) < 0)
+ goto out;
+
+ tc->dataFD[0] = -1;
+ tc->dataFD[1] = -1;
+ if (pipe(tc->dataFD) < 0) {
+ virReportError(errno, "%s", _("Unable to make pipes"));
Coverity notes that failures would cause a leak for @tc (I assume here
and of course if virThreadCreate fails. Perhaps the 'best' way to
handle that would be to set tc = NULL after ThreadCreate success and
just call libxlMigrationStopTunnel in "out:"...
John
+ goto out;
+ }
+
+ arg = &tc->tmThread;
+ /* Read from pipe */
+ arg->srcFD = tc->dataFD[0];
+ /* Write to dest stream */
+ arg->st = st;
+ if (virThreadCreate(&tc->thread, true,
+ libxlTunnel3MigrationFunc, arg) < 0) {
+ virReportError(errno, "%s",
+ _("Unable to create tunnel migration thread"));
+ goto out;
+ }
+
+ virObjectUnlock(vm);
+ /* Send data to pipe */
+ ret = libxlDoMigrateSend(driver, vm, flags, tc->dataFD[1]);
+ virObjectLock(vm);
+
+ out:
+ /* libxlMigrationStopTunnel will be called in libxlDoMigrateP2P to free
+ * all resources for us. */
+ return ret;
+}
+
[...]
8:26 a.m.
New subject: [libvirt] [PATCH v3 2/2] libxl: add tunnelled migration support
On 02/16/2017 11:47 AM, John Ferlan wrote:
FYI: Couple of Coverity issues resulted from these patches.
> int
> +libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
> + virStreamPtr st,
> + virDomainDefPtr *def,
> + const char *cookiein,
> + int cookieinlen,
> + unsigned int flags)
> +{
> + libxlMigrationCookiePtr mig = NULL;
> + libxlDriverPrivatePtr driver = dconn->privateData;
> + virDomainObjPtr vm = NULL;
> + libxlMigrationDstArgs *args = NULL;
> + virThread thread;
> + bool taint_hook = false;
> + libxlDomainObjPrivatePtr priv = NULL;
> + char *xmlout = NULL;
> + int dataFD[2] = { -1, -1 };
> + int ret = -1;
> +
> + if (libxlDomainMigrationPrepareAny(dconn, def, cookiein, cookieinlen,
> + &mig, &xmlout, &taint_hook) <
0)
Coverity notes that '@mig' will be leaked in this case when "if
(!cookiein || !cookieinlen) {" is true in libxlMigrationEatCookie and
failures from libxlDomainMigrationPrepareAny don't free it.
/nods. This diff should do then? It covers both cases I think.
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index ba1ca5c..6a7d458 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -637,6 +637,7 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
}
done:
+ libxlMigrationCookieFree(mig);
if (vm)
virObjectUnlock(vm);
> + goto error;
> +
> + if (!(vm = virDomainObjListAdd(driver->domains, *def,
> + driver->xmlopt,
> + VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
> + VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
> + NULL)))
> + goto error;
> +
> + *def = NULL;
> + priv = vm->privateData;
> +
> + if (taint_hook) {
> + /* Domain XML has been altered by a hook script. */
> + priv->hookRun = true;
> + }
> +
> + /*
> + * The data flow of tunnel3 migration in the dest side:
> + * stream -> pipe -> recvfd of libxlDomainStartRestore
> + */
> + if (pipe(dataFD) < 0)
> + goto error;
> +
> + /* Stream data will be written to pipeIn */
> + if (virFDStreamOpen(st, dataFD[1]) < 0)
> + goto error;
> + dataFD[1] = -1; /* 'st' owns the FD now & will close it */
> +
> + if (libxlMigrationDstArgsInitialize() < 0)
> + goto error;
> +
> + if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
> + goto error;
> +
> + args->conn = dconn;
> + args->vm = vm;
> + args->flags = flags;
> + args->migcookie = mig;
> + /* Receive from pipeOut */
> + args->recvfd = dataFD[0];
> + args->nsocks = 0;
> + if (virThreadCreate(&thread, false, libxlDoMigrateReceive, args) < 0) {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("Failed to create thread for receiving migration
data"));
> + goto error;
> + }
> +
> + ret = 0;
> + goto done;
> +
> + error:
> + VIR_FORCE_CLOSE(dataFD[1]);
> + VIR_FORCE_CLOSE(dataFD[0]);
> + virObjectUnref(args);
> + /* Remove virDomainObj from domain list */
> + if (vm) {
> + virDomainObjListRemove(driver->domains, vm);
> + vm = NULL;
> + }
> +
> + done:
> + if (vm)
> + virObjectUnlock(vm);
> +
> + return ret;
> +}
[...]
> +
> +static int
> +libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
> + virDomainObjPtr vm,
> + unsigned long flags,
> + virStreamPtr st,
> + struct libxlTunnelControl *tc)
> +{
> + libxlTunnelMigrationThread *arg = NULL;
> + int ret = -1;
> +
> + if (VIR_ALLOC(tc) < 0)
> + goto out;
> +
> + tc->dataFD[0] = -1;
> + tc->dataFD[1] = -1;
> + if (pipe(tc->dataFD) < 0) {
> + virReportError(errno, "%s", _("Unable to make pipes"));
Coverity notes that failures would cause a leak for @tc (I assume here
and of course if virThreadCreate fails. Perhaps the 'best' way to
handle that would be to set tc = NULL after ThreadCreate success and
just call libxlMigrationStopTunnel in "out:"...
But libxlMigrationStopTunnel is always called (hence should be freeing @tc),
whether libxlMigrationStartTunnel failed or not. How about this?
@@ -907,8 +908,9 @@ libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
unsigned long flags,
virStreamPtr st,
- struct libxlTunnelControl *tc)
+ struct libxlTunnelControl **tnl)
{
+ struct libxlTunnelControl *tc = *tnl;
libxlTunnelMigrationThread *arg = NULL;
int ret = -1;
@@ -1045,7 +1047,7 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
VIR_DEBUG("Perform3 uri=%s", NULLSTR(uri_out));
if (flags & VIR_MIGRATE_TUNNELLED)
- ret = libxlMigrationStartTunnel(driver, vm, flags, st, tc);
+ ret = libxlMigrationStartTunnel(driver, vm, flags, st, &tc);
else
ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
uri_out, NULL, flags);
8:57 a.m.
New subject: [libvirt] [PATCH v3 2/2] libxl: add tunnelled migration support
On 02/16/2017 09:26 AM, Joao Martins wrote:
On 02/16/2017 11:47 AM, John Ferlan wrote:
> FYI: Couple of Coverity issues resulted from these patches.
>
>> int
>> +libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
>> + virStreamPtr st,
>> + virDomainDefPtr *def,
>> + const char *cookiein,
>> + int cookieinlen,
>> + unsigned int flags)
>> +{
>> + libxlMigrationCookiePtr mig = NULL;
>> + libxlDriverPrivatePtr driver = dconn->privateData;
>> + virDomainObjPtr vm = NULL;
>> + libxlMigrationDstArgs *args = NULL;
>> + virThread thread;
>> + bool taint_hook = false;
>> + libxlDomainObjPrivatePtr priv = NULL;
>> + char *xmlout = NULL;
>> + int dataFD[2] = { -1, -1 };
>> + int ret = -1;
>> +
>> + if (libxlDomainMigrationPrepareAny(dconn, def, cookiein, cookieinlen,
>> + &mig, &xmlout, &taint_hook)
< 0)
>
> Coverity notes that '@mig' will be leaked in this case when "if
> (!cookiein || !cookieinlen) {" is true in libxlMigrationEatCookie and
> failures from libxlDomainMigrationPrepareAny don't free it.
/nods. This diff should do then? It covers both cases I think.
What about:
args->migcookie = mig;
and then successful virThreadCreate(&thread, false,
libxlDoMigrateReceive, args) where 'args' is unref'd, but the cookie
isn't free'd.
Certainly you wouldn't want to free the cookie and then let the thread
use it!
So I think the libxlMigrationCookieFree needs to be done in the error:
label and in the thread cleanup. Also after you assign @mig to
migcookie, set 'mig = NULL';
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index ba1ca5c..6a7d458 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -637,6 +637,7 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
}
done:
+ libxlMigrationCookieFree(mig);
if (vm)
virObjectUnlock(vm);
>> + goto error;
>> +
>> + if (!(vm = virDomainObjListAdd(driver->domains, *def,
>> + driver->xmlopt,
>> + VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
>> + VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
>> + NULL)))
>> + goto error;
>> +
>> + *def = NULL;
>> + priv = vm->privateData;
>> +
>> + if (taint_hook) {
>> + /* Domain XML has been altered by a hook script. */
>> + priv->hookRun = true;
>> + }
>> +
>> + /*
>> + * The data flow of tunnel3 migration in the dest side:
>> + * stream -> pipe -> recvfd of libxlDomainStartRestore
>> + */
>> + if (pipe(dataFD) < 0)
>> + goto error;
>> +
>> + /* Stream data will be written to pipeIn */
>> + if (virFDStreamOpen(st, dataFD[1]) < 0)
>> + goto error;
>> + dataFD[1] = -1; /* 'st' owns the FD now & will close it */
>> +
>> + if (libxlMigrationDstArgsInitialize() < 0)
>> + goto error;
>> +
>> + if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>> + goto error;
>> +
>> + args->conn = dconn;
>> + args->vm = vm;
>> + args->flags = flags;
>> + args->migcookie = mig;
>> + /* Receive from pipeOut */
>> + args->recvfd = dataFD[0];
>> + args->nsocks = 0;
>> + if (virThreadCreate(&thread, false, libxlDoMigrateReceive, args) < 0)
{
>> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> + _("Failed to create thread for receiving migration
data"));
>> + goto error;
>> + }
>> +
>> + ret = 0;
>> + goto done;
>> +
>> + error:
>> + VIR_FORCE_CLOSE(dataFD[1]);
>> + VIR_FORCE_CLOSE(dataFD[0]);
>> + virObjectUnref(args);
>> + /* Remove virDomainObj from domain list */
>> + if (vm) {
>> + virDomainObjListRemove(driver->domains, vm);
>> + vm = NULL;
>> + }
>> +
>> + done:
>> + if (vm)
>> + virObjectUnlock(vm);
>> +
>> + return ret;
>> +}
>
> [...]
>
>> +
>> +static int
>> +libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
>> + virDomainObjPtr vm,
>> + unsigned long flags,
>> + virStreamPtr st,
>> + struct libxlTunnelControl *tc)
>> +{
>> + libxlTunnelMigrationThread *arg = NULL;
>> + int ret = -1;
>> +
>> + if (VIR_ALLOC(tc) < 0)
>> + goto out;
>> +
>> + tc->dataFD[0] = -1;
>> + tc->dataFD[1] = -1;
>> + if (pipe(tc->dataFD) < 0) {
>> + virReportError(errno, "%s", _("Unable to make
pipes"));
>
> Coverity notes that failures would cause a leak for @tc (I assume here
> and of course if virThreadCreate fails. Perhaps the 'best' way to
> handle that would be to set tc = NULL after ThreadCreate success and
> just call libxlMigrationStopTunnel in "out:"...
But libxlMigrationStopTunnel is always called (hence should be freeing @tc),
whether libxlMigrationStartTunnel failed or not. How about this?
@@ -907,8 +908,9 @@ libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
unsigned long flags,
virStreamPtr st,
- struct libxlTunnelControl *tc)
+ struct libxlTunnelControl **tnl)
{
+ struct libxlTunnelControl *tc = *tnl;
This doesn't do much... since @tc would be allocated below and *tnl
wouldn't be updated... Thus, the following is better:
if (VIR_ALLOC(tc) < 0)
goto out;
*tnl = tc;
John
libxlTunnelMigrationThread *arg = NULL;
int ret = -1;
@@ -1045,7 +1047,7 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
VIR_DEBUG("Perform3 uri=%s", NULLSTR(uri_out));
if (flags & VIR_MIGRATE_TUNNELLED)
- ret = libxlMigrationStartTunnel(driver, vm, flags, st, tc);
+ ret = libxlMigrationStartTunnel(driver, vm, flags, st, &tc);
else
ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
uri_out, NULL, flags);
10:14 a.m.
New subject: [libvirt] [PATCH v3 2/2] libxl: add tunnelled migration support
On 02/16/2017 02:57 PM, John Ferlan wrote:
On 02/16/2017 09:26 AM, Joao Martins wrote:
> On 02/16/2017 11:47 AM, John Ferlan wrote:
>> FYI: Couple of Coverity issues resulted from these patches.
>>
>>> int
>>> +libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
>>> + virStreamPtr st,
>>> + virDomainDefPtr *def,
>>> + const char *cookiein,
>>> + int cookieinlen,
>>> + unsigned int flags)
>>> +{
>>> + libxlMigrationCookiePtr mig = NULL;
>>> + libxlDriverPrivatePtr driver = dconn->privateData;
>>> + virDomainObjPtr vm = NULL;
>>> + libxlMigrationDstArgs *args = NULL;
>>> + virThread thread;
>>> + bool taint_hook = false;
>>> + libxlDomainObjPrivatePtr priv = NULL;
>>> + char *xmlout = NULL;
>>> + int dataFD[2] = { -1, -1 };
>>> + int ret = -1;
>>> +
>>> + if (libxlDomainMigrationPrepareAny(dconn, def, cookiein, cookieinlen,
>>> + &mig, &xmlout,
&taint_hook) < 0)
>>
>> Coverity notes that '@mig' will be leaked in this case when "if
>> (!cookiein || !cookieinlen) {" is true in libxlMigrationEatCookie and
>> failures from libxlDomainMigrationPrepareAny don't free it.
>
> /nods. This diff should do then? It covers both cases I think.
What about:
args->migcookie = mig;
and then successful virThreadCreate(&thread, false,
libxlDoMigrateReceive, args) where 'args' is unref'd, but the cookie
isn't free'd.
Certainly you wouldn't want to free the cookie and then let the thread
use it!
\facepalm - you're right.
So I think the libxlMigrationCookieFree needs to be done in the
error:
label and in the thread cleanup. Also after you assign @mig to
migcookie, set 'mig = NULL';
Yeap, let me send that in a follow-up patch
shortly.
The thread cleanup you mean libxlDoMigrateReceive right? There I think we could
simply move unref args to cleanup label (which would free the cookie). But
probably should go into a separate patch as it seems to be an issue to all
migration paths.
> diff --git a/src/libxl/libxl_migration.c
b/src/libxl/libxl_migration.c
> index ba1ca5c..6a7d458 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -637,6 +637,7 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
> }
>
> done:
> + libxlMigrationCookieFree(mig);
> if (vm)
> virObjectUnlock(vm);
>
>
>
>>> + goto error;
>>> +
>>> + if (!(vm = virDomainObjListAdd(driver->domains, *def,
>>> + driver->xmlopt,
>>> + VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
>>> + VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
>>> + NULL)))
>>> + goto error;
>>> +
>>> + *def = NULL;
>>> + priv = vm->privateData;
>>> +
>>> + if (taint_hook) {
>>> + /* Domain XML has been altered by a hook script. */
>>> + priv->hookRun = true;
>>> + }
>>> +
>>> + /*
>>> + * The data flow of tunnel3 migration in the dest side:
>>> + * stream -> pipe -> recvfd of libxlDomainStartRestore
>>> + */
>>> + if (pipe(dataFD) < 0)
>>> + goto error;
>>> +
>>> + /* Stream data will be written to pipeIn */
>>> + if (virFDStreamOpen(st, dataFD[1]) < 0)
>>> + goto error;
>>> + dataFD[1] = -1; /* 'st' owns the FD now & will close it */
>>> +
>>> + if (libxlMigrationDstArgsInitialize() < 0)
>>> + goto error;
>>> +
>>> + if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>>> + goto error;
>>> +
>>> + args->conn = dconn;
>>> + args->vm = vm;
>>> + args->flags = flags;
>>> + args->migcookie = mig;
>>> + /* Receive from pipeOut */
>>> + args->recvfd = dataFD[0];
>>> + args->nsocks = 0;
>>> + if (virThreadCreate(&thread, false, libxlDoMigrateReceive, args)
< 0) {
>>> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>>> + _("Failed to create thread for receiving
migration data"));
>>> + goto error;
>>> + }
>>> +
>>> + ret = 0;
>>> + goto done;
>>> +
>>> + error:
>>> + VIR_FORCE_CLOSE(dataFD[1]);
>>> + VIR_FORCE_CLOSE(dataFD[0]);
>>> + virObjectUnref(args);
>>> + /* Remove virDomainObj from domain list */
>>> + if (vm) {
>>> + virDomainObjListRemove(driver->domains, vm);
>>> + vm = NULL;
>>> + }
>>> +
>>> + done:
>>> + if (vm)
>>> + virObjectUnlock(vm);
>>> +
>>> + return ret;
>>> +}
>>
>> [...]
>>
>>> +
>>> +static int
>>> +libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
>>> + virDomainObjPtr vm,
>>> + unsigned long flags,
>>> + virStreamPtr st,
>>> + struct libxlTunnelControl *tc)
>>> +{
>>> + libxlTunnelMigrationThread *arg = NULL;
>>> + int ret = -1;
>>> +
>>> + if (VIR_ALLOC(tc) < 0)
>>> + goto out;
>>> +
>>> + tc->dataFD[0] = -1;
>>> + tc->dataFD[1] = -1;
>>> + if (pipe(tc->dataFD) < 0) {
>>> + virReportError(errno, "%s", _("Unable to make
pipes"));
>>
>> Coverity notes that failures would cause a leak for @tc (I assume here
>> and of course if virThreadCreate fails. Perhaps the 'best' way to
>> handle that would be to set tc = NULL after ThreadCreate success and
>> just call libxlMigrationStopTunnel in "out:"...
>
> But libxlMigrationStopTunnel is always called (hence should be freeing @tc),
> whether libxlMigrationStartTunnel failed or not. How about this?
>
> @@ -907,8 +908,9 @@ libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
> virDomainObjPtr vm,
> unsigned long flags,
> virStreamPtr st,
> - struct libxlTunnelControl *tc)
> + struct libxlTunnelControl **tnl)
> {
> + struct libxlTunnelControl *tc = *tnl;
This doesn't do much... since @tc would be allocated below and *tnl
wouldn't be updated... Thus, the following is better:
if (VIR_ALLOC(tc) < 0)
goto out;
*tnl = tc;
OK.
> libxlTunnelMigrationThread *arg = NULL;
> int ret = -1;
>
> @@ -1045,7 +1047,7 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
>
> VIR_DEBUG("Perform3 uri=%s", NULLSTR(uri_out));
> if (flags & VIR_MIGRATE_TUNNELLED)
> - ret = libxlMigrationStartTunnel(driver, vm, flags, st, tc);
> + ret = libxlMigrationStartTunnel(driver, vm, flags, st, &tc);
> else
> ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
> uri_out, NULL, flags);
>
3:53 p.m.
Joao Martins wrote:
Hey!
Presented herewith is take 4 from tunnelled migration addressing all previous
comments. No functional changes from v2, only revert to v1 on the top level
migration functions and removing unnecessary usage of virReportError.
Thanks,
Joao
Bob Liu (1):
libxl: add tunnelled migration support
Joao Martins (1):
libxl: refactor libxlDomainMigrationPrepare
src/libxl/libxl_driver.c | 58 ++++++-
src/libxl/libxl_migration.c | 367 +++++++++++++++++++++++++++++++++++++-------
src/libxl/libxl_migration.h | 9 ++
3 files changed, 381 insertions(+), 53 deletions(-)
ACK series. I fixed up the indentation in 2/2 before pushing.
This series is newsworthy. Can you post a followup adding an entry to
docs/news.xml describing this new feature? Thanks!
Regards,
Jim
4:14 p.m.
On 02/15/2017 09:53 PM, Jim Fehlig wrote:
Joao Martins wrote:
> Hey!
>
> Presented herewith is take 4 from tunnelled migration addressing all previous
> comments. No functional changes from v2, only revert to v1 on the top level
> migration functions and removing unnecessary usage of virReportError.
>
> Thanks,
> Joao
>
> Bob Liu (1):
> libxl: add tunnelled migration support
>
> Joao Martins (1):
> libxl: refactor libxlDomainMigrationPrepare
>
> src/libxl/libxl_driver.c | 58 ++++++-
> src/libxl/libxl_migration.c | 367 +++++++++++++++++++++++++++++++++++++-------
> src/libxl/libxl_migration.h | 9 ++
> 3 files changed, 381 insertions(+), 53 deletions(-)
>
ACK series. I fixed up the indentation in 2/2 before pushing.
This series is newsworthy. Can you post a followup adding an entry to
docs/news.xml describing this new feature? Thanks!
Yeap, will do.
Thanks!
Joao
5:01 p.m.
On 02/15/2017 10:14 PM, Joao Martins wrote:
On 02/15/2017 09:53 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>> Hey!
>>
>> Presented herewith is take 4 from tunnelled migration addressing all previous
>> comments. No functional changes from v2, only revert to v1 on the top level
>> migration functions and removing unnecessary usage of virReportError.
>>
>> Thanks,
>> Joao
>>
>> Bob Liu (1):
>> libxl: add tunnelled migration support
>>
>> Joao Martins (1):
>> libxl: refactor libxlDomainMigrationPrepare
>>
>> src/libxl/libxl_driver.c | 58 ++++++-
>> src/libxl/libxl_migration.c | 367 +++++++++++++++++++++++++++++++++++++-------
>> src/libxl/libxl_migration.h | 9 ++
>> 3 files changed, 381 insertions(+), 53 deletions(-)
>>
>
> ACK series. I fixed up the indentation in 2/2 before pushing.
>
> This series is newsworthy. Can you post a followup adding an entry to
> docs/news.xml describing this new feature? Thanks!
Yeap, will do.
Just sent it over [0]. Maybe it's also worth adding news
coverage to your libxl
timer improvements, and the many maxmem related fixes :)
[0] https://www.redhat.com/archives/libvir-list/2017-February/msg00761.html
5:46 p.m.
Joao Martins wrote:
On 02/15/2017 10:14 PM, Joao Martins wrote:
> On 02/15/2017 09:53 PM, Jim Fehlig wrote:
>> Joao Martins wrote:
>>> Hey!
>>>
>>> Presented herewith is take 4 from tunnelled migration addressing all
previous
>>> comments. No functional changes from v2, only revert to v1 on the top level
>>> migration functions and removing unnecessary usage of virReportError.
>>>
>>> Thanks,
>>> Joao
>>>
>>> Bob Liu (1):
>>> libxl: add tunnelled migration support
>>>
>>> Joao Martins (1):
>>> libxl: refactor libxlDomainMigrationPrepare
>>>
>>> src/libxl/libxl_driver.c | 58 ++++++-
>>> src/libxl/libxl_migration.c | 367
+++++++++++++++++++++++++++++++++++++-------
>>> src/libxl/libxl_migration.h | 9 ++
>>> 3 files changed, 381 insertions(+), 53 deletions(-)
>>>
>> ACK series. I fixed up the indentation in 2/2 before pushing.
>>
>> This series is newsworthy. Can you post a followup adding an entry to
>> docs/news.xml describing this new feature? Thanks!
> Yeap, will do.
Just sent it over [0]. Maybe it's also worth adding news coverage to your libxl
timer improvements, and the many maxmem related fixes :)
Touche :-). I guess those are newsworthy as well. I'll send some patches in a bit.
Regards,
Jim
2836
days inactive
2837
days old
11 comments
3 participants
participants (3)
-
Jim Fehlig -
Joao Martins -
John Ferlan