conf: store snapshot source as pointer, for easier manipulation

As part of the work on backing chains, I'm finding that it would
be easier to directly manipulate chains of pointers (adding a
snapshot merely adjusts pointers to form the correct list) rather
than copy data from one struct to another. This patch converts
snapshot source to be a pointer.

In this patch, the pointer is ALWAYS allocated (any code that
increases ndisks now also allocates a source pointer for each
new disk), and all other changes are just mechanical fallout of
the new type; there should be no functional change.  It is
possible that we may want to leave the pointer NULL for internal
snapshots in a later patch, but as that requires a closer audit
of the source to ensure we don't fault on a null dereference, I
didn't do it here.

* src/conf/snapshot_conf.h (_virDomainSnapshotDiskDef): Change
type of src.
* src/conf/snapshot_conf.c: Adjust all clients.
* src/qemu/qemu_conf.c: Likewise.
* src/qemu/qemu_driver.c: Likewise.
Signed-off-by: NEric Blake <eblake@redhat.com>

src/conf/snapshot_conf.c

@@ -83,7 +83,8 @@ static void
 virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
 {
     VIR_FREE(disk->name);
-    virStorageSourceClear(&disk->src);
+    virStorageSourceFree(disk->src);
+    disk->src = NULL;
 }
 
 void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
@@ -113,6 +114,9 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
     char *type = NULL;
     xmlNodePtr cur;
 
+    if (VIR_ALLOC(def->src) < 0)
+        goto cleanup;
+
     def->name = virXMLPropString(node, "name");
     if (!def->name) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -132,35 +136,35 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
     }
 
     if ((type = virXMLPropString(node, "type"))) {
-        if ((def->src.type = virStorageTypeFromString(type)) <= 0 ||
-            def->src.type == VIR_STORAGE_TYPE_VOLUME ||
-            def->src.type == VIR_STORAGE_TYPE_DIR) {
+        if ((def->src->type = virStorageTypeFromString(type)) <= 0 ||
+            def->src->type == VIR_STORAGE_TYPE_VOLUME ||
+            def->src->type == VIR_STORAGE_TYPE_DIR) {
             virReportError(VIR_ERR_XML_ERROR,
                            _("unknown disk snapshot type '%s'"), type);
             goto cleanup;
         }
     } else {
-        def->src.type = VIR_STORAGE_TYPE_FILE;
+        def->src->type = VIR_STORAGE_TYPE_FILE;
     }
 
     for (cur = node->children; cur; cur = cur->next) {
         if (cur->type != XML_ELEMENT_NODE)
             continue;
 
-        if (!def->src.path &&
+        if (!def->src->path &&
             xmlStrEqual(cur->name, BAD_CAST "source")) {
 
-            if (virDomainDiskSourceParse(cur, &def->src) < 0)
+            if (virDomainDiskSourceParse(cur, def->src) < 0)
                 goto cleanup;
 
-        } else if (!def->src.format &&
+        } else if (!def->src->format &&
                    xmlStrEqual(cur->name, BAD_CAST "driver")) {
             char *driver = virXMLPropString(cur, "type");
             if (driver) {
-                def->src.format = virStorageFileFormatTypeFromString(driver);
-                if (def->src.format < VIR_STORAGE_FILE_BACKING) {
+                def->src->format = virStorageFileFormatTypeFromString(driver);
+                if (def->src->format < VIR_STORAGE_FILE_BACKING) {
                     virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   def->src.format <= 0
+                                   def->src->format <= 0
                                    ? _("unknown disk snapshot driver '%s'")
                                    : _("disk format '%s' lacks backing file "
                                        "support"),
@@ -173,7 +177,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
         }
     }
 
-    if (!def->snapshot && (def->src.path || def->src.format))
+    if (!def->snapshot && (def->src->path || def->src->format))
         def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
 
     ret = 0;
@@ -506,12 +510,12 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
                            disk->name, tmp);
             goto cleanup;
         }
-        if (disk->src.path &&
+        if (disk->src->path &&
             disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("file '%s' for disk '%s' requires "
                              "use of external snapshot mode"),
-                           disk->src.path, disk->name);
+                           disk->src->path, disk->name);
             goto cleanup;
         }
         if (STRNEQ(disk->name, def->dom->disks[idx]->dst)) {
@@ -534,11 +538,13 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
         if (inuse)
             continue;
         disk = &def->disks[ndisks++];
+        if (VIR_ALLOC(disk->src) < 0)
+            goto cleanup;
         if (VIR_STRDUP(disk->name, def->dom->disks[i]->dst) < 0)
             goto cleanup;
         disk->index = i;
         disk->snapshot = def->dom->disks[i]->snapshot;
-        disk->src.type = VIR_STORAGE_TYPE_FILE;
+        disk->src->type = VIR_STORAGE_TYPE_FILE;
         if (!disk->snapshot)
             disk->snapshot = default_snapshot;
     }
@@ -551,17 +557,17 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
         virDomainSnapshotDiskDefPtr disk = &def->disks[i];
 
         if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL &&
-            !disk->src.path) {
+            !disk->src->path) {
             const char *original = virDomainDiskGetSource(def->dom->disks[i]);
             const char *tmp;
             struct stat sb;
 
-            if (disk->src.type != VIR_STORAGE_TYPE_FILE) {
+            if (disk->src->type != VIR_STORAGE_TYPE_FILE) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                _("cannot generate external snapshot name "
                                  "for disk '%s' on a '%s' device"),
                                disk->name,
-                               virStorageTypeToString(disk->src.type));
+                               virStorageTypeToString(disk->src->type));
                 goto cleanup;
             }
 
