The error would be overwritten otherwise producing a meaningless error message. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1302171 --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0bf185644..30d947b3e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16657,6 +16657,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, virQEMUDriverConfigPtr cfg = NULL; const char *format = NULL; int desttype = virStorageSourceGetActualType(mirror); + virErrorPtr monitor_error = NULL; /* Preliminaries: find the disk we are editing, sanity checks */ virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | @@ -16801,6 +16802,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) { + monitor_error = virSaveLastError(); qemuDomainDiskChainElementRevoke(driver, vm, mirror); goto endjob; } @@ -16821,6 +16823,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, VIR_WARN("unable to unlink just-created %s", mirror->path); virStorageSourceFree(mirror); qemuDomainObjEndJob(driver, vm); + if (monitor_error) { + virSetError(monitor_error); + virFreeError(monitor_error); + } cleanup: VIR_FREE(device); -- 2.11.0

s/MD5 checkout/MD5 hash/ --- src/locking/sanlock.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/locking/sanlock.conf b/src/locking/sanlock.conf index 3a1a51c5f..3c356bef9 100644 --- a/src/locking/sanlock.conf +++ b/src/locking/sanlock.conf @@ -18,7 +18,7 @@ # The default location in which lockspaces are created when # automatic lease creation is enabled. For each unique disk # path, a file $LEASE_DIR/NNNNNNNNNNNNNN will be created -# where 'NNNNNNNNNNNNNN' is the MD5 checkout of the disk path. +# where 'NNNNNNNNNNNNNN' is the MD5 hash of the disk path. # # If this directory is on local storage, it will only protect # against a VM being started twice on the same host, or two -- 2.11.0

Libvirt is able to properly model what happens to the backing chain after a snapshot so there's no real need to redetect the data. Additionally with the _REUSE_EXT flag this might end up in redetecting wrong data if the user puts wrong backing chain reference into the snapshot image. --- src/qemu/qemu_driver.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 30d947b3e..8477ed0dd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14324,7 +14324,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, bool persist = false; bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; virQEMUDriverConfigPtr cfg = NULL; - virErrorPtr orig_err = NULL; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -14409,19 +14408,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, } } - /* recheck backing chains of all disks involved in the snapshot */ - orig_err = virSaveLastError(); - for (i = 0; i < snap->def->ndisks; i++) { - if (snap->def->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) - continue; - ignore_value(qemuDomainDetermineDiskChain(driver, vm, vm->def->disks[i], - true, true)); - } - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } - if (ret == 0 || !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0 || (persist && virDomainSaveConfig(cfg->configDir, driver->caps, -- 2.11.0

