snapshot: prepare to remove transient snapshot metadata

This patch is mostly code motion - moving some functions out
of qemu_driver and into qemu_domain so they can be reused by
multiple qemu_* files (since qemu_driver.h must not grow).
It also adds a new helper function, qemuDomainRemoveInactive,
which will be used in the next patch.

* src/qemu/qemu_domain.h (qemuFindQemuImgBinary)
(qemuDomainSnapshotWriteMetadata, qemuDomainSnapshotForEachQcow2)
(qemuDomainSnapshotDiscard, qemuDomainSnapshotDiscardAll)
(qemuDomainRemoveInactive): New prototypes.
(struct qemu_snap_remove): New struct.
* src/qemu/qemu_domain.c (qemuDomainRemoveInactive)
(qemuDomainSnapshotDiscardAllMetadata): New functions.
(qemuFindQemuImgBinary, qemuDomainSnapshotWriteMetadata)
(qemuDomainSnapshotForEachQcow2, qemuDomainSnapshotDiscard)
(qemuDomainSnapshotDiscardAll): Move here...
* src/qemu/qemu_driver.c (qemuFindQemuImgBinary)
(qemuDomainSnapshotWriteMetadata, qemuDomainSnapshotForEachQcow2)
(qemuDomainSnapshotDiscard, qemuDomainSnapshotDiscardAll): ...from
here.
(qemuDomainUndefineFlags): Update caller.
* src/conf/domain_conf.c (virDomainRemoveInactive): Doc fixes.

src/conf/domain_conf.c

@@ -1438,9 +1438,9 @@ virDomainObjGetPersistentDef(virCapsPtr caps,
 }
 
 /*
- * The caller must hold a lock  on the driver owning 'doms',
+ * The caller must hold a lock on the driver owning 'doms',
  * and must also have locked 'dom', to ensure no one else
- * is either waiting for 'dom' or still usingn it
+ * is either waiting for 'dom' or still using it
  */
 void virDomainRemoveInactive(virDomainObjListPtr doms,
                              virDomainObjPtr dom)

src/qemu/qemu_domain.c

@@ -1296,3 +1296,263 @@ cleanup:
     VIR_FREE(message);
     return ret;
 }
