virDomainSnapshotAlignDisks: refactor extension to all disks

Last step of the algorithm in virDomainSnapshotAlignDisks is to extend
the array of disks to all VM's disk and provide defaults. This was done
by extending the array, adding defaults at the end and then sorting it.
This requires the 'idx' variable and also a separate sorting function.

If we store the pointer to existing snapshot disk definitions in a hash
table and create a new array of snapshot disk definitions, we can fill
the new array directly by either copying the definition from the old
array or adding the default.

This avoids the sorting step and thus even the need to store the index
of the domain disk altogether.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index f6a827d2ff88266130035f54704aa86f976c385b..7090e3a1b0191d35cabc34ae81cc5b5435aa19ad 100644 (file)
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -627,16 +627,6 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def)
 }
 
 
-static int
-virDomainSnapshotCompareDiskIndex(const void *a, const void *b)
-{
-    const virDomainSnapshotDiskDef *diska = a;
-    const virDomainSnapshotDiskDef *diskb = b;
-
-    /* Integer overflow shouldn't be a problem here.  */
-    return diska->idx - diskb->idx;
-}
-
 /* Align def->disks to def->parent.dom.  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
@@ -650,9 +640,9 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
                             bool require_match)
 {
     virDomainDefPtr domdef = snapdef->parent.dom;
-    g_autoptr(virBitmap) map = NULL;
+    g_autoptr(virHashTable) map = virHashNew(NULL);
+    g_autofree virDomainSnapshotDiskDefPtr olddisks = NULL;
     size_t i;
-    int ndisks;
 
     if (!domdef) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -670,9 +660,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
     if (!domdef->ndisks)
         return 0;
 
-    if (!(map = virBitmapNew(domdef->ndisks)))
-        return -1;
-
     /* Double check requested disks.  */
     for (i = 0; i < snapdef->ndisks; i++) {
         virDomainSnapshotDiskDefPtr snapdisk = &snapdef->disks[i];
@@ -687,14 +674,15 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
 
         domdisk = domdef->disks[idx];
 
-        if (virBitmapIsBitSet(map, idx)) {
+        if (virHashHasEntry(map, domdisk->dst)) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("disk '%s' specified twice"),
                            snapdisk->name);
             return -1;
         }
-        ignore_value(virBitmapSetBit(map, idx));
-        snapdisk->idx = idx;
+
+        if (virHashAddEntry(map, domdisk->dst, snapdisk) < 0)
+            return -1;
 
         if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) {
             if (domdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT &&
@@ -729,21 +717,24 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
         }
     }
 
-    /* Provide defaults for all remaining disks.  */
-    ndisks = snapdef->ndisks;
-    if (VIR_EXPAND_N(snapdef->disks, snapdef->ndisks,
-                     domdef->ndisks - snapdef->ndisks) < 0)
-        return -1;
+    olddisks = g_steal_pointer(&snapdef->disks);
+    snapdef->disks = g_new0(virDomainSnapshotDiskDef, domdef->ndisks);
+    snapdef->ndisks = domdef->ndisks;
 
     for (i = 0; i < domdef->ndisks; i++) {
-        virDomainSnapshotDiskDefPtr snapdisk;
+        virDomainDiskDefPtr domdisk = domdef->disks[i];
+        virDomainSnapshotDiskDefPtr snapdisk = snapdef->disks + i;
+        virDomainSnapshotDiskDefPtr existing;
 
-        if (virBitmapIsBitSet(map, i))
+        /* copy existing disks */
+        if ((existing = virHashLookup(map, domdisk->dst))) {
+            memcpy(snapdisk, existing, sizeof(*snapdisk));
             continue;
-        snapdisk = &snapdef->disks[ndisks++];
+        }
+
+        /* Provide defaults for all remaining disks. */
         snapdisk->src = virStorageSourceNew();
         snapdisk->name = g_strdup(domdef->disks[i]->dst);
-        snapdisk->idx = i;
 
         /* Don't snapshot empty drives */
         if (virStorageSourceIsEmpty(domdef->disks[i]->src))
@@ -756,9 +747,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
             snapdisk->snapshot = default_snapshot;
     }
 
-    qsort(&snapdef->disks[0], snapdef->ndisks, sizeof(snapdef->disks[0]),
-          virDomainSnapshotCompareDiskIndex);
-
     /* Generate default external file names for external snapshot locations */
     if (virDomainSnapshotDefAssignExternalNames(snapdef) < 0)
         return -1;