The code at first changed the definition and then rolled it back in case of failure. This was ridiculous. Refactor the code so that the image in the definition is changed only when the snapshot is successful. The refactor will also simplify further fix of image locking when doing snapshots. --- src/qemu/qemu_driver.c | 352 ++++++++++++++++++++++++++++--------------------- 1 file changed, 203 insertions(+), 149 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8477ed0dd..742b6ceda 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14156,75 +14156,186 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, } +struct _qemuDomainSnapshotDiskData { + virStorageSourcePtr src; + bool initialized; /* @src was initialized in the storage driver */ + bool created; /* @src was created by the snapshot code */ + bool prepared; /* @src was prepared using qemuDomainDiskChainElementPrepare */ + virDomainDiskDefPtr disk; + + virStorageSourcePtr persistsrc; + virDomainDiskDefPtr persistdisk; +}; + +typedef struct _qemuDomainSnapshotDiskData qemuDomainSnapshotDiskData; +typedef qemuDomainSnapshotDiskData *qemuDomainSnapshotDiskDataPtr; + + +static void +qemuDomainSnapshotDiskDataFree(qemuDomainSnapshotDiskDataPtr data, + size_t ndata, + virQEMUDriverPtr driver, + virDomainObjPtr vm) +{ + size_t i; + + if (!data) + return; + + for (i = 0; i < ndata; i++) { + /* on success of the snapshot the 'src' and 'persistsrc' properties will + * be set to NULL by qemuDomainSnapshotUpdateDiskSources */ + if (data[i].src) { + if (data[i].initialized) + virStorageFileDeinit(data[i].src); + + if (data[i].prepared) + qemuDomainDiskChainElementRevoke(driver, vm, data[i].src); + + virStorageSourceFree(data[i].src); + } + virStorageSourceFree(data[i].persistsrc); + } + + VIR_FREE(data); +} + + +/** + * qemuDomainSnapshotDiskDataCollect: + * + * Collects and prepares a list of structures that hold information about disks + * that are selected for the snapshot. + */ +static qemuDomainSnapshotDiskDataPtr +qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap) +{ + size_t i; + qemuDomainSnapshotDiskDataPtr ret; + qemuDomainSnapshotDiskDataPtr dd; + + if (VIR_ALLOC_N(ret, snap->def->ndisks) < 0) + return NULL; + + for (i = 0; i < snap->def->ndisks; i++) { + if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) + continue; + + dd = ret + i; + + dd->disk = vm->def->disks[i]; + + if (!(dd->src = virStorageSourceCopy(snap->def->disks[i].src, false))) + goto error; + + if (virStorageSourceInitChainElement(dd->src, dd->disk->src, false) < 0) + goto error; + + if (qemuDomainStorageFileInit(driver, vm, dd->src) < 0) + goto error; + + dd->initialized = true; + + /* Note that it's unsafe to assume that the disks in the persistent + * definition match up with the disks in the live definition just by + * checking that the target name is the same. We've done that + * historically this way though. */ + if (vm->newDef && + (dd->persistdisk = virDomainDiskByName(vm->newDef, dd->disk->dst, + false))) { + + if (!(dd->persistsrc = virStorageSourceCopy(dd->src, false))) + goto error; + + if (virStorageSourceInitChainElement(dd->persistsrc, + dd->persistdisk->src, false) < 0) + goto error; + } + } + + return ret; + + error: + qemuDomainSnapshotDiskDataFree(ret, snap->def->ndisks, driver, vm); + return NULL; +} + + +/** + * qemuDomainSnapshotUpdateDiskSources: + * @dd: snapshot disk data object + * @persist: set to true if persistent config of the VM was changed + * + * Updates disk definition after a successful snapshot. + */ +static void +qemuDomainSnapshotUpdateDiskSources(qemuDomainSnapshotDiskDataPtr dd, + bool *persist) +{ + if (!dd->src) + return; + + /* storage driver access won'd be needed */ + if (dd->initialized) + virStorageFileDeinit(dd->src); + + dd->src->backingStore = dd->disk->src; + dd->disk->src = dd->src; + dd->src = NULL; + + if (dd->persistdisk) { + dd->persistsrc->backingStore = dd->persistdisk->src; + dd->persistdisk->src = dd->persistsrc; + dd->persistsrc = NULL; + *persist = true; + } +} + + /* The domain is expected to hold monitor lock. */ static int qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainSnapshotDiskDefPtr snap, - virDomainDiskDefPtr disk, - virDomainDiskDefPtr persistDisk, + qemuDomainSnapshotDiskDataPtr dd, virJSONValuePtr actions, bool reuse, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; - virStorageSourcePtr newDiskSrc = NULL; - virStorageSourcePtr persistDiskSrc = NULL; char *device = NULL; char *source = NULL; const char *formatStr = NULL; int ret = -1, rc; - bool need_unlink = false; - if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("unexpected code path")); - return -1; - } - - if (!(device = qemuAliasFromDisk(disk))) - goto cleanup; - - if (!(newDiskSrc = virStorageSourceCopy(snap->src, false))) - goto cleanup; - - if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) + if (!(device = qemuAliasFromDisk(dd->disk))) goto cleanup; - if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) + if (qemuGetDriveSourceString(dd->src, NULL, &source) < 0) goto cleanup; - if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) - goto cleanup; - - if (persistDisk) { - if (!(persistDiskSrc = virStorageSourceCopy(snap->src, false))) - goto cleanup; - - if (virStorageSourceInitChainElement(persistDiskSrc, persistDisk->src, - false) < 0) - goto cleanup; - } - /* pre-create the image file so that we can label it before handing it to qemu */ - if (!reuse && newDiskSrc->type != VIR_STORAGE_TYPE_BLOCK) { - if (virStorageFileCreate(newDiskSrc) < 0) { + if (!reuse && dd->src->type != VIR_STORAGE_TYPE_BLOCK) { + if (virStorageFileCreate(dd->src) < 0) { virReportSystemError(errno, _("failed to create image file '%s'"), source); goto cleanup; } - need_unlink = true; + dd->created = true; } /* set correct security, cgroup and locking options on the new image */ - if (qemuDomainDiskChainElementPrepare(driver, vm, newDiskSrc, false) < 0) { - qemuDomainDiskChainElementRevoke(driver, vm, newDiskSrc); + if (qemuDomainDiskChainElementPrepare(driver, vm, dd->src, false) < 0) { + qemuDomainDiskChainElementRevoke(driver, vm, dd->src); goto cleanup; } + dd->prepared = true; + /* create the actual snapshot */ - if (newDiskSrc->format) - formatStr = virStorageFileFormatTypeToString(newDiskSrc->format); + if (dd->src->format) + formatStr = virStorageFileFormatTypeToString(dd->src->format); /* The monitor is only accessed if qemu doesn't support transactions. * Otherwise the following monitor command only constructs the command. @@ -14240,74 +14351,15 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, ret = -1; } - virDomainAuditDisk(vm, disk->src, snap->src, "snapshot", rc >= 0); - if (ret < 0) - goto cleanup; - - /* Update vm in place to match changes. */ - need_unlink = false; - - newDiskSrc->backingStore = disk->src; - disk->src = newDiskSrc; - newDiskSrc = NULL; - - if (persistDisk) { - persistDiskSrc->backingStore = persistDisk->src; - persistDisk->src = persistDiskSrc; - persistDiskSrc = NULL; - } + virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", rc >= 0); cleanup: - if (need_unlink && virStorageFileUnlink(newDiskSrc)) - VIR_WARN("unable to unlink just-created %s", source); - virStorageFileDeinit(newDiskSrc); - virStorageSourceFree(newDiskSrc); - virStorageSourceFree(persistDiskSrc); VIR_FREE(device); VIR_FREE(source); return ret; } -/* The domain is expected to hold monitor lock. This is the - * counterpart to qemuDomainSnapshotCreateSingleDiskActive, called - * only on a failed transaction. */ -static void -qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - virDomainDiskDefPtr persistDisk, - bool need_unlink) -{ - virStorageSourcePtr tmp; - struct stat st; - - ignore_value(virStorageFileInit(disk->src)); - - qemuDomainDiskChainElementRevoke(driver, vm, disk->src); - - 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); - - virStorageFileDeinit(disk->src); - - /* Update vm in place to match changes. */ - tmp = disk->src; - disk->src = tmp->backingStore; - tmp->backingStore = NULL; - virStorageSourceFree(tmp); - - if (persistDisk) { - tmp = persistDisk->src; - persistDisk->src = tmp->backingStore; - tmp->backingStore = NULL; - virStorageSourceFree(tmp); - } -} - - /* The domain is expected to be locked and active. */ static int qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, @@ -14324,6 +14376,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, bool persist = false; bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; virQEMUDriverConfigPtr cfg = NULL; + qemuDomainSnapshotDiskDataPtr diskdata = NULL; + virErrorPtr orig_err = NULL; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -14341,81 +14395,81 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, return -1; } + /* prepare a list of objects to use in the vm definition so that we don't + * have to roll back later */ + if (!(diskdata = qemuDomainSnapshotDiskDataCollect(driver, vm, snap))) + return -1; + cfg = virQEMUDriverGetConfig(driver); - /* No way to roll back if first disk succeeds but later disks - * fail, unless we have transaction support. - * Based on earlier qemuDomainSnapshotPrepare, all - * disks in this list are now either SNAPSHOT_NO, or - * SNAPSHOT_EXTERNAL with a valid file name and qcow2 format. */ + /* Based on earlier qemuDomainSnapshotPrepare, all disks in this list are + * now either SNAPSHOT_NO, or SNAPSHOT_EXTERNAL with a valid file name and + * qcow2 format. */ for (i = 0; i < snap->def->ndisks; i++) { - virDomainDiskDefPtr persistDisk = NULL; - if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; - if (vm->newDef && - (persistDisk = virDomainDiskByName(vm->newDef, - vm->def->disks[i]->dst, - false))) - persist = true; ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, - &snap->def->disks[i], - vm->def->disks[i], - persistDisk, actions, - reuse, asyncJob); + &diskdata[i], + actions, reuse, asyncJob); + + /* without transaction support the change can't be rolled back */ + if (!actions) + qemuDomainSnapshotUpdateDiskSources(&diskdata[i], &persist); + if (ret < 0) - break; + goto cleanup; do_transaction = true; } - if (actions) { - if (ret == 0 && do_transaction) { - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { - ret = qemuMonitorTransaction(priv->mon, actions); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; - } else { - /* failed to enter monitor, clean stuff up and quit */ - ret = -1; - } - } else { - VIR_DEBUG("no disks to snapshot, skipping 'transaction' command"); + + if (actions && do_transaction) { + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + goto cleanup; + + ret = qemuMonitorTransaction(priv->mon, actions); + + if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) { + ret = -1; + goto cleanup; } - virJSONValueFree(actions); + for (i = 0; i < snap->def->ndisks; i++) + qemuDomainSnapshotUpdateDiskSources(&diskdata[i], &persist); + } - if (ret < 0) { - /* Transaction failed; undo the changes to vm. */ - bool need_unlink = !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT); - while (i-- > 0) { - virDomainDiskDefPtr persistDisk = NULL; + cleanup: + if (ret < 0) { + orig_err = virSaveLastError(); + for (i = 0; i < snap->def->ndisks; i++) { + if (!diskdata[i].src) + continue; - if (snap->def->disks[i].snapshot == - VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) - continue; - if (vm->newDef && - (persistDisk = virDomainDiskByName(vm->newDef, - vm->def->disks[i]->dst, - false))) - persist = true; - - qemuDomainSnapshotUndoSingleDiskActive(driver, vm, - vm->def->disks[i], - persistDisk, - need_unlink); - } + if (diskdata[i].prepared) + qemuDomainDiskChainElementRevoke(driver, vm, diskdata[i].src); + + if (diskdata[i].created && + virStorageFileUnlink(diskdata[i].src) < 0) + VIR_WARN("Unable to remove just-created %s", diskdata[i].src->path); } } - if (ret == 0 || !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { + if (ret == 0 || !actions) { if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0 || (persist && virDomainSaveConfig(cfg->configDir, driver->caps, vm->newDef) < 0)) ret = -1; } + + qemuDomainSnapshotDiskDataFree(diskdata, snap->def->ndisks, driver, vm); + virJSONValueFree(actions); virObjectUnref(cfg); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + return ret; } -- 2.11.0

On 12/16/2016 11:24 AM, Peter Krempa wrote: Should this be disk.src? Or handle the _NONE case? since src only gets filled in for that. ACK in principal... John

For the blockjobs, where libvirt is able to track the state internally we can fix locking of images we can remove the appropriate locks. Also when doing a pivoting operation we should not acquire the lock on any of those images since both are actually locked already. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1302168 --- src/qemu/qemu_blockjob.c | 26 ++++++++++++++++---------- src/qemu/qemu_driver.c | 4 +--- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 83a5a3f7c..4ada4cd27 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -33,6 +33,7 @@ #include "virstoragefile.h" #include "virthread.h" #include "virtime.h" +#include "locking/domain_lock.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -139,17 +140,19 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, } } - /* XXX We want to revoke security labels and disk - * lease, as well as audit that revocation, before - * dropping the original source. But it gets tricky - * if both source and mirror share common backing - * files (we want to only revoke the non-shared - * portion of the chain); so for now, we leak the - * access to the original. */ + /* XXX We want to revoke security labels as well as audit that + * revocation, before dropping the original source. But it gets + * tricky if both source and mirror share common backing files (we + * want to only revoke the non-shared portion of the chain); so for + * now, we leak the access to the original. */ + virDomainLockImageDetach(driver->lockManager, vm, disk->src); virStorageSourceFree(disk->src); disk->src = disk->mirror; } else { - virStorageSourceFree(disk->mirror); + if (disk->mirror) { + virDomainLockImageDetach(driver->lockManager, vm, disk->mirror); + virStorageSourceFree(disk->mirror); + } } /* Recompute the cached backing chain to match our @@ -172,8 +175,11 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, case VIR_DOMAIN_BLOCK_JOB_FAILED: case VIR_DOMAIN_BLOCK_JOB_CANCELED: - virStorageSourceFree(disk->mirror); - disk->mirror = NULL; + if (disk->mirror) { + virDomainLockImageDetach(driver->lockManager, vm, disk->mirror); + virStorageSourceFree(disk->mirror); + disk->mirror = NULL; + } disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; save = true; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 13da035c2..dcb010e9a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16206,9 +16206,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, if (disk->mirror->format && disk->mirror->format != VIR_STORAGE_FILE_RAW && - (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, - disk) < 0 || - qemuDomainNamespaceSetupDisk(driver, vm, disk) < 0 || + (qemuDomainNamespaceSetupDisk(driver, vm, disk) < 0 || qemuSetupDiskCgroup(vm, disk) < 0 || qemuSecuritySetDiskLabel(driver, vm, disk) < 0)) goto cleanup; -- 2.11.0