[PATCH 09/10] qemu: snapshot: align disks consistently

[Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>]

There is inconsitency in how we set disk snapshot attribute for missing disk
and for disk explicitly present in snapshot definition in virDomainSnapshotAlignDisks.

First for explicit disk we do not check if disk source is present. After the
previous patch this will cause snapshot failures so we'd better disable
snapshotting of such disk at this place. (For the record: we could have
failures before previous patch too, it just does not makes sense to snapshot
disk without source).

Second for missing disks with empty source disabling snapshot take precedence
over user settings. This does not feel right. It is better report to user that
option he wanted in not supported/possible rather then ignoring it. This
can break things a bit. Hopefully nobody really uses domain disk snapshot
setting.

Next let's remove setting disk snapshot on parse stage and move it altogether
to one place - disk align function. Other hypervisors does not check this
attribute in domain disk config so we are free here.

Now we can place logic for setting disk snapshot value in one function
- virDomainSnapshotSetDiskSnapshot.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
---
 src/conf/domain_conf.c   |  6 +-----
 src/conf/snapshot_conf.c | 38 +++++++++++++++++++++++---------------
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d8dfd16..0fbd739 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9759,8 +9759,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
                            snapshot);
             goto error;
         }
-    } else if (def->src->readonly) {
-        def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
     }
 
     if (rawio) {
@@ -24285,9 +24283,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
     if (def->sgio)
         virBufferAsprintf(buf, " sgio='%s'", sgio);
 
-    if (def->snapshot &&
-        !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE &&
-          def->src->readonly))
+    if (def->snapshot)
         virBufferAsprintf(buf, " snapshot='%s'",
                           virDomainSnapshotLocationTypeToString(def->snapshot));
     virBufferAddLit(buf, ">\n");
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 002cd45..172dff8 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -521,6 +521,25 @@ virDomainSnapshotCompareDiskIndex(const void *a, const void *b)
     return diska->idx - diskb->idx;
 }
 
+
+static void
+virDomainSnapshotSetDiskSnapshot(virDomainSnapshotDiskDefPtr disk,
+                                 virDomainDiskDefPtr dom_disk,
+                                 int default_snapshot)
+{
+    if (disk->snapshot)
+        return;
+
+    if (dom_disk->snapshot)
+        disk->snapshot = dom_disk->snapshot;
+    else if (dom_disk->src->readonly ||
+             virStorageSourceIsEmpty(dom_disk->src))
+        disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
+    else
+        disk->snapshot = default_snapshot;
+}
+
+
 /* Align def->disks to def->domain.  Sort the list of def->disks,
  * filling in any missing disks or snapshot state defaults given by
  * the domain, with a fallback to a passed in default.  Convert paths
@@ -562,7 +581,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
         virDomainSnapshotDiskDefPtr disk = &def->disks[i];
         virDomainDiskDefPtr dom_disk;
         int idx = virDomainDiskIndexByName(def->dom, disk->name, false);
-        int disk_snapshot;
 
         if (idx < 0) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -580,13 +598,8 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
         disk->idx = idx;
         dom_disk = def->dom->disks[idx];
 
-        disk_snapshot = dom_disk->snapshot;
-        if (!disk->snapshot) {
-            if (disk_snapshot)
-                disk->snapshot = disk_snapshot;
-            else
-                disk->snapshot = default_snapshot;
-        }
+        virDomainSnapshotSetDiskSnapshot(disk, dom_disk, default_snapshot);
+
         if (STRNEQ(disk->name, dom_disk->dst)) {
             VIR_FREE(disk->name);
             if (VIR_STRDUP(disk->name, dom_disk->dst) < 0)
@@ -612,15 +625,10 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
             goto cleanup;
         disk->idx = i;
 
-        /* Don't snapshot empty drives */
-        if (virStorageSourceIsEmpty(def->dom->disks[i]->src))
-            disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
-        else
-            disk->snapshot = def->dom->disks[i]->snapshot;
+        virDomainSnapshotSetDiskSnapshot(disk, def->dom->disks[i],
+                                         default_snapshot);
 
         disk->src->type = VIR_STORAGE_TYPE_FILE;
-        if (!disk->snapshot)
-            disk->snapshot = default_snapshot;
     }
 
     qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]),
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list