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

Re: [libvirt] [PATCH] [3/4] Implement managed save operations for qemu driver



On Fri, Apr 02, 2010 at 09:56:00PM +0200, Daniel Veillard wrote:
> [ v2 with new saving path, bug fix and typo fix ]
> 
> Implement managed save operations for qemu driver
> 
> The images are saved in /var/lib/libvirt/qemu/save/
> and named $domainname.save . The directory is created appropriately
> at daemon startup. When a domain is started while a saved image is
> available, libvirt will try to load this saved image, and start the
> domain as usual in case of failure. In any case the saved image is
> discarded once the domain is created.
> 
> * src/qemu/qemu_conf.h: adds an extra save path to the driver config
> * src/qemu/qemu_driver.c: implement the 3 new operations and handling
>   of the image directory
> 
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 39518ca..0b247d6 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -120,6 +120,7 @@ struct qemud_driver {
>       * the QEMU user/group */
>      char *libDir;
>      char *cacheDir;
> +    char *saveDir;
>      unsigned int vncTLS : 1;
>      unsigned int vncTLSx509verify : 1;
>      unsigned int vncSASL : 1;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 94f7fef..5a678c9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1399,6 +1399,9 @@ qemudStartup(int privileged) {
>          if (virAsprintf(&qemu_driver->cacheDir,
>                        "%s/cache/libvirt/qemu", LOCAL_STATE_DIR) == -1)
>              goto out_of_memory;
> +        if (virAsprintf(&qemu_driver->saveDir,
> +                      "%s/lib/libvirt/qemu/save/", LOCAL_STATE_DIR) == -1)
> +            goto out_of_memory;
>      } else {
>          uid_t uid = geteuid();
>          char *userdir = virGetUserDirectory(uid);
> @@ -1423,6 +1426,8 @@ qemudStartup(int privileged) {
>              goto out_of_memory;
>          if (virAsprintf(&qemu_driver->cacheDir, "%s/qemu/cache", base) == -1)
>              goto out_of_memory;
> +        if (virAsprintf(&qemu_driver->saveDir, "%s/qemu/save", base) == -1)
> +            goto out_of_memory;
>      }
>  
>      if (virFileMakePath(qemu_driver->stateDir) != 0) {
> @@ -1443,6 +1448,12 @@ qemudStartup(int privileged) {
>                    qemu_driver->cacheDir, virStrerror(errno, ebuf, sizeof ebuf));
>          goto error;
>      }
> +    if (virFileMakePath(qemu_driver->saveDir) != 0) {
> +        char ebuf[1024];
> +        VIR_ERROR(_("Failed to create save dir '%s': %s"),
> +                  qemu_driver->saveDir, virStrerror(errno, ebuf, sizeof ebuf));
> +        goto error;
> +    }
>  
>      /* Configuration paths are either ~/.libvirt/qemu/... (session) or
>       * /etc/libvirt/qemu/... (system).
> @@ -1493,6 +1504,12 @@ qemudStartup(int privileged) {
>                                   qemu_driver->cacheDir, qemu_driver->user, qemu_driver->group);
>              goto error;
>          }
> +        if (chown(qemu_driver->saveDir, qemu_driver->user, qemu_driver->group) < 0) {
> +            virReportSystemError(errno,
> +                                 _("unable to set ownership of '%s' to %d:%d"),
> +                                 qemu_driver->saveDir, qemu_driver->user, qemu_driver->group);
> +            goto error;
> +        }
>      }
>  
>      /* If hugetlbfs is present, then we need to create a sub-directory within
> @@ -1645,6 +1662,7 @@ qemudShutdown(void) {
>      VIR_FREE(qemu_driver->stateDir);
>      VIR_FREE(qemu_driver->libDir);
>      VIR_FREE(qemu_driver->cacheDir);
> +    VIR_FREE(qemu_driver->saveDir);
>      VIR_FREE(qemu_driver->vncTLSx509certdir);
>      VIR_FREE(qemu_driver->vncListen);
>      VIR_FREE(qemu_driver->vncPassword);
> @@ -4570,8 +4588,8 @@ endjob:
>  }
>  
>  
> -static int qemudDomainSave(virDomainPtr dom,
> -                           const char *path)
> +static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
> +                               int compressed)
>  {
>      struct qemud_driver *driver = dom->conn->privateData;
>      virDomainObjPtr vm = NULL;
> @@ -4589,18 +4607,8 @@ static int qemudDomainSave(virDomainPtr dom,
>      header.version = QEMUD_SAVE_VERSION;
>  
>      qemuDriverLock(driver);
> -    if (driver->saveImageFormat == NULL)
> -        header.compressed = QEMUD_SAVE_FORMAT_RAW;
> -    else {
> -        header.compressed =
> -            qemudSaveCompressionTypeFromString(driver->saveImageFormat);
> -        if (header.compressed < 0) {
> -            qemuReportError(VIR_ERR_OPERATION_FAILED,
> -                            "%s", _("Invalid save image format specified "
> -                                    "in configuration file"));
> -            goto cleanup;
> -        }
> -    }
> +
> +    header.compressed = compressed;
>  
>      vm = virDomainFindByUUID(&driver->domains, dom->uuid);
>  
> @@ -4829,6 +4837,165 @@ cleanup:
>      return ret;
>  }
>  
> +static int qemudDomainSave(virDomainPtr dom, const char *path)
> +{
> +    struct qemud_driver *driver = dom->conn->privateData;
> +    int compressed;
> +
> +    /* Hm, is this safe against qemudReload? */
> +    if (driver->saveImageFormat == NULL)
> +        compressed = QEMUD_SAVE_FORMAT_RAW;
> +    else {
> +        compressed = qemudSaveCompressionTypeFromString(driver->saveImageFormat);
> +        if (compressed < 0) {
> +            qemuReportError(VIR_ERR_OPERATION_FAILED,
> +                            "%s", _("Invalid save image format specified "
> +                                    "in configuration file"));
> +            return -1;
> +        }
> +    }
> +
> +    return qemudDomainSaveFlag(dom, path, compressed);
> +}
> +
> +static char *
> +qemuDomainManagedSavePath(struct qemud_driver *driver, virDomainObjPtr vm) {
> +    char *ret;
> +
> +    if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, vm->def->name) < 0) {
> +        virReportOOMError();
> +        return(NULL);
> +    }
> +
> +    return(ret);
> +}
> +
> +static int
> +qemuDomainManagedSave(virDomainPtr dom,
> +                      unsigned int flags ATTRIBUTE_UNUSED)
> +{
> +    struct qemud_driver *driver = dom->conn->privateData;
> +    virDomainObjPtr vm = NULL;
> +    char *name = NULL;
> +    int ret = -1;
> +    int compressed;
> +
> +    qemuDriverLock(driver);
> +    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +    if (!vm) {
> +        char uuidstr[VIR_UUID_STRING_BUFLEN];
> +        virUUIDFormat(dom->uuid, uuidstr);
> +        qemuReportError(VIR_ERR_NO_DOMAIN,
> +                        _("no domain with matching uuid '%s'"), uuidstr);
> +        goto error;
> +    }
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        qemuReportError(VIR_ERR_OPERATION_INVALID,
> +                        "%s", _("domain is not running"));
> +        goto error;
> +    }
> +
> +    name = qemuDomainManagedSavePath(driver, vm);
> +    if (name == NULL)
> +        goto error;
> +
> +    VIR_DEBUG("Saving state to %s", name);
> +
> +    /* FIXME: we should take the flags parameter, and use bits out
> +     * of there to control whether we are compressing or not
> +     */
> +    compressed = QEMUD_SAVE_FORMAT_RAW;
> +
> +    /* we have to drop our locks here because qemudDomainSaveFlag is
> +     * going to pick them back up.  Unfortunately it opens a race window
> +     * between us dropping and qemudDomainSaveFlag picking it back up, but
> +     * if we want to allow other operations to be able to happen while
> +     * qemuDomainSaveFlag is running (possibly for a long time), I'm not
> +     * sure if there is a better solution
> +     */
> +    virDomainObjUnlock(vm);
> +    qemuDriverUnlock(driver);
> +
> +    ret = qemudDomainSaveFlag(dom, name, compressed);

