On Tue, Nov 13, 2012 at 9:03 PM, liguang <lig.fnst@cn.fujitsu.com> wrote:

try to do non-shared migration without bothering to create disk images at target by hand.

consider this situation: 1. non-shared migration virsh migrate --copy-storage-all ... 2. migration fails 3. create disk images required qemu-img create ... 4 migration run smoothly so, try do remove step 2, 3, 4

this kind of usage had been discussed before, http://www.redhat.com/archives/libvir-list/2011-December/msg00451.html

this patch depends on my support offline migration patch: https://www.redhat.com/archives/libvir-list/2012-November/msg00512.html

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> --- src/qemu/qemu_migration.c | 283 ++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 282 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2479114..6d4a4d7 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -50,6 +50,7 @@ #include "storage_file.h" #include "viruri.h" #include "hooks.h" +#include "dirname.h"

#define VIR_FROM_THIS VIR_FROM_QEMU @@ -72,6 +73,7 @@ enum qemuMigrationCookieFlags { QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE, QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT, QEMU_MIGRATION_COOKIE_FLAG_NETWORK, + QEMU_MIGRATION_COOKIE_FLAG_COPYSTORAGE,

QEMU_MIGRATION_COOKIE_FLAG_LAST }; @@ -79,13 +81,14 @@ enum qemuMigrationCookieFlags { VIR_ENUM_DECL(qemuMigrationCookieFlag); VIR_ENUM_IMPL(qemuMigrationCookieFlag, QEMU_MIGRATION_COOKIE_FLAG_LAST, - "graphics", "lockstate", "persistent", "network"); + "graphics", "lockstate", "persistent", "network", "storage");

enum qemuMigrationCookieFeatures { QEMU_MIGRATION_COOKIE_GRAPHICS = (1 << QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS), QEMU_MIGRATION_COOKIE_LOCKSTATE = (1 << QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE), QEMU_MIGRATION_COOKIE_PERSISTENT = (1 << QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT), QEMU_MIGRATION_COOKIE_NETWORK = (1 << QEMU_MIGRATION_COOKIE_FLAG_NETWORK), + QEMU_MIGRATION_COOKIE_COPYSTORAGE = (1 << QEMU_MIGRATION_COOKIE_FLAG_COPYSTORAGE), };

typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics; @@ -119,6 +122,19 @@ struct _qemuMigrationCookieNetwork { qemuMigrationCookieNetDataPtr net; };

+typedef struct _qemuMigrationCookieStorageData qemuMigrationCookieStorageData; +typedef qemuMigrationCookieStorageData *qemuMigrationCookieStorageDataPtr; +struct _qemuMigrationCookieStorageData { + char *dsize; +}; + +typedef struct _qemuMigrationCookieStorage qemuMigrationCookieStorage; +typedef qemuMigrationCookieStorage *qemuMigrationCookieStoragePtr; +struct _qemuMigrationCookieStorage { + int ndisks; + qemuMigrationCookieStorageDataPtr disk; +}; + typedef struct _qemuMigrationCookie qemuMigrationCookie; typedef qemuMigrationCookie *qemuMigrationCookiePtr; struct _qemuMigrationCookie { @@ -147,6 +163,9 @@ struct _qemuMigrationCookie {

/* If (flags & QEMU_MIGRATION_COOKIE_NETWORK) */ qemuMigrationCookieNetworkPtr network; + + /* If (flags & QEMU_MIGRATION_COOKIE_COPYSTORAGE) */ + qemuMigrationCookieStoragePtr storage; };

static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap) @@ -175,6 +194,21 @@ qemuMigrationCookieNetworkFree(qemuMigrationCookieNetworkPtr network) VIR_FREE(network); }

+static void +qemuMigrationCookieStorageFree(qemuMigrationCookieStoragePtr storage) +{ + int i; + + if (!storage) + return; + + if (storage->disk) { + for (i = 0; i < storage->ndisks; i++) + VIR_FREE(storage->disk[i].dsize); + } + VIR_FREE(storage->disk); + VIR_FREE(storage); +}

static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig) { @@ -187,6 +221,9 @@ static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig) if (mig->flags & QEMU_MIGRATION_COOKIE_NETWORK) qemuMigrationCookieNetworkFree(mig->network);

+ if (mig->flags & QEMU_MIGRATION_COOKIE_COPYSTORAGE) + qemuMigrationCookieStorageFree(mig->storage); + VIR_FREE(mig->localHostname); VIR_FREE(mig->remoteHostname); VIR_FREE(mig->name); @@ -356,6 +393,64 @@ error: return NULL; }