@@ -583,7 +589,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
 
             tmp = strrchr(original, '.');
             if (!tmp || strchr(tmp, '/')) {
-                if (virAsprintf(&disk->src.path, "%s.%s", original,
+                if (virAsprintf(&disk->src->path, "%s.%s", original,
                                 def->name) < 0)
                     goto cleanup;
             } else {
@@ -592,7 +598,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
                                    _("integer overflow"));
                     goto cleanup;
                 }
-                if (virAsprintf(&disk->src.path, "%.*s.%s",
+                if (virAsprintf(&disk->src->path, "%.*s.%s",
                                 (int) (tmp - original), original,
                                 def->name) < 0)
                     goto cleanup;
@@ -611,7 +617,7 @@ static void
 virDomainSnapshotDiskDefFormat(virBufferPtr buf,
                                virDomainSnapshotDiskDefPtr disk)
 {
-    int type = disk->src.type;
+    int type = disk->src->type;
 
     if (!disk->name)
         return;
@@ -621,7 +627,7 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
         virBufferAsprintf(buf, " snapshot='%s'",
                           virDomainSnapshotLocationTypeToString(disk->snapshot));
 
-    if (!disk->src.path && disk->src.format == 0) {
+    if (!disk->src->path && disk->src->format == 0) {
         virBufferAddLit(buf, "/>\n");
         return;
     }
@@ -629,10 +635,10 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
     virBufferAsprintf(buf, " type='%s'>\n", virStorageTypeToString(type));
     virBufferAdjustIndent(buf, 2);
 
-    if (disk->src.format > 0)
+    if (disk->src->format > 0)
         virBufferEscapeString(buf, "<driver type='%s'/>\n",
-                              virStorageFileFormatTypeToString(disk->src.format));
-    virDomainDiskSourceFormat(buf, &disk->src, 0, 0);
+                              virStorageFileFormatTypeToString(disk->src->format));
+    virDomainDiskSourceFormat(buf, disk->src, 0, 0);
 
     virBufferAdjustIndent(buf, -2);
     virBufferAddLit(buf, "</disk>\n");

src/conf/snapshot_conf.h

@@ -52,7 +52,9 @@ struct _virDomainSnapshotDiskDef {
     int index;      /* index within snapshot->dom->disks that matches name */
     int snapshot;   /* virDomainSnapshotLocation */
 
-    virStorageSource src; /* new wrapper file when snapshot is external */
+    /* details of wrapper external file. src is always non-NULL.
+     * XXX optimize this to allow NULL for internal snapshots? */
+    virStorageSourcePtr src;
 };
 
 /* Stores the complete snapshot metadata */

src/qemu/qemu_conf.c

@@ -1427,7 +1427,7 @@ int
 qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn ATTRIBUTE_UNUSED,
                                     virDomainSnapshotDiskDefPtr def)
 {
-    if (def->src.type != VIR_STORAGE_TYPE_VOLUME)
+    if (def->src->type != VIR_STORAGE_TYPE_VOLUME)
         return 0;
 
     virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

src/qemu/qemu_driver.c

@@ -12249,14 +12249,14 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
         if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
             continue;
 
-        if (!snapdisk->src.format)
-            snapdisk->src.format = VIR_STORAGE_FILE_QCOW2;
+        if (!snapdisk->src->format)
+            snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
 
         /* creates cmd line args: qemu-img create -f qcow2 -o */
         if (!(cmd = virCommandNewArgList(qemuImgPath,
                                          "create",
                                          "-f",
-                                         virStorageFileFormatTypeToString(snapdisk->src.format),
+                                         virStorageFileFormatTypeToString(snapdisk->src->format),
                                          "-o",
                                          NULL)))
             goto cleanup;
@@ -12280,10 +12280,10 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
         }
 
         /* adds cmd line args: /path/to/target/file */
-        virCommandAddArg(cmd, snapdisk->src.path);
+        virCommandAddArg(cmd, snapdisk->src->path);
 
         /* If the target does not exist, we're going to create it possibly */
-        if (!virFileExists(snapdisk->src.path))
+        if (!virFileExists(snapdisk->src->path))
             ignore_value(virBitmapSetBit(created, i));
 
         if (virCommandRun(cmd, NULL) < 0)
@@ -12300,11 +12300,11 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
 
         if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
             VIR_FREE(defdisk->src.path);
-            if (VIR_STRDUP(defdisk->src.path, snapdisk->src.path) < 0) {
+            if (VIR_STRDUP(defdisk->src.path, snapdisk->src->path) < 0) {
                 /* we cannot rollback here in a sane way */
                 goto cleanup;
             }
-            defdisk->src.format = snapdisk->src.format;
+            defdisk->src.format = snapdisk->src->format;
         }
     }
 
@@ -12318,9 +12318,9 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
         ssize_t bit = -1;
         while ((bit = virBitmapNextSetBit(created, bit)) >= 0) {
             snapdisk = &(snap->def->disks[bit]);
-            if (unlink(snapdisk->src.path) < 0)
+            if (unlink(snapdisk->src->path) < 0)
                 VIR_WARN("Failed to remove snapshot image '%s'",
-                         snapdisk->src.path);
+                         snapdisk->src->path);
         }
     }
     virBitmapFree(created);
