[libvirt] [PATCH v3 1/2] helper of copy-storage-* features

[li guang]

在 2012-11-08四的 22:10 -0700，Eric Blake写道：
> 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...
> 

OK, got it.

> > 
> > 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).

can qemu automatically enlarge?
I tried, but failed.

> 
> >   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.
> 

as we talk before.

> >  
> > +/*
> > +  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.

right, will fix.

> 
> > +{
> > +    char *tmp_dir = NULL, *outbuf = NULL;
> > +    char *img_tool = driver->qemuImgBinary;
> 
> Don't grab this field directly.  Instead, use qemuFindQemuImgBinary(driver).

driver->qemuImgBinary is useful, I've initiated it.
why don't we use it?

> 
> > +    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'.

OK.

> 
> > +        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.
> 

passing by cookie seems a little rough, 
these sizes are not random, they're 
the max size of their format.
does setting max size hurt? the disk images are sparse files,
and after qemu migration finished, size will be right.

> > +        } 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?
> 

-- 
li guang  lig.fnst at cn.fujitsu.com
linux kernel team at FNST, china