+static qemuMigrationCookieStoragePtr +qemuMigrationCookieStorageAlloc(struct qemud_driver *driver, + virDomainDefPtr def) +{ + int i, exitstatus; + char *info = NULL, *start, *end, *tmp = NULL; + virCommandPtr cmd = NULL; + qemuMigrationCookieStoragePtr mig; + + if (VIR_ALLOC(mig) < 0) + goto no_memory; + + if (VIR_ALLOC_N(mig->disk, def->ndisks) < 0) + goto no_memory; + if (!driver->qemuImgBinary) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("unable to find kvm-img or qemu-img"));

Use qemuFindQemuImgBinary() instead.

+ goto error; + } + mig->ndisks = def->ndisks; + + for (i = 0; i < mig->ndisks; i++) { + cmd = virCommandNewArgList(driver->qemuImgBinary, "info", + def->disks[i]->src, NULL); + virCommandAddEnvString(cmd, "LC_ALL=C"); + virCommandSetOutputBuffer(cmd, &info); + virCommandClearCaps(cmd); + if (virCommandRun(cmd, &exitstatus) < 0) + goto error; + if ((start = strstr(info, "virtual size: ")) == NULL || + ((end = strstr(start, "\n")) == NULL)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to parse qemu-img output '%s'"), + info); + goto error; + } + if ((tmp = strstr(start, " (")) && tmp < end) + end = tmp; + start += strlen("virtual size: "); + if (start >= end) + goto error; + if (!(mig->disk[i].dsize = strndup(start, end-start))) + goto error; + VIR_FREE(info); + virCommandFree(cmd); + } + + return mig; + +no_memory: + virReportOOMError(); +error: + virCommandFree(cmd); + VIR_FREE(info); + qemuMigrationCookieStorageFree(mig); + return NULL; +}

Honestly this entire function should be dropped. Doing a callout to qemu-img isn't good due to the fact that we might have other file types in play and this would effectively circumvent the disk format probing prevention. Among other reasons.. Really I think you should take a look at qemuDomainGetBlockInfo(), you're looking for the capacity value. It'd probably be worthwhile to refactor some of that code into a helper function that can be used by both qemuDomainGetBlockInfo() and qemuMigrationCookieStorageAlloc(). It might also be possible for you to just call qemuDomainGetBlockInfo() directly if you have all the input available in the callee, which is possible.

+ static qemuMigrationCookiePtr qemuMigrationCookieNew(virDomainObjPtr dom) { @@ -491,6 +586,25 @@ qemuMigrationCookieAddNetwork(qemuMigrationCookiePtr mig, return 0; }

+static int qemuMigrationCookieAddStorage(qemuMigrationCookiePtr mig, + struct qemud_driver *driver, + virDomainObjPtr dom) +{ + if (mig->flags & QEMU_MIGRATION_COOKIE_COPYSTORAGE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("migration storage data already present")); + return -1; + } + + if (dom->def->ndisks > 0) { + mig->storage = qemuMigrationCookieStorageAlloc(driver, dom->def); + if (!mig->storage) + return -1; + mig->flags |= QEMU_MIGRATION_COOKIE_COPYSTORAGE; + } + + return 0; +}

static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf, qemuMigrationCookieGraphicsPtr grap) @@ -540,6 +654,19 @@ qemuMigrationCookieNetworkXMLFormat(virBufferPtr buf, virBufferAddLit(buf, " </network>\n"); }

+static void +qemuMigrationCookieStorageXMLFormat(virBufferPtr buf, + qemuMigrationCookieStoragePtr dsz) +{ + int i = 0; + + for (i = 0; i < dsz->ndisks; i++) { + char *dsize = dsz->disk[i].dsize; + virBufferAsprintf(buf, " <copystorage>\n"); + virBufferAsprintf(buf, " <disksize>%s</disksize>\n", dsize); + virBufferAddLit(buf, " </copystorage>\n"); + }

Is it safe to assume this list will be ordered the same way on the src and dest machines with regard to target file/partition? If not we need to carry some value along to label each disk.

+}

static int qemuMigrationCookieXMLFormat(struct qemud_driver *driver, @@ -594,6 +721,9 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver, if ((mig->flags & QEMU_MIGRATION_COOKIE_NETWORK) && mig->network) qemuMigrationCookieNetworkXMLFormat(buf, mig->network);

+ if ((mig->flags & QEMU_MIGRATION_COOKIE_COPYSTORAGE) && mig->storage) + qemuMigrationCookieStorageXMLFormat(buf, mig->storage); + virBufferAddLit(buf, "</qemu-migration>\n"); return 0; } @@ -722,6 +852,44 @@ error: goto cleanup; }

+static qemuMigrationCookieStoragePtr +qemuMigrationCookieStorageXMLParse(xmlXPathContextPtr ctxt) +{ + qemuMigrationCookieStoragePtr dsz; + int i, n; + xmlNodePtr *storage = NULL; + + if (VIR_ALLOC(dsz) < 0) + goto no_memory; + + if ((n = virXPathNodeSet("./copystorage", ctxt, &storage)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing storage information")); + goto error; + } + + dsz->ndisks = n; + if (VIR_ALLOC_N(dsz->disk, dsz->ndisks) < 0) + goto no_memory; + + for (i = 0; i < dsz->ndisks; i++) { + ctxt->node = storage[i]; + if (!(dsz->disk[i].dsize = virXPathString("string(./disksize[1])", ctxt))) + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing tlsPort attribute in migration data"));

Wrong error message here.

+ } + +cleanup: + VIR_FREE(storage); + return dsz; + +no_memory: + virReportOOMError(); +error: + qemuMigrationCookieStorageFree(dsz); + dsz = NULL; + goto cleanup; +}

