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

Re: [libvirt] [PATCHv3 2/3] save: let qemu driver manipulate save files



On 07/22/2011 12:21 AM, Eric Blake wrote:
The goal here is that save-image-dumpxml fed back to save
image-define without changing the save file; anywhere that
this is not the case is probably a bug in domain_conf.c.

* src/qemu/qemu_driver.c (qemuDomainSaveImageGetXMLDesc)
(qemuDomainSaveImageDefineXML): New functions.
(qemuDomainSaveImageOpen): Add parameter.
(qemuDomainRestoreFlags, qemuDomainObjRestore): Adjust clients.
---

v3: short cut exit with special value if no edits need to be made

  src/qemu/qemu_driver.c |  120 ++++++++++++++++++++++++++++++++++++++++++++---
  1 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2b1df6c..f1a4e8a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3706,29 +3706,32 @@ cleanup:
      return ret;
  }

-static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6)
+/* Return -1 on failure, -2 if edit was specified but xmlin does not
+ * represent any changes, and opened fd on all other success.  */
+static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
  qemuDomainSaveImageOpen(struct qemud_driver *driver,
                          const char *path,
                          virDomainDefPtr *ret_def,
                          struct qemud_save_header *ret_header,
                          bool bypass_cache, virFileDirectFdPtr *directFd,
-                        const char *xmlin)
+                        const char *xmlin, bool edit)
  {
      int fd;
      struct qemud_save_header header;
      char *xml = NULL;
      virDomainDefPtr def = NULL;
-    int directFlag = 0;
+    int oflags = edit ? O_RDWR : O_RDONLY;

      if (bypass_cache) {
-        directFlag = virFileDirectFdFlag();
+        int directFlag = virFileDirectFdFlag();
          if (directFlag<  0) {
              qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
                              _("bypass cache unsupported by this system"));
              goto error;
          }
+        oflags |= directFlag;
      }
