[libvirt] [PATCHv2 17/16] blockjob: refactor qemu disk chain permission grants

Laine Stump laine at laine.org
Thu Oct 18 00:09:40 UTC 2012


On 10/16/2012 06:03 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.
> ---
>  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 04b28a1..59c475e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10452,6 +10452,65 @@ 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.  */
> +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->chain;
> +    bool origreadonly = disk->readonly;
> +    int ret = -1;
> +
> +    disk->src = file;
> +    disk->format = VIR_STORAGE_FILE_RAW;
> +    disk->chain = 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(driver, 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);

This feels like a bit of a hackish way to get around an inadequate
security driver (and lock and cgroup?) API by hijacking what's
available. But then I haven't looked at just how deep the changes would
need to be to make it work properly, so I won't dismiss it out of hand.
It seems like something that should require a promise of later cleanup
though :-)


> +    } else if (virDomainLockDiskAttach(driver->lockManager, driver->uri,
> +                                       vm, disk) < 0 ||
> +               (cgroup && qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) ||
> +               virSecurityManagerSetImageLabel(driver->securityManager,
> +                                               vm->def, disk) < 0) {
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> +cleanup:
> +    disk->src = origsrc;
> +    disk->format = origformat;
> +    disk->chain = origchain;
> +    disk->readonly = origreadonly;
> +    return ret;
> +}
> +
> +
>  static bool
>  qemuDomainSnapshotIsAllowed(virDomainObjPtr vm)
>  {
> @@ -10742,6 +10801,7 @@ cleanup:
>      return ret;
>  }
>
> +
>  /* The domain is expected to hold monitor lock.  */
>  static int
>  qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
> @@ -10761,8 +10821,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) {
> @@ -10789,10 +10847,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->chain if
>       * successful, so we nuke the existing chain so that future
>       * commands will recompute it.  Better would be storing the chain
> @@ -10802,27 +10856,13 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
>      virStorageFileFreeMetadata(disk->chain);
>      disk->chain = NULL;
>
> -    if (virDomainLockDiskAttach(driver->lockManager, driver->uri,
> -                                vm, disk) < 0)
> -        goto cleanup;
> -    if (cgroup && qemuSetupDiskCgroup(driver, 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(driver, 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);
> @@ -10846,10 +10886,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);
> @@ -10881,13 +10917,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(driver, 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);




More information about the libvir-list mailing list