static int qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, @@ -874,6 +1042,11 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, (!(mig->network = qemuMigrationCookieNetworkXMLParse(ctxt)))) goto error;

+ if ((flags & QEMU_MIGRATION_COOKIE_COPYSTORAGE) && + virXPathBoolean("count(./copystorage) > 0", ctxt) && + (!(mig->storage = qemuMigrationCookieStorageXMLParse(ctxt)))) + goto error; + return 0;

error: @@ -938,6 +1111,11 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig, return -1; }

+ if (flags & QEMU_MIGRATION_COOKIE_COPYSTORAGE && + qemuMigrationCookieAddStorage(mig, driver, dom) < 0) { + return -1; + } + if (!(*cookieout = qemuMigrationCookieXMLFormatStr(driver, mig))) return -1;

@@ -1443,6 +1621,11 @@ char *qemuMigrationBegin(struct qemud_driver *driver, QEMU_MIGRATION_COOKIE_LOCKSTATE) < 0) goto cleanup;

+ if (qemuMigrationBakeCookie(mig, driver, vm, + cookieout, cookieoutlen, + QEMU_MIGRATION_COOKIE_COPYSTORAGE) < 0) + goto cleanup; + if (flags & VIR_MIGRATE_OFFLINE) { if (flags & (VIR_MIGRATE_NON_SHARED_DISK| VIR_MIGRATE_NON_SHARED_INC)) { @@ -1484,6 +1667,89 @@ cleanup: }

+/* + if gen is true, find out disk images migration required, + so try to generate them at target, + if gen is false, delete disk images generated before. +*/ +static int qemuMigrationHandleDiskFiles(struct qemud_driver *driver, + virDomainDefPtr def, bool gen, + qemuMigrationCookiePtr mig) +{ + char *tmp_dir = NULL, *outbuf = NULL; + char *img_tool = driver->qemuImgBinary;

Should be using qemuFindQemuImgBinary().

+ const char *disk_format[] = {"none", "raw", "none", "none", "none", + "cow", "none", "none", "qcow", "qcow2", + "qed", "vmdk", "vpc","none", + };

Drop this whole list and....

+ virCommandPtr cmd = NULL; + int i, ret = -1; + + if (!def->ndisks) + return 0; + + if (img_tool == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("unable to find kvm-img or qemu-img")); + goto error; + } + + for (i = 0; i < def->ndisks; i++) { + if (STRNEQ(def->disks[i]->driverName, "qemu")) + continue; + if (def->disks[i]->src == NULL) + continue; + if (virFileExists(def->disks[i]->src) && gen) + continue; + if (!gen && !virFileExists(def->disks[i]->src)) + continue; + if ((tmp_dir = mdir_name(def->disks[i]->src)) == NULL) + continue; + if (!virFileExists(tmp_dir)) + if (virFileMakePath(tmp_dir) < 0) + continue; + if (STREQ(disk_format[def->disks[i]->format], "none")) + continue; + + if (gen) { + char *dsize = mig->storage->disk[i].dsize; + if (def->disks[i]->format < VIR_STORAGE_FILE_RAW) + goto error; + cmd = virCommandNewArgList(img_tool, "create", "-f", + disk_format[def->disks[i]->format],

...use virStorageFileFormatTypeToString().

+ def->disks[i]->src, NULL); + virCommandAddArgFormat(cmd, "%s", dsize); + if (def->disks[i]->encryption) + virCommandAddArgList(cmd, "-o", "encryption=on", NULL); + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, NULL) < 0) { + virReportSystemError(errno, "%s", outbuf); + goto cleanup; + } + } else { + if (unlink(def->disks[i]->src) < 0) { + virReportError(errno, "%s", _("fail to unlink disk image file")); + goto cleanup; + } + } + virCommandFree(cmd); + VIR_FREE(tmp_dir); + VIR_FREE(outbuf); + } + + ret = 0; + +cleanup: + if (ret < 0) { + virCommandFree(cmd); + VIR_FREE(tmp_dir); + VIR_FREE(outbuf); + } +error: + return ret; +} + + /* Prepare is the first step, and it runs on the destination host. */