+
+/* Locate an appropriate 'qemu-img' binary.  */
+const char *
+qemuFindQemuImgBinary(struct qemud_driver *driver)
+{
+    if (!driver->qemuImgBinary) {
+        driver->qemuImgBinary = virFindFileInPath("kvm-img");
+        if (!driver->qemuImgBinary)
+            driver->qemuImgBinary = virFindFileInPath("qemu-img");
+        if (!driver->qemuImgBinary)
+            qemuReportError(VIR_ERR_INTERNAL_ERROR,
+                            "%s", _("unable to find kvm-img or qemu-img"));
+    }
+
+    return driver->qemuImgBinary;
+}
+
+int
+qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
+                                virDomainSnapshotObjPtr snapshot,
+                                char *snapshotDir)
+{
+    int fd = -1;
+    char *newxml = NULL;
+    int ret = -1;
+    char *snapDir = NULL;
+    char *snapFile = NULL;
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
+    char *tmp;
+
+    virUUIDFormat(vm->def->uuid, uuidstr);
+    newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def,
+                                        VIR_DOMAIN_XML_SECURE, 1);
+    if (newxml == NULL) {
+        virReportOOMError();
+        return -1;
+    }
+
+    if (virAsprintf(&snapDir, "%s/%s", snapshotDir, vm->def->name) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+    if (virFileMakePath(snapDir) < 0) {
+        virReportSystemError(errno, _("cannot create snapshot directory '%s'"),
+                             snapDir);
+        goto cleanup;
+    }
+
+    if (virAsprintf(&snapFile, "%s/%s.xml", snapDir, snapshot->def->name) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+    fd = open(snapFile, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR);
+    if (fd < 0) {
+        qemuReportError(VIR_ERR_OPERATION_FAILED,
+                        _("failed to create snapshot file '%s'"), snapFile);
+        goto cleanup;
+    }
+
+    if (virAsprintf(&tmp, "snapshot-edit %s", vm->def->name) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+    virEmitXMLWarning(fd, snapshot->def->name, tmp);
+    VIR_FREE(tmp);
+
+    if (safewrite(fd, newxml, strlen(newxml)) != strlen(newxml)) {
+        virReportSystemError(errno, _("Failed to write snapshot data to %s"),
+                             snapFile);
+        goto cleanup;
+    }
+
+    ret = 0;
+
+cleanup:
+    VIR_FREE(snapFile);
+    VIR_FREE(snapDir);
+    VIR_FREE(newxml);
+    VIR_FORCE_CLOSE(fd);
+    return ret;
+}
+
+/* The domain is expected to be locked and inactive. Return -1 on normal
+ * failure, 1 if we skipped a disk due to try_all.  */
+int
+qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
+                               virDomainObjPtr vm,
+                               virDomainSnapshotObjPtr snap,
+                               const char *op,
+                               bool try_all)
+{
+    const char *qemuimgarg[] = { NULL, "snapshot", NULL, NULL, NULL, NULL };
+    int i;
+    bool skipped = false;
+
+    qemuimgarg[0] = qemuFindQemuImgBinary(driver);
+    if (qemuimgarg[0] == NULL) {
+        /* qemuFindQemuImgBinary set the error */
+        return -1;
+    }
+
+    qemuimgarg[2] = op;
+    qemuimgarg[3] = snap->def->name;
+
+    for (i = 0; i < vm->def->ndisks; i++) {
+        /* FIXME: we also need to handle LVM here */
+        /* FIXME: if we fail halfway through this loop, we are in an
+         * inconsistent state.  I'm not quite sure what to do about that
+         */
+        if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+            if (!vm->def->disks[i]->driverType ||
+                STRNEQ(vm->def->disks[i]->driverType, "qcow2")) {
+                if (try_all) {
+                    /* Continue on even in the face of error, since other
+                     * disks in this VM may have the same snapshot name.
+                     */
+                    VIR_WARN("skipping snapshot action on %s",
+                             vm->def->disks[i]->dst);
+                    skipped = true;
+                    continue;
+                }
+                qemuReportError(VIR_ERR_OPERATION_INVALID,
+                                _("Disk device '%s' does not support"
+                                  " snapshotting"),
+                                vm->def->disks[i]->dst);
+                return -1;
+            }
+
+            qemuimgarg[4] = vm->def->disks[i]->src;
+
+            if (virRun(qemuimgarg, NULL) < 0) {
+                if (try_all) {
+                    VIR_WARN("skipping snapshot action on %s",
+                             vm->def->disks[i]->info.alias);
+                    skipped = true;
+                    continue;
+                }
+                return -1;
+            }
+        }
+    }
+
+    return skipped ? 1 : 0;
+}
+
+/* Discard one snapshot (or its metadata), without reparenting any children.  */
+int
+qemuDomainSnapshotDiscard(struct qemud_driver *driver,
+                          virDomainObjPtr vm,
+                          virDomainSnapshotObjPtr snap,
+                          bool update_current,
+                          bool metadata_only)
+{
+    char *snapFile = NULL;
+    int ret = -1;
+    qemuDomainObjPrivatePtr priv;
+    virDomainSnapshotObjPtr parentsnap = NULL;
+
+    if (!metadata_only) {
+        if (!virDomainObjIsActive(vm)) {
+            /* Ignore any skipped disks */
+            if (qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-d",
+                                               true) < 0)
+                goto cleanup;
+        } else {
+            priv = vm->privateData;
+            qemuDomainObjEnterMonitorWithDriver(driver, vm);
+            /* we continue on even in the face of error */
+            qemuMonitorDeleteSnapshot(priv->mon, snap->def->name);
+            qemuDomainObjExitMonitorWithDriver(driver, vm);
+        }
+    }
+
+    if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir,
+                    vm->def->name, snap->def->name) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    if (snap == vm->current_snapshot) {
+        if (update_current && snap->def->parent) {
+            parentsnap = virDomainSnapshotFindByName(&vm->snapshots,
+                                                     snap->def->parent);
+            if (!parentsnap) {
+                VIR_WARN("missing parent snapshot matching name '%s'",
+                         snap->def->parent);
+            } else {
+                parentsnap->def->current = true;
+                if (qemuDomainSnapshotWriteMetadata(vm, parentsnap,
+                                                    driver->snapshotDir) < 0) {
+                    VIR_WARN("failed to set parent snapshot '%s' as current",
+                             snap->def->parent);
+                    parentsnap->def->current = false;
+                    parentsnap = NULL;
+                }
+            }
+        }
+        vm->current_snapshot = parentsnap;
+    }
+
+    if (unlink(snapFile) < 0)
+        VIR_WARN("Failed to unlink %s", snapFile);
+    virDomainSnapshotObjListRemove(&vm->snapshots, snap);
+
+    ret = 0;
+
+cleanup:
+    VIR_FREE(snapFile);
+
+    return ret;
+}
+
+/* Hash iterator callback to discard multiple snapshots.  */
+void qemuDomainSnapshotDiscardAll(void *payload,
+                                  const void *name ATTRIBUTE_UNUSED,
+                                  void *data)
+{
+    virDomainSnapshotObjPtr snap = payload;
+    struct qemu_snap_remove *curr = data;
+    int err;
+
+    if (snap->def->current)
+        curr->current = true;
+    err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, false,
+                                    curr->metadata_only);
+    if (err && !curr->err)
+        curr->err = err;
+}
+
+int
+qemuDomainSnapshotDiscardAllMetadata(struct qemud_driver *driver,
+                                     virDomainObjPtr vm)
+{
+    struct qemu_snap_remove rem;
+
+    rem.driver = driver;
+    rem.vm = vm;
+    rem.metadata_only = true;
+    rem.err = 0;
+    virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardAll, &rem);
+
+    /* XXX also do rmdir ? */
+    return rem.err;
+}
+
+/*
+ * The caller must hold a lock on both driver and vm, and there must
+ * be no remaining references to vm.
+ */
+void
+qemuDomainRemoveInactive(struct qemud_driver *driver,
+                         virDomainObjPtr vm)
+{
+    /* Remove any snapshot metadata prior to removing the domain */
+    if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0) {
+        VIR_WARN("unable to remove all snapshots for domain %s",
+                 vm->def->name);
+    }
+    virDomainRemoveInactive(&driver->domains, vm);
+}