-    if ((fd = virFileOpenAs(path, O_RDONLY | directFlag, 0,
+    if ((fd = virFileOpenAs(path, oflags, 0,
                              getuid(), getgid(), 0))<  0) {
          if ((fd != -EACCES&&  fd != -EPERM) ||
              driver->user == getuid()) {
@@ -3739,7 +3742,7 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver,

          /* Opening as root failed, but qemu runs as a different user
           * that might have better luck. */
-        if ((fd = virFileOpenAs(path, O_RDONLY | directFlag, 0,
+        if ((fd = virFileOpenAs(path, oflags, 0,
                                  driver->user, driver->group,
                                  VIR_FILE_OPEN_AS_UID))<  0) {
              qemuReportError(VIR_ERR_OPERATION_FAILED,
@@ -3791,6 +3794,15 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver,
          goto error;
      }

+    if (edit&&  STREQ(xml, xmlin)) {
+        VIR_FREE(xml);
+        if (VIR_CLOSE(fd)<  0) {
+            virReportSystemError(errno, _("cannot close file: %s"), path);
+            goto error;
+        }
+        return -2;
+    }


So "unchanged" is indicated with a < 0 return code, but that only happens when edit == true, and only one of the callers does that (and properly handles the special case).


+
      /* Create a domain from this XML */
      if (!(def = virDomainDefParseString(driver->caps, xml,
                                          QEMU_EXPECTED_VIRT_TYPES,
@@ -3949,7 +3961,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,

      fd = qemuDomainSaveImageOpen(driver, path,&def,&header,
                                   (flags&  VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0,
-&directFd, dxml);
+&directFd, dxml, false);
      if (fd<  0)
          goto cleanup;

@@ -3995,6 +4007,96 @@ qemuDomainRestore(virConnectPtr conn,
      return qemuDomainRestoreFlags(conn, path, NULL, 0);
  }

+static char *
+qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path,
+                              unsigned int flags)
+{
+    struct qemud_driver *driver = conn->privateData;
+    char *ret = NULL;
+    virDomainDefPtr def = NULL;
+    int fd = -1;
+    struct qemud_save_header header;
+
+    /* We only take subset of virDomainDefFormat flags.  */
+    virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL);
+
+    qemuDriverLock(driver);
+
+    fd = qemuDomainSaveImageOpen(driver, path,&def,&header, false, NULL,
+                                 NULL, false);
+
+    if (fd<  0)
+        goto cleanup;
+
+    ret = qemuDomainDefFormatXML(driver, def, flags);
+
+cleanup:
+    virDomainDefFree(def);
+    VIR_FORCE_CLOSE(fd);
+    qemuDriverUnlock(driver);
+    return ret;
+}
+
+static int
+qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path,
+                             const char *dxml, unsigned int flags)
+{
+    struct qemud_driver *driver = conn->privateData;
+    int ret = -1;
+    virDomainDefPtr def = NULL;
+    int fd = -1;
+    struct qemud_save_header header;
+    char *xml = NULL;
+    size_t len;
+
+    virCheckFlags(0, -1);
+
+    qemuDriverLock(driver);
+
+    fd = qemuDomainSaveImageOpen(driver, path,&def,&header, false, NULL,
+                                 dxml, true);
+
+    if (fd<  0) {
+        /* Check for special case of no change needed.  */
+        if (fd == -2)
+            ret = 0;


i.e. right here.


+        goto cleanup;
+    }
+
+    xml = qemuDomainDefFormatXML(driver, def, VIR_DOMAIN_XML_SECURE);
+    if (!xml)
+        goto cleanup;
+    len = strlen(xml) + 1;
+
+    if (len>  header.xml_len) {
+        qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                        _("new xml too large to fit in file"));
+    }

Did you forget a goto cleanup there?

+    if (VIR_EXPAND_N(xml, len, header.xml_len - len)<  0) {

because if len > header.xml_len, that's going to be a *very large* number :-)

+        virReportOOMError();
+        goto cleanup;
+    }

+
+    if (lseek(fd, sizeof(header), SEEK_SET) != sizeof(header)) {
+        virReportSystemError(errno, _("cannot seek in '%s'"), path);
+        goto cleanup;
+    }
+    if (safewrite(fd, xml, len) != len ||
+        VIR_CLOSE(fd)<  0) {
+        virReportSystemError(errno, _("failed to write xml to '%s'"), path);
+        goto cleanup;
+    }
+
+    ret = 0;
+
+cleanup:
+    virDomainDefFree(def);
+    VIR_FORCE_CLOSE(fd);
+    VIR_FREE(xml);
+    qemuDriverUnlock(driver);
+    return ret;
+}
+
  static int
  qemuDomainObjRestore(virConnectPtr conn,
                       struct qemud_driver *driver,
@@ -4009,7 +4111,7 @@ qemuDomainObjRestore(virConnectPtr conn,
      virFileDirectFdPtr directFd = NULL;

      fd = qemuDomainSaveImageOpen(driver, path,&def,&header,
-                                 bypass_cache,&directFd, NULL);
+                                 bypass_cache,&directFd, NULL, false);
      if (fd<  0)
          goto cleanup;

@@ -9070,6 +9172,8 @@ static virDriver qemuDriver = {
      .domainSaveFlags = qemuDomainSaveFlags, /* 0.9.4 */
      .domainRestore = qemuDomainRestore, /* 0.2.0 */
      .domainRestoreFlags = qemuDomainRestoreFlags, /* 0.9.4 */
+    .domainSaveImageGetXMLDesc = qemuDomainSaveImageGetXMLDesc, /* 0.9.4 */
+    .domainSaveImageDefineXML = qemuDomainSaveImageDefineXML, /* 0.9.4 */
      .domainCoreDump = qemudDomainCoreDump, /* 0.7.0 */
      .domainScreenshot = qemuDomainScreenshot, /* 0.9.2 */
      .domainSetVcpus = qemuDomainSetVcpus, /* 0.4.4 */

ACK with the goto cleanup added in when the new length is too big to fit (unless I've misunderstood the code).


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