On Fri, Apr 02, 2010 at 02:26:10PM -0600, Eric Blake wrote:
> 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.
there is a block of 4 allocations that way, I think they should be
kept similar
> > } 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.
same thing, kept code similar to previous blocks.
We could optimize, yes, but it's run once in libvirtd lifetime :-)
> > -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?
we can have multiple compression options, so an int, yes
> > +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.
Hum, we used to not check undefined flags, we are changing that now,
it makes sense. But no capital/yelling please !
> > + /* 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.
Not in the current code as I understand it. This would need some
refactoring.
> > +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?
not really, that could be "but ..." or "but one should check for 0
anyway"
I end up with the following additional patch,
thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5a678c9..d5ee16a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4871,8 +4871,7 @@ qemuDomainManagedSavePath(struct qemud_driver *driver, virDomainObjPtr vm) {
}
static int
-qemuDomainManagedSave(virDomainPtr dom,
- unsigned int flags ATTRIBUTE_UNUSED)
+qemuDomainManagedSave(virDomainPtr dom, unsigned int flags)
{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm = NULL;
@@ -4880,6 +4879,13 @@ qemuDomainManagedSave(virDomainPtr dom,
int ret = -1;
int compressed;
+ if (flags != 0) {
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("unsupported flags (0x%x) passed to '%s'"),
+ flags, __FUNCTION__);
+ return -1;
+ }
+
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (!vm) {
@@ -4932,14 +4938,20 @@ error:
}
static int
-qemuDomainHasManagedSaveImage(virDomainPtr dom,
- unsigned int flags ATTRIBUTE_UNUSED)
+qemuDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags)
{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm = NULL;
int ret = -1;
char *name = NULL;
+ if (flags != 0) {
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("unsupported flags (0x%x) passed to '%s'"),
+ flags, __FUNCTION__);
+ return -1;
+ }
+
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (!vm) {
@@ -4965,14 +4977,20 @@ cleanup:
}
static int
-qemuDomainManagedSaveRemove(virDomainPtr dom,
- unsigned int flags ATTRIBUTE_UNUSED)
+qemuDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags)
{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm = NULL;
int ret = -1;
char *name = NULL;
+ if (flags != 0) {
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("unsupported flags (0x%x) passed to '%s'"),
+ flags, __FUNCTION__);
+ return -1;
+ }
+
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (!vm) {
@@ -6174,7 +6192,10 @@ static int qemudDomainStart(virDomainPtr dom) {
*/
managed_save = qemuDomainManagedSavePath(driver, vm);
if ((managed_save) && (virFileExists(managed_save))) {
- /* We should still have a reference left to vm but */
+ /*
+ * We should still have a reference left to vm but
+ * but one should check for 0 anyway
+ */
if (qemuDomainObjEndJob(vm) == 0)
vm = NULL;
virDomainObjUnlock(vm);
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list