[libvirt] [PATCH v2 00/10] active commit to backing file
This enables active commit (collapsing a temporary qcow2 wrapper file back into the permanent backing file), when the destination is a regular file or block device. Further plans for this work before 1.2.6 is released: figure out how to add a qemu capability probe (qemu 2.0 supports active commit but did a lousy job of advertising it, and I found a corner case bug in it that won't be fixed until 2.1), and integrate with Peter's work to allow active commit into a network backing file. Earlier posts related to this series: https://www.redhat.com/archives/libvir-list/2014-May/msg00564.html https://www.redhat.com/archives/libvir-list/2014-May/msg00723.html There are definitely some merge conflicts to still work out (both Peter and I are adding a new flag to virDomainBlockCommit, so it is touching some of the same code). I can rebase again on top of Peter's pending patches, if needed. I also think the switch from nested struct to pointer for a disk definition's source will make it easier for Benoit's desire to implement quorums. Eric Blake (10): conf: store snapshot source as pointer, for easier manipulation conf: consolidate disk def allocation conf: store disk source as pointer, for easier manipulation conf: store mirroring information in virStorageSource conf: alter disk mirror xml output blockcommit: document semantics of committing active layer virsh: expose new active commit controls blockcommit: update error messages related to block jobs blockcommit: track job type in xml blockcommit: turn on active commit docs/formatdomain.html.in | 25 +- docs/schemas/domaincommon.rng | 35 +- include/libvirt/libvirt.h.in | 12 +- src/conf/domain_conf.c | 275 ++++++++----- src/conf/domain_conf.h | 7 +- src/conf/snapshot_conf.c | 56 +-- src/conf/snapshot_conf.h | 2 +- src/libvirt.c | 55 ++- src/libvirt_private.syms | 1 + src/lxc/lxc_controller.c | 8 +- src/lxc/lxc_driver.c | 8 +- src/parallels/parallels_driver.c | 2 +- src/qemu/qemu_command.c | 290 ++++++------- src/qemu/qemu_conf.c | 88 ++-- src/qemu/qemu_domain.c | 22 +- src/qemu/qemu_driver.c | 451 ++++++++++++--------- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 4 +- src/qemu/qemu_process.c | 39 +- src/security/security_selinux.c | 4 +- src/vbox/vbox_tmpl.c | 6 +- src/vmx/vmx.c | 2 +- src/xenxs/xen_sxpr.c | 8 +- src/xenxs/xen_xm.c | 4 +- .../qemuxml2argv-disk-active-commit.xml | 37 ++ .../qemuxml2argv-disk-mirror-old.xml | 47 +++ .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 9 +- .../qemuxml2xmlout-disk-mirror-old-inactive.xml | 41 ++ .../qemuxml2xmlout-disk-mirror-old.xml | 52 +++ tests/qemuxml2xmltest.c | 2 + tests/securityselinuxlabeltest.c | 6 +- tools/virsh-domain.c | 57 ++- tools/virsh.pod | 27 +- 33 files changed, 1072 insertions(+), 612 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-mirror-old.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml -- 1.9.3
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@redhat.com> --- 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
On 06/06/14 00:52, Eric Blake wrote:
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.
Agreed, that can be done later as it's really just a micro optimization.
* 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@redhat.com> --- 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(-)
ACK. Peter
A future patch wants to create disk definitions with non-zero default contents; to avoid crashes, all callers that allocate a disk definition should go through a common point. I found allocation points by looking for any code that increments ndisks, as well as any matches for ALLOC.*disk. Most places that modified ndisks were covered by the parse from XML to domain/device definition by initial domain creation or device hotplug; I also hand-checked all drivers that generate a device struct on the fly during getXMLDesc. * src/conf/domain_conf.h (virDomainDiskDefNew): New prototype. * src/conf/domain_conf.c (virDomainDiskDefNew): New function. (virDomainDiskDefParseXML): Use it. * src/parallels/parallels_driver.c (parallelsAddHddInfo): Likewise. * src/qemu/qemu_command.c (qemuParseCommandLine): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainGetXMLDesc): Likewise. * src/vmx/vmx.c (virVMXParseDisk): Likewise. * src/xenxs/xen_sxpr.c (xenParseSxprDisks, xenParseSxpr): Likewise. * src/xenxs/xen_xm.c (xenParseXM): Likewise. * src/libvirt_private.syms (domain_conf.h): Export it. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 12 +++++++++++- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/parallels/parallels_driver.c | 2 +- src/qemu/qemu_command.c | 4 ++-- src/vbox/vbox_tmpl.c | 6 +++--- src/vmx/vmx.c | 2 +- src/xenxs/xen_sxpr.c | 8 ++++---- src/xenxs/xen_xm.c | 4 ++-- 9 files changed, 26 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fe06921..62be7ae 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1181,6 +1181,16 @@ void virDomainLeaseDefFree(virDomainLeaseDefPtr def) } +virDomainDiskDefPtr +virDomainDiskDefNew(void) +{ + virDomainDiskDefPtr ret; + + ignore_value(VIR_ALLOC(ret)); + return ret; +} + + void virDomainDiskDefFree(virDomainDiskDefPtr def) { @@ -5232,7 +5242,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, int expected_secret_usage = -1; int auth_secret_usage = -1; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDiskDefNew())) return NULL; def->geometry.cylinders = 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2de807d..ffe3583 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2129,6 +2129,7 @@ void virDomainPanicDefFree(virDomainPanicDefPtr panic); void virDomainResourceDefFree(virDomainResourceDefPtr resource); void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def); void virDomainInputDefFree(virDomainInputDefPtr def); +virDomainDiskDefPtr virDomainDiskDefNew(void); void virDomainDiskDefFree(virDomainDiskDefPtr def); void virDomainLeaseDefFree(virDomainLeaseDefPtr def); int virDomainDiskGetType(virDomainDiskDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d73a9f5..c9b163f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -207,6 +207,7 @@ virDomainDiskDefAssignAddress; virDomainDiskDefForeachPath; virDomainDiskDefFree; virDomainDiskDefGetSecurityLabelDef; +virDomainDiskDefNew; virDomainDiskDeviceTypeToString; virDomainDiskDiscardTypeToString; virDomainDiskErrorPolicyTypeFromString; diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index ab59599..a2665dd 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -381,7 +381,7 @@ parallelsAddHddInfo(virDomainDefPtr def, const char *key, virJSONValuePtr value) { virDomainDiskDefPtr disk = NULL; - if (VIR_ALLOC(disk) < 0) + if (!(disk = virDomainDiskDefNew())) goto error; if (parallelsGetHddInfo(def, disk, key, value)) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e6acced..841e59d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10997,7 +10997,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, STRPREFIX(arg, "-fd") || STREQ(arg, "-cdrom")) { WANT_VALUE(); - if (VIR_ALLOC(disk) < 0) + if (!(disk = virDomainDiskDefNew())) goto error; if (STRPREFIX(val, "/dev/")) @@ -11297,7 +11297,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, goto error; } } else if (STRPREFIX(val, "disk:")) { - if (VIR_ALLOC(disk) < 0) + if (!(disk = virDomainDiskDefNew())) goto error; if (VIR_STRDUP(disk->src.path, val + strlen("disk:")) < 0) goto error; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index e124e69..6fef074 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2768,7 +2768,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { if ((def->ndisks > 0) && (VIR_ALLOC_N(def->disks, def->ndisks) >= 0)) { for (i = 0; i < def->ndisks; i++) { - if (VIR_ALLOC(def->disks[i]) >= 0) { + if ((def->disks[i] = virDomainDiskDefNew())) { def->disks[i]->device = VIR_DOMAIN_DISK_DEVICE_DISK; def->disks[i]->bus = VIR_DOMAIN_DISK_BUS_IDE; virDomainDiskSetType(def->disks[i], @@ -3247,7 +3247,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { def->ndisks++; if (VIR_REALLOC_N(def->disks, def->ndisks) >= 0) { - if (VIR_ALLOC(def->disks[def->ndisks - 1]) >= 0) { + if ((def->disks[def->ndisks - 1] = virDomainDiskDefNew())) { def->disks[def->ndisks - 1]->device = VIR_DOMAIN_DISK_DEVICE_CDROM; def->disks[def->ndisks - 1]->bus = VIR_DOMAIN_DISK_BUS_IDE; virDomainDiskSetType(def->disks[def->ndisks - 1], @@ -3294,7 +3294,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { def->ndisks++; if (VIR_REALLOC_N(def->disks, def->ndisks) >= 0) { - if (VIR_ALLOC(def->disks[def->ndisks - 1]) >= 0) { + if ((def->disks[def->ndisks - 1] = virDomainDiskDefNew())) { def->disks[def->ndisks - 1]->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; def->disks[def->ndisks - 1]->bus = VIR_DOMAIN_DISK_BUS_FDC; virDomainDiskSetType(def->disks[def->ndisks - 1], diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 169440c..9b576f7 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1998,7 +1998,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con return -1; } - if (VIR_ALLOC(*def) < 0) + if (!(*def = virDomainDiskDefNew())) return -1; (*def)->device = device; diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 29316a4..aacf74c 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -370,7 +370,7 @@ xenParseSxprDisks(virDomainDefPtr def, bootable = sexpr_node(node, "device/tap/bootable"); } - if (VIR_ALLOC(disk) < 0) + if (!(disk = virDomainDiskDefNew())) goto error; if (dst == NULL) { @@ -1304,7 +1304,7 @@ xenParseSxpr(const struct sexpr *root, tmp = sexpr_node(root, "domain/image/hvm/cdrom"); if ((tmp != NULL) && (tmp[0] != 0)) { virDomainDiskDefPtr disk; - if (VIR_ALLOC(disk) < 0) + if (!(disk = virDomainDiskDefNew())) goto error; if (virDomainDiskSetSource(disk, tmp) < 0) { virDomainDiskDefFree(disk); @@ -1339,10 +1339,10 @@ xenParseSxpr(const struct sexpr *root, tmp = sexpr_fmt_node(root, "domain/image/hvm/%s", fds[i]); if ((tmp != NULL) && (tmp[0] != 0)) { virDomainDiskDefPtr disk; - if (VIR_ALLOC(disk) < 0) + if (!(disk = virDomainDiskDefNew())) goto error; if (virDomainDiskSetSource(disk, tmp) < 0) { - VIR_FREE(disk); + virDomainDiskDefFree(disk); goto error; } virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE); diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index c0422cf..b2db97d 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -486,7 +486,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, goto skipdisk; head = list->str; - if (VIR_ALLOC(disk) < 0) + if (!(disk = virDomainDiskDefNew())) goto cleanup; /* @@ -632,7 +632,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (xenXMConfigGetString(conf, "cdrom", &str, NULL) < 0) goto cleanup; if (str) { - if (VIR_ALLOC(disk) < 0) + if (!(disk = virDomainDiskDefNew())) goto cleanup; virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE); -- 1.9.3
On 06/06/14 00:52, Eric Blake wrote:
A future patch wants to create disk definitions with non-zero default contents; to avoid crashes, all callers that allocate a disk definition should go through a common point.
I found allocation points by looking for any code that increments ndisks, as well as any matches for ALLOC.*disk. Most places that modified ndisks were covered by the parse from XML to domain/device definition by initial domain creation or device hotplug; I also hand-checked all drivers that generate a device struct on the fly during getXMLDesc.
* src/conf/domain_conf.h (virDomainDiskDefNew): New prototype. * src/conf/domain_conf.c (virDomainDiskDefNew): New function. (virDomainDiskDefParseXML): Use it. * src/parallels/parallels_driver.c (parallelsAddHddInfo): Likewise. * src/qemu/qemu_command.c (qemuParseCommandLine): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainGetXMLDesc): Likewise. * src/vmx/vmx.c (virVMXParseDisk): Likewise. * src/xenxs/xen_sxpr.c (xenParseSxprDisks, xenParseSxpr): Likewise. * src/xenxs/xen_xm.c (xenParseXM): Likewise. * src/libvirt_private.syms (domain_conf.h): Export it.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 12 +++++++++++- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/parallels/parallels_driver.c | 2 +- src/qemu/qemu_command.c | 4 ++-- src/vbox/vbox_tmpl.c | 6 +++--- src/vmx/vmx.c | 2 +- src/xenxs/xen_sxpr.c | 8 ++++---- src/xenxs/xen_xm.c | 4 ++-- 9 files changed, 26 insertions(+), 14 deletions(-)
ACK, Peter
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 domain disk source to be a pointer. In this patch, the pointer is ALWAYS allocated (thanks in part to the previous patch forwarding all disk def allocation through a common point), and all other changse 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 a cdrom with no medium 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/domain_conf.h (_virDomainDiskDef): Change type of src. * src/conf/domain_conf.c: Adjust all clients. * src/security/security_selinux.c: Likewise. * src/qemu/qemu_domain.c: Likewise. * src/qemu/qemu_command.c: Likewise. * src/qemu/qemu_conf.c: Likewise. * src/qemu/qemu_process.c: Likewise. * src/qemu/qemu_migration.c: Likewise. * src/qemu/qemu_driver.c: Likewise. * src/lxc/lxc_driver.c: Likewise. * src/lxc/lxc_controller.c: Likewise. * tests/securityselinuxlabeltest.c: Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 141 +++++++++---------- src/conf/domain_conf.h | 2 +- src/lxc/lxc_controller.c | 8 +- src/lxc/lxc_driver.c | 8 +- src/qemu/qemu_command.c | 286 ++++++++++++++++++++------------------- src/qemu/qemu_conf.c | 86 ++++++------ src/qemu/qemu_domain.c | 14 +- src/qemu/qemu_driver.c | 232 +++++++++++++++---------------- src/qemu/qemu_migration.c | 4 +- src/qemu/qemu_process.c | 8 +- src/security/security_selinux.c | 4 +- tests/securityselinuxlabeltest.c | 6 +- 12 files changed, 403 insertions(+), 396 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 62be7ae..361d087 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1186,7 +1186,10 @@ virDomainDiskDefNew(void) { virDomainDiskDefPtr ret; - ignore_value(VIR_ALLOC(ret)); + if (VIR_ALLOC(ret) < 0) + return NULL; + if (VIR_ALLOC(ret->src) < 0) + VIR_FREE(ret); return ret; } @@ -1197,7 +1200,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def) if (!def) return; - virStorageSourceClear(&def->src); + virStorageSourceFree(def->src); VIR_FREE(def->serial); VIR_FREE(def->dst); VIR_FREE(def->mirror); @@ -1213,21 +1216,21 @@ virDomainDiskDefFree(virDomainDiskDefPtr def) int virDomainDiskGetType(virDomainDiskDefPtr def) { - return def->src.type; + return def->src->type; } void virDomainDiskSetType(virDomainDiskDefPtr def, int type) { - def->src.type = type; + def->src->type = type; } const char * virDomainDiskGetSource(virDomainDiskDefPtr def) { - return def->src.path; + return def->src->path; } @@ -1235,11 +1238,11 @@ int virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src) { int ret; - char *tmp = def->src.path; + char *tmp = def->src->path; - ret = VIR_STRDUP(def->src.path, src); + ret = VIR_STRDUP(def->src->path, src); if (ret < 0) - def->src.path = tmp; + def->src->path = tmp; else VIR_FREE(tmp); return ret; @@ -1249,7 +1252,7 @@ virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src) const char * virDomainDiskGetDriver(virDomainDiskDefPtr def) { - return def->src.driverName; + return def->src->driverName; } @@ -1257,11 +1260,11 @@ int virDomainDiskSetDriver(virDomainDiskDefPtr def, const char *name) { int ret; - char *tmp = def->src.driverName; + char *tmp = def->src->driverName; - ret = VIR_STRDUP(def->src.driverName, name); + ret = VIR_STRDUP(def->src->driverName, name); if (ret < 0) - def->src.driverName = tmp; + def->src->driverName = tmp; else VIR_FREE(tmp); return ret; @@ -1271,14 +1274,14 @@ virDomainDiskSetDriver(virDomainDiskDefPtr def, const char *name) int virDomainDiskGetFormat(virDomainDiskDefPtr def) { - return def->src.format; + return def->src->format; } void virDomainDiskSetFormat(virDomainDiskDefPtr def, int format) { - def->src.format = format; + def->src->format = format; } @@ -5257,13 +5260,13 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, type = virXMLPropString(node, "type"); if (type) { - if ((def->src.type = virStorageTypeFromString(type)) <= 0) { + if ((def->src->type = virStorageTypeFromString(type)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk type '%s'"), type); goto error; } } else { - def->src.type = VIR_STORAGE_TYPE_FILE; + def->src->type = VIR_STORAGE_TYPE_FILE; } snapshot = virXMLPropString(node, "snapshot"); @@ -5274,18 +5277,18 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { - if (!source && !def->src.hosts && !def->src.srcpool && + if (!source && !def->src->hosts && !def->src->srcpool && xmlStrEqual(cur->name, BAD_CAST "source")) { sourceNode = cur; - if (virDomainDiskSourceParse(cur, &def->src) < 0) + if (virDomainDiskSourceParse(cur, def->src) < 0) goto error; - source = def->src.path; + source = def->src->path; - if (def->src.type == VIR_STORAGE_TYPE_NETWORK) { - if (def->src.protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) + if (def->src->type == VIR_STORAGE_TYPE_NETWORK) { + if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) expected_secret_usage = VIR_SECRET_USAGE_TYPE_ISCSI; - else if (def->src.protocol == VIR_STORAGE_NET_PROTOCOL_RBD) + else if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) expected_secret_usage = VIR_SECRET_USAGE_TYPE_CEPH; } @@ -5394,7 +5397,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - def->src.auth.secretType = VIR_STORAGE_SECRET_TYPE_NONE; + def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_NONE; child = cur->children; while (child != NULL) { if (child->type == XML_ELEMENT_NODE && @@ -5431,17 +5434,17 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } if (authUUID != NULL) { - def->src.auth.secretType = VIR_STORAGE_SECRET_TYPE_UUID; + def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_UUID; if (virUUIDParse(authUUID, - def->src.auth.secret.uuid) < 0) { + def->src->auth.secret.uuid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("malformed uuid %s"), authUUID); goto error; } } else if (authUsage != NULL) { - def->src.auth.secretType = VIR_STORAGE_SECRET_TYPE_USAGE; - def->src.auth.secret.usage = authUsage; + def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_USAGE; + def->src->auth.secret.usage = authUsage; authUsage = NULL; } } @@ -5586,7 +5589,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, /* Only CDROM and Floppy devices are allowed missing source path * to indicate no media present. LUN is for raw access CD-ROMs * that are not attached to a physical device presently */ - if (source == NULL && def->src.hosts == NULL && !def->src.srcpool && + if (source == NULL && def->src->hosts == NULL && !def->src->srcpool && def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && def->device != VIR_DOMAIN_DISK_DEVICE_LUN && def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { @@ -5599,8 +5602,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (sourceNode) { xmlNodePtr saved_node = ctxt->node; ctxt->node = sourceNode; - if (virSecurityDeviceLabelDefParseXML(&def->src.seclabels, - &def->src.nseclabels, + if (virSecurityDeviceLabelDefParseXML(&def->src->seclabels, + &def->src->nseclabels, vmSeclabels, nvmSeclabels, ctxt, @@ -5610,10 +5613,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } if (target == NULL) { - if (def->src.srcpool) { + if (def->src->srcpool) { char *tmp; if (virAsprintf(&tmp, "pool = '%s', volume = '%s'", - def->src.srcpool->pool, def->src.srcpool->volume) < 0) + def->src->srcpool->pool, def->src->srcpool->volume) < 0) goto error; virReportError(VIR_ERR_NO_TARGET, "%s", tmp); @@ -5878,7 +5881,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if (def->src.type == VIR_STORAGE_TYPE_NETWORK) { + if (def->src->type == VIR_STORAGE_TYPE_NETWORK) { virReportError(VIR_ERR_XML_ERROR, _("Setting disk %s is not allowed for " "disk of network type"), @@ -5899,14 +5902,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->dst = target; target = NULL; - def->src.auth.username = authUsername; + def->src->auth.username = authUsername; authUsername = NULL; - def->src.driverName = driverName; + def->src->driverName = driverName; driverName = NULL; def->mirror = mirror; mirror = NULL; def->mirroring = mirroring; - def->src.encryption = encryption; + def->src->encryption = encryption; encryption = NULL; def->serial = serial; serial = NULL; @@ -5918,8 +5921,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, product = NULL; if (driverType) { - def->src.format = virStorageFileFormatTypeFromString(driverType); - if (def->src.format <= 0) { + def->src->format = virStorageFileFormatTypeFromString(driverType); + if (def->src->format <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown driver format value '%s'"), driverType); @@ -5941,7 +5944,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, && virDomainDiskDefAssignAddress(xmlopt, def) < 0) goto error; - if (virDomainDiskBackingStoreParse(ctxt, &def->src) < 0) + if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) goto error; cleanup: @@ -15021,7 +15024,7 @@ virDomainDiskDefFormat(virBufferPtr buf, virDomainDiskDefPtr def, unsigned int flags) { - const char *type = virStorageTypeToString(def->src.type); + const char *type = virStorageTypeToString(def->src->type); const char *device = virDomainDiskDeviceTypeToString(def->device); const char *bus = virDomainDiskBusTypeToString(def->bus); const char *cachemode = virDomainDiskCacheTypeToString(def->cachemode); @@ -15036,9 +15039,9 @@ virDomainDiskDefFormat(virBufferPtr buf, char uuidstr[VIR_UUID_STRING_BUFLEN]; - if (!type || !def->src.type) { + if (!type || !def->src->type) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk type %d"), def->src.type); + _("unexpected disk type %d"), def->src->type); return -1; } if (!device) { @@ -15088,16 +15091,16 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); - if (def->src.driverName || def->src.format > 0 || def->cachemode || + if (def->src->driverName || def->src->format > 0 || def->cachemode || def->error_policy || def->rerror_policy || def->iomode || def->ioeventfd || def->event_idx || def->copy_on_read || def->discard) { virBufferAddLit(buf, "<driver"); - if (def->src.driverName) - virBufferAsprintf(buf, " name='%s'", def->src.driverName); - if (def->src.format > 0) + if (def->src->driverName) + virBufferAsprintf(buf, " name='%s'", def->src->driverName); + if (def->src->format > 0) virBufferAsprintf(buf, " type='%s'", - virStorageFileFormatTypeToString(def->src.format)); + virStorageFileFormatTypeToString(def->src->format)); if (def->cachemode) virBufferAsprintf(buf, " cache='%s'", cachemode); if (def->error_policy) @@ -15117,37 +15120,37 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - if (def->src.auth.username) { + if (def->src->auth.username) { virBufferEscapeString(buf, "<auth username='%s'>\n", - def->src.auth.username); + def->src->auth.username); virBufferAdjustIndent(buf, 2); - if (def->src.protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) { + if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) { virBufferAddLit(buf, "<secret type='iscsi'"); - } else if (def->src.protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { + } else if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { virBufferAddLit(buf, "<secret type='ceph'"); } - if (def->src.auth.secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - virUUIDFormat(def->src.auth.secret.uuid, uuidstr); + if (def->src->auth.secretType == VIR_STORAGE_SECRET_TYPE_UUID) { + virUUIDFormat(def->src->auth.secret.uuid, uuidstr); virBufferAsprintf(buf, " uuid='%s'/>\n", uuidstr); } - if (def->src.auth.secretType == VIR_STORAGE_SECRET_TYPE_USAGE) { + if (def->src->auth.secretType == VIR_STORAGE_SECRET_TYPE_USAGE) { virBufferEscapeString(buf, " usage='%s'/>\n", - def->src.auth.secret.usage); + def->src->auth.secret.usage); } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</auth>\n"); } - if (virDomainDiskSourceFormat(buf, &def->src, def->startupPolicy, + if (virDomainDiskSourceFormat(buf, def->src, def->startupPolicy, flags) < 0) return -1; /* Don't format backingStore to inactive XMLs until the code for * persistent storage of backing chains is ready. */ if (!(flags & VIR_DOMAIN_XML_INACTIVE) && - virDomainDiskBackingStoreFormat(buf, def->src.backingStore, - def->src.backingStoreRaw, 1) < 0) + virDomainDiskBackingStoreFormat(buf, def->src->backingStore, + def->src->backingStoreRaw, 1) < 0) return -1; virDomainDiskGeometryDefFormat(buf, def); @@ -15233,8 +15236,8 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<wwn>%s</wwn>\n", def->wwn); virBufferEscapeString(buf, "<vendor>%s</vendor>\n", def->vendor); virBufferEscapeString(buf, "<product>%s</product>\n", def->product); - if (def->src.encryption && - virStorageEncryptionFormat(buf, def->src.encryption) < 0) + if (def->src->encryption && + virStorageEncryptionFormat(buf, def->src->encryption) < 0) return -1; if (virDomainDeviceInfoFormat(buf, &def->info, flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT) < 0) @@ -18693,7 +18696,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, char *brokenRaw = NULL; if (!ignoreOpenFailure) { - if (virStorageFileChainGetBroken(&disk->src, &brokenRaw) < 0) + if (virStorageFileChainGetBroken(disk->src, &brokenRaw) < 0) goto cleanup; if (brokenRaw) { @@ -18704,7 +18707,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, } } - for (tmp = &disk->src; tmp; tmp = tmp->backingStore) { + for (tmp = disk->src; tmp; tmp = tmp->backingStore) { int actualType = virStorageSourceGetActualType(tmp); /* execute the callback only for local storage */ if (actualType != VIR_STORAGE_TYPE_NETWORK && @@ -19461,9 +19464,9 @@ virDomainDiskDefGetSecurityLabelDef(virDomainDiskDefPtr def, const char *model) if (def == NULL) return NULL; - for (i = 0; i < def->src.nseclabels; i++) { - if (STREQ_NULLABLE(def->src.seclabels[i]->model, model)) - return def->src.seclabels[i]; + for (i = 0; i < def->src->nseclabels; i++) { + if (STREQ_NULLABLE(def->src->seclabels[i]->model, model)) + return def->src->seclabels[i]; } return NULL; } @@ -19554,14 +19557,14 @@ virDomainDiskSourceIsBlockType(virDomainDiskDefPtr def) * If it's a block type source pool, then it's possible */ if (virDomainDiskGetType(def) == VIR_STORAGE_TYPE_VOLUME && - def->src.srcpool && - def->src.srcpool->voltype == VIR_STORAGE_VOL_BLOCK) { + def->src->srcpool && + def->src->srcpool->voltype == VIR_STORAGE_VOL_BLOCK) { /* We don't think the volume accessed by remote URI is * block type source, since we can't/shouldn't manage it * (e.g. set sgio=filtered|unfiltered for it) in libvirt. */ - if (def->src.srcpool->pooltype == VIR_STORAGE_POOL_ISCSI && - def->src.srcpool->mode == VIR_STORAGE_SOURCE_POOL_MODE_DIRECT) + if (def->src->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI && + def->src->srcpool->mode == VIR_STORAGE_SOURCE_POOL_MODE_DIRECT) return false; return true; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ffe3583..3585537 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -598,7 +598,7 @@ typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr; /* Stores the virtual disk configuration */ struct _virDomainDiskDef { - virStorageSource src; + virStorageSourcePtr src; int device; /* enum virDomainDiskDevice */ int bus; /* enum virDomainDiskBus */ diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 5521c6e..fe2a5dc 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1669,7 +1669,7 @@ static int virLXCControllerSetupDisk(virLXCControllerPtr ctrl, int ret = -1; struct stat sb; mode_t mode; - char *tmpsrc = def->src.path; + char *tmpsrc = def->src->path; if (virDomainDiskGetType(def) != VIR_STORAGE_TYPE_BLOCK) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -1686,7 +1686,7 @@ static int virLXCControllerSetupDisk(virLXCControllerPtr ctrl, LXC_STATE_DIR, ctrl->def->name, def->dst) < 0) goto cleanup; - if (stat(def->src.path, &sb) < 0) { + if (stat(def->src->path, &sb) < 0) { virReportSystemError(errno, _("Unable to access %s"), tmpsrc); goto cleanup; @@ -1726,14 +1726,14 @@ static int virLXCControllerSetupDisk(virLXCControllerPtr ctrl, /* Labelling normally operates on src, but we need * to actually label the dst here, so hack the config */ - def->src.path = dst; + def->src->path = dst; if (virSecurityManagerSetImageLabel(securityDriver, ctrl->def, def) < 0) goto cleanup; ret = 0; cleanup: - def->src.path = tmpsrc; + def->src->path = tmpsrc; VIR_FREE(dst); return ret; } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 1086289..d2852a7 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3897,14 +3897,14 @@ lxcDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, switch (data->def->type) { case VIR_DOMAIN_DEVICE_DISK: { virDomainDiskDefPtr def = data->def->data.disk; - char *tmpsrc = def->src.path; - def->src.path = data->file; + char *tmpsrc = def->src->path; + def->src->path = data->file; if (virSecurityManagerSetImageLabel(data->driver->securityManager, data->vm->def, def) < 0) { - def->src.path = tmpsrc; + def->src->path = tmpsrc; goto cleanup; } - def->src.path = tmpsrc; + def->src->path = tmpsrc; } break; case VIR_DOMAIN_DEVICE_HOSTDEV: { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 841e59d..3cf279e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2700,7 +2700,7 @@ static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport) size_t skip; char **parts; - if (VIR_EXPAND_N(disk->src.hosts, disk->src.nhosts, 1) < 0) + if (VIR_EXPAND_N(disk->src->hosts, disk->src->nhosts, 1) < 0) return -1; if ((port = strchr(hostport, ']'))) { @@ -2715,29 +2715,29 @@ static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport) if (port) { *port = '\0'; port += skip; - if (VIR_STRDUP(disk->src.hosts[disk->src.nhosts - 1].port, port) < 0) + if (VIR_STRDUP(disk->src->hosts[disk->src->nhosts - 1].port, port) < 0) goto error; } else { - if (VIR_STRDUP(disk->src.hosts[disk->src.nhosts - 1].port, "6789") < 0) + if (VIR_STRDUP(disk->src->hosts[disk->src->nhosts - 1].port, "6789") < 0) goto error; } parts = virStringSplit(hostport, "\\:", 0); if (!parts) goto error; - disk->src.hosts[disk->src.nhosts-1].name = virStringJoin((const char **)parts, ":"); + disk->src->hosts[disk->src->nhosts-1].name = virStringJoin((const char **)parts, ":"); virStringFreeList(parts); - if (!disk->src.hosts[disk->src.nhosts-1].name) + if (!disk->src->hosts[disk->src->nhosts-1].name) goto error; - disk->src.hosts[disk->src.nhosts-1].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - disk->src.hosts[disk->src.nhosts-1].socket = NULL; + disk->src->hosts[disk->src->nhosts-1].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + disk->src->hosts[disk->src->nhosts-1].socket = NULL; return 0; error: - VIR_FREE(disk->src.hosts[disk->src.nhosts-1].port); - VIR_FREE(disk->src.hosts[disk->src.nhosts-1].name); + VIR_FREE(disk->src->hosts[disk->src->nhosts-1].port); + VIR_FREE(disk->src->hosts[disk->src->nhosts-1].name); return -1; } @@ -2747,7 +2747,7 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk) char *options = NULL; char *p, *e, *next; - p = strchr(disk->src.path, ':'); + p = strchr(disk->src->path, ':'); if (p) { if (VIR_STRDUP(options, p + 1) < 0) goto error; @@ -2776,7 +2776,7 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk) } if (STRPREFIX(p, "id=") && - VIR_STRDUP(disk->src.auth.username, p + strlen("id=")) < 0) + VIR_STRDUP(disk->src->auth.username, p + strlen("id=")) < 0) goto error; if (STRPREFIX(p, "mon_host=")) { char *h, *sep; @@ -2819,7 +2819,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, char *volimg = NULL; char *secret = NULL; - if (VIR_ALLOC(def->src.hosts) < 0) + if (VIR_ALLOC(def->src->hosts) < 0) goto error; transp = strchr(uri->scheme, '+'); @@ -2833,30 +2833,30 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, } if (!transp) { - def->src.hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + def->src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; } else { - def->src.hosts->transport = virStorageNetHostTransportTypeFromString(transp); - if (def->src.hosts->transport < 0) { + def->src->hosts->transport = virStorageNetHostTransportTypeFromString(transp); + if (def->src->hosts->transport < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid %s transport type '%s'"), scheme, transp); goto error; } } - def->src.nhosts = 0; /* set to 1 once everything succeeds */ + def->src->nhosts = 0; /* set to 1 once everything succeeds */ - if (def->src.hosts->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX) { - if (VIR_STRDUP(def->src.hosts->name, uri->server) < 0) + if (def->src->hosts->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX) { + if (VIR_STRDUP(def->src->hosts->name, uri->server) < 0) goto error; - if (virAsprintf(&def->src.hosts->port, "%d", uri->port) < 0) + if (virAsprintf(&def->src->hosts->port, "%d", uri->port) < 0) goto error; } else { - def->src.hosts->name = NULL; - def->src.hosts->port = 0; + def->src->hosts->name = NULL; + def->src->hosts->port = 0; if (uri->query) { if (STRPREFIX(uri->query, "socket=")) { sock = strchr(uri->query, '=') + 1; - if (VIR_STRDUP(def->src.hosts->socket, sock) < 0) + if (VIR_STRDUP(def->src->hosts->socket, sock) < 0) goto error; } else { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2867,11 +2867,11 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, } if (uri->path) { volimg = uri->path + 1; /* skip the prefix slash */ - VIR_FREE(def->src.path); - if (VIR_STRDUP(def->src.path, volimg) < 0) + VIR_FREE(def->src->path); + if (VIR_STRDUP(def->src->path, volimg) < 0) goto error; } else { - VIR_FREE(def->src.path); + VIR_FREE(def->src->path); } if (uri->user) { @@ -2879,11 +2879,11 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, if (secret) *secret = '\0'; - if (VIR_STRDUP(def->src.auth.username, uri->user) < 0) + if (VIR_STRDUP(def->src->auth.username, uri->user) < 0) goto error; } - def->src.nhosts = 1; + def->src->nhosts = 1; ret = 0; cleanup: @@ -2892,8 +2892,8 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, return ret; error: - virStorageNetHostDefClear(def->src.hosts); - VIR_FREE(def->src.hosts); + virStorageNetHostDefClear(def->src->hosts); + VIR_FREE(def->src->hosts); goto cleanup; } @@ -2902,7 +2902,7 @@ qemuParseGlusterString(virDomainDiskDefPtr def) { virURIPtr uri = NULL; - if (!(uri = virURIParse(def->src.path))) + if (!(uri = virURIParse(def->src->path))) return -1; return qemuParseDriveURIString(def, uri, "gluster"); @@ -2915,7 +2915,7 @@ qemuParseISCSIString(virDomainDiskDefPtr def) char *slash; unsigned lun; - if (!(uri = virURIParse(def->src.path))) + if (!(uri = virURIParse(def->src->path))) return -1; if (uri->path && @@ -2926,7 +2926,7 @@ qemuParseISCSIString(virDomainDiskDefPtr def) else if (virStrToLong_ui(slash + 1, NULL, 10, &lun) == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid name '%s' for iSCSI disk"), - def->src.path); + def->src->path); return -1; } } @@ -2943,8 +2943,8 @@ qemuParseNBDString(virDomainDiskDefPtr disk) virURIPtr uri = NULL; - if (strstr(disk->src.path, "://")) { - if (!(uri = virURIParse(disk->src.path))) + if (strstr(disk->src->path, "://")) { + if (!(uri = virURIParse(disk->src->path))) return -1; return qemuParseDriveURIString(disk, uri, "nbd"); } @@ -2952,7 +2952,7 @@ qemuParseNBDString(virDomainDiskDefPtr disk) if (VIR_ALLOC(h) < 0) goto error; - host = disk->src.path + strlen("nbd:"); + host = disk->src->path + strlen("nbd:"); if (STRPREFIX(host, "unix:/")) { src = strchr(host + strlen("unix:"), ':'); if (src) @@ -2965,7 +2965,7 @@ qemuParseNBDString(virDomainDiskDefPtr disk) port = strchr(host, ':'); if (!port) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), disk->src.path); + _("cannot parse nbd filename '%s'"), disk->src->path); goto error; } @@ -2988,10 +2988,10 @@ qemuParseNBDString(virDomainDiskDefPtr disk) src = NULL; } - VIR_FREE(disk->src.path); - disk->src.path = src; - disk->src.nhosts = 1; - disk->src.hosts = h; + VIR_FREE(disk->src->path); + disk->src->path = src; + disk->src->nhosts = 1; + disk->src->hosts = h; return 0; error: @@ -3369,7 +3369,7 @@ qemuBuildDriveStr(virConnectPtr conn, int idx = virDiskNameToIndex(disk->dst); int busid = -1, unitid = -1; char *source = NULL; - int actualType = virStorageSourceGetActualType(&disk->src); + int actualType = virStorageSourceGetActualType(disk->src); if (idx < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -3451,7 +3451,7 @@ qemuBuildDriveStr(virConnectPtr conn, break; } - if (qemuGetDriveSourceString(&disk->src, conn, &source) < 0) + if (qemuGetDriveSourceString(disk->src, conn, &source) < 0) goto error; if (source && @@ -3464,11 +3464,11 @@ qemuBuildDriveStr(virConnectPtr conn, switch (actualType) { case VIR_STORAGE_TYPE_DIR: /* QEMU only supports magic FAT format for now */ - if (disk->src.format > 0 && - disk->src.format != VIR_STORAGE_FILE_FAT) { + if (disk->src->format > 0 && + disk->src->format != VIR_STORAGE_FILE_FAT) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported disk driver type for '%s'"), - virStorageFileFormatTypeToString(disk->src.format)); + virStorageFileFormatTypeToString(disk->src->format)); goto error; } @@ -3488,7 +3488,7 @@ qemuBuildDriveStr(virConnectPtr conn, case VIR_STORAGE_TYPE_BLOCK: if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - disk->src.type == VIR_STORAGE_TYPE_VOLUME ? + disk->src->type == VIR_STORAGE_TYPE_VOLUME ? _("tray status 'open' is invalid for block type volume") : _("tray status 'open' is invalid for block type disk")); goto error; @@ -3548,11 +3548,11 @@ qemuBuildDriveStr(virConnectPtr conn, _("transient disks not supported yet")); goto error; } - if (disk->src.format > 0 && - disk->src.type != VIR_STORAGE_TYPE_DIR && + if (disk->src->format > 0 && + disk->src->type != VIR_STORAGE_TYPE_DIR && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_FORMAT)) virBufferAsprintf(&opt, ",format=%s", - virStorageFileFormatTypeToString(disk->src.format)); + virStorageFileFormatTypeToString(disk->src->format)); /* generate geometry command string */ if (disk->geometry.cylinders > 0 && @@ -3763,11 +3763,11 @@ qemuBuildDriveDevStr(virDomainDefPtr def, bus); goto error; } - if (disk->src.type == VIR_STORAGE_TYPE_NETWORK) { - if (disk->src.protocol != VIR_STORAGE_NET_PROTOCOL_ISCSI) { + if (disk->src->type == VIR_STORAGE_TYPE_NETWORK) { + if (disk->src->protocol != VIR_STORAGE_NET_PROTOCOL_ISCSI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk device='lun' is not supported for protocol='%s'"), - virStorageNetProtocolTypeToString(disk->src.protocol)); + virStorageNetProtocolTypeToString(disk->src->protocol)); goto error; } } else if (!virDomainDiskSourceIsBlockType(disk)) { @@ -7908,11 +7908,11 @@ qemuBuildCommandLine(virConnectPtr conn, for (i = 0; i < def->ndisks; i++) { virDomainDiskDefPtr disk = def->disks[i]; - if (disk->src.driverName != NULL && - !STREQ(disk->src.driverName, "qemu")) { + if (disk->src->driverName != NULL && + !STREQ(disk->src->driverName, "qemu")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported driver name '%s' for disk '%s'"), - disk->src.driverName, disk->src.path); + disk->src->driverName, disk->src->path); goto error; } } @@ -8037,11 +8037,11 @@ qemuBuildCommandLine(virConnectPtr conn, !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { virCommandAddArg(cmd, "-usbdevice"); - virCommandAddArgFormat(cmd, "disk:%s", disk->src.path); + virCommandAddArgFormat(cmd, "disk:%s", disk->src->path); } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported usb disk type for '%s'"), - disk->src.path); + disk->src->path); goto error; } continue; @@ -8127,7 +8127,7 @@ qemuBuildCommandLine(virConnectPtr conn, const char *fmt; virDomainDiskDefPtr disk = def->disks[i]; - if ((disk->src.type == VIR_STORAGE_TYPE_BLOCK) && + if ((disk->src->type == VIR_STORAGE_TYPE_BLOCK) && (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("tray status 'open' is invalid for " @@ -8138,11 +8138,11 @@ qemuBuildCommandLine(virConnectPtr conn, if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { virCommandAddArg(cmd, "-usbdevice"); - virCommandAddArgFormat(cmd, "disk:%s", disk->src.path); + virCommandAddArgFormat(cmd, "disk:%s", disk->src->path); } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported usb disk type for '%s'"), - disk->src.path); + disk->src->path); goto error; } continue; @@ -8150,7 +8150,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (STREQ(disk->dst, "hdc") && disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { - if (disk->src.path) { + if (disk->src->path) { snprintf(dev, NAME_MAX, "-%s", "cdrom"); } else { continue; @@ -8166,13 +8166,13 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if (disk->src.type == VIR_STORAGE_TYPE_DIR) { + if (disk->src->type == VIR_STORAGE_TYPE_DIR) { /* QEMU only supports magic FAT format for now */ - if (disk->src.format > 0 && - disk->src.format != VIR_STORAGE_FILE_FAT) { + if (disk->src->format > 0 && + disk->src->format != VIR_STORAGE_FILE_FAT) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported disk driver type for '%s'"), - virStorageFileFormatTypeToString(disk->src.format)); + virStorageFileFormatTypeToString(disk->src->format)); goto error; } if (!disk->readonly) { @@ -8187,12 +8187,12 @@ qemuBuildCommandLine(virConnectPtr conn, if (virAsprintf(&file, fmt, disk->src) < 0) goto error; - } else if (disk->src.type == VIR_STORAGE_TYPE_NETWORK) { + } else if (disk->src->type == VIR_STORAGE_TYPE_NETWORK) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("network disks are only supported with -drive")); goto error; } else { - if (VIR_STRDUP(file, disk->src.path) < 0) { + if (VIR_STRDUP(file, disk->src->path) < 0) { goto error; } } @@ -9693,6 +9693,8 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, if (VIR_ALLOC(def) < 0) goto cleanup; + if (VIR_ALLOC(def->src) < 0) + goto error; if (((dom->os.arch == VIR_ARCH_PPC64) && dom->os.machine && STREQ(dom->os.machine, "pseries"))) @@ -9700,28 +9702,28 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, else def->bus = VIR_DOMAIN_DISK_BUS_IDE; def->device = VIR_DOMAIN_DISK_DEVICE_DISK; - def->src.type = VIR_STORAGE_TYPE_FILE; + def->src->type = VIR_STORAGE_TYPE_FILE; for (i = 0; i < nkeywords; i++) { if (STREQ(keywords[i], "file")) { if (values[i] && STRNEQ(values[i], "")) { - def->src.path = values[i]; + def->src->path = values[i]; values[i] = NULL; - if (STRPREFIX(def->src.path, "/dev/")) - def->src.type = VIR_STORAGE_TYPE_BLOCK; - else if (STRPREFIX(def->src.path, "nbd:") || - STRPREFIX(def->src.path, "nbd+")) { - def->src.type = VIR_STORAGE_TYPE_NETWORK; - def->src.protocol = VIR_STORAGE_NET_PROTOCOL_NBD; + if (STRPREFIX(def->src->path, "/dev/")) + def->src->type = VIR_STORAGE_TYPE_BLOCK; + else if (STRPREFIX(def->src->path, "nbd:") || + STRPREFIX(def->src->path, "nbd+")) { + def->src->type = VIR_STORAGE_TYPE_NETWORK; + def->src->protocol = VIR_STORAGE_NET_PROTOCOL_NBD; if (qemuParseNBDString(def) < 0) goto error; - } else if (STRPREFIX(def->src.path, "rbd:")) { - char *p = def->src.path; + } else if (STRPREFIX(def->src->path, "rbd:")) { + char *p = def->src->path; - def->src.type = VIR_STORAGE_TYPE_NETWORK; - def->src.protocol = VIR_STORAGE_NET_PROTOCOL_RBD; - if (VIR_STRDUP(def->src.path, p + strlen("rbd:")) < 0) + def->src->type = VIR_STORAGE_TYPE_NETWORK; + def->src->protocol = VIR_STORAGE_NET_PROTOCOL_RBD; + if (VIR_STRDUP(def->src->path, p + strlen("rbd:")) < 0) goto error; /* old-style CEPH_ARGS env variable is parsed later */ if (!old_style_ceph_args && qemuParseRBDString(def) < 0) { @@ -9730,31 +9732,31 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, } VIR_FREE(p); - } else if (STRPREFIX(def->src.path, "gluster:") || - STRPREFIX(def->src.path, "gluster+")) { - def->src.type = VIR_STORAGE_TYPE_NETWORK; - def->src.protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER; + } else if (STRPREFIX(def->src->path, "gluster:") || + STRPREFIX(def->src->path, "gluster+")) { + def->src->type = VIR_STORAGE_TYPE_NETWORK; + def->src->protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER; if (qemuParseGlusterString(def) < 0) goto error; - } else if (STRPREFIX(def->src.path, "iscsi:")) { - def->src.type = VIR_STORAGE_TYPE_NETWORK; - def->src.protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; + } else if (STRPREFIX(def->src->path, "iscsi:")) { + def->src->type = VIR_STORAGE_TYPE_NETWORK; + def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; if (qemuParseISCSIString(def) < 0) goto error; - } else if (STRPREFIX(def->src.path, "sheepdog:")) { - char *p = def->src.path; + } else if (STRPREFIX(def->src->path, "sheepdog:")) { + char *p = def->src->path; char *port, *vdi; - def->src.type = VIR_STORAGE_TYPE_NETWORK; - def->src.protocol = VIR_STORAGE_NET_PROTOCOL_SHEEPDOG; - if (VIR_STRDUP(def->src.path, p + strlen("sheepdog:")) < 0) + def->src->type = VIR_STORAGE_TYPE_NETWORK; + def->src->protocol = VIR_STORAGE_NET_PROTOCOL_SHEEPDOG; + if (VIR_STRDUP(def->src->path, p + strlen("sheepdog:")) < 0) goto error; VIR_FREE(p); - /* def->src.path must be [vdiname] or [host]:[port]:[vdiname] */ - port = strchr(def->src.path, ':'); + /* def->src->path must be [vdiname] or [host]:[port]:[vdiname] */ + port = strchr(def->src->path, ':'); if (port) { *port = '\0'; vdi = strchr(port + 1, ':'); @@ -9762,26 +9764,26 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, *port = ':'; virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse sheepdog filename '%s'"), - def->src.path); + def->src->path); goto error; } port++; *vdi++ = '\0'; - if (VIR_ALLOC(def->src.hosts) < 0) + if (VIR_ALLOC(def->src->hosts) < 0) goto error; - def->src.nhosts = 1; - def->src.hosts->name = def->src.path; - if (VIR_STRDUP(def->src.hosts->port, port) < 0) + def->src->nhosts = 1; + def->src->hosts->name = def->src->path; + if (VIR_STRDUP(def->src->hosts->port, port) < 0) goto error; - def->src.hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - def->src.hosts->socket = NULL; - if (VIR_STRDUP(def->src.path, vdi) < 0) + def->src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + def->src->hosts->socket = NULL; + if (VIR_STRDUP(def->src->path, vdi) < 0) goto error; } } else - def->src.type = VIR_STORAGE_TYPE_FILE; + def->src->type = VIR_STORAGE_TYPE_FILE; } else { - def->src.type = VIR_STORAGE_TYPE_FILE; + def->src->type = VIR_STORAGE_TYPE_FILE; } } else if (STREQ(keywords[i], "if")) { if (STREQ(values[i], "ide")) { @@ -9807,9 +9809,9 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, } else if (STREQ(values[i], "floppy")) def->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; } else if (STREQ(keywords[i], "format")) { - if (VIR_STRDUP(def->src.driverName, "qemu") < 0) + if (VIR_STRDUP(def->src->driverName, "qemu") < 0) goto error; - def->src.format = virStorageFileFormatTypeFromString(values[i]); + def->src->format = virStorageFileFormatTypeFromString(values[i]); } else if (STREQ(keywords[i], "cache")) { if (STREQ(values[i], "off") || STREQ(values[i], "none")) @@ -9914,9 +9916,9 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, if (def->rerror_policy == def->error_policy) def->rerror_policy = 0; - if (!def->src.path && + if (!def->src->path && def->device == VIR_DOMAIN_DISK_DEVICE_DISK && - def->src.type != VIR_STORAGE_TYPE_NETWORK) { + def->src->type != VIR_STORAGE_TYPE_NETWORK) { virReportError(VIR_ERR_INTERNAL_ERROR, _("missing file parameter in drive '%s'"), val); goto error; @@ -11001,23 +11003,23 @@ qemuParseCommandLine(virCapsPtr qemuCaps, goto error; if (STRPREFIX(val, "/dev/")) - disk->src.type = VIR_STORAGE_TYPE_BLOCK; + disk->src->type = VIR_STORAGE_TYPE_BLOCK; else if (STRPREFIX(val, "nbd:")) { - disk->src.type = VIR_STORAGE_TYPE_NETWORK; - disk->src.protocol = VIR_STORAGE_NET_PROTOCOL_NBD; + disk->src->type = VIR_STORAGE_TYPE_NETWORK; + disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_NBD; } else if (STRPREFIX(val, "rbd:")) { - disk->src.type = VIR_STORAGE_TYPE_NETWORK; - disk->src.protocol = VIR_STORAGE_NET_PROTOCOL_RBD; + disk->src->type = VIR_STORAGE_TYPE_NETWORK; + disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_RBD; val += strlen("rbd:"); } else if (STRPREFIX(val, "gluster")) { - disk->src.type = VIR_STORAGE_TYPE_NETWORK; - disk->src.protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER; + disk->src->type = VIR_STORAGE_TYPE_NETWORK; + disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER; } else if (STRPREFIX(val, "sheepdog:")) { - disk->src.type = VIR_STORAGE_TYPE_NETWORK; - disk->src.protocol = VIR_STORAGE_NET_PROTOCOL_SHEEPDOG; + disk->src->type = VIR_STORAGE_TYPE_NETWORK; + disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_SHEEPDOG; val += strlen("sheepdog:"); } else - disk->src.type = VIR_STORAGE_TYPE_FILE; + disk->src->type = VIR_STORAGE_TYPE_FILE; if (STREQ(arg, "-cdrom")) { disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM; if (((def->os.arch == VIR_ARCH_PPC64) && @@ -11043,13 +11045,13 @@ qemuParseCommandLine(virCapsPtr qemuCaps, if (VIR_STRDUP(disk->dst, arg + 1) < 0) goto error; } - if (VIR_STRDUP(disk->src.path, val) < 0) + if (VIR_STRDUP(disk->src->path, val) < 0) goto error; - if (disk->src.type == VIR_STORAGE_TYPE_NETWORK) { + if (disk->src->type == VIR_STORAGE_TYPE_NETWORK) { char *port; - switch ((virStorageNetProtocol) disk->src.protocol) { + switch ((virStorageNetProtocol) disk->src->protocol) { case VIR_STORAGE_NET_PROTOCOL_NBD: if (qemuParseNBDString(disk) < 0) goto error; @@ -11061,7 +11063,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, break; case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: /* disk->src must be [vdiname] or [host]:[port]:[vdiname] */ - port = strchr(disk->src.path, ':'); + port = strchr(disk->src->path, ':'); if (port) { char *vdi; @@ -11073,13 +11075,13 @@ qemuParseCommandLine(virCapsPtr qemuCaps, goto error; } *vdi++ = '\0'; - if (VIR_ALLOC(disk->src.hosts) < 0) + if (VIR_ALLOC(disk->src->hosts) < 0) goto error; - disk->src.nhosts = 1; - disk->src.hosts->name = disk->src.path; - if (VIR_STRDUP(disk->src.hosts->port, port) < 0) + disk->src->nhosts = 1; + disk->src->hosts->name = disk->src->path; + if (VIR_STRDUP(disk->src->hosts->port, port) < 0) goto error; - if (VIR_STRDUP(disk->src.path, vdi) < 0) + if (VIR_STRDUP(disk->src->path, vdi) < 0) goto error; } break; @@ -11299,12 +11301,12 @@ qemuParseCommandLine(virCapsPtr qemuCaps, } else if (STRPREFIX(val, "disk:")) { if (!(disk = virDomainDiskDefNew())) goto error; - if (VIR_STRDUP(disk->src.path, val + strlen("disk:")) < 0) + if (VIR_STRDUP(disk->src->path, val + strlen("disk:")) < 0) goto error; - if (STRPREFIX(disk->src.path, "/dev/")) - disk->src.type = VIR_STORAGE_TYPE_BLOCK; + if (STRPREFIX(disk->src->path, "/dev/")) + disk->src->type = VIR_STORAGE_TYPE_BLOCK; else - disk->src.type = VIR_STORAGE_TYPE_FILE; + disk->src->type = VIR_STORAGE_TYPE_FILE; disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; disk->bus = VIR_DOMAIN_DISK_BUS_USB; disk->removable = VIR_DOMAIN_FEATURE_STATE_DEFAULT; @@ -11535,8 +11537,8 @@ qemuParseCommandLine(virCapsPtr qemuCaps, char *hosts, *port, *saveptr = NULL, *token; virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0; i < def->ndisks; i++) { - if (def->disks[i]->src.type == VIR_STORAGE_TYPE_NETWORK && - def->disks[i]->src.protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { + if (def->disks[i]->src->type == VIR_STORAGE_TYPE_NETWORK && + def->disks[i]->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { first_rbd_disk = def->disks[i]; break; } @@ -11556,11 +11558,11 @@ qemuParseCommandLine(virCapsPtr qemuCaps, } if (VIR_STRDUP(hosts, strchr(ceph_args, ' ') + 1) < 0) goto error; - first_rbd_disk->src.nhosts = 0; + first_rbd_disk->src->nhosts = 0; token = strtok_r(hosts, ",", &saveptr); while (token != NULL) { - if (VIR_REALLOC_N(first_rbd_disk->src.hosts, - first_rbd_disk->src.nhosts + 1) < 0) { + if (VIR_REALLOC_N(first_rbd_disk->src->hosts, + first_rbd_disk->src->nhosts + 1) < 0) { VIR_FREE(hosts); goto error; } @@ -11572,21 +11574,21 @@ qemuParseCommandLine(virCapsPtr qemuCaps, goto error; } } - first_rbd_disk->src.hosts[first_rbd_disk->src.nhosts].port = port; - if (VIR_STRDUP(first_rbd_disk->src.hosts[first_rbd_disk->src.nhosts].name, + first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].port = port; + if (VIR_STRDUP(first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].name, token) < 0) { VIR_FREE(hosts); goto error; } - first_rbd_disk->src.hosts[first_rbd_disk->src.nhosts].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - first_rbd_disk->src.hosts[first_rbd_disk->src.nhosts].socket = NULL; + first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].socket = NULL; - first_rbd_disk->src.nhosts++; + first_rbd_disk->src->nhosts++; token = strtok_r(NULL, ",", &saveptr); } VIR_FREE(hosts); - if (first_rbd_disk->src.nhosts == 0) { + if (first_rbd_disk->src->nhosts == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("found no rbd hosts in CEPH_ARGS '%s'"), ceph_args); goto error; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index c14d700..8a3bdef 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -802,8 +802,8 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices, virReportError(VIR_ERR_OPERATION_INVALID, _("sgio of shared disk 'pool=%s' 'volume=%s' conflicts " "with other active domains"), - disk->src.srcpool->pool, - disk->src.srcpool->volume); + disk->src->srcpool->pool, + disk->src->srcpool->volume); } else { virReportError(VIR_ERR_OPERATION_INVALID, _("sgio of shared disk '%s' conflicts with other " @@ -1163,33 +1163,33 @@ qemuAddISCSIPoolSourceHost(virDomainDiskDefPtr def, } /* iscsi pool only supports one host */ - def->src.nhosts = 1; + def->src->nhosts = 1; - if (VIR_ALLOC_N(def->src.hosts, def->src.nhosts) < 0) + if (VIR_ALLOC_N(def->src->hosts, def->src->nhosts) < 0) goto cleanup; - if (VIR_STRDUP(def->src.hosts[0].name, pooldef->source.hosts[0].name) < 0) + if (VIR_STRDUP(def->src->hosts[0].name, pooldef->source.hosts[0].name) < 0) goto cleanup; - if (virAsprintf(&def->src.hosts[0].port, "%d", + if (virAsprintf(&def->src->hosts[0].port, "%d", pooldef->source.hosts[0].port ? pooldef->source.hosts[0].port : 3260) < 0) goto cleanup; /* iscsi volume has name like "unit:0:0:1" */ - if (!(tokens = virStringSplit(def->src.srcpool->volume, ":", 0))) + if (!(tokens = virStringSplit(def->src->srcpool->volume, ":", 0))) goto cleanup; if (virStringListLength(tokens) != 4) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected iscsi volume name '%s'"), - def->src.srcpool->volume); + def->src->srcpool->volume); goto cleanup; } /* iscsi pool has only one source device path */ - if (virAsprintf(&def->src.path, "%s/%s", + if (virAsprintf(&def->src->path, "%s/%s", pooldef->source.devices[0].path, tokens[3]) < 0) goto cleanup; @@ -1197,10 +1197,10 @@ qemuAddISCSIPoolSourceHost(virDomainDiskDefPtr def, /* Storage pool have not supported these 2 attributes yet, * use the defaults. */ - def->src.hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - def->src.hosts[0].socket = NULL; + def->src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + def->src->hosts[0].socket = NULL; - def->src.protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; + def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; ret = 0; @@ -1225,34 +1225,34 @@ qemuTranslateDiskSourcePoolAuth(virDomainDiskDefPtr def, * into the virDomainDiskDef */ if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_CHAP) { - if (VIR_STRDUP(def->src.auth.username, + if (VIR_STRDUP(def->src->auth.username, pooldef->source.auth.chap.username) < 0) goto cleanup; if (pooldef->source.auth.chap.secret.uuidUsable) { - def->src.auth.secretType = VIR_STORAGE_SECRET_TYPE_UUID; - memcpy(def->src.auth.secret.uuid, + def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_UUID; + memcpy(def->src->auth.secret.uuid, pooldef->source.auth.chap.secret.uuid, VIR_UUID_BUFLEN); } else { - if (VIR_STRDUP(def->src.auth.secret.usage, + if (VIR_STRDUP(def->src->auth.secret.usage, pooldef->source.auth.chap.secret.usage) < 0) goto cleanup; - def->src.auth.secretType = VIR_STORAGE_SECRET_TYPE_USAGE; + def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_USAGE; } } else if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_CEPHX) { - if (VIR_STRDUP(def->src.auth.username, + if (VIR_STRDUP(def->src->auth.username, pooldef->source.auth.cephx.username) < 0) goto cleanup; if (pooldef->source.auth.cephx.secret.uuidUsable) { - def->src.auth.secretType = VIR_STORAGE_SECRET_TYPE_UUID; - memcpy(def->src.auth.secret.uuid, + def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_UUID; + memcpy(def->src->auth.secret.uuid, pooldef->source.auth.cephx.secret.uuid, VIR_UUID_BUFLEN); } else { - if (VIR_STRDUP(def->src.auth.secret.usage, + if (VIR_STRDUP(def->src->auth.secret.usage, pooldef->source.auth.cephx.secret.usage) < 0) goto cleanup; - def->src.auth.secretType = VIR_STORAGE_SECRET_TYPE_USAGE; + def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_USAGE; } } ret = 0; @@ -1274,24 +1274,24 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, int ret = -1; virErrorPtr savedError = NULL; - if (def->src.type != VIR_STORAGE_TYPE_VOLUME) + if (def->src->type != VIR_STORAGE_TYPE_VOLUME) return 0; - if (!def->src.srcpool) + if (!def->src->srcpool) return 0; - if (!(pool = virStoragePoolLookupByName(conn, def->src.srcpool->pool))) + if (!(pool = virStoragePoolLookupByName(conn, def->src->srcpool->pool))) return -1; if (virStoragePoolIsActive(pool) != 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("storage pool '%s' containing volume '%s' " "is not active"), - def->src.srcpool->pool, def->src.srcpool->volume); + def->src->srcpool->pool, def->src->srcpool->volume); goto cleanup; } - if (!(vol = virStorageVolLookupByName(pool, def->src.srcpool->volume))) + if (!(vol = virStorageVolLookupByName(pool, def->src->srcpool->volume))) goto cleanup; if (virStorageVolGetInfo(vol, &info) < 0) @@ -1303,19 +1303,19 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, if (!(pooldef = virStoragePoolDefParseString(poolxml))) goto cleanup; - def->src.srcpool->pooltype = pooldef->type; - def->src.srcpool->voltype = info.type; + def->src->srcpool->pooltype = pooldef->type; + def->src->srcpool->voltype = info.type; - if (def->src.srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI) { + if (def->src->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI) { virReportError(VIR_ERR_XML_ERROR, "%s", _("disk source mode is only valid when " "storage pool is of iscsi type")); goto cleanup; } - VIR_FREE(def->src.path); - virStorageNetHostDefFree(def->src.nhosts, def->src.hosts); - virStorageSourceAuthClear(&def->src); + VIR_FREE(def->src->path); + virStorageNetHostDefFree(def->src->nhosts, def->src->hosts); + virStorageSourceAuthClear(def->src); switch ((virStoragePoolType) pooldef->type) { case VIR_STORAGE_POOL_DIR: @@ -1324,7 +1324,7 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, case VIR_STORAGE_POOL_LOGICAL: case VIR_STORAGE_POOL_DISK: case VIR_STORAGE_POOL_SCSI: - if (!(def->src.path = virStorageVolGetPath(vol))) + if (!(def->src->path = virStorageVolGetPath(vol))) goto cleanup; if (def->startupPolicy && info.type != VIR_STORAGE_VOL_FILE) { @@ -1337,15 +1337,15 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, switch (info.type) { case VIR_STORAGE_VOL_FILE: - def->src.srcpool->actualtype = VIR_STORAGE_TYPE_FILE; + def->src->srcpool->actualtype = VIR_STORAGE_TYPE_FILE; break; case VIR_STORAGE_VOL_DIR: - def->src.srcpool->actualtype = VIR_STORAGE_TYPE_DIR; + def->src->srcpool->actualtype = VIR_STORAGE_TYPE_DIR; break; case VIR_STORAGE_VOL_BLOCK: - def->src.srcpool->actualtype = VIR_STORAGE_TYPE_BLOCK; + def->src->srcpool->actualtype = VIR_STORAGE_TYPE_BLOCK; break; case VIR_STORAGE_VOL_NETWORK: @@ -1368,20 +1368,20 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, goto cleanup; } - switch (def->src.srcpool->mode) { + switch (def->src->srcpool->mode) { case VIR_STORAGE_SOURCE_POOL_MODE_DEFAULT: case VIR_STORAGE_SOURCE_POOL_MODE_LAST: - def->src.srcpool->mode = VIR_STORAGE_SOURCE_POOL_MODE_HOST; + def->src->srcpool->mode = VIR_STORAGE_SOURCE_POOL_MODE_HOST; /* fallthrough */ case VIR_STORAGE_SOURCE_POOL_MODE_HOST: - def->src.srcpool->actualtype = VIR_STORAGE_TYPE_BLOCK; - if (!(def->src.path = virStorageVolGetPath(vol))) + def->src->srcpool->actualtype = VIR_STORAGE_TYPE_BLOCK; + if (!(def->src->path = virStorageVolGetPath(vol))) goto cleanup; break; case VIR_STORAGE_SOURCE_POOL_MODE_DIRECT: - def->src.srcpool->actualtype = VIR_STORAGE_TYPE_NETWORK; - def->src.protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; + def->src->srcpool->actualtype = VIR_STORAGE_TYPE_NETWORK; + def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0) goto cleanup; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bbe32a0..ccdface 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2258,7 +2258,7 @@ qemuDiskChainCheckBroken(virDomainDiskDefPtr disk) if (!virDomainDiskGetSource(disk)) return 0; - if (virStorageFileChainGetBroken(&disk->src, &brokenFile) < 0) + if (virStorageFileChainGetBroken(disk->src, &brokenFile) < 0) return -1; if (brokenFile) { @@ -2286,7 +2286,7 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainDiskDefPtr disk = vm->def->disks[idx]; const char *path = virDomainDiskGetSource(disk); virStorageFileFormat format = virDomainDiskGetFormat(disk); - virStorageType type = virStorageSourceGetActualType(&disk->src); + virStorageType type = virStorageSourceGetActualType(disk->src); if (!path) continue; @@ -2428,22 +2428,22 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, int ret = 0; uid_t uid; gid_t gid; - int type = virStorageSourceGetActualType(&disk->src); + int type = virStorageSourceGetActualType(disk->src); if (type != VIR_STORAGE_TYPE_NETWORK && - !disk->src.path) + !disk->src->path) goto cleanup; - if (disk->src.backingStore) { + if (disk->src->backingStore) { if (force) - virStorageSourceClearBackingStore(&disk->src); + virStorageSourceClearBackingStore(disk->src); else goto cleanup; } qemuDomainGetImageIds(cfg, vm, disk, &uid, &gid); - if (virStorageFileGetMetadata(&disk->src, + if (virStorageFileGetMetadata(disk->src, uid, gid, cfg->allowDiskFormatProbing) < 0) ret = -1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e79ac09..32c5a09 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6917,18 +6917,18 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, * Update 'orig' * We allow updating src/type//driverType/cachemode/ */ - VIR_FREE(orig->src.path); - orig->src.path = disk->src.path; - orig->src.type = disk->src.type; + VIR_FREE(orig->src->path); + orig->src->path = disk->src->path; + orig->src->type = disk->src->type; orig->cachemode = disk->cachemode; - if (disk->src.driverName) { - VIR_FREE(orig->src.driverName); - orig->src.driverName = disk->src.driverName; - disk->src.driverName = NULL; + if (disk->src->driverName) { + VIR_FREE(orig->src->driverName); + orig->src->driverName = disk->src->driverName; + disk->src->driverName = NULL; } - if (disk->src.format) - orig->src.format = disk->src.format; - disk->src.path = NULL; + if (disk->src->format) + orig->src->format = disk->src->format; + disk->src->path = NULL; break; case VIR_DOMAIN_DEVICE_NET: @@ -9506,8 +9506,8 @@ qemuDomainBlockResize(virDomainPtr dom, /* qcow2 and qed must be sized on 512 byte blocks/sectors, * so adjust size if necessary to round up. */ - if (disk->src.format == VIR_STORAGE_FILE_QCOW2 || - disk->src.format == VIR_STORAGE_FILE_QED) + if (disk->src->format == VIR_STORAGE_FILE_QCOW2 || + disk->src->format == VIR_STORAGE_FILE_QED) size = VIR_ROUND_UP(size, 512); if (virAsprintf(&device, "%s%s", QEMU_DRIVE_HOST_PREFIX, @@ -12076,25 +12076,27 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, int ret = -1; /* XXX Labelling of non-local files isn't currently supported */ - if (virStorageSourceGetActualType(&disk->src) == VIR_STORAGE_TYPE_NETWORK) + if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_NETWORK) return 0; cfg = virQEMUDriverGetConfig(driver); - /* XXX This would be easier when disk->src will be a pointer */ - memcpy(&origdisk, &disk->src, sizeof(origdisk)); - memcpy(&disk->src, elem, sizeof(*elem)); + /* XXX Need to refactor the security manager and lock daemon to + * operate directly on a virStorageSourcePtr plus tidbits rather + * than a full virDomainDiskDef. */ + memcpy(&origdisk, disk->src, sizeof(origdisk)); + memcpy(disk->src, elem, sizeof(*elem)); disk->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; if (mode == VIR_DISK_CHAIN_NO_ACCESS) { if (virSecurityManagerRestoreImageLabel(driver->securityManager, vm->def, disk) < 0) - VIR_WARN("Unable to restore security label on %s", disk->src.path); + VIR_WARN("Unable to restore security label on %s", disk->src->path); if (qemuTeardownDiskCgroup(vm, disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", - disk->src.path); + disk->src->path); if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) - VIR_WARN("Unable to release lock on %s", disk->src.path); + VIR_WARN("Unable to release lock on %s", disk->src->path); } else if (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, disk) < 0 || qemuSetupDiskCgroup(vm, disk) < 0 || @@ -12106,7 +12108,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, ret = 0; cleanup: - memcpy(&disk->src, &origdisk, sizeof(origdisk)); + memcpy(disk->src, &origdisk, sizeof(origdisk)); disk->readonly = origreadonly; virObjectUnref(cfg); return ret; @@ -12267,22 +12269,22 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, NULL))) goto cleanup; - if (defdisk->src.format > 0) { + if (defdisk->src->format > 0) { /* adds cmd line arg: backing_file=/path/to/backing/file,backing_fmd=format */ virCommandAddArgFormat(cmd, "backing_file=%s,backing_fmt=%s", - defdisk->src.path, - virStorageFileFormatTypeToString(defdisk->src.format)); + defdisk->src->path, + virStorageFileFormatTypeToString(defdisk->src->format)); } else { if (!cfg->allowDiskFormatProbing) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown image format of '%s' and " "format probing is disabled"), - defdisk->src.path); + defdisk->src->path); goto cleanup; } /* adds cmd line arg: backing_file=/path/to/backing/file */ - virCommandAddArgFormat(cmd, "backing_file=%s", defdisk->src.path); + virCommandAddArgFormat(cmd, "backing_file=%s", defdisk->src->path); } /* adds cmd line args: /path/to/target/file */ @@ -12305,12 +12307,12 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, defdisk = vm->def->disks[snapdisk->index]; if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { - VIR_FREE(defdisk->src.path); - if (VIR_STRDUP(defdisk->src.path, snapdisk->src->path) < 0) { + VIR_FREE(defdisk->src->path); + 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; } } @@ -12425,7 +12427,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn, static int qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) { - int actualType = virStorageSourceGetActualType(&disk->src); + int actualType = virStorageSourceGetActualType(disk->src); switch ((virStorageType) actualType) { case VIR_STORAGE_TYPE_BLOCK: @@ -12433,7 +12435,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) return 0; case VIR_STORAGE_TYPE_NETWORK: - switch ((virStorageNetProtocol) disk->src.protocol) { + switch ((virStorageNetProtocol) disk->src->protocol) { case VIR_STORAGE_NET_PROTOCOL_NONE: case VIR_STORAGE_NET_PROTOCOL_NBD: case VIR_STORAGE_NET_PROTOCOL_RBD: @@ -12449,7 +12451,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) virReportError(VIR_ERR_INTERNAL_ERROR, _("external inactive snapshots are not supported on " "'network' disks using '%s' protocol"), - virStorageNetProtocolTypeToString(disk->src.protocol)); + virStorageNetProtocolTypeToString(disk->src->protocol)); return -1; } break; @@ -12471,7 +12473,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) static int qemuDomainSnapshotPrepareDiskExternalBackingActive(virDomainDiskDefPtr disk) { - int actualType = virStorageSourceGetActualType(&disk->src); + int actualType = virStorageSourceGetActualType(disk->src); if (actualType == VIR_STORAGE_TYPE_BLOCK && disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { @@ -12634,7 +12636,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, if (qemuTranslateDiskSourcePool(conn, disk) < 0) return -1; - actualType = virStorageSourceGetActualType(&disk->src); + actualType = virStorageSourceGetActualType(disk->src); switch ((virStorageType) actualType) { case VIR_STORAGE_TYPE_BLOCK: @@ -12642,7 +12644,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, return 0; case VIR_STORAGE_TYPE_NETWORK: - switch ((virStorageNetProtocol) disk->src.protocol) { + switch ((virStorageNetProtocol) disk->src->protocol) { case VIR_STORAGE_NET_PROTOCOL_NONE: case VIR_STORAGE_NET_PROTOCOL_NBD: case VIR_STORAGE_NET_PROTOCOL_RBD: @@ -12658,7 +12660,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, virReportError(VIR_ERR_INTERNAL_ERROR, _("internal inactive snapshots are not supported on " "'network' disks using '%s' protocol"), - virStorageNetProtocolTypeToString(disk->src.protocol)); + virStorageNetProtocolTypeToString(disk->src->protocol)); return -1; } break; @@ -12720,19 +12722,19 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, active) < 0) goto cleanup; - if (dom_disk->src.type == VIR_STORAGE_TYPE_NETWORK && - (dom_disk->src.protocol == VIR_STORAGE_NET_PROTOCOL_SHEEPDOG || - dom_disk->src.protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) { + if (dom_disk->src->type == VIR_STORAGE_TYPE_NETWORK && + (dom_disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_SHEEPDOG || + dom_disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) { break; } - if (vm->def->disks[i]->src.format > 0 && - vm->def->disks[i]->src.format != VIR_STORAGE_FILE_QCOW2) { + if (vm->def->disks[i]->src->format > 0 && + vm->def->disks[i]->src->format != VIR_STORAGE_FILE_QCOW2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("internal snapshot for disk %s unsupported " "for storage type %s"), disk->name, virStorageFileFormatTypeToString( - vm->def->disks[i]->src.format)); + vm->def->disks[i]->src->format)); goto cleanup; } break; @@ -12868,7 +12870,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, * 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); + virStorageSourceClearBackingStore(disk->src); if (virStorageFileInit(snap->src) < 0) goto cleanup; @@ -12962,37 +12964,37 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } } - virDomainAuditDisk(vm, disk->src.path, source, "snapshot", ret >= 0); + virDomainAuditDisk(vm, disk->src->path, source, "snapshot", ret >= 0); if (ret < 0) goto cleanup; /* Update vm in place to match changes. */ need_unlink = false; - VIR_FREE(disk->src.path); - virStorageNetHostDefFree(disk->src.nhosts, disk->src.hosts); + VIR_FREE(disk->src->path); + virStorageNetHostDefFree(disk->src->nhosts, disk->src->hosts); - 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.hosts = newhosts; + 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->hosts = newhosts; newsource = NULL; newhosts = NULL; if (persistDisk) { - VIR_FREE(persistDisk->src.path); - virStorageNetHostDefFree(persistDisk->src.nhosts, - persistDisk->src.hosts); + VIR_FREE(persistDisk->src->path); + virStorageNetHostDefFree(persistDisk->src->nhosts, + persistDisk->src->hosts); - 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.hosts = persistHosts; + 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->hosts = persistHosts; persistSource = NULL; persistHosts = NULL; @@ -13026,46 +13028,46 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, char *persistSource = NULL; struct stat st; - ignore_value(virStorageFileInit(&disk->src)); + ignore_value(virStorageFileInit(disk->src)); - if (VIR_STRDUP(source, origdisk->src.path) < 0 || + if (VIR_STRDUP(source, origdisk->src->path) < 0 || (persistDisk && VIR_STRDUP(persistSource, source) < 0)) goto cleanup; - qemuDomainPrepareDiskChainElement(driver, vm, disk, &disk->src, + qemuDomainPrepareDiskChainElement(driver, vm, disk, disk->src, VIR_DISK_CHAIN_NO_ACCESS); if (need_unlink && - virStorageFileStat(&disk->src, &st) == 0 && S_ISREG(st.st_mode) && - virStorageFileUnlink(&disk->src) < 0) - VIR_WARN("Unable to remove just-created %s", disk->src.path); + virStorageFileStat(disk->src, &st) == 0 && S_ISREG(st.st_mode) && + virStorageFileUnlink(disk->src) < 0) + VIR_WARN("Unable to remove just-created %s", disk->src->path); /* Update vm in place to match changes. */ - VIR_FREE(disk->src.path); - disk->src.path = source; + VIR_FREE(disk->src->path); + disk->src->path = source; source = NULL; - disk->src.format = origdisk->src.format; - disk->src.type = origdisk->src.type; - disk->src.protocol = origdisk->src.protocol; - virStorageNetHostDefFree(disk->src.nhosts, disk->src.hosts); - disk->src.nhosts = origdisk->src.nhosts; - disk->src.hosts = virStorageNetHostDefCopy(origdisk->src.nhosts, - origdisk->src.hosts); + disk->src->format = origdisk->src->format; + disk->src->type = origdisk->src->type; + disk->src->protocol = origdisk->src->protocol; + virStorageNetHostDefFree(disk->src->nhosts, disk->src->hosts); + disk->src->nhosts = origdisk->src->nhosts; + disk->src->hosts = virStorageNetHostDefCopy(origdisk->src->nhosts, + origdisk->src->hosts); if (persistDisk) { - VIR_FREE(persistDisk->src.path); - persistDisk->src.path = persistSource; + VIR_FREE(persistDisk->src->path); + persistDisk->src->path = persistSource; persistSource = NULL; - persistDisk->src.format = origdisk->src.format; - persistDisk->src.type = origdisk->src.type; - persistDisk->src.protocol = origdisk->src.protocol; - virStorageNetHostDefFree(persistDisk->src.nhosts, - persistDisk->src.hosts); - persistDisk->src.nhosts = origdisk->src.nhosts; - persistDisk->src.hosts = virStorageNetHostDefCopy(origdisk->src.nhosts, - origdisk->src.hosts); + persistDisk->src->format = origdisk->src->format; + persistDisk->src->type = origdisk->src->type; + persistDisk->src->protocol = origdisk->src->protocol; + virStorageNetHostDefFree(persistDisk->src->nhosts, + persistDisk->src->hosts); + persistDisk->src->nhosts = origdisk->src->nhosts; + persistDisk->src->hosts = virStorageNetHostDefCopy(origdisk->src->nhosts, + origdisk->src->hosts); } cleanup: - virStorageFileDeinit(&disk->src); + virStorageFileDeinit(disk->src); VIR_FREE(source); VIR_FREE(persistSource); } @@ -14929,16 +14931,16 @@ qemuDomainBlockPivot(virConnectPtr conn, * label the entire chain. This action is safe even if the * backing chain has already been labeled; but only necessary when * we know for sure that there is a backing chain. */ - oldsrc = disk->src.path; - oldformat = disk->src.format; - oldchain = disk->src.backingStore; - disk->src.path = disk->mirror; - disk->src.format = disk->mirrorFormat; - disk->src.backingStore = NULL; + oldsrc = disk->src->path; + oldformat = disk->src->format; + oldchain = disk->src->backingStore; + disk->src->path = disk->mirror; + disk->src->format = disk->mirrorFormat; + disk->src->backingStore = NULL; if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) { - disk->src.path = oldsrc; - disk->src.format = oldformat; - disk->src.backingStore = oldchain; + disk->src->path = oldsrc; + disk->src->format = oldformat; + disk->src->backingStore = oldchain; goto cleanup; } if (disk->mirrorFormat && disk->mirrorFormat != VIR_STORAGE_FILE_RAW && @@ -14947,9 +14949,9 @@ qemuDomainBlockPivot(virConnectPtr conn, qemuSetupDiskCgroup(vm, disk) < 0 || virSecurityManagerSetImageLabel(driver->securityManager, vm->def, disk) < 0)) { - disk->src.path = oldsrc; - disk->src.format = oldformat; - disk->src.backingStore = oldchain; + disk->src->path = oldsrc; + disk->src->format = oldformat; + disk->src->backingStore = oldchain; goto cleanup; } @@ -14978,10 +14980,10 @@ qemuDomainBlockPivot(virConnectPtr conn, * 'query-block', to see what state we really got left in * before killing the mirroring job? And just as on the * success case, there's security labeling to worry about. */ - disk->src.path = oldsrc; - disk->src.format = oldformat; - virStorageSourceFree(disk->src.backingStore); - disk->src.backingStore = oldchain; + disk->src->path = oldsrc; + disk->src->format = oldformat; + virStorageSourceFree(disk->src->backingStore); + disk->src->backingStore = oldchain; VIR_FREE(disk->mirror); } disk->mirrorFormat = VIR_STORAGE_FILE_NONE; @@ -15087,8 +15089,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, if (base && (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 || - !(baseSource = virStorageFileChainLookup(&disk->src, - disk->src.backingStore, + !(baseSource = virStorageFileChainLookup(disk->src, + disk->src->backingStore, base, baseIndex, NULL)))) goto endjob; @@ -15125,7 +15127,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, if (!async) { int type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL; int status = VIR_DOMAIN_BLOCK_JOB_CANCELED; - event = virDomainEventBlockJobNewFromObj(vm, disk->src.path, type, + event = virDomainEventBlockJobNewFromObj(vm, disk->src->path, type, status); } else if (!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { while (1) { @@ -15298,7 +15300,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) && STREQ_NULLABLE(format, "raw") && - disk->src.backingStore->path) { + disk->src->backingStore->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' has backing file, so raw shallow copy " "is not possible"), @@ -15334,7 +15336,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, goto endjob; VIR_FORCE_CLOSE(fd); if (!format) - disk->mirrorFormat = disk->src.format; + disk->mirrorFormat = disk->src->format; } else if (format) { disk->mirrorFormat = virStorageFileFormatTypeFromString(format); if (disk->mirrorFormat <= 0) { @@ -15498,7 +15500,7 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; disk = vm->def->disks[idx]; - if (!disk->src.path) { + if (!disk->src->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk %s has no source file to be committed"), disk->dst); @@ -15508,10 +15510,10 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; if (!top) - topSource = &disk->src; + topSource = disk->src; else if (virStorageFileParseChainIndex(disk->dst, top, &topIndex) < 0 || - !(topSource = virStorageFileChainLookup(&disk->src, - disk->src.backingStore, + !(topSource = virStorageFileChainLookup(disk->src, + disk->src->backingStore, top, topIndex, &top_parent))) goto endjob; @@ -15519,7 +15521,7 @@ qemuDomainBlockCommit(virDomainPtr dom, /* FIXME: qemu 2.0 supports active commit, but as a two-stage * process; qemu 2.1 is further improving active commit. We need * to start supporting it in libvirt. */ - if (topSource == &disk->src) { + if (topSource == disk->src) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("committing the active layer not supported yet")); goto endjob; @@ -15535,7 +15537,7 @@ qemuDomainBlockCommit(virDomainPtr dom, if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) baseSource = topSource->backingStore; else if (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 || - !(baseSource = virStorageFileChainLookup(&disk->src, topSource, + !(baseSource = virStorageFileChainLookup(disk->src, topSource, base, baseIndex, NULL))) goto endjob; @@ -15558,7 +15560,7 @@ qemuDomainBlockCommit(virDomainPtr dom, clean_access = true; if (qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource, VIR_DISK_CHAIN_READ_WRITE) < 0 || - (top_parent && top_parent != disk->src.path && + (top_parent && top_parent != disk->src->path && qemuDomainPrepareDiskChainElementPath(driver, vm, disk, top_parent, VIR_DISK_CHAIN_READ_WRITE) < 0)) @@ -15581,7 +15583,7 @@ qemuDomainBlockCommit(virDomainPtr dom, /* Revert access to read-only, if possible. */ qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource, VIR_DISK_CHAIN_READ_ONLY); - if (top_parent && top_parent != disk->src.path) + if (top_parent && top_parent != disk->src->path) qemuDomainPrepareDiskChainElementPath(driver, vm, disk, top_parent, VIR_DISK_CHAIN_READ_ONLY); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5754f73..842f782 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1542,8 +1542,8 @@ qemuMigrationIsSafe(virDomainDefPtr def) return false; else if (rc == 1) continue; - } else if (disk->src.type == VIR_STORAGE_TYPE_NETWORK && - disk->src.protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { + } else if (disk->src->type == VIR_STORAGE_TYPE_NETWORK && + disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { continue; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d719716..d906494 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -421,13 +421,13 @@ qemuProcessGetVolumeQcowPassphrase(virConnectPtr conn, int ret = -1; virStorageEncryptionPtr enc; - if (!disk->src.encryption) { + if (!disk->src->encryption) { virReportError(VIR_ERR_INTERNAL_ERROR, _("disk %s does not have any encryption information"), - disk->src.path); + disk->src->path); return -1; } - enc = disk->src.encryption; + enc = disk->src->encryption; if (!conn) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2234,7 +2234,7 @@ qemuProcessInitPasswords(virConnectPtr conn, size_t secretLen; const char *alias; - if (!vm->def->disks[i]->src.encryption || + if (!vm->def->disks[i]->src->encryption || !virDomainDiskGetSource(vm->def->disks[i])) continue; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 04dbfbb..8380bba 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1148,7 +1148,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, * be tracked in domain XML, at which point labelskip should be a * per-file attribute instead of a disk attribute. */ if (disk_seclabel && disk_seclabel->labelskip && - !disk->src.backingStore) + !disk->src->backingStore) return 0; /* Don't restore labels on readoly/shared disks, because @@ -1236,7 +1236,7 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, if (!disk_seclabel) return -1; disk_seclabel->labelskip = true; - if (VIR_APPEND_ELEMENT(disk->src.seclabels, disk->src.nseclabels, + if (VIR_APPEND_ELEMENT(disk->src->seclabels, disk->src->nseclabels, disk_seclabel) < 0) { virSecurityDeviceLabelDefFree(disk_seclabel); return -1; diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 047356e..88ec35a 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -169,11 +169,11 @@ testSELinuxLoadDef(const char *testname) goto cleanup; for (i = 0; i < def->ndisks; i++) { - if (def->disks[i]->src.type != VIR_STORAGE_TYPE_FILE && - def->disks[i]->src.type != VIR_STORAGE_TYPE_BLOCK) + if (def->disks[i]->src->type != VIR_STORAGE_TYPE_FILE && + def->disks[i]->src->type != VIR_STORAGE_TYPE_BLOCK) continue; - if (testSELinuxMungePath(&def->disks[i]->src.path) < 0) + if (testSELinuxMungePath(&def->disks[i]->src->path) < 0) goto cleanup; } -- 1.9.3
On 06/06/14 00:52, Eric Blake wrote:
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 domain disk source to be a pointer.
In this patch, the pointer is ALWAYS allocated (thanks in part to the previous patch forwarding all disk def allocation through a common point), and all other changse 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 a cdrom with no medium 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/domain_conf.h (_virDomainDiskDef): Change type of src. * src/conf/domain_conf.c: Adjust all clients. * src/security/security_selinux.c: Likewise. * src/qemu/qemu_domain.c: Likewise. * src/qemu/qemu_command.c: Likewise. * src/qemu/qemu_conf.c: Likewise. * src/qemu/qemu_process.c: Likewise. * src/qemu/qemu_migration.c: Likewise. * src/qemu/qemu_driver.c: Likewise. * src/lxc/lxc_driver.c: Likewise. * src/lxc/lxc_controller.c: Likewise. * tests/securityselinuxlabeltest.c: Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 141 +++++++++---------- src/conf/domain_conf.h | 2 +- src/lxc/lxc_controller.c | 8 +- src/lxc/lxc_driver.c | 8 +- src/qemu/qemu_command.c | 286 ++++++++++++++++++++------------------- src/qemu/qemu_conf.c | 86 ++++++------ src/qemu/qemu_domain.c | 14 +- src/qemu/qemu_driver.c | 232 +++++++++++++++---------------- src/qemu/qemu_migration.c | 4 +- src/qemu/qemu_process.c | 8 +- src/security/security_selinux.c | 4 +- tests/securityselinuxlabeltest.c | 6 +- 12 files changed, 403 insertions(+), 396 deletions(-)
Missing change in src/security/virt-aa-helper.c which breaks the build: diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index bf540b4..1d246c7 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -951,9 +951,9 @@ get_files(vahControl * ctl) /* XXX - if we knew the qemu user:group here we could send it in * so that the open could be re-tried as that user:group. */ - if (!disk->src.backingStore) { + if (!disk->src->backingStore) { bool probe = ctl->allowDiskFormatProbing; - virStorageFileGetMetadata(&disk->src, -1, -1, probe); + virStorageFileGetMetadata(disk->src, -1, -1, probe); } /* XXX passing ignoreOpenFailure = true to get back to the behavior
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ffe3583..3585537 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -598,7 +598,7 @@ typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr;
/* Stores the virtual disk configuration */ struct _virDomainDiskDef { - virStorageSource src; + virStorageSourcePtr src;
Maybe we should mention here that src is currently guaranteed to be non-NULL.
int device; /* enum virDomainDiskDevice */ int bus; /* enum virDomainDiskBus */
ACK with the nits addressed. Peter
The current implementation of 'virsh blockcopy' (virDomainBlockRebase) is limited to copying to a local file name. But future patches want to extend it to also copy to network disks. This patch converts over to a virStorageSourcePtr, although it should have no semantic change visible to the user, in anticipation of those future patches being able to use more fields for non-file destinations. * src/conf/domain_conf.h (_virDomainDiskDef): Change type of mirror information. * src/conf/domain_conf.c (virDomainDiskDefParseXML): Localize mirror parsing into new object. (virDomainDiskDefFormat): Adjust clients. * src/qemu/qemu_domain.c (qemuDomainDeviceDefPostParse): Likewise. * src/qemu/qemu_driver.c (qemuDomainBlockPivot) (qemuDomainBlockJobImpl, qemuDomainBlockCopy): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 50 ++++++++++++++++++++--------------------- src/conf/domain_conf.h | 3 +-- src/qemu/qemu_domain.c | 8 +++---- src/qemu/qemu_driver.c | 60 +++++++++++++++++++++++++++++++------------------ src/qemu/qemu_process.c | 15 ++++++++----- 5 files changed, 77 insertions(+), 59 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 361d087..b8e7c50 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5223,9 +5223,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *ioeventfd = NULL; char *event_idx = NULL; char *copy_on_read = NULL; - char *mirror = NULL; - char *mirrorFormat = NULL; - bool mirroring = false; char *devaddr = NULL; virStorageEncryptionPtr encryption = NULL; char *serial = NULL; @@ -5374,19 +5371,37 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, event_idx = virXMLPropString(cur, "event_idx"); copy_on_read = virXMLPropString(cur, "copy_on_read"); discard = virXMLPropString(cur, "discard"); - } else if (!mirror && xmlStrEqual(cur->name, BAD_CAST "mirror") && + } else if (!def->mirror && + xmlStrEqual(cur->name, BAD_CAST "mirror") && !(flags & VIR_DOMAIN_XML_INACTIVE)) { char *ready; - mirror = virXMLPropString(cur, "file"); - if (!mirror) { + char *mirrorFormat; + + if (VIR_ALLOC(def->mirror) < 0) + goto error; + def->mirror->type = VIR_STORAGE_TYPE_FILE; + def->mirror->path = virXMLPropString(cur, "file"); + if (!def->mirror->path) { virReportError(VIR_ERR_XML_ERROR, "%s", _("mirror requires file name")); goto error; } mirrorFormat = virXMLPropString(cur, "format"); + if (mirrorFormat) { + def->mirror->format = + virStorageFileFormatTypeFromString(mirrorFormat); + if (def->mirror->format <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown mirror format value '%s'"), + mirrorFormat); + VIR_FREE(mirrorFormat); + goto error; + } + VIR_FREE(mirrorFormat); + } ready = virXMLPropString(cur, "ready"); if (ready) { - mirroring = true; + def->mirroring = true; VIR_FREE(ready); } } else if (xmlStrEqual(cur->name, BAD_CAST "auth")) { @@ -5906,9 +5921,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, authUsername = NULL; def->src->driverName = driverName; driverName = NULL; - def->mirror = mirror; - mirror = NULL; - def->mirroring = mirroring; def->src->encryption = encryption; encryption = NULL; def->serial = serial; @@ -5930,16 +5942,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } - if (mirrorFormat) { - def->mirrorFormat = virStorageFileFormatTypeFromString(mirrorFormat); - if (def->mirrorFormat <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown mirror format value '%s'"), - driverType); - goto error; - } - } - if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDiskDefAssignAddress(xmlopt, def) < 0) goto error; @@ -5964,8 +5966,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(authUsage); VIR_FREE(driverType); VIR_FREE(driverName); - VIR_FREE(mirror); - VIR_FREE(mirrorFormat); VIR_FREE(cachetag); VIR_FREE(error_policy); VIR_FREE(rerror_policy); @@ -15160,10 +15160,10 @@ virDomainDiskDefFormat(virBufferPtr buf, * for live domains, therefore we ignore it on input except for * the internal parse on libvirtd restart. */ if (def->mirror && !(flags & VIR_DOMAIN_XML_INACTIVE)) { - virBufferEscapeString(buf, "<mirror file='%s'", def->mirror); - if (def->mirrorFormat) + virBufferEscapeString(buf, "<mirror file='%s'", def->mirror->path); + if (def->mirror->format) virBufferAsprintf(buf, " format='%s'", - virStorageFileFormatTypeToString(def->mirrorFormat)); + virStorageFileFormatTypeToString(def->mirror->format)); if (def->mirroring) virBufferAddLit(buf, " ready='yes'"); virBufferAddLit(buf, "/>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3585537..11d5c68 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -606,8 +606,7 @@ struct _virDomainDiskDef { int tray_status; /* enum virDomainDiskTray */ int removable; /* enum virDomainFeatureState */ - char *mirror; - int mirrorFormat; /* virStorageFileFormat */ + virStorageSourcePtr mirror; bool mirroring; struct { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ccdface..962698b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -887,8 +887,8 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, /* default disk format for mirrored drive */ if (disk->mirror && - disk->mirrorFormat == VIR_STORAGE_FILE_NONE) - disk->mirrorFormat = VIR_STORAGE_FILE_AUTO; + disk->mirror->format == VIR_STORAGE_FILE_NONE) + disk->mirror->format = VIR_STORAGE_FILE_AUTO; } else { /* default driver if probing is forbidden */ if (!virDomainDiskGetDriver(disk) && @@ -903,8 +903,8 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, /* default disk format for mirrored drive */ if (disk->mirror && - disk->mirrorFormat == VIR_STORAGE_FILE_NONE) - disk->mirrorFormat = VIR_STORAGE_FILE_RAW; + disk->mirror->format == VIR_STORAGE_FILE_NONE) + disk->mirror->format = VIR_STORAGE_FILE_RAW; } } } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 32c5a09..b0f80a5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14870,13 +14870,22 @@ qemuDomainBlockPivot(virConnectPtr conn, int ret = -1, rc; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainBlockJobInfo info; - const char *format = virStorageFileFormatTypeToString(disk->mirrorFormat); + const char *format = NULL; bool resume = false; char *oldsrc = NULL; int oldformat; virStorageSourcePtr oldchain = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + if (!disk->mirror) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("pivot of disk '%s' requires an active copy job"), + disk->dst); + goto cleanup; + } + + format = virStorageFileFormatTypeToString(disk->mirror->format); + /* Probe the status, if needed. */ if (!disk->mirroring) { qemuDomainObjEnterMonitor(driver, vm); @@ -14934,8 +14943,8 @@ qemuDomainBlockPivot(virConnectPtr conn, oldsrc = disk->src->path; oldformat = disk->src->format; oldchain = disk->src->backingStore; - disk->src->path = disk->mirror; - disk->src->format = disk->mirrorFormat; + disk->src->path = disk->mirror->path; + disk->src->format = disk->mirror->format; disk->src->backingStore = NULL; if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) { disk->src->path = oldsrc; @@ -14943,7 +14952,7 @@ qemuDomainBlockPivot(virConnectPtr conn, disk->src->backingStore = oldchain; goto cleanup; } - if (disk->mirrorFormat && disk->mirrorFormat != VIR_STORAGE_FILE_RAW && + if (disk->mirror->format && disk->mirror->format != VIR_STORAGE_FILE_RAW && (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, disk) < 0 || qemuSetupDiskCgroup(vm, disk) < 0 || @@ -14957,7 +14966,7 @@ qemuDomainBlockPivot(virConnectPtr conn, /* Attempt the pivot. */ qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror, format); + ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror->path, format); qemuDomainObjExitMonitor(driver, vm); if (ret == 0) { @@ -14970,7 +14979,7 @@ qemuDomainBlockPivot(virConnectPtr conn, * for now, we leak the access to the original. */ VIR_FREE(oldsrc); virStorageSourceFree(oldchain); - disk->mirror = NULL; + disk->mirror->path = NULL; } else { /* On failure, qemu abandons the mirror, and reverts back to * the source disk (RHEL 6.3 has a bug where the revert could @@ -14984,9 +14993,9 @@ qemuDomainBlockPivot(virConnectPtr conn, disk->src->format = oldformat; virStorageSourceFree(disk->src->backingStore); disk->src->backingStore = oldchain; - VIR_FREE(disk->mirror); } - disk->mirrorFormat = VIR_STORAGE_FILE_NONE; + virStorageSourceFree(disk->mirror); + disk->mirror = NULL; disk->mirroring = false; cleanup: @@ -15112,8 +15121,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, if (mode == BLOCK_JOB_ABORT && disk->mirror) { /* XXX We should also revoke security labels and disk lease on * the mirror, and audit that fact, before dropping things. */ - VIR_FREE(disk->mirror); - disk->mirrorFormat = VIR_STORAGE_FILE_NONE; + virStorageSourceFree(disk->mirror); + disk->mirror = NULL; disk->mirroring = false; } @@ -15248,7 +15257,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, int idx; struct stat st; bool need_unlink = false; - char *mirror = NULL; + virStorageSourcePtr mirror = NULL; virQEMUDriverConfigPtr cfg = NULL; /* Preliminaries: find the disk we are editing, sanity checks */ @@ -15329,6 +15338,11 @@ qemuDomainBlockCopy(virDomainObjPtr vm, goto endjob; } + if (VIR_ALLOC(mirror) < 0) + goto endjob; + /* XXX Allow non-file mirror destinations */ + mirror->type = VIR_STORAGE_TYPE_FILE; + if (!(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { int fd = qemuOpenFile(driver, vm, dest, O_WRONLY | O_TRUNC | O_CREAT, &need_unlink, NULL); @@ -15336,10 +15350,10 @@ qemuDomainBlockCopy(virDomainObjPtr vm, goto endjob; VIR_FORCE_CLOSE(fd); if (!format) - disk->mirrorFormat = disk->src->format; + mirror->format = disk->src->format; } else if (format) { - disk->mirrorFormat = virStorageFileFormatTypeFromString(format); - if (disk->mirrorFormat <= 0) { + mirror->format = virStorageFileFormatTypeFromString(format); + if (mirror->format <= 0) { virReportError(VIR_ERR_INVALID_ARG, _("unrecognized format '%s'"), format); goto endjob; @@ -15349,12 +15363,12 @@ qemuDomainBlockCopy(virDomainObjPtr vm, * also passed the RAW flag (and format is non-NULL), or it is * safe for us to probe the format from the file that we will * be using. */ - disk->mirrorFormat = virStorageFileProbeFormat(dest, cfg->user, - cfg->group); + mirror->format = virStorageFileProbeFormat(dest, cfg->user, + cfg->group); } - if (!format && disk->mirrorFormat > 0) - format = virStorageFileFormatTypeToString(disk->mirrorFormat); - if (VIR_STRDUP(mirror, dest) < 0) + if (!format && mirror->format > 0) + format = virStorageFileFormatTypeToString(mirror->format); + if (VIR_STRDUP(mirror->path, dest) < 0) goto endjob; if (qemuDomainPrepareDiskChainElementPath(driver, vm, disk, dest, @@ -15384,9 +15398,11 @@ qemuDomainBlockCopy(virDomainObjPtr vm, endjob: if (need_unlink && unlink(dest)) VIR_WARN("unable to unlink just-created %s", dest); - if (ret < 0 && disk) - disk->mirrorFormat = VIR_STORAGE_FILE_NONE; - VIR_FREE(mirror); + if (ret < 0 && disk) { + virStorageSourceFree(disk->mirror); + disk->mirror = NULL; + } + virStorageSourceFree(mirror); if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d906494..e4845ba 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1033,12 +1033,15 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT) && status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) qemuDomainDetermineDiskChain(driver, vm, disk, true); - if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY && - status == VIR_DOMAIN_BLOCK_JOB_READY) - disk->mirroring = true; - if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY && - status == VIR_DOMAIN_BLOCK_JOB_FAILED) - VIR_FREE(disk->mirror); + if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + if (status == VIR_DOMAIN_BLOCK_JOB_READY) { + disk->mirroring = true; + } else if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { + virStorageSourceFree(disk->mirror); + disk->mirror = NULL; + disk->mirroring = false; + } + } } virObjectUnlock(vm); -- 1.9.3
On 06/06/14 00:52, Eric Blake wrote:
The current implementation of 'virsh blockcopy' (virDomainBlockRebase) is limited to copying to a local file name. But future patches want to extend it to also copy to network disks. This patch converts over to a virStorageSourcePtr, although it should have no semantic change visible to the user, in anticipation of those future patches being able to use more fields for non-file destinations.
* src/conf/domain_conf.h (_virDomainDiskDef): Change type of mirror information. * src/conf/domain_conf.c (virDomainDiskDefParseXML): Localize mirror parsing into new object. (virDomainDiskDefFormat): Adjust clients. * src/qemu/qemu_domain.c (qemuDomainDeviceDefPostParse): Likewise. * src/qemu/qemu_driver.c (qemuDomainBlockPivot) (qemuDomainBlockJobImpl, qemuDomainBlockCopy): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 50 ++++++++++++++++++++--------------------- src/conf/domain_conf.h | 3 +-- src/qemu/qemu_domain.c | 8 +++---- src/qemu/qemu_driver.c | 60 +++++++++++++++++++++++++++++++------------------ src/qemu/qemu_process.c | 15 ++++++++----- 5 files changed, 77 insertions(+), 59 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3585537..11d5c68 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -606,8 +606,7 @@ struct _virDomainDiskDef { int tray_status; /* enum virDomainDiskTray */ int removable; /* enum virDomainFeatureState */
- char *mirror; - int mirrorFormat; /* virStorageFileFormat */ + virStorageSourcePtr mirror;
You adjust the type here, but don't adjust the code in virDomainDiskDefFree ...
bool mirroring;
struct {
You need to change: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b8e7c50..827c401 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1203,7 +1203,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def) virStorageSourceFree(def->src); VIR_FREE(def->serial); VIR_FREE(def->dst); - VIR_FREE(def->mirror); + virStorageSourceFree(def->mirror); VIR_FREE(def->wwn); VIR_FREE(def->vendor); VIR_FREE(def->product); ACK with that addressed, Peter
On 06/06/2014 03:25 AM, Peter Krempa wrote:
On 06/06/14 00:52, Eric Blake wrote:
The current implementation of 'virsh blockcopy' (virDomainBlockRebase) is limited to copying to a local file name. But future patches want to extend it to also copy to network disks. This patch converts over to a virStorageSourcePtr, although it should have no semantic change visible to the user, in anticipation of those future patches being able to use more fields for non-file destinations.
- char *mirror; - int mirrorFormat; /* virStorageFileFormat */ + virStorageSourcePtr mirror;
You adjust the type here, but don't adjust the code in virDomainDiskDefFree ...
bool mirroring;
struct {
You need to change:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b8e7c50..827c401 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1203,7 +1203,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def) virStorageSourceFree(def->src); VIR_FREE(def->serial); VIR_FREE(def->dst); - VIR_FREE(def->mirror); + virStorageSourceFree(def->mirror);
D'oh - I missed one (I guess I had been grep'ing for disk->mirror, rather than .*->mirror). Thanks for spotting it.
ACK with that addressed,
Findings in 3 and 4 addressed, and I've pushed through this point of the series. I realized off-list that active commit has a major difference from blockcopy: right now, libvirt refuses to do blockcopy on anything but a transient domain, so there is no impact to a persistent domain definition. But since I allowed block-commit on a persistent domain, if the transient definition is updated due to a pivot at the end of a blockcopy, the persistent definition must also be altered so that the next boot of the domain will use the right file (on a pivot, the old active file becomes stale and will no longer contain a consistent view of guest data). The alteration of the persistent definition is similar to what must happen when snapshot creation alters a disk source. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Now that we track a disk mirror as a virStorageSource, we might as well update the XML to theoretically allow any type of mirroring destination (not just a local file). A later patch will also be reusing <mirror> to track the block commit of the top layer of a chain, which is another case where libvirt needs to update the backing chain after the job is finally pivoted, and since backing chains can have network backing files as the destination to commit into, it makes more sense to display that in the XML. This patch changes output-only XML; it was already documented that <mirror> does not affect a domain definition at this point (because qemu doesn't provide persistent bitmaps yet). Any application that was starting a block copy job with older libvirt and then relying on the domain XML to determine if it was complete will no longer be able to access the file= and format= attributes of mirror that were previously used. However, this is not going to be a problem in practice: the only time a block copy job works is on a transient domain, and any app that is managing a transient domain probably already does enough of its own bookkeeping to know which file it is mirroring into without having to re-read it from the libvirt XML. The one thing that was likely to be used in a mirroring job was the ready= attribute, which is unchanged. Meanwhile, I made sure the schema and parser still accept the old format, even if we no longer output it, so that upgrading from an older version of libvirt is seamless. * docs/schemas/domaincommon.rng (diskMirror): Alter definition. * src/conf/domain_conf.c (virDomainDiskDefParseXML): Parse two styles of mirror elements. (virDomainDiskDefFormat): Output new style. * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror-old.xml: New file, copied from... * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: ...here before modernizing. * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old*: New files. * tests/qemuxml2xmltest.c (mymain): Test both styles. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/formatdomain.html.in | 20 +++++-- docs/schemas/domaincommon.rng | 29 ++++++--- src/conf/domain_conf.c | 69 ++++++++++++++++------ .../qemuxml2argv-disk-mirror-old.xml | 47 +++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 9 ++- .../qemuxml2xmlout-disk-mirror-old-inactive.xml | 41 +++++++++++++ .../qemuxml2xmlout-disk-mirror-old.xml | 52 ++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 8 files changed, 235 insertions(+), 33 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-mirror-old.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 691a451..1b6ced8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1879,15 +1879,23 @@ <dt><code>mirror</code></dt> <dd> This element is present if the hypervisor has started a block - copy operation (via the <code>virDomainBlockCopy</code> API), - where the mirror location in attribute <code>file</code> will - eventually have the same contents as the source, and with the - file format in attribute <code>format</code> (which might - differ from the format of the source). If + copy operation (via the <code>virDomainBlockRebase</code> + API), where the mirror location in the <code>source</code> + sub-element will eventually have the same contents as the + source, and with the file format in the + sub-element <code>format</code> (which might differ from the + format of the source). The details of the <code>source</code> + sub-element are determined by the <code>type</code> attribute + of the mirror, similar to what is done for the + overall <code>disk</code> device element. If attribute <code>ready</code> is present, then it is known the disk is ready to pivot; otherwise, the disk is probably still copying. For now, this element only valid in output; it is - ignored on input. <span class="since">Since 0.9.12</span> + ignored on input. <span class="since">Since 1.2.6</span> + (Older libvirt <span class="since">since 0.9.12</span> allowed + only mirroring to a file, with the information reported + in <code>file</code> and <code>format</code> attributes + of <code>mirror</code> rather than subelements.) </dd> <dt><code>target</code></dt> <dd>The <code>target</code> element controls the bus / device diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index af67123..6cc922c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4172,16 +4172,29 @@ <empty/> </element> </define> + <define name='diskMirror'> <element name='mirror'> - <attribute name='file'> - <ref name='absFilePath'/> - </attribute> - <optional> - <attribute name='format'> - <ref name='storageFormat'/> - </attribute> - </optional> + <choice> + <group> + <attribute name='file'> + <ref name='absFilePath'/> + </attribute> + <optional> + <attribute name='format'> + <ref name='storageFormat'/> + </attribute> + </optional> + </group> + <group> + <interleave> + <ref name="diskSource"/> + <optional> + <ref name="diskFormat"/> + </optional> + </interleave> + </group> + </choice> <optional> <attribute name='ready'> <value>yes</value> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b8e7c50..fd75fb6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5239,6 +5239,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *vendor = NULL; char *product = NULL; char *discard = NULL; + char *mirrorFormat = NULL; + char *mirrorType = NULL; int expected_secret_usage = -1; int auth_secret_usage = -1; @@ -5375,18 +5377,34 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlStrEqual(cur->name, BAD_CAST "mirror") && !(flags & VIR_DOMAIN_XML_INACTIVE)) { char *ready; - char *mirrorFormat; if (VIR_ALLOC(def->mirror) < 0) goto error; - def->mirror->type = VIR_STORAGE_TYPE_FILE; - def->mirror->path = virXMLPropString(cur, "file"); - if (!def->mirror->path) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("mirror requires file name")); - goto error; + + mirrorType = virXMLPropString(cur, "type"); + if (mirrorType) { + def->mirror->type = virStorageTypeFromString(mirrorType); + if (def->mirror->type <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown mirror backing store " + "type '%s'"), mirrorType); + goto error; + } + mirrorFormat = virXPathString("string(./mirror/format/@type)", + ctxt); + } else { + /* For back-compat reasons, we handle a file name + * encoded as attributes, even though we prefer + * modern output in the style of backingStore */ + def->mirror->type = VIR_STORAGE_TYPE_FILE; + def->mirror->path = virXMLPropString(cur, "file"); + if (!def->mirror->path) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("mirror requires file name")); + goto error; + } + mirrorFormat = virXMLPropString(cur, "format"); } - mirrorFormat = virXMLPropString(cur, "format"); if (mirrorFormat) { def->mirror->format = virStorageFileFormatTypeFromString(mirrorFormat); @@ -5394,10 +5412,19 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown mirror format value '%s'"), mirrorFormat); - VIR_FREE(mirrorFormat); goto error; } - VIR_FREE(mirrorFormat); + } + if (mirrorType) { + xmlNodePtr mirrorNode; + + if (!(mirrorNode = virXPathNode("./mirror/source", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("mirror requires source element")); + goto error; + } + if (virDomainDiskSourceParse(mirrorNode, def->mirror) < 0) + goto error; } ready = virXMLPropString(cur, "ready"); if (ready) { @@ -5983,6 +6010,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(wwn); VIR_FREE(vendor); VIR_FREE(product); + VIR_FREE(mirrorType); + VIR_FREE(mirrorFormat); ctxt->node = save_ctxt; return def; @@ -15158,15 +15187,21 @@ virDomainDiskDefFormat(virBufferPtr buf, /* For now, mirroring is currently output-only: we only output it * for live domains, therefore we ignore it on input except for - * the internal parse on libvirtd restart. */ + * the internal parse on libvirtd restart. We only output the + * new style similar to backingStore, even though the parser + * code still accepts old style across libvirtd upgrades. */ if (def->mirror && !(flags & VIR_DOMAIN_XML_INACTIVE)) { - virBufferEscapeString(buf, "<mirror file='%s'", def->mirror->path); + virBufferAsprintf(buf, "<mirror type='%s'%s>\n", + virStorageTypeToString(def->mirror->type), + def->mirroring ? " ready='yes'" : ""); + virBufferAdjustIndent(buf, 2); if (def->mirror->format) - virBufferAsprintf(buf, " format='%s'", - virStorageFileFormatTypeToString(def->mirror->format)); - if (def->mirroring) - virBufferAddLit(buf, " ready='yes'"); - virBufferAddLit(buf, "/>\n"); + virBufferEscapeString(buf, "<format type='%s'/>\n", + virStorageFileFormatTypeToString(def->mirror->format)); + if (virDomainDiskSourceFormat(buf, def->mirror, 0, 0) < 0) + return -1; + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</mirror>\n"); } virBufferAsprintf(buf, "<target dev='%s' bus='%s'", diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror-old.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror-old.xml new file mode 100644 index 0000000..faa0b8c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror-old.xml @@ -0,0 +1,47 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <backingStore/> + <mirror file='/dev/HostVG/QEMUGuest1Copy' ready='yes'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='block' device='cdrom'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <backingStore/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/data.img'/> + <backingStore/> + <mirror file='/tmp/copy.img' format='qcow2'/> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/logs.img'/> + <backingStore/> + <target dev='vdb' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml index faa0b8c..a937d0a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml @@ -17,7 +17,9 @@ <disk type='block' device='disk'> <source dev='/dev/HostVG/QEMUGuest1'/> <backingStore/> - <mirror file='/dev/HostVG/QEMUGuest1Copy' ready='yes'/> + <mirror type='file' ready='yes'> + <source file='/dev/HostVG/QEMUGuest1Copy'/> + </mirror> <target dev='hda' bus='ide'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> @@ -31,7 +33,10 @@ <disk type='file' device='disk'> <source file='/tmp/data.img'/> <backingStore/> - <mirror file='/tmp/copy.img' format='qcow2'/> + <mirror type='file'> + <format type='qcow2'/> + <source file='/tmp/copy.img'/> + </mirror> <target dev='vda' bus='virtio'/> </disk> <disk type='file' device='disk'> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old-inactive.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old-inactive.xml new file mode 100644 index 0000000..b3d8c59 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old-inactive.xml @@ -0,0 +1,41 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='block' device='cdrom'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/data.img'/> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/logs.img'/> + <target dev='vdb' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml new file mode 100644 index 0000000..a937d0a --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml @@ -0,0 +1,52 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <backingStore/> + <mirror type='file' ready='yes'> + <source file='/dev/HostVG/QEMUGuest1Copy'/> + </mirror> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='block' device='cdrom'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <backingStore/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/data.img'/> + <backingStore/> + <mirror type='file'> + <format type='qcow2'/> + <source file='/tmp/copy.img'/> + </mirror> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/logs.img'/> + <backingStore/> + <target dev='vdb' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index da528da..200d50f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -224,6 +224,7 @@ mymain(void) DO_TEST("disk-scsi-virtio-scsi"); DO_TEST("disk-virtio-scsi-num_queues"); DO_TEST("disk-scsi-megasas"); + DO_TEST_DIFFERENT("disk-mirror-old"); DO_TEST_FULL("disk-mirror", false, WHEN_ACTIVE); DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE); DO_TEST("graphics-listen-network"); -- 1.9.3
On 06/06/14 00:52, Eric Blake wrote:
Now that we track a disk mirror as a virStorageSource, we might as well update the XML to theoretically allow any type of mirroring destination (not just a local file). A later patch will also be reusing <mirror> to track the block commit of the top layer of a chain, which is another case where libvirt needs to update the backing chain after the job is finally pivoted, and since backing chains can have network backing files as the destination to commit into, it makes more sense to display that in the XML.
This patch changes output-only XML; it was already documented that <mirror> does not affect a domain definition at this point (because qemu doesn't provide persistent bitmaps yet). Any application that was starting a block copy job with older libvirt and then relying on the domain XML to determine if it was complete will no longer be able to access the file= and format= attributes of mirror that were previously used. However, this is not going to be a problem in practice: the only time a block copy job works is on a transient domain, and any app that is managing a transient domain probably already does enough of its own bookkeeping to know which file it is mirroring into without having to re-read it from the libvirt XML. The one thing that
Hmm, although the change is externally visible I agree that it shouldn't be a problem.
was likely to be used in a mirroring job was the ready= attribute, which is unchanged. Meanwhile, I made sure the schema and parser still accept the old format, even if we no longer output it, so that upgrading from an older version of libvirt is seamless.
* docs/schemas/domaincommon.rng (diskMirror): Alter definition. * src/conf/domain_conf.c (virDomainDiskDefParseXML): Parse two styles of mirror elements. (virDomainDiskDefFormat): Output new style. * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror-old.xml: New file, copied from... * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: ...here before modernizing. * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old*: New files. * tests/qemuxml2xmltest.c (mymain): Test both styles.
Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/formatdomain.html.in | 20 +++++-- docs/schemas/domaincommon.rng | 29 ++++++--- src/conf/domain_conf.c | 69 ++++++++++++++++------ .../qemuxml2argv-disk-mirror-old.xml | 47 +++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 9 ++- .../qemuxml2xmlout-disk-mirror-old-inactive.xml | 41 +++++++++++++ .../qemuxml2xmlout-disk-mirror-old.xml | 52 ++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 8 files changed, 235 insertions(+), 33 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-mirror-old.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml
ACK, Peter
On 06/06/2014 05:36 AM, Peter Krempa wrote:
On 06/06/14 00:52, Eric Blake wrote:
Now that we track a disk mirror as a virStorageSource, we might as well update the XML to theoretically allow any type of mirroring destination (not just a local file). A later patch will also be reusing <mirror> to track the block commit of the top layer of a chain, which is another case where libvirt needs to update the backing chain after the job is finally pivoted, and since backing chains can have network backing files as the destination to commit into, it makes more sense to display that in the XML.
ACK,
I made a minor tweak (in virDomainDiskDefFormat, I split the initial <mirror type='...' ready='...'> into two actions, per your comment on 9/10), and pushed this one. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/06/2014 03:46 PM, Eric Blake wrote:
On 06/06/2014 05:36 AM, Peter Krempa wrote:
On 06/06/14 00:52, Eric Blake wrote:
Now that we track a disk mirror as a virStorageSource, we might as well update the XML to theoretically allow any type of mirroring destination (not just a local file). A later patch will also be reusing <mirror> to track the block commit of the top layer of a chain, which is another case where libvirt needs to update the backing chain after the job is finally pivoted, and since backing chains can have network backing files as the destination to commit into, it makes more sense to display that in the XML.
ACK,
I made a minor tweak (in virDomainDiskDefFormat, I split the initial <mirror type='...' ready='...'> into two actions, per your comment on 9/10), and pushed this one.
On IRC, John pointed out that this change tripped up the libvirt regression testsuite (so there IS code depending on the older <mirror file='/path/to/file'/> notation), and danpb convinced me I need to provide the older semantics even while adding newer semantics, so as not to break older clients. I'll be preparing a followup patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Now that qemu 2.0 allows commit of the active layer, people are attempting to use virsh blockcommit and getting into a stuck state, because libvirt is unprepared to handle the two-phase commit required by qemu. Stepping back a bit, there are two valid semantics for a commit operation: 1. Maintain a 'golden' base, and a transient overlay. Make changes in the overlay, and if everything appears to work, commit those changes into the base, but still keep the overlay for the next round of changes; repeat the cycle as desired. 2. Create an external snapshot, then back up the stable state in the backing file. Once the backup is complete, commit the overlay back into the base, and delete the temporary snapshot. Since qemu doesn't know up front which of the two styles is preferred, a block commit of the active layer merely gets the job into a synchronized state, and sends an event; then the user must either cancel (case 1) or complete (case 2), where qemu then sends a second event that actually ends the job. However, until commit e6bcbcd, libvirt was blindly assuming the semantics that apply to a commit of an intermediate image, where there is only one sane conclusion (the job automatically ends with fewer elements in the chain); and getting stuck because it wasn't prepared for qemu to enter a second phase of the job. This patch adds a flag to the libvirt API that a user MUST supply in order to acknowledge that they will be using two-phase semantics. It might be possible to have a mode where if the flag is omitted, we automatically do the case 2 semantics on the user's behalf; but before that happens, I must do additional patches to track the fact that we are doing an active commit in the domain XML. Later patches will add support of the flag, and once 2-phase semantics are working, we can then decide whether to relax things to allow an omitted flag to cause an automatic pivot. * include/libvirt/libvirt.h.in (VIR_DOMAIN_BLOCK_COMMIT_ACTIVE) (VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT): New enums. * src/libvirt.c (virDomainBlockCommit): Document two-phase job when committing active layer, through new flag. (virDomainBlockJobAbort): Document that pivot also occurs after active commit. * tools/virsh-domain.c (vshDomainBlockJob): Cover new job. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Explicitly reject active copy; later patches will add it in. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 12 ++++++---- src/libvirt.c | 55 +++++++++++++++++++++++++++++--------------- src/qemu/qemu_driver.c | 21 ++++++++++++++--- tools/virsh-domain.c | 3 ++- 4 files changed, 65 insertions(+), 26 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 260c971..127de11 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2513,6 +2513,7 @@ typedef enum { VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1, VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2, VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3, + VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4, #ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_BLOCK_JOB_TYPE_LAST @@ -2523,7 +2524,8 @@ typedef enum { * virDomainBlockJobAbortFlags: * * VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC: Request only, do not wait for completion - * VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT: Pivot to mirror when ending a copy job + * VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT: Pivot to new file when ending a copy or + * active commit job */ typedef enum { VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC = 1 << 0, @@ -2588,6 +2590,8 @@ typedef enum { VIR_DOMAIN_BLOCK_COMMIT_DELETE = 1 << 1, /* Delete any files that are now invalid after their contents have been committed */ + VIR_DOMAIN_BLOCK_COMMIT_ACTIVE = 1 << 2, /* Allow a two-phase commit when + top is the active layer */ } virDomainBlockCommitFlags; int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base, @@ -4823,8 +4827,8 @@ typedef void (*virConnectDomainEventGraphicsCallback)(virConnectPtr conn, /** * virConnectDomainEventBlockJobStatus: * - * The final status of a virDomainBlockPull() or virDomainBlockRebase() - * operation + * Tracks status of a virDomainBlockPull(), virDomainBlockRebase(), + * or virDomainBlockCommit() operation */ typedef enum { VIR_DOMAIN_BLOCK_JOB_COMPLETED = 0, @@ -4843,7 +4847,7 @@ typedef enum { * @dom: domain on which the event occurred * @disk: fully-qualified filename of the affected disk * @type: type of block job (virDomainBlockJobType) - * @status: final status of the operation (virConnectDomainEventBlockJobStatus) + * @status: status of the operation (virConnectDomainEventBlockJobStatus) * @opaque: application specified data * * The callback signature to use when registering for an event of type diff --git a/src/libvirt.c b/src/libvirt.c index f01b6dd..6c4a124 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19473,13 +19473,16 @@ virDomainOpenChannel(virDomainPtr dom, * * If the current block job for @disk is VIR_DOMAIN_BLOCK_JOB_TYPE_COPY, then * the default is to abort the mirroring and revert to the source disk; - * adding @flags of VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT causes this call to - * fail with VIR_ERR_BLOCK_COPY_ACTIVE if the copy is not fully populated, - * otherwise it will swap the disk over to the copy to end the mirroring. An - * event will be issued when the job is ended, and it is possible to use - * VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC to control whether this command waits - * for the completion of the job. Restarting this job requires starting - * over from the beginning of the first phase. + * likewise, if the current job is VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT, + * the default is to abort without changing the active layer of @disk. + * Adding @flags of VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT causes this call to + * fail with VIR_ERR_BLOCK_COPY_ACTIVE if the copy or commit is not yet + * ready; otherwise it will swap the disk over to the new active image + * to end the mirroring or active commit. An event will be issued when the + * job is ended, and it is possible to use VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC + * to control whether this command waits for the completion of the job. + * Restarting a copy or active commit job requires starting over from the + * beginning of the first phase. * * Returns -1 in case of failure, 0 when successful. */ @@ -19849,17 +19852,32 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, * the job is aborted, it is up to the hypervisor whether starting a new * job will resume from the same point, or start over. * + * As a special case, if @top is the active image (or NULL), and @flags + * includes VIR_DOMAIN_BLOCK_COMMIT_ACTIVE, the block job will have a type + * of VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT, and operates in two phases. + * In the first phase, the contents are being committed into @base, and the + * job can only be canceled. The job transitions to the second phase when + * the job info states cur == end, and remains alive to keep all further + * changes to @top synchronized into @base; an event with status + * VIR_DOMAIN_BLOCK_JOB_READY is also issued to mark the job transition. + * Once in the second phase, the user must choose whether to cancel the job + * (keeping @top as the active image, but now containing only the changes + * since the time the job ended) or to pivot the job (adjusting to @base as + * the active image, and invalidating @top). + * * Be aware that this command may invalidate files even if it is aborted; * the user is cautioned against relying on the contents of invalidated - * intermediate files such as @top without manually rebasing those files - * to use a backing file of a read-only copy of @base prior to the point - * where the commit operation was started (although such a rebase cannot - * be safely done until the commit has successfully completed). However, - * the domain itself will not have any issues; the active layer remains - * valid throughout the entire commit operation. As a convenience, - * if @flags contains VIR_DOMAIN_BLOCK_COMMIT_DELETE, this command will - * unlink all files that were invalidated, after the commit successfully - * completes. + * intermediate files such as @top (when @top is not the active image) + * without manually rebasing those files to use a backing file of a + * read-only copy of @base prior to the point where the commit operation + * was started (and such a rebase cannot be safely done until the commit + * has successfully completed). However, the domain itself will not have + * any issues; the active layer remains valid throughout the entire commit + * operation. + * + * Some hypervisors may support a shortcut where if @flags contains + * VIR_DOMAIN_BLOCK_COMMIT_DELETE, then this command will unlink all files + * that were invalidated, after the commit successfully completes. * * By default, if @base is NULL, the commit target will be the bottom of * the backing chain; if @flags contains VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, @@ -19867,8 +19885,9 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, * is NULL, the active image at the top of the chain will be used. Some * hypervisors place restrictions on how much can be committed, and might * fail if @base is not the immediate backing file of @top, or if @top is - * the active layer in use by a running domain, or if @top is not the - * top-most file; restrictions may differ for online vs. offline domains. + * the active layer in use by a running domain but @flags did not include + * VIR_DOMAIN_BLOCK_COMMIT_ACTIVE, or if @top is not the top-most file; + * restrictions may differ for online vs. offline domains. * * The @disk parameter is either an unambiguous source name of the * block device (the <source file='...'/> sub-element, such as diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b0f80a5..b61b01b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15134,6 +15134,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, * to prevent newly scheduled block jobs from confusing us. */ if (mode == BLOCK_JOB_ABORT) { if (!async) { + /* Older qemu that lacked async reporting also lacked + * active commit, so we can hardcode the event to pull */ int type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL; int status = VIR_DOMAIN_BLOCK_JOB_CANCELED; event = virDomainEventBlockJobNewFromObj(vm, disk->src->path, type, @@ -15488,6 +15490,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *top_parent = NULL; bool clean_access = false; + /* XXX Add support for COMMIT_ACTIVE, COMMIT_DELETE */ virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1); if (!(vm = qemuDomObjFromDomain(dom))) @@ -15538,9 +15541,21 @@ qemuDomainBlockCommit(virDomainPtr dom, * process; qemu 2.1 is further improving active commit. We need * to start supporting it in libvirt. */ if (topSource == disk->src) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("committing the active layer not supported yet")); - goto endjob; + /* We assume that no one will backport qemu 2.0 active commit + * to an earlier qemu without also backporting the block job + * ready event; but this makes sure of that fact */ + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("active commit not supported with this QEMU binary")); + goto endjob; + } + /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */ + if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE)) { + virReportError(VIR_ERR_INVALID_ARG, + _("commit of '%s' active layer requires active flag"), + disk->dst); + goto endjob; + } } if (!topSource->backingStore) { diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 84a6706..6da8a17 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1968,7 +1968,8 @@ VIR_ENUM_IMPL(vshDomainBlockJob, N_("Unknown job"), N_("Block Pull"), N_("Block Copy"), - N_("Block Commit")) + N_("Block Commit"), + N_("Active Block Commit")) static const char * vshDomainBlockJobToString(int type) -- 1.9.3
On 06/06/14 00:52, Eric Blake wrote:
Now that qemu 2.0 allows commit of the active layer, people are attempting to use virsh blockcommit and getting into a stuck state, because libvirt is unprepared to handle the two-phase commit required by qemu.
Stepping back a bit, there are two valid semantics for a commit operation:
1. Maintain a 'golden' base, and a transient overlay. Make changes in the overlay, and if everything appears to work, commit those changes into the base, but still keep the overlay for the next round of changes; repeat the cycle as desired.
2. Create an external snapshot, then back up the stable state in the backing file. Once the backup is complete, commit the overlay back into the base, and delete the temporary snapshot.
Since qemu doesn't know up front which of the two styles is preferred, a block commit of the active layer merely gets the job into a synchronized state, and sends an event; then the user must either cancel (case 1) or complete (case 2), where qemu then sends a second event that actually ends the job. However, until commit e6bcbcd, libvirt was blindly assuming the semantics that apply to a commit of an intermediate image, where there is only one sane conclusion (the job automatically ends with fewer elements in the chain); and getting stuck because it wasn't prepared for qemu to enter a second phase of the job.
This patch adds a flag to the libvirt API that a user MUST supply in order to acknowledge that they will be using two-phase semantics. It might be possible to have a mode where if the flag is omitted, we automatically do the case 2 semantics on the user's behalf; but before that happens, I must do additional patches to track the fact that we are doing an active commit in the domain XML. Later patches will add support of the flag, and once 2-phase semantics are working, we can then decide whether to relax things to allow an omitted flag to cause an automatic pivot.
* include/libvirt/libvirt.h.in (VIR_DOMAIN_BLOCK_COMMIT_ACTIVE) (VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT): New enums. * src/libvirt.c (virDomainBlockCommit): Document two-phase job when committing active layer, through new flag. (virDomainBlockJobAbort): Document that pivot also occurs after active commit. * tools/virsh-domain.c (vshDomainBlockJob): Cover new job. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Explicitly reject active copy; later patches will add it in.
Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 12 ++++++---- src/libvirt.c | 55 +++++++++++++++++++++++++++++--------------- src/qemu/qemu_driver.c | 21 ++++++++++++++--- tools/virsh-domain.c | 3 ++- 4 files changed, 65 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b0f80a5..b61b01b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
@@ -15538,9 +15541,21 @@ qemuDomainBlockCommit(virDomainPtr dom, * process; qemu 2.1 is further improving active commit. We need * to start supporting it in libvirt. */ if (topSource == disk->src) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("committing the active layer not supported yet")); - goto endjob; + /* We assume that no one will backport qemu 2.0 active commit + * to an earlier qemu without also backporting the block job + * ready event; but this makes sure of that fact */ + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("active commit not supported with this QEMU binary")); + goto endjob; + } + /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */
I think we should do it in the future and possibly document the expected behavior.
+ if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE)) { + virReportError(VIR_ERR_INVALID_ARG, + _("commit of '%s' active layer requires active flag"), + disk->dst); + goto endjob; + } }
if (!topSource->backingStore) {
ACK, adding the docs for the auto-pivot commit if you don't specify any flag can be postponed. Peter
On 06/06/2014 05:51 AM, Peter Krempa wrote:
On 06/06/14 00:52, Eric Blake wrote:
Now that qemu 2.0 allows commit of the active layer, people are attempting to use virsh blockcommit and getting into a stuck state, because libvirt is unprepared to handle the two-phase commit required by qemu.
This patch adds a flag to the libvirt API that a user MUST supply in order to acknowledge that they will be using two-phase semantics. It might be possible to have a mode where if the flag is omitted, we automatically do the case 2 semantics on the user's behalf; but before that happens, I must do additional patches to track the fact that we are doing an active commit in the domain XML. Later patches will add support of the flag, and once 2-phase semantics are working, we can then decide whether to relax things to allow an omitted flag to cause an automatic pivot.
+ /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */
I think we should do it in the future and possibly document the expected behavior.
+ if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE)) { + virReportError(VIR_ERR_INVALID_ARG, + _("commit of '%s' active layer requires active flag"), + disk->dst); + goto endjob; + } }
if (!topSource->backingStore) {
ACK, adding the docs for the auto-pivot commit if you don't specify any flag can be postponed.
Agreed - we can document the new semantics once they are implemented. I've pushed this patch now to reserve the bit, even though I still need to post a v3 of the rest of the series with changes such as ensuring the persistent definition is updated appropriately. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add knobs to virsh to manage a 2-phase active commit of the top layer, similar to knobs already present on blockcopy. While this code will fail until later patches actually implement the new knobs in the qemu driver, doing it now proves that the API is usable and also makes it easier for testing the qemu changes as they are made. * tools/virsh-domain.c (cmdBlockCommit): Add --active, --pivot, and --finish options, modeled after blockcopy. (blockJobImpl): Support --active flag. * tools/virsh.pod (blockcommit): Document new flags. (blockjob): Mention 2-phase commit interaction. Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++-- tools/virsh.pod | 27 +++++++++++++++++++------- 2 files changed, 72 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6da8a17..a879316 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1492,6 +1492,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, flags |= VIR_DOMAIN_BLOCK_COMMIT_SHALLOW; if (vshCommandOptBool(cmd, "delete")) flags |= VIR_DOMAIN_BLOCK_COMMIT_DELETE; + if (vshCommandOptBool(cmd, "active")) + flags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; ret = virDomainBlockCommit(dom, path, base, top, bandwidth, flags); break; case VSH_CMD_BLOCK_JOB_COPY: @@ -1592,13 +1594,18 @@ static const vshCmdOptDef opts_block_commit[] = { .type = VSH_OT_DATA, .help = N_("path of top file to commit from (default top of chain)") }, + {.name = "active", + .type = VSH_OT_BOOL, + .help = N_("trigger two-stage active commit of top file") + }, {.name = "delete", .type = VSH_OT_BOOL, .help = N_("delete files that were successfully committed") }, {.name = "wait", .type = VSH_OT_BOOL, - .help = N_("wait for job to complete") + .help = N_("wait for job to complete " + "(with --active, wait for job to sync)") }, {.name = "verbose", .type = VSH_OT_BOOL, @@ -1608,6 +1615,14 @@ static const vshCmdOptDef opts_block_commit[] = { .type = VSH_OT_INT, .help = N_("with --wait, abort if copy exceeds timeout (in seconds)") }, + {.name = "pivot", + .type = VSH_OT_BOOL, + .help = N_("with --active --wait, pivot when commit is synced") + }, + {.name = "finish", + .type = VSH_OT_BOOL, + .help = N_("with --active --wait, quit when commit is synced") + }, {.name = "async", .type = VSH_OT_BOOL, .help = N_("with --wait, don't wait for cancel to finish") @@ -1622,6 +1637,9 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) bool ret = false; bool blocking = vshCommandOptBool(cmd, "wait"); bool verbose = vshCommandOptBool(cmd, "verbose"); + bool active = vshCommandOptBool(cmd, "active"); + bool pivot = vshCommandOptBool(cmd, "pivot"); + bool finish = vshCommandOptBool(cmd, "finish"); int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; @@ -1632,6 +1650,21 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) bool quit = false; int abort_flags = 0; + if (pivot || finish) { + if (pivot && finish) { + vshError(ctl, "%s", _("cannot mix --pivot and --finish")); + return false; + } + if (!active) { + vshError(ctl, "%s", _("missing --active option")); + return false; + } + if (!blocking) { + vshError(ctl, "%s", _("missing --wait option")); + return false; + } + } + if (blocking) { if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) return false; @@ -1683,6 +1716,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) if (verbose) vshPrintJobProgress(_("Block Commit"), info.end - info.cur, info.end); + if (active && info.cur == info.end) + break; GETTIMEOFDAY(&curr); if (intCaught || (timeout && @@ -1709,7 +1744,22 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) /* printf [100 %] */ vshPrintJobProgress(_("Block Commit"), 0, 1); } - vshPrint(ctl, "\n%s", quit ? _("Commit aborted") : _("Commit complete")); + if (!quit && pivot) { + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; + if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to pivot job for disk %s"), path); + goto cleanup; + } + } else if (finish && !quit && + virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to finish job for disk %s"), path); + goto cleanup; + } + vshPrint(ctl, "\n%s", + quit ? _("Commit aborted") : + pivot ? _("Successfully pivoted") : + !finish ? _("Now in synchronized phase") : + _("Commit complete")); ret = true; cleanup: diff --git a/tools/virsh.pod b/tools/virsh.pod index 02671b4..fc81fac 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -767,8 +767,9 @@ address of virtual interface (such as I<detach-interface> or I<domif-setlink>) will accept the MAC address printed by this command. =item B<blockcommit> I<domain> I<path> [I<bandwidth>] -{[I<base>] | [I<--shallow>]} [I<top>] [I<--delete>] -[I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] +{[I<base>] | [I<--shallow>]} [I<top>] [I<--delete>] [I<--active>] +[I<--wait> [I<--verbose>] [{I<--pivot> | I<--finish>}] +[I<--timeout> B<seconds>] [I<--async>]] Reduce the length of a backing image chain, by committing changes at the top of the chain (snapshot or delta files) into backing images. By @@ -778,8 +779,17 @@ operation is constrained to committing just that portion of the chain; I<--shallow> can be used instead of I<base> to specify the immediate backing file of the resulting top image to be committed. The files being committed are rendered invalid, possibly as soon as the operation -starts; using the I<--delete> flag will remove these files at the successful -completion of the commit operation. +starts; using the I<--delete> flag will attempt to remove these invalidated +files at the successful completion of the commit operation. + +When I<top> is omitted or specified as the active image, it is also +possible to specify I<--active> to trigger a two-phase active commit. In +the first phase, I<top> is copied into I<base> and the job can only be +canceled, with top still containing data not yet in base. In the second +phase, I<top> and I<base> remain identical until a call to B<blockjob> +with the I<--abort> flag (keeping top as the active image that tracks +changes from that point in time) or the I<--pivot> flag (making base +the new active image and invalidating top). By default, this command returns as soon as possible, and data for the entire disk is committed in the background; the progress of the @@ -790,7 +800,10 @@ or SIGINT is sent (usually with C<Ctrl-C>). Using I<--verbose> along with I<--wait> will produce periodic status updates. If job cancellation is triggered, I<--async> will return control to the user as fast as possible, otherwise the command may continue to block a little while -longer until the job is done cleaning up. +longer until the job is done cleaning up. When I<--active> is used +alongside I<--wait>, then it is also possible to use I<--pivot> or +I<--finish> to end the job cleanly rather than leaving things in the +synchronized state. I<path> specifies fully-qualified path of the disk; it corresponds to a unique target name (<target dev='name'/>) or source file (<source @@ -916,8 +929,8 @@ also B<domblklist> for listing these names). If I<--abort> is specified, the active job on the specified disk will be aborted. If I<--async> is also specified, this command will return immediately, rather than waiting for the cancellation to complete. If -I<--pivot> is specified, this requests that an active copy job -be pivoted over to the new copy. +I<--pivot> is specified, this requests that an active copy or active +commit job be pivoted over to the new image. If I<--info> is specified, the active job information on the specified disk will be printed. I<bandwidth> can be used to set bandwidth limit for the active job. -- 1.9.3
On 06/06/14 00:52, Eric Blake wrote:
Add knobs to virsh to manage a 2-phase active commit of the top layer, similar to knobs already present on blockcopy. While this code will fail until later patches actually implement the new knobs in the qemu driver, doing it now proves that the API is usable and also makes it easier for testing the qemu changes as they are made.
* tools/virsh-domain.c (cmdBlockCommit): Add --active, --pivot, and --finish options, modeled after blockcopy. (blockJobImpl): Support --active flag. * tools/virsh.pod (blockcommit): Document new flags. (blockjob): Mention 2-phase commit interaction.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++-- tools/virsh.pod | 27 +++++++++++++++++++------- 2 files changed, 72 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6da8a17..a879316 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
@@ -1608,6 +1615,14 @@ static const vshCmdOptDef opts_block_commit[] = { .type = VSH_OT_INT, .help = N_("with --wait, abort if copy exceeds timeout (in seconds)") }, + {.name = "pivot", + .type = VSH_OT_BOOL, + .help = N_("with --active --wait, pivot when commit is synced") + }, + {.name = "finish", + .type = VSH_OT_BOOL, + .help = N_("with --active --wait, quit when commit is synced")
Finish seems a bit too generic ... shouldn't we go with something like --keep-overlay or other more specific flag?
+ }, {.name = "async", .type = VSH_OT_BOOL, .help = N_("with --wait, don't wait for cancel to finish")
@@ -1709,7 +1744,22 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) /* printf [100 %] */ vshPrintJobProgress(_("Block Commit"), 0, 1); } - vshPrint(ctl, "\n%s", quit ? _("Commit aborted") : _("Commit complete")); + if (!quit && pivot) { + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; + if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to pivot job for disk %s"), path); + goto cleanup; + } + } else if (finish && !quit && + virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to finish job for disk %s"), path); + goto cleanup; + } + vshPrint(ctl, "\n%s", + quit ? _("Commit aborted") : + pivot ? _("Successfully pivoted") : + !finish ? _("Now in synchronized phase") : + _("Commit complete"));
Uhhh, embedded ternaries? Please change it to hierarchical if's or something.
ret = true; cleanup:
diff --git a/tools/virsh.pod b/tools/virsh.pod index 02671b4..fc81fac 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -767,8 +767,9 @@ address of virtual interface (such as I<detach-interface> or I<domif-setlink>) will accept the MAC address printed by this command.
=item B<blockcommit> I<domain> I<path> [I<bandwidth>] -{[I<base>] | [I<--shallow>]} [I<top>] [I<--delete>] -[I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] +{[I<base>] | [I<--shallow>]} [I<top>] [I<--delete>] [I<--active>] +[I<--wait> [I<--verbose>] [{I<--pivot> | I<--finish>}] +[I<--timeout> B<seconds>] [I<--async>]]
Reduce the length of a backing image chain, by committing changes at the top of the chain (snapshot or delta files) into backing images. By @@ -778,8 +779,17 @@ operation is constrained to committing just that portion of the chain; I<--shallow> can be used instead of I<base> to specify the immediate backing file of the resulting top image to be committed. The files being committed are rendered invalid, possibly as soon as the operation -starts; using the I<--delete> flag will remove these files at the successful -completion of the commit operation. +starts; using the I<--delete> flag will attempt to remove these invalidated +files at the successful completion of the commit operation. + +When I<top> is omitted or specified as the active image, it is also +possible to specify I<--active> to trigger a two-phase active commit. In +the first phase, I<top> is copied into I<base> and the job can only be +canceled, with top still containing data not yet in base. In the second +phase, I<top> and I<base> remain identical until a call to B<blockjob> +with the I<--abort> flag (keeping top as the active image that tracks +changes from that point in time) or the I<--pivot> flag (making base +the new active image and invalidating top).
By default, this command returns as soon as possible, and data for the entire disk is committed in the background; the progress of the @@ -790,7 +800,10 @@ or SIGINT is sent (usually with C<Ctrl-C>). Using I<--verbose> along with I<--wait> will produce periodic status updates. If job cancellation is triggered, I<--async> will return control to the user as fast as possible, otherwise the command may continue to block a little while -longer until the job is done cleaning up. +longer until the job is done cleaning up. When I<--active> is used +alongside I<--wait>, then it is also possible to use I<--pivot> or +I<--finish> to end the job cleanly rather than leaving things in the
I'd drop cleanly. The synchronized state is "clean" too imho.
+synchronized state.
I<path> specifies fully-qualified path of the disk; it corresponds to a unique target name (<target dev='name'/>) or source file (<source @@ -916,8 +929,8 @@ also B<domblklist> for listing these names). If I<--abort> is specified, the active job on the specified disk will be aborted. If I<--async> is also specified, this command will return immediately, rather than waiting for the cancellation to complete. If -I<--pivot> is specified, this requests that an active copy job -be pivoted over to the new copy. +I<--pivot> is specified, this requests that an active copy or active +commit job be pivoted over to the new image. If I<--info> is specified, the active job information on the specified disk will be printed. I<bandwidth> can be used to set bandwidth limit for the active job.
We should discuss the --finish flag name a little bit more. Peter
On 06/06/2014 05:57 AM, Peter Krempa wrote:
On 06/06/14 00:52, Eric Blake wrote:
Add knobs to virsh to manage a 2-phase active commit of the top layer, similar to knobs already present on blockcopy. While this code will fail until later patches actually implement the new knobs in the qemu driver, doing it now proves that the API is usable and also makes it easier for testing the qemu changes as they are made.
* tools/virsh-domain.c (cmdBlockCommit): Add --active, --pivot, and --finish options, modeled after blockcopy. (blockJobImpl): Support --active flag. * tools/virsh.pod (blockcommit): Document new flags. (blockjob): Mention 2-phase commit interaction.
+ {.name = "finish", + .type = VSH_OT_BOOL, + .help = N_("with --active --wait, quit when commit is synced")
Finish seems a bit too generic ... shouldn't we go with something like --keep-overlay or other more specific flag?
blockcopy has the same flag, but yeah, I could go with keep-overlay. It's shorthand for doing a two-command sequence: blockcommit --wait blockjob --abort but with a nicer name for what the abort actually does in the second phase.
+ }, {.name = "async", .type = VSH_OT_BOOL, .help = N_("with --wait, don't wait for cancel to finish")
@@ -1709,7 +1744,22 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) /* printf [100 %] */ vshPrintJobProgress(_("Block Commit"), 0, 1); } - vshPrint(ctl, "\n%s", quit ? _("Commit aborted") : _("Commit complete")); + if (!quit && pivot) { + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; + if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to pivot job for disk %s"), path); + goto cleanup; + } + } else if (finish && !quit && + virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to finish job for disk %s"), path); + goto cleanup; + } + vshPrint(ctl, "\n%s", + quit ? _("Commit aborted") : + pivot ? _("Successfully pivoted") : + !finish ? _("Now in synchronized phase") : + _("Commit complete"));
Uhhh, embedded ternaries? Please change it to hierarchical if's or something.
Copied from blockcopy; I guess I can fix both instances.
We should discuss the --finish flag name a little bit more.
Is it better to have blockcopy and blockcommit look alike, or to make each command have a better description of what it is doing? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/06/14 14:12, Eric Blake wrote:
On 06/06/2014 05:57 AM, Peter Krempa wrote:
On 06/06/14 00:52, Eric Blake wrote:
Add knobs to virsh to manage a 2-phase active commit of the top layer, similar to knobs already present on blockcopy. While this code will fail until later patches actually implement the new knobs in the qemu driver, doing it now proves that the API is usable and also makes it easier for testing the qemu changes as they are made.
* tools/virsh-domain.c (cmdBlockCommit): Add --active, --pivot, and --finish options, modeled after blockcopy. (blockJobImpl): Support --active flag. * tools/virsh.pod (blockcommit): Document new flags. (blockjob): Mention 2-phase commit interaction.
+ {.name = "finish", + .type = VSH_OT_BOOL, + .help = N_("with --active --wait, quit when commit is synced")
Finish seems a bit too generic ... shouldn't we go with something like --keep-overlay or other more specific flag?
blockcopy has the same flag, but yeah, I could go with keep-overlay. It's shorthand for doing a two-command sequence:
blockcommit --wait blockjob --abort
but with a nicer name for what the abort actually does in the second phase.
Well, the --keep-overlay (or keep-*) option is better in my opinion as you can guess what it does according to the name, where I couldn't guess that --finish does it.
+ }, {.name = "async", .type = VSH_OT_BOOL, .help = N_("with --wait, don't wait for cancel to finish")
@@ -1709,7 +1744,22 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) /* printf [100 %] */ vshPrintJobProgress(_("Block Commit"), 0, 1); } - vshPrint(ctl, "\n%s", quit ? _("Commit aborted") : _("Commit complete")); + if (!quit && pivot) { + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; + if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to pivot job for disk %s"), path); + goto cleanup; + } + } else if (finish && !quit && + virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to finish job for disk %s"), path); + goto cleanup; + } + vshPrint(ctl, "\n%s", + quit ? _("Commit aborted") : + pivot ? _("Successfully pivoted") : + !finish ? _("Now in synchronized phase") : + _("Commit complete"));
Uhhh, embedded ternaries? Please change it to hierarchical if's or something.
Copied from blockcopy; I guess I can fix both instances.
We should discuss the --finish flag name a little bit more.
Is it better to have blockcopy and blockcommit look alike, or to make each command have a better description of what it is doing?
They aren't that much semantically similar from a high-level point of view so it should be okay if we diverge there. On the other hand, those options aren't used that wildly that it should matter that much. I probably don't care that much so I'd force you to change it but it would be better understandable if you do.
ACK to this patch with or without --finish changed. (the change of the ternary to something more readable is still required though). Peter
A future patch will add two-phase block commit jobs; as the mechanism for managing them is similar to managing a block copy job, existing errors should be made generic enough to occur for either job type. * src/conf/domain_conf.c (virDomainHasDiskMirror): Update comment. * src/qemu/qemu_driver.c (qemuDomainDefineXML) (qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot) (qemuDomainBlockJobImpl, qemuDomainBlockCopy): Update error message. * src/qemu/qemu_hotplug.c (qemuDomainDetachDiskDevice): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_driver.c | 10 +++++----- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fd75fb6..2b7dc26 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10379,7 +10379,7 @@ virDomainDiskRemoveByName(virDomainDefPtr def, const char *name) } /* Return true if VM has at least one disk involved in a current block - * copy job (that is, with a <mirror> element in the disk xml). */ + * copy/commit job (that is, with a <mirror> element in the disk xml). */ bool virDomainHasDiskMirror(virDomainObjPtr vm) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b61b01b..6574294 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6257,7 +6257,7 @@ static virDomainPtr qemuDomainDefineXML(virConnectPtr conn, const char *xml) def = NULL; if (virDomainHasDiskMirror(vm)) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", - _("domain has active block copy job")); + _("domain has active block job")); virDomainObjAssignDef(vm, NULL, false, NULL); goto cleanup; } @@ -13467,7 +13467,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } if (virDomainHasDiskMirror(vm)) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", - _("domain has active block copy job")); + _("domain has active block job")); goto cleanup; } @@ -14077,7 +14077,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (virDomainHasDiskMirror(vm)) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", - _("domain has active block copy job")); + _("domain has active block job")); goto cleanup; } @@ -15077,7 +15077,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, if (mode == BLOCK_JOB_PULL && disk->mirror) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, - _("disk '%s' already in active block copy job"), + _("disk '%s' already in active block job"), disk->dst); goto endjob; } @@ -15285,7 +15285,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, disk = vm->def->disks[idx]; if (disk->mirror) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, - _("disk '%s' already in active block copy job"), + _("disk '%s' already in active block job"), disk->dst); goto endjob; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cd43dd5..7289055 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2986,7 +2986,7 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, if (detach->mirror) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, - _("disk '%s' is in an active block copy job"), + _("disk '%s' is in an active block job"), detach->dst); goto cleanup; } -- 1.9.3
On 06/06/14 00:52, Eric Blake wrote:
A future patch will add two-phase block commit jobs; as the mechanism for managing them is similar to managing a block copy job, existing errors should be made generic enough to occur for either job type.
* src/conf/domain_conf.c (virDomainHasDiskMirror): Update comment. * src/qemu/qemu_driver.c (qemuDomainDefineXML) (qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot) (qemuDomainBlockJobImpl, qemuDomainBlockCopy): Update error message. * src/qemu/qemu_hotplug.c (qemuDomainDetachDiskDevice): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_driver.c | 10 +++++----- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)
ACK, Peter
A future patch is going to wire up qemu active block commit jobs; but as they have similar events and are canceled/pivoted in the same way as block copy jobs, it is easiest to track all bookkeeping for the commit job by reusing the <mirror> element. This patch adds domain XML to track which job was responsible for creating a mirroring situation, and adds a job='copy' attribute to all existing uses of <mirror>. Along the way, it also massages the qemu monitor backend to read the new field in order to generate the correct type of libvirt job (even though it requires a future patch to actually cause a qemu event that can be reported as an active commit). * docs/schemas/domaincommon.rng: Enhance schema. * docs/formatdomain.html.in: Document it. * src/conf/domain_conf.h (_virDomainDiskDef): Add a field. * src/conf/domain_conf.c (virDomainBlockJobType): String conversion. (virDomainDiskDefParseXML): Parse job type. (virDomainDiskDefFormat): Output job type. * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Distinguish active from regular commit. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Set job type. (qemuDomainBlockPivot, qemuDomainBlockJobImpl): Clean up job type on completion. * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml: Update tests. * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Likewise. * tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml: New file. * tests/qemuxml2xmltest.c (mymain): Drive new test. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/formatdomain.html.in | 15 +++++---- docs/schemas/domaincommon.rng | 6 ++++ src/conf/domain_conf.c | 31 ++++++++++++++++-- src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 3 ++ src/qemu/qemu_process.c | 18 +++++++---- .../qemuxml2argv-disk-active-commit.xml | 37 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4 +-- .../qemuxml2xmlout-disk-mirror-old.xml | 4 +-- tests/qemuxml2xmltest.c | 1 + 10 files changed, 100 insertions(+), 20 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1b6ced8..3bf1f6d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1878,13 +1878,16 @@ </dd> <dt><code>mirror</code></dt> <dd> - This element is present if the hypervisor has started a block - copy operation (via the <code>virDomainBlockRebase</code> - API), where the mirror location in the <code>source</code> - sub-element will eventually have the same contents as the - source, and with the file format in the + This element is present if the hypervisor has started a + long-running block job operation, where the mirror location in + the <code>source</code> sub-element will eventually have the + same contents as the source, and with the file format in the sub-element <code>format</code> (which might differ from the - format of the source). The details of the <code>source</code> + format of the source). The <code>job</code> attribute + mentions which API started the operation ("copy" + for the <code>virDomainBlockRebase</code> API, or "commit" + for the <code>virDomainBlockCommit</code> API). The details of + the <code>source</code> sub-element are determined by the <code>type</code> attribute of the mirror, similar to what is done for the overall <code>disk</code> device element. If diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6cc922c..3badb6d 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4187,6 +4187,12 @@ </optional> </group> <group> + <attribute name='job'> + <choice> + <value>copy</value> + <value>active-commit</value> + </choice> + </attribute> <interleave> <ref name="diskSource"/> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2b7dc26..92009ce 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -760,6 +760,11 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST, "unmap", "ignore") +/* Internal mapping of block job types associated with <mirror> */ +VIR_ENUM_DECL(virDomainBlockJob) +VIR_ENUM_IMPL(virDomainBlockJob, VIR_DOMAIN_BLOCK_JOB_TYPE_LAST, + "", "", "copy", "", "active-commit") + #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE @@ -5377,10 +5382,26 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlStrEqual(cur->name, BAD_CAST "mirror") && !(flags & VIR_DOMAIN_XML_INACTIVE)) { char *ready; + char *blockJob; if (VIR_ALLOC(def->mirror) < 0) goto error; + blockJob = virXMLPropString(cur, "job"); + if (blockJob) { + def->mirrorJob = virDomainBlockJobTypeFromString(blockJob); + if (def->mirrorJob <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown mirror job type '%s'"), + blockJob); + VIR_FREE(blockJob); + goto error; + } + VIR_FREE(blockJob); + } else { + def->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY; + } + mirrorType = virXMLPropString(cur, "type"); if (mirrorType) { def->mirror->type = virStorageTypeFromString(mirrorType); @@ -15191,9 +15212,13 @@ virDomainDiskDefFormat(virBufferPtr buf, * new style similar to backingStore, even though the parser * code still accepts old style across libvirtd upgrades. */ if (def->mirror && !(flags & VIR_DOMAIN_XML_INACTIVE)) { - virBufferAsprintf(buf, "<mirror type='%s'%s>\n", - virStorageTypeToString(def->mirror->type), - def->mirroring ? " ready='yes'" : ""); + virBufferAsprintf(buf, "<mirror type='%s'", + virStorageTypeToString(def->mirror->type)); + virBufferEscapeString(buf, " job='%s'", + virDomainBlockJobTypeToString(def->mirrorJob)); + if (def->mirroring) + virBufferAddLit(buf, " ready='yes'"); + virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); if (def->mirror->format) virBufferEscapeString(buf, "<format type='%s'/>\n", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 11d5c68..f141f7d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -608,6 +608,7 @@ struct _virDomainDiskDef { virStorageSourcePtr mirror; bool mirroring; + int mirrorJob; /* virDomainBlockJobType */ struct { unsigned int cylinders; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6574294..5903144 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14997,6 +14997,7 @@ qemuDomainBlockPivot(virConnectPtr conn, virStorageSourceFree(disk->mirror); disk->mirror = NULL; disk->mirroring = false; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; cleanup: if (resume && virDomainObjIsActive(vm) && @@ -15124,6 +15125,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, virStorageSourceFree(disk->mirror); disk->mirror = NULL; disk->mirroring = false; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; } /* With synchronous block cancel, we must synthesize an event, and @@ -15396,6 +15398,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, need_unlink = false; disk->mirror = mirror; mirror = NULL; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY; endjob: if (need_unlink && unlink(dest)) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e4845ba..ed2991a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1020,26 +1020,30 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias); if (disk) { + if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT && + disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) + type = disk->mirrorJob; path = virDomainDiskGetSource(disk); event = virDomainEventBlockJobNewFromObj(vm, path, type, status); /* XXX If we completed a block pull or commit, then recompute * the cached backing chain to match. 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. For that matter, if - * qemu gains support for committing the active layer, we have - * to update disk->src. */ + * the chain ourselves rather than reprobing, but we haven't + * quite completed that conversion to use our XML tracking. */ if ((type == VIR_DOMAIN_BLOCK_JOB_TYPE_PULL || - type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT) && + type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT || + type == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) && status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) qemuDomainDetermineDiskChain(driver, vm, disk, true); - if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + if (disk->mirror && + (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY || + type == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)) { if (status == VIR_DOMAIN_BLOCK_JOB_READY) { disk->mirroring = true; } else if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { virStorageSourceFree(disk->mirror); disk->mirror = NULL; disk->mirroring = false; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; } } } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml new file mode 100644 index 0000000..6ba5724 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml @@ -0,0 +1,37 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/tmp/HostVG/QEMUGuest1-snap'/> + <backingStore type='block' index='1'> + <format type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <backingStore/> + </backingStore> + <mirror type='block' job='active-commit'> + <format type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + </mirror> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml index a937d0a..6c28687 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml @@ -17,7 +17,7 @@ <disk type='block' device='disk'> <source dev='/dev/HostVG/QEMUGuest1'/> <backingStore/> - <mirror type='file' ready='yes'> + <mirror type='file' job='copy' ready='yes'> <source file='/dev/HostVG/QEMUGuest1Copy'/> </mirror> <target dev='hda' bus='ide'/> @@ -33,7 +33,7 @@ <disk type='file' device='disk'> <source file='/tmp/data.img'/> <backingStore/> - <mirror type='file'> + <mirror type='file' job='copy'> <format type='qcow2'/> <source file='/tmp/copy.img'/> </mirror> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml index a937d0a..6c28687 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml @@ -17,7 +17,7 @@ <disk type='block' device='disk'> <source dev='/dev/HostVG/QEMUGuest1'/> <backingStore/> - <mirror type='file' ready='yes'> + <mirror type='file' job='copy' ready='yes'> <source file='/dev/HostVG/QEMUGuest1Copy'/> </mirror> <target dev='hda' bus='ide'/> @@ -33,7 +33,7 @@ <disk type='file' device='disk'> <source file='/tmp/data.img'/> <backingStore/> - <mirror type='file'> + <mirror type='file' job='copy'> <format type='qcow2'/> <source file='/tmp/copy.img'/> </mirror> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 200d50f..98b2c15 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -227,6 +227,7 @@ mymain(void) DO_TEST_DIFFERENT("disk-mirror-old"); DO_TEST_FULL("disk-mirror", false, WHEN_ACTIVE); DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE); + DO_TEST_FULL("disk-active-commit", false, WHEN_ACTIVE); DO_TEST("graphics-listen-network"); DO_TEST("graphics-vnc"); DO_TEST("graphics-vnc-websocket"); -- 1.9.3
On 06/06/14 00:52, Eric Blake wrote:
A future patch is going to wire up qemu active block commit jobs; but as they have similar events and are canceled/pivoted in the same way as block copy jobs, it is easiest to track all bookkeeping for the commit job by reusing the <mirror> element. This patch adds domain XML to track which job was responsible for creating a mirroring situation, and adds a job='copy' attribute to all existing uses of <mirror>. Along the way, it also massages the qemu monitor backend to read the new field in order to generate the correct type of libvirt job (even though it requires a future patch to actually cause a qemu event that can be reported as an active commit).
* docs/schemas/domaincommon.rng: Enhance schema. * docs/formatdomain.html.in: Document it. * src/conf/domain_conf.h (_virDomainDiskDef): Add a field. * src/conf/domain_conf.c (virDomainBlockJobType): String conversion. (virDomainDiskDefParseXML): Parse job type. (virDomainDiskDefFormat): Output job type. * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Distinguish active from regular commit. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Set job type. (qemuDomainBlockPivot, qemuDomainBlockJobImpl): Clean up job type on completion. * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml: Update tests. * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Likewise. * tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml: New file. * tests/qemuxml2xmltest.c (mymain): Drive new test.
Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/formatdomain.html.in | 15 +++++---- docs/schemas/domaincommon.rng | 6 ++++ src/conf/domain_conf.c | 31 ++++++++++++++++-- src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 3 ++ src/qemu/qemu_process.c | 18 +++++++---- .../qemuxml2argv-disk-active-commit.xml | 37 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4 +-- .../qemuxml2xmlout-disk-mirror-old.xml | 4 +-- tests/qemuxml2xmltest.c | 1 + 10 files changed, 100 insertions(+), 20 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1b6ced8..3bf1f6d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1878,13 +1878,16 @@ </dd> <dt><code>mirror</code></dt> <dd> - This element is present if the hypervisor has started a block - copy operation (via the <code>virDomainBlockRebase</code> - API), where the mirror location in the <code>source</code> - sub-element will eventually have the same contents as the - source, and with the file format in the + This element is present if the hypervisor has started a + long-running block job operation, where the mirror location in + the <code>source</code> sub-element will eventually have the + same contents as the source, and with the file format in the sub-element <code>format</code> (which might differ from the - format of the source). The details of the <code>source</code> + format of the source). The <code>job</code> attribute + mentions which API started the operation ("copy" + for the <code>virDomainBlockRebase</code> API, or "commit" + for the <code>virDomainBlockCommit</code> API). The details of + the <code>source</code>
Although this is HTML the rest of the paragraph should be re-flowed.
sub-element are determined by the <code>type</code> attribute of the mirror, similar to what is done for the overall <code>disk</code> device element. If
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2b7dc26..92009ce 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -760,6 +760,11 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST, "unmap", "ignore")
+/* Internal mapping of block job types associated with <mirror> */
Probably worth mentioning, that the values will be present in the XML and shouldn't be changed.
+VIR_ENUM_DECL(virDomainBlockJob) +VIR_ENUM_IMPL(virDomainBlockJob, VIR_DOMAIN_BLOCK_JOB_TYPE_LAST, + "", "", "copy", "", "active-commit") + #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE
@@ -15191,9 +15212,13 @@ virDomainDiskDefFormat(virBufferPtr buf, * new style similar to backingStore, even though the parser * code still accepts old style across libvirtd upgrades. */ if (def->mirror && !(flags & VIR_DOMAIN_XML_INACTIVE)) { - virBufferAsprintf(buf, "<mirror type='%s'%s>\n", - virStorageTypeToString(def->mirror->type), - def->mirroring ? " ready='yes'" : ""); + virBufferAsprintf(buf, "<mirror type='%s'", + virStorageTypeToString(def->mirror->type)); + virBufferEscapeString(buf, " job='%s'", + virDomainBlockJobTypeToString(def->mirrorJob)); + if (def->mirroring) + virBufferAddLit(buf, " ready='yes'"); + virBufferAddLit(buf, ">\n");
Hmmm in one of your previous patches you've changed the lines above to be part of the deleted virBufferAsprintf and now you are returning them. But not worth changing the patch due to this.
virBufferAdjustIndent(buf, 2); if (def->mirror->format) virBufferEscapeString(buf, "<format type='%s'/>\n",
ACK with the two comment/docs nits addressed. Peter
With this in place, I can (finally!) now do: virsh blockcommit $dom vda --shallow --wait --verbose --pivot and watch qemu shorten the backing chain by one, followed by libvirt automatically updating the dumpxml output, effectively undoing the work of virsh snapshot-commit --no-metadata --disk-only. Commit is SOOOO much faster than blockpull, when I'm still fairly close in time to when the temporary qcow2 wrapper file was created via a snapshot operation! * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Implement live commit to regular files. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5903144..c0fc5e9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15492,9 +15492,11 @@ qemuDomainBlockCommit(virDomainPtr dom, unsigned int baseIndex = 0; const char *top_parent = NULL; bool clean_access = false; + virStorageSourcePtr mirror = NULL; - /* XXX Add support for COMMIT_ACTIVE, COMMIT_DELETE */ - virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1); + /* XXX Add support for COMMIT_DELETE */ + virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | + VIR_DOMAIN_BLOCK_COMMIT_ACTIVE, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -15540,13 +15542,14 @@ qemuDomainBlockCommit(virDomainPtr dom, &top_parent))) goto endjob; - /* FIXME: qemu 2.0 supports active commit, but as a two-stage - * process; qemu 2.1 is further improving active commit. We need - * to start supporting it in libvirt. */ if (topSource == disk->src) { /* We assume that no one will backport qemu 2.0 active commit * to an earlier qemu without also backporting the block job - * ready event; but this makes sure of that fact */ + * ready event; but this makes sure of that fact. + * + * XXX Also need to check other capability bit(s): qemu 1.7 + * supports async blockjob but not active commit; and qemu 2.0 + * active commit misbehaves on 0-byte file. */ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("active commit not supported with this QEMU binary")); @@ -15559,6 +15562,14 @@ qemuDomainBlockCommit(virDomainPtr dom, disk->dst); goto endjob; } + if (disk->mirror) { + virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, + _("disk '%s' already in active block job"), + disk->dst); + goto endjob; + } + if (VIR_ALLOC(mirror) < 0) + goto endjob; } if (!topSource->backingStore) { @@ -15600,6 +15611,21 @@ qemuDomainBlockCommit(virDomainPtr dom, VIR_DISK_CHAIN_READ_WRITE) < 0)) goto endjob; + /* For an active commit, clone enough of the base to act as the mirror */ + if (mirror) { + /* XXX Support network commits */ + if (baseSource->type != VIR_STORAGE_TYPE_FILE && + baseSource->type != VIR_STORAGE_TYPE_BLOCK) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("active commit to non-file not supported yet")); + goto endjob; + } + mirror->type = baseSource->type; + if (VIR_STRDUP(mirror->path, baseSource->path) < 0) + goto endjob; + mirror->format = baseSource->format; + } + /* Start the commit operation. Pass the user's original spelling, * if any, through to qemu, since qemu may behave differently * depending on whether the input was specified as relative or @@ -15612,6 +15638,12 @@ qemuDomainBlockCommit(virDomainPtr dom, bandwidth); qemuDomainObjExitMonitor(driver, vm); + if (ret == 0 && mirror) { + disk->mirror = mirror; + mirror = NULL; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; + } + endjob: if (ret < 0 && clean_access) { /* Revert access to read-only, if possible. */ @@ -15622,6 +15654,7 @@ qemuDomainBlockCommit(virDomainPtr dom, top_parent, VIR_DISK_CHAIN_READ_ONLY); } + virStorageSourceFree(mirror); if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; -- 1.9.3
On 06/06/14 00:52, Eric Blake wrote:
With this in place, I can (finally!) now do:
virsh blockcommit $dom vda --shallow --wait --verbose --pivot
and watch qemu shorten the backing chain by one, followed by libvirt automatically updating the dumpxml output, effectively undoing the work of virsh snapshot-commit --no-metadata --disk-only. Commit is SOOOO much faster than blockpull, when I'm still fairly close in time to when the temporary qcow2 wrapper file was created via a snapshot operation!
* src/qemu/qemu_driver.c (qemuDomainBlockCommit): Implement live commit to regular files.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5903144..c0fc5e9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
@@ -15540,13 +15542,14 @@ qemuDomainBlockCommit(virDomainPtr dom, &top_parent))) goto endjob;
- /* FIXME: qemu 2.0 supports active commit, but as a two-stage - * process; qemu 2.1 is further improving active commit. We need - * to start supporting it in libvirt. */ if (topSource == disk->src) { /* We assume that no one will backport qemu 2.0 active commit * to an earlier qemu without also backporting the block job - * ready event; but this makes sure of that fact */ + * ready event; but this makes sure of that fact. + * + * XXX Also need to check other capability bit(s): qemu 1.7 + * supports async blockjob but not active commit; and qemu 2.0 + * active commit misbehaves on 0-byte file. */ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("active commit not supported with this QEMU binary"));
ACK once you figure out the detection of the capability. Peter
participants (2)
-
Eric Blake -
Peter Krempa