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

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.

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

> +static int > +qemuDomainHasManagedSaveImage(virDomainPtr dom, > + unsigned int flags ATTRIBUTE_UNUSED) Again, we MUST ensure flags==0 now, to allow future compatibility.

> +static int > +qemuDomainManagedSaveRemove(virDomainPtr dom, > + unsigned int flags ATTRIBUTE_UNUSED) And again.

> + managed_save = qemuDomainManagedSavePath(driver, vm); > + if ((managed_save) && (virFileExists(managed_save))) { > + /* We should still have a reference left to vm but */ Incomplete comment?