src/qemu/qemu_domain.h

@@ -237,4 +237,40 @@ int qemuDomainAppendLog(struct qemud_driver *driver,
                         int logFD,
                         const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(4, 5);
 
+const char *qemuFindQemuImgBinary(struct qemud_driver *driver);
+
+int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
+                                    virDomainSnapshotObjPtr snapshot,
+                                    char *snapshotDir);
+
+int qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
+                                   virDomainObjPtr vm,
+                                   virDomainSnapshotObjPtr snap,
+                                   const char *op,
+                                   bool try_all);
+
+int qemuDomainSnapshotDiscard(struct qemud_driver *driver,
+                              virDomainObjPtr vm,
+                              virDomainSnapshotObjPtr snap,
+                              bool update_current,
+                              bool metadata_only);
+
+struct qemu_snap_remove {
+    struct qemud_driver *driver;
+    virDomainObjPtr vm;
+    int err;
+    bool metadata_only;
+    bool current;
+};
+
+void qemuDomainSnapshotDiscardAll(void *payload,
+                                  const void *name,
+                                  void *data);
+
+int qemuDomainSnapshotDiscardAllMetadata(struct qemud_driver *driver,
+                                         virDomainObjPtr vm);
+
+void qemuDomainRemoveInactive(struct qemud_driver *driver,
+                              virDomainObjPtr vm);
+
 #endif /* __QEMU_DOMAIN_H__ */

src/qemu/qemu_driver.c

@@ -1596,243 +1596,6 @@ cleanup:
 }
 
 
