[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH RFC] helper of copy-storage-* features



在 2012-10-21日的 21:09 -0500,Doug Goldstein写道:
> On Sun, Oct 21, 2012 at 7:53 PM, liguang <lig fnst 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.
> 
> >

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

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 redhat com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> 
> 
> 

-- 
liguang    lig fnst cn fujitsu com
FNST linux kernel team



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]