On 10/17/2012 06:30 PM, Eric Blake wrote: > Previously, snapshot code did its own permission granting (lock > manager, cgroup device controller, and security manager labeling) > inline. But now that we are adding block-commit and block-copy > which also have to change permissions, it's better to reuse > common code for the task. While snapshot should fall back to > no access if read-write access failed, block-commit will want to > fall back to read-only access. The common code doesn't know > whether failure to grant read-write access should revert to no > access (snapshot, block-copy) or read-only access (block-commit). > This code can also be used to revoke access to unused files after > block-pull. > > * src/qemu/qemu_driver.c (qemuDomainPrepareDiskChainElement): New > function. > (qemuDomainSnapshotCreateSingleDiskActive) > (qemuDomainSnapshotUndoSingleDiskActive): Use it. > --- > > v3: rebase to earlier patches in series > > src/qemu/qemu_driver.c | 101 ++++++++++++++++++++++++++++++++----------------- > 1 file changed, 66 insertions(+), 35 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 73804fe..072ec17 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -10453,6 +10453,66 @@ cleanup: > return ret; > } > > +typedef enum { > + VIR_DISK_CHAIN_NO_ACCESS, > + VIR_DISK_CHAIN_READ_ONLY, > + VIR_DISK_CHAIN_READ_WRITE, > +} qemuDomainDiskChainMode; > + > +/* Several operations end up adding or removing a single element of a > + * disk backing file chain; this helper function ensures that the lock > + * manager, cgroup device controller, and security manager labelling > + * are all aware of each new file before it is added to a chain, and > + * can revoke access to a file no longer needed in a chain. */ > +static int > +qemuDomainPrepareDiskChainElement(struct qemud_driver *driver, > + virDomainObjPtr vm, > + virCgroupPtr cgroup, > + virDomainDiskDefPtr disk, > + char *file, > + qemuDomainDiskChainMode mode) > +{ > + /* The easiest way to label a single file with the same > + * permissions it would have as if part of the disk chain is to > + * temporarily modify the disk in place. */ > + char *origsrc = disk->src; > + int origformat = disk->format; > + virStorageFileMetadataPtr origchain = disk->backingChain; > + bool origreadonly = disk->readonly; > + int ret = -1; > + > + disk->src = file; > + disk->format = VIR_STORAGE_FILE_RAW; > + disk->backingChain = NULL; > + 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); > + if (cgroup && qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) > + VIR_WARN("Failed to teardown cgroup for disk path %s", disk->src); > + if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) > + VIR_WARN("Unable to release lock on %s", disk->src); > + } else if (virDomainLockDiskAttach(driver->lockManager, driver->uri, > + vm, disk) < 0 || > + (cgroup && qemuSetupDiskCgroup(vm, cgroup, disk) < 0) || > + virSecurityManagerSetImageLabel(driver->securityManager, > + vm->def, disk) < 0) { > + goto cleanup; > + } > + > + ret = 0; > + > +cleanup: > + disk->src = origsrc; > + disk->format = origformat; > + disk->backingChain = origchain; > + disk->readonly = origreadonly; > + return ret; > +} > + > + > static bool > qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) > { > @@ -10762,8 +10822,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, > char *persistSource = NULL; > int ret = -1; > int fd = -1; > - char *origsrc = NULL; > - int origdriver; > bool need_unlink = false; > > if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { > @@ -10790,10 +10848,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, > VIR_FORCE_CLOSE(fd); > } > > - origsrc = disk->src; > - disk->src = source; > - origdriver = disk->format; > - disk->format = VIR_STORAGE_FILE_RAW; /* Don't want to probe backing files */ > /* XXX Here, we know we are about to alter disk->backingChain if > * successful, so we nuke the existing chain so that future > * commands will recompute it. Better would be storing the chain > @@ -10803,27 +10857,13 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, > virStorageFileFreeMetadata(disk->backingChain); > disk->backingChain = NULL; > > - if (virDomainLockDiskAttach(driver->lockManager, driver->uri, > - vm, disk) < 0) > - goto cleanup; > - if (cgroup && qemuSetupDiskCgroup(vm, cgroup, disk) < 0) { > - if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) > - VIR_WARN("Unable to release lock on %s", source); > - goto cleanup; > - } > - if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def, > - disk) < 0) { > - if (cgroup && qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) > - VIR_WARN("Failed to teardown cgroup for disk path %s", source); > - if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) > - VIR_WARN("Unable to release lock on %s", source); > + if (qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, source, > + VIR_DISK_CHAIN_READ_WRITE) < 0) { > + qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, source, > + VIR_DISK_CHAIN_NO_ACCESS); > goto cleanup; > } > > - disk->src = origsrc; > - origsrc = NULL; > - disk->format = origdriver; > - > /* create the actual snapshot */ > if (snap->format) > formatStr = virStorageFileFormatTypeToString(snap->format); > @@ -10847,10 +10887,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, > } > > cleanup: > - if (origsrc) { > - disk->src = origsrc; > - disk->format = origdriver; > - } > if (need_unlink && unlink(source)) > VIR_WARN("unable to unlink just-created %s", source); > VIR_FREE(device); > @@ -10882,13 +10918,8 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver, > goto cleanup; > } > > - if (virSecurityManagerRestoreImageLabel(driver->securityManager, > - vm->def, disk) < 0) > - VIR_WARN("Unable to restore security label on %s", disk->src); > - if (cgroup && qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) > - VIR_WARN("Failed to teardown cgroup for disk path %s", disk->src); > - if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) > - VIR_WARN("Unable to release lock on %s", disk->src); > + qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, origdisk->src, > + VIR_DISK_CHAIN_NO_ACCESS); > if (need_unlink && stat(disk->src, &st) == 0 && > S_ISREG(st.st_mode) && unlink(disk->src) < 0) > VIR_WARN("Unable to remove just-created %s", disk->src); ACK, with qualifying remarks from the review of the previous version of the patch (PATCHv2 17/16) -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list