@@ -12482,7 +12482,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingActive(virDomainDiskDefPtr disk)
 static int
 qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr disk)
 {
-    int actualType = virStorageSourceGetActualType(&disk->src);
+    int actualType = virStorageSourceGetActualType(disk->src);
 
     switch ((virStorageType) actualType) {
     case VIR_STORAGE_TYPE_BLOCK:
@@ -12490,7 +12490,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
         return 0;
 
     case VIR_STORAGE_TYPE_NETWORK:
-        switch ((virStorageNetProtocol) disk->src.protocol) {
+        switch ((virStorageNetProtocol) disk->src->protocol) {
         case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
             return 0;
 
@@ -12508,7 +12508,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("external active snapshots are not supported on "
                              "'network' disks using '%s' protocol"),
-                           virStorageNetProtocolTypeToString(disk->src.protocol));
+                           virStorageNetProtocolTypeToString(disk->src->protocol));
             return -1;
 
         }
@@ -12531,7 +12531,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
 static int
 qemuDomainSnapshotPrepareDiskExternalOverlayInactive(virDomainSnapshotDiskDefPtr disk)
 {
-    int actualType = virStorageSourceGetActualType(&disk->src);
+    int actualType = virStorageSourceGetActualType(disk->src);
 
     switch ((virStorageType) actualType) {
     case VIR_STORAGE_TYPE_BLOCK:
@@ -12583,33 +12583,33 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn,
             return -1;
     }
 
-    if (virStorageFileInit(&snapdisk->src) < 0)
+    if (virStorageFileInit(snapdisk->src) < 0)
         return -1;
 
-    if (virStorageFileStat(&snapdisk->src, &st) < 0) {
+    if (virStorageFileStat(snapdisk->src, &st) < 0) {
         if (errno != ENOENT) {
             virReportSystemError(errno,
                                  _("unable to stat for disk %s: %s"),
-                                 snapdisk->name, snapdisk->src.path);
+                                 snapdisk->name, snapdisk->src->path);
             goto cleanup;
         } else if (reuse) {
             virReportSystemError(errno,
                                  _("missing existing file for disk %s: %s"),
-                                 snapdisk->name, snapdisk->src.path);
+                                 snapdisk->name, snapdisk->src->path);
             goto cleanup;
         }
     } else if (!S_ISBLK(st.st_mode) && st.st_size && !reuse) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("external snapshot file for disk %s already "
                          "exists and is not a block device: %s"),
-                       snapdisk->name, snapdisk->src.path);
+                       snapdisk->name, snapdisk->src->path);
         goto cleanup;
     }
 
     ret = 0;
 
  cleanup:
