在 2012-10-21日的 21:09 -0500，Doug Goldstein写道： > On Sun, Oct 21, 2012 at 7:53 PM, liguang <lig.fnst@xxxxxxxxxxxxxx> wrote: > > help to create disk images copy-storage-* required, > > 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 use had been discussed before, > > http://www.redhat.com/archives/libvir-list/2011-December/msg00451.html > > > > maybe there're some flaws: > > - It did not handle more about complete situations > > suggested by Daniel P. Berrange, > > https://www.redhat.com/archives/libvir-list/2012-October/msg00407.html > > but may try to take care of them later. > > so, now only normal disk image files be handled. > > The problem is it will silently blow up for people using LVM or other > backend types which is definitely not ok. You should do preflight > checks to ensure that you can handle all the disks before attempting > the migration. > > > - for creation of disk images, size was setting as 0xffffffff boldly, > > hope it can consolidate qemu, haven't constructed a comfortable > > idea to solve it. > > What if the virtual disk coming over is larger? Will this go boom? Or > are you expecting that qemu will resize? If you're expecting that qemu > will resize it then you should likely only create the disk 1 block big > or so. If not you need to send the sizes as part of the cookie. > > > I don't know exactly what action qemu will take if pre-alloc is smaller, sorry about it. my pre-allocation will only take small real disk space, and hope it's greater than real disk space migration required. pretty roughly way! or, any good idea about it? but, I really don't want to take disk size from source to target. > > Signed-off-by: liguang <lig.fnst@xxxxxxxxxxxxxx> > > --- > > src/qemu/qemu_migration.c | 87 +++++++++++++++++++++++++++++++++++++++++++- > > 1 files changed, 85 insertions(+), 2 deletions(-) > > > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > > index db69a0a..5c3ad9f 100644 > > --- a/src/qemu/qemu_migration.c > > +++ b/src/qemu/qemu_migration.c > > @@ -49,6 +49,7 @@ > > #include "storage_file.h" > > #include "viruri.h" > > #include "hooks.h" > > +#include "dirname.h" > > > > > > #define VIR_FROM_THIS VIR_FROM_QEMU > > @@ -70,6 +71,7 @@ enum qemuMigrationCookieFlags { > > QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS, > > QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE, > > QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT, > > + QEMU_MIGRATION_COOKIE_FLAG_COPYSTORAGE, > > > > QEMU_MIGRATION_COOKIE_FLAG_LAST > > }; > > @@ -77,12 +79,13 @@ enum qemuMigrationCookieFlags { > > VIR_ENUM_DECL(qemuMigrationCookieFlag); > > VIR_ENUM_IMPL(qemuMigrationCookieFlag, > > QEMU_MIGRATION_COOKIE_FLAG_LAST, > > - "graphics", "lockstate", "persistent"); > > + "graphics", "lockstate", "persistent", "copystorage"); > > > > 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_COPYSTORAGE = (1 << QEMU_MIGRATION_COOKIE_FLAG_COPYSTORAGE), > > }; > > > > typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics; > > @@ -439,6 +442,9 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver, > > virBufferAdjustIndent(buf, -2); > > } > > > > + if (mig->flags & QEMU_MIGRATION_COOKIE_COPYSTORAGE) > > + virBufferAsprintf(buf, " <copystorage/>\n"); > > + > > virBufferAddLit(buf, "</qemu-migration>\n"); > > return 0; > > } > > @@ -662,6 +668,11 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, > > VIR_FREE(nodes); > > } > > > > + if ((flags & QEMU_MIGRATION_COOKIE_COPYSTORAGE)) { > > + if (virXPathBoolean("count(./copystorage) > 0", ctxt)) > > + mig->flags |= QEMU_MIGRATION_COOKIE_COPYSTORAGE; > > + } > > + > > return 0; > > > > error: > > @@ -721,6 +732,9 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig, > > qemuMigrationCookieAddPersistent(mig, dom) < 0) > > return -1; > > > > + if (flags & QEMU_MIGRATION_COOKIE_COPYSTORAGE) > > + mig->flags |= QEMU_MIGRATION_COOKIE_COPYSTORAGE; > > + > > if (!(*cookieout = qemuMigrationCookieXMLFormatStr(driver, mig))) > > return -1; > > > > @@ -1168,6 +1182,14 @@ char *qemuMigrationBegin(struct qemud_driver *driver, > > QEMU_MIGRATION_COOKIE_LOCKSTATE) < 0) > > goto cleanup; > > > > + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | > > + VIR_MIGRATE_NON_SHARED_INC)) { > > + if (qemuMigrationBakeCookie(mig, driver, vm, > > + cookieout, cookieoutlen, > > + QEMU_MIGRATION_COOKIE_COPYSTORAGE) < 0) > > + goto cleanup; > > + } > > + > > if (xmlin) { > > if (!(def = virDomainDefParseString(driver->caps, xmlin, > > QEMU_EXPECTED_VIRT_TYPES, > > @@ -1215,6 +1237,54 @@ qemuMigrationPrepareCleanup(struct qemud_driver *driver, > > qemuDomainObjDiscardAsyncJob(driver, vm); > > } > > > > +static int qemuMigrationHandleDiskFiles(virDomainDefPtr def, int pin) > > Please provide some kind of documentation as to what the function is > expected to do and potentially return. Also describe the arguments, > specifically 'pin'. It seems poorly named. It appears to mean you want > the disk file created or deleted. > > > +{ > > + char *tmp_dir = NULL, *outbuf = NULL; > > + char *size = _("0xffffffff"); > > + virCommandPtr cmd = NULL; > > + int i, ret = -1; > > + > > + if (!def->ndisks) > > + return 0; > > Redundant with the for loop. > > > + for (i = 0; i < def->ndisks; i++) { > > Some code comments above these checks would definitely make this > clearer and wouldn't cause someone to have to grok all the checks to > know what's about to happen. > > > + if (STRNEQ(def->disks[i]->driverName, "qemu")) > > + continue; > > + if (def->disks[i]->src == NULL) > > + continue; > > + if (def->disks[i]->driverType == NULL) > > + goto cleanup; > > If you got a NULL driver don't you want to try the next disk instead of exiting? > > > + if (virFileExists(def->disks[i]->src) && pin) > > + continue; > > + if (!pin && !virFileExists(def->disks[i]->src)) > > + continue; > > At least align the above arguments to make it clearer. Or refactor it > to make it even clearer. > > > + if ((tmp_dir = mdir_name(def->disks[i]->src)) == NULL) > > + continue; > > Based on Eric's comments I thought you were going to make a wrapper > function for this and handle the NULL case. > > mdir_name() also allocates memory and you're calling it inside of a > loop but only freeing it outside of the loop so this is a memory leak. yes, thanks! > > > + if (!virFileExists(tmp_dir)) > > + if (virFileMakePath(tmp_dir) < 0) > > + continue; > > + if (pin) { > > + cmd = virCommandNewArgList("qemu-img", "create", "-f", > > + def->disks[i]->driverType, def->disks[i]->src, > > + size, NULL); > > + virCommandSetOutputBuffer(cmd, &outbuf); > > You aren't doing anything with outbuf and you aren't freeing it either. > > > + } else > > + cmd = virCommandNewArgList("rm", "-f", def->disks[i]->src, NULL); > > + > > + if (virCommandRun(cmd, NULL) < 0) { > > + goto cleanup; > > + virReportSystemError(errno, "%s", > > + _("unable create disk images by qemu-img")); > > + } > > The error message reported doesn't match what happened when the rm -f > fails. Also 'cmd' is being leaked here. > > > + } > > + > > + ret = 0; > > + > > +cleanup: > > + virCommandFree(cmd); > > + VIR_FREE(tmp_dir); > > + return ret; > > +} > > + > > static int > > qemuMigrationPrepareAny(struct qemud_driver *driver, > > virConnectPtr dconn, > > @@ -1308,6 +1378,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 (mig->flags & QEMU_MIGRATION_COOKIE_COPYSTORAGE) > > + if (qemuMigrationHandleDiskFiles(def, 1) < 0) > > + goto cleanup; > > + > > def = NULL; > > priv = vm->privateData; > > priv->origname = origname; > > @@ -2929,7 +3008,7 @@ qemuMigrationFinish(struct qemud_driver *driver, > > int newVM = 1; > > qemuMigrationCookiePtr mig = NULL; > > virErrorPtr orig_err = NULL; > > - int cookie_flags = 0; > > + int cookie_flags = 0, migration_status = 0; > > qemuDomainObjPrivatePtr priv = vm->privateData; > > > > VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " > > @@ -3088,7 +3167,11 @@ qemuMigrationFinish(struct qemud_driver *driver, > > if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, 0) < 0) > > VIR_WARN("Unable to encode migration cookie"); > > > > + migration_status = 1; > > + > > endjob: > > + if (!migration_status) > > + qemuMigrationHandleDiskFiles(vm->def, 0); > > if (qemuMigrationJobFinish(driver, vm) == 0) { > > vm = NULL; > > } else if (!vm->persistent && !virDomainObjIsActive(vm)) { > > -- > > 1.7.2.5 > > > > -- > > libvir-list mailing list > > libvir-list@xxxxxxxxxx > > https://www.redhat.com/mailman/listinfo/libvir-list > > > -- liguang lig.fnst@xxxxxxxxxxxxxx FNST linux kernel team -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list