-/* Locate an appropriate 'qemu-img' binary.  */
-static const char *
-qemuFindQemuImgBinary(struct qemud_driver *driver)
-{
-    if (!driver->qemuImgBinary) {
-        driver->qemuImgBinary = virFindFileInPath("kvm-img");
-        if (!driver->qemuImgBinary)
-            driver->qemuImgBinary = virFindFileInPath("qemu-img");
-        if (!driver->qemuImgBinary)
-            qemuReportError(VIR_ERR_INTERNAL_ERROR,
-                            "%s", _("unable to find kvm-img or qemu-img"));
-    }
-
-    return driver->qemuImgBinary;
-}
-
-static int
-qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
-                                virDomainSnapshotObjPtr snapshot,
-                                char *snapshotDir)
-{
-    int fd = -1;
-    char *newxml = NULL;
-    int ret = -1;
-    char *snapDir = NULL;
-    char *snapFile = NULL;
-    char uuidstr[VIR_UUID_STRING_BUFLEN];
-    char *tmp;
-
-    virUUIDFormat(vm->def->uuid, uuidstr);
-    newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def,
-                                        VIR_DOMAIN_XML_SECURE, 1);
-    if (newxml == NULL) {
-        virReportOOMError();
-        return -1;
-    }
-
-    if (virAsprintf(&snapDir, "%s/%s", snapshotDir, vm->def->name) < 0) {
-        virReportOOMError();
-        goto cleanup;
-    }
-    if (virFileMakePath(snapDir) < 0) {
-        virReportSystemError(errno, _("cannot create snapshot directory '%s'"),
-                             snapDir);
-        goto cleanup;
-    }
-
-    if (virAsprintf(&snapFile, "%s/%s.xml", snapDir, snapshot->def->name) < 0) {
-        virReportOOMError();
-        goto cleanup;
-    }
-    fd = open(snapFile, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR);
-    if (fd < 0) {
-        qemuReportError(VIR_ERR_OPERATION_FAILED,
-                        _("failed to create snapshot file '%s'"), snapFile);
-        goto cleanup;
-    }
-
-    if (virAsprintf(&tmp, "snapshot-edit %s", vm->def->name) < 0) {
-        virReportOOMError();
-        goto cleanup;
-    }
-    virEmitXMLWarning(fd, snapshot->def->name, tmp);
-    VIR_FREE(tmp);
-
-    if (safewrite(fd, newxml, strlen(newxml)) != strlen(newxml)) {
-        virReportSystemError(errno, _("Failed to write snapshot data to %s"),
-                             snapFile);
-        goto cleanup;
-    }
-
-    ret = 0;
-
-cleanup:
-    VIR_FREE(snapFile);
-    VIR_FREE(snapDir);
-    VIR_FREE(newxml);
-    VIR_FORCE_CLOSE(fd);
-    return ret;
-}
-
-/* The domain is expected to be locked and inactive. Return -1 on normal
- * failure, 1 if we skipped a disk due to try_all.  */
-static int
-qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
-                               virDomainObjPtr vm,
-                               virDomainSnapshotObjPtr snap,
-                               const char *op,
-                               bool try_all)
-{
-    const char *qemuimgarg[] = { NULL, "snapshot", NULL, NULL, NULL, NULL };
-    int i;
-    bool skipped = false;
-
-    qemuimgarg[0] = qemuFindQemuImgBinary(driver);
-    if (qemuimgarg[0] == NULL) {
-        /* qemuFindQemuImgBinary set the error */
-        return -1;
-    }
-
-    qemuimgarg[2] = op;
-    qemuimgarg[3] = snap->def->name;
-
-    for (i = 0; i < vm->def->ndisks; i++) {
-        /* FIXME: we also need to handle LVM here */
-        /* FIXME: if we fail halfway through this loop, we are in an
-         * inconsistent state.  I'm not quite sure what to do about that
-         */
-        if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
-            if (!vm->def->disks[i]->driverType ||
-                STRNEQ(vm->def->disks[i]->driverType, "qcow2")) {
-                if (try_all) {
-                    /* Continue on even in the face of error, since other
-                     * disks in this VM may have the same snapshot name.
-                     */
-                    VIR_WARN("skipping snapshot action on %s",
-                             vm->def->disks[i]->dst);
-                    skipped = true;
-                    continue;
-                }
-                qemuReportError(VIR_ERR_OPERATION_INVALID,
-                                _("Disk device '%s' does not support"
-                                  " snapshotting"),
-                                vm->def->disks[i]->dst);
-                return -1;
-            }
-
-            qemuimgarg[4] = vm->def->disks[i]->src;
-
-            if (virRun(qemuimgarg, NULL) < 0) {
-                if (try_all) {
-                    VIR_WARN("skipping snapshot action on %s",
-                             vm->def->disks[i]->info.alias);
-                    skipped = true;
-                    continue;
-                }
-                return -1;
-            }
-        }
-    }
-
-    return skipped ? 1 : 0;
-}
-
-/* Discard one snapshot (or its metadata), without reparenting any children.  */
-static int
-qemuDomainSnapshotDiscard(struct qemud_driver *driver,
-                          virDomainObjPtr vm,
-                          virDomainSnapshotObjPtr snap,
-                          bool update_current,
-                          bool metadata_only)
-{
-    char *snapFile = NULL;
-    int ret = -1;
-    qemuDomainObjPrivatePtr priv;
-    virDomainSnapshotObjPtr parentsnap = NULL;
-
-    if (!metadata_only) {
-        if (!virDomainObjIsActive(vm)) {
-            /* Ignore any skipped disks */
-            if (qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-d",
-                                               true) < 0)
-                goto cleanup;
-        } else {
-            priv = vm->privateData;
-            qemuDomainObjEnterMonitorWithDriver(driver, vm);
-            /* we continue on even in the face of error */
-            qemuMonitorDeleteSnapshot(priv->mon, snap->def->name);
-            qemuDomainObjExitMonitorWithDriver(driver, vm);
-        }
-    }
-
-    if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir,
-                    vm->def->name, snap->def->name) < 0) {
-        virReportOOMError();
-        goto cleanup;
-    }
-
-    if (snap == vm->current_snapshot) {
-        if (update_current && snap->def->parent) {
-            parentsnap = virDomainSnapshotFindByName(&vm->snapshots,
-                                                     snap->def->parent);
-            if (!parentsnap) {
-                VIR_WARN("missing parent snapshot matching name '%s'",
-                         snap->def->parent);
-            } else {
-                parentsnap->def->current = true;
-                if (qemuDomainSnapshotWriteMetadata(vm, parentsnap,
-                                                    driver->snapshotDir) < 0) {
-                    VIR_WARN("failed to set parent snapshot '%s' as current",
-                             snap->def->parent);
-                    parentsnap->def->current = false;
-                    parentsnap = NULL;
-                }
-            }
-        }
-        vm->current_snapshot = parentsnap;
-    }
-
-    if (unlink(snapFile) < 0)
-        VIR_WARN("Failed to unlink %s", snapFile);
-    virDomainSnapshotObjListRemove(&vm->snapshots, snap);
-
-    ret = 0;
-
-cleanup:
-    VIR_FREE(snapFile);
-
-    return ret;
-}
-
-struct snap_remove {
-    struct qemud_driver *driver;
-    virDomainObjPtr vm;
-    int err;
-    bool metadata_only;
-    bool current;
-};
-
-/* Hash iterator callback to discard multiple snapshots.  */
-static void
-qemuDomainSnapshotDiscardAll(void *payload,
-                             const void *name ATTRIBUTE_UNUSED,
-                             void *data)
-{
-    virDomainSnapshotObjPtr snap = payload;
-    struct snap_remove *curr = data;
-    int err;
-
-    if (snap->def->current)
-        curr->current = true;
-    err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, false,
-                                    curr->metadata_only);
-    if (err && !curr->err)
-        curr->err = err;
-}
-
 /* Count how many snapshots in a set have external disk snapshots.  */
 static void
 qemuDomainSnapshotCountExternal(void *payload,
@@ -5126,8 +4889,6 @@ qemuDomainUndefineFlags(virDomainPtr dom,
 
     if (!virDomainObjIsActive(vm) &&
         (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) {
-        struct snap_remove rem;
-
         if (!(flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA)) {
             qemuReportError(VIR_ERR_OPERATION_INVALID,
                             _("cannot delete inactive domain with %d "
@@ -5135,13 +4896,7 @@ qemuDomainUndefineFlags(virDomainPtr dom,
                             nsnapshots);
             goto cleanup;
         }
-
+        if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0)
-        rem.driver = driver;
-        rem.vm = vm;
-        rem.metadata_only = true;
-        rem.err = 0;
-        virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardAll, &rem);
-        if (rem.err < 0)
             goto cleanup;
     }
 
@@ -10132,7 +9887,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
     int ret = -1;
     virDomainSnapshotObjPtr snap = NULL;
     char uuidstr[VIR_UUID_STRING_BUFLEN];
-    struct snap_remove rem;
+    struct qemu_snap_remove rem;
     struct snap_reparent rep;
     bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY);
     int external = 0;

src/qemu/qemu_process.c

@@ -106,7 +106,7 @@ qemuProcessRemoveDomainStatus(struct qemud_driver *driver,
 extern struct qemud_driver *qemu_driver;
 
 /*
- * This is a callback registered with a qemuMonitorPtr  instance,
+ * This is a callback registered with a qemuMonitorPtr instance,
  * and to be invoked when the monitor console hits an end of file
  * condition, or error, thus indicating VM shutdown should be
  * performed