[libvirt] [PATCH v2 01/10] 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: Eric Blake <eblake(a)redhat.com&gt; --- src/conf/snapshot_conf.c | 56 +++++++++++++------------ src/conf/snapshot_conf.h | 2 +- src/qemu/qemu_conf.c | 2 +- src/qemu/qemu_driver.c | 104 +++++++++++++++++++++++------------------------ 4 files changed, 85 insertions(+), 79 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index b0e4700..441162c 100644 --- a/src/conf/snapshot_conf.c +++ b/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"); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 1eb697f..21b5b62 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -52,7 +52,7 @@ struct _virDomainSnapshotDiskDef { int index; /* index within snapshot->dom->disks that matches name */ int snapshot; /* virDomainSnapshotLocation */ - virStorageSource src; /* new wrapper file when snapshot is external */ + virStorageSourcePtr src; /* new wrapper file when snapshot is external */ }; /* Stores the complete snapshot metadata */ diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index f273056..c14d700 100644 --- a/src/qemu/qemu_conf.c +++ b/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", diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3a7622a..e79ac09 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12255,14 +12255,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; @@ -12286,10 +12286,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) @@ -12306,11 +12306,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; } } @@ -12324,9 +12324,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); @@ -12488,7 +12488,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: @@ -12496,7 +12496,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; @@ -12514,7 +12514,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; } @@ -12537,7 +12537,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: @@ -12589,33 +12589,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; } @@ -12738,15 +12738,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; } @@ -12847,7 +12847,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; @@ -12863,27 +12863,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 */ @@ -12899,24 +12899,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; @@ -12925,7 +12925,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; @@ -12936,13 +12936,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. @@ -12974,9 +12974,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; @@ -12989,9 +12989,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; @@ -12999,15 +12999,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; } -- 1.9.3