What you need todo here is to


 - Instead of passing virDomainPtr into qemudDomainSaveFlag,
   pass the 'driver' and 'vm' objects.

 - Move all the locking from qemudDomainSaveFlag into qemudDomainSavePath


So both qemudDomainSavePath() and this qemuDomainManagedSave() can
then be passing a pre-locked object into the qemudDomainSaveFlag() 
method.


> @@ -6000,6 +6168,32 @@ static int qemudDomainStart(virDomainPtr dom) {
>          goto endjob;
>      }
>  
> +    /*
> +     * If there is a managed saved state restore it instead of starting
> +     * from scratch. In any case the old state is removed.
> +     */
> +    managed_save = qemuDomainManagedSavePath(driver, vm);
> +    if ((managed_save) && (virFileExists(managed_save))) {
> +        /* We should still have a reference left to vm but */
> +        if (qemuDomainObjEndJob(vm) == 0)
> +            vm = NULL;
> +        virDomainObjUnlock(vm);
> +        qemuDriverUnlock(driver);
> +        ret = qemudDomainRestore(dom->conn, managed_save);
> +
> +        if (unlink(managed_save) < 0) {
> +            VIR_WARN("Failed to remove the managed state %s", managed_save);
> +        }
> +
> +        if (ret == 0) {
> +            /* qemudDomainRestore should have sent the Started/Restore event */
> +            VIR_FREE(managed_save);
> +            return(ret);
> +        }
> +        qemuDriverLock(driver);
> +        virDomainObjLock(vm);
> +    }

Here as well, I think we need to split qemudDomainRestore() into two
methods. qemudDomainRestoreInternal() which takes a driver + vm object
which have been pre-locked. This qemudDomainStart() would call the
qemudDomainRestoreInternal() method directly. And likewise qemudDomainRestore
would then also call qemudDomainRestoreInternal.

The general rule with the locking is that driver API entry points should
not call other driver API entry points. The common functionality should
be pulled out into a separate method  which is passed pre-locked objects
of type virDomainObjPtr instead of virDomainPtr.


Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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