[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 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 veillard com  | 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);

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