@@ -1599,6 +1865,15 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, /* virDomainAssignDef already set the error */ goto cleanup; } + + if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, + QEMU_MIGRATION_COOKIE_COPYSTORAGE))) + goto cleanup; + + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_DISK)) + if (qemuMigrationHandleDiskFiles(driver, def, true, mig) < 0) + goto endjob; + def = NULL; priv = vm->privateData; priv->origname = origname; @@ -3239,6 +3514,7 @@ qemuMigrationFinish(struct qemud_driver *driver, virErrorPtr orig_err = NULL; int cookie_flags = 0; qemuDomainObjPrivatePtr priv = vm->privateData; + bool migration_status = false;

VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=%lx, retcode=%d", @@ -3404,7 +3680,12 @@ qemuMigrationFinish(struct qemud_driver *driver, if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, 0) < 0) VIR_WARN("Unable to encode migration cookie");

+ migration_status = true; + endjob: + if (!migration_status && flags & + (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_DISK)) + qemuMigrationHandleDiskFiles(driver, vm->def, false, NULL); if (qemuMigrationJobFinish(driver, vm) == 0) { vm = NULL; } else if (!vm->persistent && !virDomainObjIsActive(vm)) { -- 1.7.1

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Just a quick look through tonight but given some comments I think this warrants another revision. Its possible we might want to not do some of the creation manually and ask the storage backend to be doing that for us as well. -- Doug Goldstein

On Wed, Nov 14, 2012 at 1:56 AM, li guang <lig.fnst@cn.fujitsu.com> wrote:

在 2012-11-14三的 01:38 -0600，Doug Goldstein写道：

...

On Tue, Nov 13, 2012 at 9:03 PM, liguang <lig.fnst@cn.fujitsu.com> wrote:

...

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a5592b9..eb5a3d1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1647,7 +1647,7 @@ qemuDomainSnapshotForEachQcow2Raw(struct qemud_driver *driver, int i; bool skipped = false;

- qemuimgarg[0] = qemuFindQemuImgBinary(driver); + qemuimgarg[0] = driver->qemuImgBinary; if (qemuimgarg[0] == NULL) { /* qemuFindQemuImgBinary set the error */ return -1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5f91688..a25fb53 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -626,6 +626,9 @@ qemudStartup(int privileged) { if (!qemu_driver->domainEventState) goto error;

+ /* find kvm-img or qemu-img */ + qemuFindQemuImgBinary(qemu_driver); + /* read the host sysinfo */ if (privileged) qemu_driver->hostsysinfo = virSysinfoRead(); -- 1.7.1

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Honestly this entire patch can be dropped because its not correct. qemuFindQemuImgBinary() only looks up the path to the binary the first time and then caches it from there on in. So you should always be using qemuFindQemuImgBinary(). As far as the addition to the startup of looking it up, it seems like that's really a pre-mature optimization for your second patch.

So I'd have to say NACK on this change.

your reason sounds strange, you call qemuFindQemuImgBinary() to get qemu-img handler every time you want to use it, I use the handler freely for I've I've initialized before why can't I use driver->qemuImgBinary instead of qemuFindQemuImgBinary()?

I would argue that its bad form to do that for at least 2 reasons. 1. The fact that driver->qemuImgBinary is "hidden" behind a function to access it means you should effectively treat it as opaque and not directly access it. 2. Its only found when its actually needed rather than always looking it up when libvirt starts up. qemuFindQemuImgBinary() is functionally equivalent to driver->qemuImgBinary after the first call. -- Doug Goldstein