[libvirt] [PATCH v3 1/2] helper of copy-storage-* features
Eric Blake
eblake at redhat.com
Fri Nov 9 05:10:32 UTC 2012
On 10/31/2012 06:40 PM, liguang 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 usage had been discussed before,
> http://www.redhat.com/archives/libvir-list/2011-December/msg00451.html
Typically, you would put your signed off line here, followed by ---, so
that the rest of this commentary...
>
> 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.
> - for creation of disk images, size was setting as 0xffffffff boldly,
Setting to MAX_INT sounds wrong. Either you create the destination
image empty and then let qemu automatically enlarge it as it populates
incoming data (preferred), or you need to pass size information in the
cookie (harder).
> hope it can consolidate qemu, haven't constructed a comfortable
> idea to solve it.
>
> v2:
> 1. handle disk encrytion case
> 2. check kvm-img & qemu-img
> 3. set disk image size to 0xfffffffK bytes blindly
>
> v3:
> 1.use qemuImgBinary to create disk image
> 2.set max size for different disk image format respectively
> qcow and qcow2: 1PiB
> qed:64TiB
> raw:1TiB
> from qemu's setting,
> qcow and qcow2's max size is 2EiB,
> qed's max size is 64TiB
> raw's max size is 1TiB
...remains useful for reviewing on list but is not codified in git when
the patch is finally approved. That is, when reading 'git log', we
don't care how many versions it took us to get to the right solution,
only that the right solution is in git. 'git am' automatically strips
comments after the first ---, making it a useful divider between the
commit description and the notes for reviewers.
>
> Signed-off-by: liguang <lig.fnst at cn.fujitsu.com>
> ---
> src/qemu/qemu_migration.c | 122 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 120 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index db69a0a..80abb51 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,
>
I'm not yet convinced whether you need to do anything with the cookie.
Rather, it should be sufficient to add a new public VIR_MIGRATE_* flag
that says whether libvirt should be attempting to create destination
files when given VIR_MIGRATE_NON_SHARED_{DISK,INC}. Then, just like the
offline migration case, all you have to do is make sure the flag is
properly passed through the entire migration chain to all the points
that care about it. But if you DO need the cookie, the I think you need
more than just a single flag - you need to pass a list of all file
information (such as size) that will be needed to create the same types
of files on the destination.
>
> +/*
> + if gen_del is 1, find out disk images migration required,
> + so try to generate them at target,
> + if gen_del is 0, delete disk images generated before.
> +*/
> +static int qemuMigrationHandleDiskFiles(struct qemud_driver *driver,
> + virDomainDefPtr def, int gen_del)
It sounds like you are using gen_del as a bool - if so, type it as bool,
not int. And its name is confusing; I might go with 'bool generate',
where true means generate, and false means delete.
> +{
> + char *tmp_dir = NULL, *outbuf = NULL;
> + char *img_tool = driver->qemuImgBinary;
Don't grab this field directly. Instead, use qemuFindQemuImgBinary(driver).
> + 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;
You need to rebase your patches on top of the latest libvirt.git. The
driverName field no longer exists; it is now an enum value named 'format'.
> + if (def->disks[i]->src == NULL)
> + continue;
> + if (def->disks[i]->driverType == NULL)
> + continue;
> + if (virFileExists(def->disks[i]->src) && gen_del)
> + continue;
> + if (!gen_del && !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 (gen_del) {
> + cmd = virCommandNewArgList(img_tool, "create", "-f",
> + def->disks[i]->driverType, def->disks[i]->src, NULL);
> + if (STREQ(def->disks[i]->driverType, "qcow2") ||
> + STREQ(def->disks[i]->driverType, "qcow"))
> + virCommandAddArgFormat(cmd, "%lluK", 0xffffffffff);
Ouch. You should not be passing in random sizes to qemu - if you need
to know the source size in order to create a file on the destination
side, then we HAVE to modify the migration cookie in order to pass
sizing information when the new flag is present.
> + } else {
> + if (unlink(def->disks[i]->src) < 0) {
> + virReportError(errno, "%s", _("fail to unlink disk image file"));
> + goto cleanup;
Are you sure that this only ever unlink()s files that you just created,
or does it have the possibility of unlinking a file that already existed
prior to the migration attempt?
--
Eric Blake eblake at redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121108/dbede5b2/attachment-0001.sig>
More information about the libvir-list
mailing list