[libvirt] [PATCH RFC] helper of copy-storage-* features
Doug Goldstein
cardoe at gentoo.org
Mon Oct 22 02:09:24 UTC 2012
On Sun, Oct 21, 2012 at 7:53 PM, liguang <lig.fnst at cn.fujitsu.com> 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.
>
> Signed-off-by: liguang <lig.fnst at cn.fujitsu.com>
> ---
> 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.
> + 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Doug Goldstein
More information about the libvir-list
mailing list