-    virStorageFileDeinit(&snapdisk->src);
+    virStorageFileDeinit(snapdisk->src);
     return ret;
 }
 
@@ -12732,15 +12732,15 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
             break;
 
         case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL:
-            if (!disk->src.format) {
-                disk->src.format = VIR_STORAGE_FILE_QCOW2;
-            } else if (disk->src.format != VIR_STORAGE_FILE_QCOW2 &&
-                       disk->src.format != VIR_STORAGE_FILE_QED) {
+            if (!disk->src->format) {
+                disk->src->format = VIR_STORAGE_FILE_QCOW2;
+            } else if (disk->src->format != VIR_STORAGE_FILE_QCOW2 &&
+                       disk->src->format != VIR_STORAGE_FILE_QED) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                _("external snapshot format for disk %s "
                                  "is unsupported: %s"),
                                disk->name,
-                               virStorageFileFormatTypeToString(disk->src.format));
+                               virStorageFileFormatTypeToString(disk->src->format));
                 goto cleanup;
             }
 
@@ -12841,7 +12841,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
     char *newsource = NULL;
     virStorageNetHostDefPtr newhosts = NULL;
     virStorageNetHostDefPtr persistHosts = NULL;
-    int format = snap->src.format;
+    int format = snap->src->format;
     const char *formatStr = NULL;
     char *persistSource = NULL;
     int ret = -1;
@@ -12857,27 +12857,27 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
     if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0)
         goto cleanup;
 
-    /* XXX Here, we know we are about to alter disk->src.backingStore if
+    /* XXX Here, we know we are about to alter disk->src->backingStore if
      * successful, so we nuke the existing chain so that future commands will
      * recompute it.  Better would be storing the chain ourselves rather than
      * reprobing, but this requires modifying domain_conf and our XML to fully
      * track the chain across libvirtd restarts.  */
     virStorageSourceClearBackingStore(&disk->src);
 
-    if (virStorageFileInit(&snap->src) < 0)
+    if (virStorageFileInit(snap->src) < 0)
         goto cleanup;
 
-    if (qemuGetDriveSourceString(&snap->src, NULL, &source) < 0)
+    if (qemuGetDriveSourceString(snap->src, NULL, &source) < 0)
         goto cleanup;
 
-    if (VIR_STRDUP(newsource, snap->src.path) < 0)
+    if (VIR_STRDUP(newsource, snap->src->path) < 0)
         goto cleanup;
 
     if (persistDisk &&
-        VIR_STRDUP(persistSource, snap->src.path) < 0)
+        VIR_STRDUP(persistSource, snap->src->path) < 0)
         goto cleanup;
 
