On 04/02/2010 01:56 PM, Daniel Veillard wrote:
> +++ 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;
Would it be more efficient to write a followup patch that does:
if ((qemu_driver->savedir
= strdup(LOCAL_STATE_DIR "/lib/libvirt/qemu/save/") == NULL)
goto out_of_memory;
on the grounds that compile-time concatenation and strdup are much
lighter-weight than run-time concatenation of virAsprintf?
But that doesn't affect the quality of your patch.
> } 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;
And constructs like this still need virAsprintf.
> -static int qemudDomainSave(virDomainPtr dom,
> - const char *path)
> +static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
> + int compressed)
Should that be bool compressed, since we are only using it as a binary?
Or are there plans to extend it in the future to allow choice between
multiple acceptable compression algorithms, in which case it is better
as an unsigned int?
> +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);
If you convert the type of the last argument of qemudDomanSaveFlag, then
here is where you'd convert from int (qemudSaveCompressionTypeFromString
must remain int, to detect failure) to bool.
> +static int
> +qemuDomainManagedSave(virDomainPtr dom,
> + unsigned int flags ATTRIBUTE_UNUSED)
No - for future compability, we NEED to check that flags==0 or fail now.
Otherwise, a future version, where flags has meaning, will mistakenly
think that our older version can honor those flags.
> +{
> + 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
> + */
Is there a way to tell qemudDomainSaveFlag that we called it while the
lock was already held (that is, consume one of the bits of the flag
argument that we pass to qemudDomainSaveFlag for internal use)? That
way, we don't have to drop the lock here, but let qemudDomainSaveFlag
drop it on our behalf.
> + virDomainObjUnlock(vm);
> + qemuDriverUnlock(driver);
> +
> + ret = qemudDomainSaveFlag(dom, name, compressed);
> +
> +cleanup:
> + VIR_FREE(name);
> +
> + return ret;
> +
> +error:
> + if (vm)
> + virDomainObjUnlock(vm);
> + qemuDriverUnlock(driver);
> + goto cleanup;
> +}
> +
> +static int
> +qemuDomainHasManagedSaveImage(virDomainPtr dom,
> + unsigned int flags ATTRIBUTE_UNUSED)
Again, we MUST ensure flags==0 now, to allow future compatibility.
> +{
> + struct qemud_driver *driver = dom->conn->privateData;
> + virDomainObjPtr vm = NULL;
> + int ret = -1;
> + char *name = NULL;
> +
> + 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 cleanup;
> + }
> +
> + name = qemuDomainManagedSavePath(driver, vm);
> + if (name == NULL)
> + goto cleanup;
> +
> + ret = virFileExists(name);
> +
> +cleanup:
> + VIR_FREE(name);
> + if (vm)
> + virDomainObjUnlock(vm);
> + qemuDriverUnlock(driver);
> + return ret;
> +}
> +
> +static int
> +qemuDomainManagedSaveRemove(virDomainPtr dom,
> + unsigned int flags ATTRIBUTE_UNUSED)
And again.
> +{
> + struct qemud_driver *driver = dom->conn->privateData;
> + virDomainObjPtr vm = NULL;
> + int ret = -1;
> + char *name = NULL;
> +
> + 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 cleanup;
> + }
> +
> + name = qemuDomainManagedSavePath(driver, vm);
> + if (name == NULL)
> + goto cleanup;
> +
> + ret = unlink(name);
> +
> +cleanup:
> + VIR_FREE(name);
> + if (vm)
> + virDomainObjUnlock(vm);
> + qemuDriverUnlock(driver);
> + return ret;
> +}
>
> static int qemudDomainCoreDump(virDomainPtr dom,
> const char *path,
> @@ -5979,6 +6146,7 @@ static int qemudDomainStart(virDomainPtr dom) {
> virDomainObjPtr vm;
> int ret = -1;
> virDomainEventPtr event = NULL;
> + char *managed_save = NULL;
>
> qemuDriverLock(driver);
> vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> @@ -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 */
Incomplete comment?
> + 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);
> + }
> +
Overall, it looks like it does what you claim, but I think there's still
some work needed before an ACK.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list