-    switch ((virStorageType)snap->src.type) {
+    switch ((virStorageType)snap->src->type) {
     case VIR_STORAGE_TYPE_BLOCK:
         reuse = true;
         /* fallthrough */
@@ -12893,24 +12893,24 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
             VIR_FORCE_CLOSE(fd);
         }
 
-        if (qemuDomainPrepareDiskChainElement(driver, vm, disk, &snap->src,
+        if (qemuDomainPrepareDiskChainElement(driver, vm, disk, snap->src,
                                               VIR_DISK_CHAIN_READ_WRITE) < 0) {
-            qemuDomainPrepareDiskChainElement(driver, vm, disk, &snap->src,
+            qemuDomainPrepareDiskChainElement(driver, vm, disk, snap->src,
                                               VIR_DISK_CHAIN_NO_ACCESS);
             goto cleanup;
         }
         break;
 
     case VIR_STORAGE_TYPE_NETWORK:
-        switch (snap->src.protocol) {
+        switch (snap->src->protocol) {
         case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
-            if (!(newhosts = virStorageNetHostDefCopy(snap->src.nhosts,
-                                                      snap->src.hosts)))
+            if (!(newhosts = virStorageNetHostDefCopy(snap->src->nhosts,
+                                                      snap->src->hosts)))
                 goto cleanup;
 
             if (persistDisk &&
-                !(persistHosts = virStorageNetHostDefCopy(snap->src.nhosts,
-                                                          snap->src.hosts)))
+                !(persistHosts = virStorageNetHostDefCopy(snap->src->nhosts,
+                                                          snap->src->hosts)))
                 goto cleanup;
 
             break;
@@ -12919,7 +12919,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
             virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
                            _("snapshots on volumes using '%s' protocol "
                              "are not supported"),
-                           virStorageNetProtocolTypeToString(snap->src.protocol));
+                           virStorageNetProtocolTypeToString(snap->src->protocol));
             goto cleanup;
         }
         break;
@@ -12930,13 +12930,13 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
     case VIR_STORAGE_TYPE_LAST:
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
                        _("snapshots are not supported on '%s' volumes"),
-                       virStorageTypeToString(snap->src.type));
+                       virStorageTypeToString(snap->src->type));
         goto cleanup;
     }
 
     /* create the actual snapshot */
-    if (snap->src.format)
-        formatStr = virStorageFileFormatTypeToString(snap->src.format);
+    if (snap->src->format)
+        formatStr = virStorageFileFormatTypeToString(snap->src->format);
 
     /* The monitor is only accessed if qemu doesn't support transactions.
      * Otherwise the following monitor command only constructs the command.
@@ -12968,9 +12968,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
 
     disk->src.path = newsource;
     disk->src.format = format;
-    disk->src.type = snap->src.type;
-    disk->src.protocol = snap->src.protocol;
-    disk->src.nhosts = snap->src.nhosts;
+    disk->src.type = snap->src->type;
+    disk->src.protocol = snap->src->protocol;
+    disk->src.nhosts = snap->src->nhosts;
     disk->src.hosts = newhosts;
 
     newsource = NULL;
@@ -12983,9 +12983,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
 
         persistDisk->src.path = persistSource;
         persistDisk->src.format = format;
-        persistDisk->src.type = snap->src.type;
-        persistDisk->src.protocol = snap->src.protocol;
-        persistDisk->src.nhosts = snap->src.nhosts;
+        persistDisk->src.type = snap->src->type;
+        persistDisk->src.protocol = snap->src->protocol;
+        persistDisk->src.nhosts = snap->src->nhosts;
         persistDisk->src.hosts = persistHosts;
 
         persistSource = NULL;
@@ -12993,15 +12993,15 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
     }
 
  cleanup:
-    if (need_unlink && virStorageFileUnlink(&snap->src))
+    if (need_unlink && virStorageFileUnlink(snap->src))
         VIR_WARN("unable to unlink just-created %s", source);
-    virStorageFileDeinit(&snap->src);
+    virStorageFileDeinit(snap->src);
     VIR_FREE(device);
     VIR_FREE(source);
     VIR_FREE(newsource);
     VIR_FREE(persistSource);
-    virStorageNetHostDefFree(snap->src.nhosts, newhosts);
-    virStorageNetHostDefFree(snap->src.nhosts, persistHosts);
+    virStorageNetHostDefFree(snap->src->nhosts, newhosts);
+    virStorageNetHostDefFree(snap->src->nhosts, persistHosts);
     return ret;
 }