[libvirt] [PATCH 00/11] qemu: Labelling cleanup and fix labelling for block copy pivot (blockdev-add saga)
Refactor some labelling code and then move out the backing chain labelling from the block copy pivot operation into starting of the job. Peter Krempa (11): qemu: domain: Clarify temp variable scope in qemuDomainDetermineDiskChain qemu: domain: Allow overriding disk source in qemuDomainDetermineDiskChain qemu: cgroup: Change qemu[Setup|Teardown]DiskCgroup to take virStorageSource security: Remove security driver internals for disk labelling qemu: security: Add 'backingChain' flag to qemuSecurity[Set|Restore]ImageLabel qemu: security: Replace and remove qemuSecurity[Set|Restore]DiskLabel security: Remove disk labelling functions and fix callers qemu: driver: Remove disk source munging in qemuDomainBlockPivot locking: Use virDomainLockImage[Attach|Detach] instead of *Disk qemu: hotplug: Refactor qemuHotplugPrepareDiskAccess to work on virStorageSource qemu: Label backing chain of user-provided target of blockCopy when starting the job src/libvirt_private.syms | 4 -- src/libxl/libxl_driver.c | 14 +++--- src/locking/domain_lock.c | 17 ------- src/locking/domain_lock.h | 8 ---- src/lxc/lxc_controller.c | 3 +- src/lxc/lxc_driver.c | 4 +- src/qemu/qemu_blockjob.c | 2 +- src/qemu/qemu_cgroup.c | 14 +++--- src/qemu/qemu_cgroup.h | 8 ++-- src/qemu/qemu_domain.c | 60 +++++++++++++++--------- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 53 +++++++++------------ src/qemu/qemu_hotplug.c | 79 +++++++++++++------------------- src/qemu/qemu_process.c | 26 ++++++++++- src/qemu/qemu_security.c | 74 +++--------------------------- src/qemu/qemu_security.h | 14 ++---- src/security/security_apparmor.c | 24 ++-------- src/security/security_dac.c | 40 +++++----------- src/security/security_driver.h | 15 ++---- src/security/security_manager.c | 70 ++++------------------------ src/security/security_manager.h | 12 ++--- src/security/security_nop.c | 25 ++-------- src/security/security_selinux.c | 42 +++++------------ src/security/security_stack.c | 50 +++----------------- 24 files changed, 202 insertions(+), 457 deletions(-) -- 2.20.1
The function at first validates the top image of the chain, then traverses the chain as declared in the XML (if any) and then procedes to detect the rest of the chain from images. All of the steps have their own temporary iterator. Clarify the use scope of the steps by introducing a new temp variable holding the top level source and adding comments. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 32a43f2064..8e3d0dd374 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8945,43 +8945,49 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, bool report_broken) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - virStorageSourcePtr src = disk->src; - virStorageSourcePtr n; + virStorageSourcePtr disksrc = NULL; /* disk source */ + virStorageSourcePtr src; /* iterator for the backing chain declared in XML */ + virStorageSourcePtr n; /* iterator for the backing chain detected from disk */ qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; uid_t uid; gid_t gid; - if (virStorageSourceIsEmpty(src)) { + if (!disksrc) + disksrc = disk->src; + + src = disksrc; + + if (virStorageSourceIsEmpty(disksrc)) { ret = 0; goto cleanup; } /* There is no need to check the backing chain for disks without backing * support */ - if (virStorageSourceIsLocalStorage(src) && - src->format > VIR_STORAGE_FILE_NONE && - src->format < VIR_STORAGE_FILE_BACKING) { + if (virStorageSourceIsLocalStorage(disksrc) && + disksrc->format > VIR_STORAGE_FILE_NONE && + disksrc->format < VIR_STORAGE_FILE_BACKING) { - if (!virFileExists(src->path)) { + if (!virFileExists(disksrc->path)) { if (report_broken) - virStorageFileReportBrokenChain(errno, src, disk->src); + virStorageFileReportBrokenChain(errno, disksrc, disksrc); goto cleanup; } /* terminate the chain for such images as the code below would do */ - if (!src->backingStore && - VIR_ALLOC(src->backingStore) < 0) + if (!disksrc->backingStore && + VIR_ALLOC(disksrc->backingStore) < 0) goto cleanup; /* host cdrom requires special treatment in qemu, so we need to check * whether a block device is a cdrom */ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM && - src->format == VIR_STORAGE_FILE_RAW && - virStorageSourceIsBlockLocal(src) && - virFileIsCDROM(src->path) == 1) - src->hostcdrom = true; + disksrc->format == VIR_STORAGE_FILE_RAW && + virStorageSourceIsBlockLocal(disksrc) && + virFileIsCDROM(disksrc->path) == 1) + disksrc->hostcdrom = true; ret = 0; goto cleanup; @@ -8996,11 +9002,11 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, goto cleanup; if (rv > 0) { - if (qemuDomainStorageFileInit(driver, vm, src, disk->src) < 0) + if (qemuDomainStorageFileInit(driver, vm, src, disksrc) < 0) goto cleanup; if (virStorageFileAccess(src, F_OK) < 0) { - virStorageFileReportBrokenChain(errno, src, disk->src); + virStorageFileReportBrokenChain(errno, src, disksrc); virStorageFileDeinit(src); goto cleanup; } @@ -9018,7 +9024,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, goto cleanup; } - qemuDomainGetImageIds(cfg, vm, src, disk->src, &uid, &gid); + qemuDomainGetImageIds(cfg, vm, src, disksrc, &uid, &gid); if (virStorageFileGetMetadata(src, uid, gid, report_broken) < 0) goto cleanup; -- 2.20.1
On 1/23/19 11:10 AM, Peter Krempa wrote:
The function at first validates the top image of the chain, then traverses the chain as declared in the XML (if any) and then procedes to detect the rest of the chain from images. All of the steps have their own temporary iterator.
Clarify the use scope of the steps by introducing a new temp variable holding the top level source and adding comments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 32a43f2064..8e3d0dd374 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8945,43 +8945,49 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, bool report_broken) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - virStorageSourcePtr src = disk->src; - virStorageSourcePtr n; + virStorageSourcePtr disksrc = NULL; /* disk source */
NIT: Chould be disksrc = disk->src here and then in the next patch it's removed and [1] becomes part of the next patch.
+ virStorageSourcePtr src; /* iterator for the backing chain declared in XML */ + virStorageSourcePtr n; /* iterator for the backing chain detected from disk */ qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; uid_t uid; gid_t gid;
- if (virStorageSourceIsEmpty(src)) { + if (!disksrc) + disksrc = disk->src;
[1] always going to happen with this patch... Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
When we need to detect a chain for a image which will become the new source for a disk (e.g. after a disk media change or a blockjob) we'd need to replace disk->src temporarily to do so. Move the 'disksrc' temporary variable as an argument and adjust callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 2 +- src/qemu/qemu_domain.c | 15 ++++++++++++++- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_process.c | 4 ++-- 6 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 9b638b7ef6..e69aa1b6fb 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -287,7 +287,7 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver, disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; disk->src->id = 0; virStorageSourceBackingStoreClear(disk->src); - ignore_value(qemuDomainDetermineDiskChain(driver, vm, disk, true)); + ignore_value(qemuDomainDetermineDiskChain(driver, vm, disk, NULL, true)); ignore_value(qemuBlockNodeNamesDetect(driver, vm, asyncJob)); qemuBlockJobTerminate(job); } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8e3d0dd374..e42b93e051 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8938,14 +8938,27 @@ qemuDomainStorageAlias(const char *device, int depth) } +/** + * qemuDomainDetermineDiskChain: + * @driver: qemu driver object + * @vm: domain object + * @disk: disk definition + * @disksrc: source to determine the chain for, may be NULL + * @report_broken: report broken chain verbosely + * + * Prepares and initializes the backing chain of disk @disk. In cases where + * a new source is to be associated with @disk the @disksrc parameter can be + * used to override the source. If @report_broken is true missing images + * in the backing chain are reported. + */ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, + virStorageSourcePtr disksrc, bool report_broken) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - virStorageSourcePtr disksrc = NULL; /* disk source */ virStorageSourcePtr src; /* iterator for the backing chain declared in XML */ virStorageSourcePtr n; /* iterator for the backing chain detected from disk */ qemuDomainObjPrivatePtr priv = vm->privateData; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index defbffbf94..e7c5a0a49c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -742,6 +742,7 @@ int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, + virStorageSourcePtr disksrc, bool report_broken); bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 90319261ff..b254e96131 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17183,7 +17183,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, oldsrc = disk->src; disk->src = disk->mirror; - if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) + if (qemuDomainDetermineDiskChain(driver, vm, disk, disk->mirror, true) < 0) goto cleanup; if (disk->mirror->format && diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1df80fcab6..615105d595 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -820,7 +820,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, sharedAdded = true; - if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) + if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL, true) < 0) goto cleanup; if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) @@ -1197,7 +1197,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver, if (qemuSetUnprivSGIO(dev) < 0) goto cleanup; - if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) + if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL, true) < 0) goto cleanup; for (i = 0; i < vm->def->ndisks; i++) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8120201eb6..fb596d960f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6210,7 +6210,7 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, if (qemuDomainDiskIsMissingLocalOptional(disk) && cold_boot) VIR_INFO("optional disk '%s' source file is missing, " "skip checking disk chain", disk->dst); - else if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) + else if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL, true) >= 0) continue; if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0) @@ -8032,7 +8032,7 @@ qemuProcessReconnect(void *opaque) * qemuDomainDetermineDiskChain with @report_broken == false * to guarantee best-effort domain reconnect */ virStorageSourceBackingStoreClear(disk->src); - if (qemuDomainDetermineDiskChain(driver, obj, disk, false) < 0) + if (qemuDomainDetermineDiskChain(driver, obj, disk, NULL, false) < 0) goto error; } else { VIR_DEBUG("skipping backing chain detection for '%s'", disk->dst); -- 2.20.1
On 1/23/19 11:10 AM, Peter Krempa wrote:
When we need to detect a chain for a image which will become the new source for a disk (e.g. after a disk media change or a blockjob) we'd need to replace disk->src temporarily to do so.
Move the 'disksrc' temporary variable as an argument and adjust callers.
s/as an/to an API/
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 2 +- src/qemu/qemu_domain.c | 15 ++++++++++++++- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_process.c | 4 ++-- 6 files changed, 21 insertions(+), 7 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Since the disk is necessary only to get the source modify the functions to take the source directly and rename them to qemu[Setup|Teardown]ImageChainCgroup. Additionally drop a pointless comment containing the old function name. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_cgroup.c | 14 +++++++------- src/qemu/qemu_cgroup.h | 8 ++++---- src/qemu/qemu_domain.c | 3 --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- 5 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 4931fb6575..9ceecb884e 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -204,13 +204,13 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, int -qemuSetupDiskCgroup(virDomainObjPtr vm, - virDomainDiskDefPtr disk) +qemuSetupImageChainCgroup(virDomainObjPtr vm, + virStorageSourcePtr src) { virStorageSourcePtr next; bool forceReadonly = false; - for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) { + for (next = src; virStorageSourceIsBacking(next); next = next->backingStore) { if (qemuSetupImageCgroupInternal(vm, next, forceReadonly) < 0) return -1; @@ -223,12 +223,12 @@ qemuSetupDiskCgroup(virDomainObjPtr vm, int -qemuTeardownDiskCgroup(virDomainObjPtr vm, - virDomainDiskDefPtr disk) +qemuTeardownImageChainCgroup(virDomainObjPtr vm, + virStorageSourcePtr src) { virStorageSourcePtr next; - for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) { + for (next = src; virStorageSourceIsBacking(next); next = next->backingStore) { if (qemuTeardownImageCgroup(vm, next) < 0) return -1; } @@ -720,7 +720,7 @@ qemuSetupDevicesCgroup(virDomainObjPtr vm) goto cleanup; for (i = 0; i < vm->def->ndisks; i++) { - if (qemuSetupDiskCgroup(vm, vm->def->disks[i]) < 0) + if (qemuSetupImageChainCgroup(vm, vm->def->disks[i]->src) < 0) goto cleanup; } diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 17a565244f..dc6d173fce 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -31,10 +31,10 @@ int qemuSetupImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src); int qemuTeardownImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src); -int qemuSetupDiskCgroup(virDomainObjPtr vm, - virDomainDiskDefPtr disk); -int qemuTeardownDiskCgroup(virDomainObjPtr vm, - virDomainDiskDefPtr disk); +int qemuSetupImageChainCgroup(virDomainObjPtr vm, + virStorageSourcePtr src); +int qemuTeardownImageChainCgroup(virDomainObjPtr vm, + virStorageSourcePtr src); int qemuSetupInputCgroup(virDomainObjPtr vm, virDomainInputDefPtr dev); int qemuTeardownInputCgroup(virDomainObjPtr vm, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e42b93e051..9ec30099a1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11238,9 +11238,6 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; - /* Follow qemuSetupDiskCgroup() and qemuSetImageCgroupInternal() - * which does nothing for non local storage - */ VIR_DEBUG("Not updating /dev for hostdev iSCSI path '%s'", iscsisrc->src->path); } else { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b254e96131..fbc2a20915 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17189,7 +17189,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, if (disk->mirror->format && disk->mirror->format != VIR_STORAGE_FILE_RAW && (qemuDomainNamespaceSetupDisk(vm, disk->src) < 0 || - qemuSetupDiskCgroup(vm, disk) < 0 || + qemuSetupImageChainCgroup(vm, disk->src) < 0 || qemuSecuritySetDiskLabel(driver, vm, disk) < 0)) goto cleanup; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 615105d595..000102ac3f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -116,14 +116,14 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, if (qemuSecuritySetDiskLabel(driver, vm, disk) < 0) goto rollback_namespace; - if (qemuSetupDiskCgroup(vm, disk) < 0) + if (qemuSetupImageChainCgroup(vm, disk->src) < 0) goto rollback_label; ret = 0; goto cleanup; rollback_cgroup: - if (qemuTeardownDiskCgroup(vm, disk) < 0) + if (qemuTeardownImageChainCgroup(vm, disk->src) < 0) VIR_WARN("Unable to tear down cgroup access on %s", NULLSTR(virDomainDiskGetSource(disk))); rollback_label: -- 2.20.1
On 1/23/19 11:10 AM, Peter Krempa wrote:
Since the disk is necessary only to get the source modify the functions to take the source directly and rename them to
s/ to/ to
qemu[Setup|Teardown]ImageChainCgroup.
Additionally drop a pointless comment containing the old function name.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_cgroup.c | 14 +++++++------- src/qemu/qemu_cgroup.h | 8 ++++---- src/qemu/qemu_domain.c | 3 --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- 5 files changed, 14 insertions(+), 17 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Security labelling of disks consists of labelling of the disk image itself and it's backing chain. Modify virSecurityManager[Set|Restore]ImageLabel to take a boolean flag that will label the full chain rather than the top image itself. This allows to delete/unify some parts of the code and will also simplify callers in some cases. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_security.c | 6 ++-- src/security/security_apparmor.c | 24 +++------------ src/security/security_dac.c | 40 +++++++------------------ src/security/security_driver.h | 15 +++------- src/security/security_manager.c | 20 ++++++++----- src/security/security_manager.h | 6 ++-- src/security/security_nop.c | 25 +++------------- src/security/security_selinux.c | 42 ++++++++------------------- src/security/security_stack.c | 50 +++++--------------------------- 9 files changed, 60 insertions(+), 168 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 5faa34a4fd..4940195216 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -170,8 +170,7 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, goto cleanup; if (virSecurityManagerSetImageLabel(driver->securityManager, - vm->def, - src) < 0) + vm->def, src, false) < 0) goto cleanup; if (virSecurityManagerTransactionCommit(driver->securityManager, @@ -201,8 +200,7 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, goto cleanup; if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, - src) < 0) + vm->def, src, false) < 0) goto cleanup; if (virSecurityManagerTransactionCommit(driver->securityManager, diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 43310361ba..a61105cbb7 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -691,7 +691,8 @@ AppArmorClearSecuritySocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, static int AppArmorRestoreSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingStore ATTRIBUTE_UNUSED) { if (!virStorageSourceIsLocalStorage(src)) return 0; @@ -699,13 +700,6 @@ AppArmorRestoreSecurityImageLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, def, NULL, false); } -static int -AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) -{ - return AppArmorRestoreSecurityImageLabel(mgr, def, disk->src); -} /* Called when hotplugging */ static int @@ -799,7 +793,8 @@ AppArmorRestoreInputLabel(virSecurityManagerPtr mgr, static int AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingStore ATTRIBUTE_UNUSED) { int rc = -1; char *profile_name = NULL; @@ -844,14 +839,6 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, return rc; } -static int -AppArmorSetSecurityDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) -{ - return AppArmorSetSecurityImageLabel(mgr, def, disk->src); -} - static int AppArmorSecurityVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def) @@ -1188,9 +1175,6 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSecurityVerify = AppArmorSecurityVerify, - .domainSetSecurityDiskLabel = AppArmorSetSecurityDiskLabel, - .domainRestoreSecurityDiskLabel = AppArmorRestoreSecurityDiskLabel, - .domainSetSecurityImageLabel = AppArmorSetSecurityImageLabel, .domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel, diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 533d990de1..08ff0d89c0 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -897,22 +897,17 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, static int virSecurityDACSetImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingChain) { - return virSecurityDACSetImageLabelInternal(mgr, def, src, NULL); -} - -static int -virSecurityDACSetDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) + virStorageSourcePtr n; -{ - virStorageSourcePtr next; - - for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) { - if (virSecurityDACSetImageLabelInternal(mgr, def, next, disk->src) < 0) + for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (virSecurityDACSetImageLabelInternal(mgr, def, n, src) < 0) return -1; + + if (!backingChain) + break; } return 0; @@ -969,21 +964,13 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, static int virSecurityDACRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingChain ATTRIBUTE_UNUSED) { return virSecurityDACRestoreImageLabelInt(mgr, def, src, false); } -static int -virSecurityDACRestoreDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) -{ - return virSecurityDACRestoreImageLabelInt(mgr, def, disk->src, false); -} - - static int virSecurityDACSetHostdevLabelHelper(const char *file, void *opaque) @@ -1853,9 +1840,7 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, /* XXX fixme - we need to recursively label the entire tree :-( */ if (virDomainDiskGetType(def->disks[i]) == VIR_STORAGE_TYPE_DIR) continue; - if (virSecurityDACSetDiskLabel(mgr, - def, - def->disks[i]) < 0) + if (virSecurityDACSetImageLabel(mgr, def, def->disks[i]->src, true) < 0) return -1; } @@ -2295,9 +2280,6 @@ virSecurityDriver virSecurityDriverDAC = { .domainSecurityVerify = virSecurityDACVerify, - .domainSetSecurityDiskLabel = virSecurityDACSetDiskLabel, - .domainRestoreSecurityDiskLabel = virSecurityDACRestoreDiskLabel, - .domainSetSecurityImageLabel = virSecurityDACSetImageLabel, .domainRestoreSecurityImageLabel = virSecurityDACRestoreImageLabel, diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 70c8cde50b..df270cdc02 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -54,18 +54,12 @@ typedef int (*virSecurityDriverTransactionCommit) (virSecurityManagerPtr mgr, bool lock); typedef void (*virSecurityDriverTransactionAbort) (virSecurityManagerPtr mgr); -typedef int (*virSecurityDomainRestoreDiskLabel) (virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk); typedef int (*virSecurityDomainSetDaemonSocketLabel)(virSecurityManagerPtr mgr, virDomainDefPtr vm); typedef int (*virSecurityDomainSetSocketLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def); typedef int (*virSecurityDomainClearSocketLabel)(virSecurityManagerPtr mgr, virDomainDefPtr def); -typedef int (*virSecurityDomainSetDiskLabel) (virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk); typedef int (*virSecurityDomainRestoreHostdevLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainHostdevDefPtr dev, @@ -119,10 +113,12 @@ typedef int (*virSecurityDomainSetHugepages) (virSecurityManagerPtr mgr, const char *path); typedef int (*virSecurityDomainSetImageLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, - virStorageSourcePtr src); + virStorageSourcePtr src, + bool backingChain); typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, - virStorageSourcePtr src); + virStorageSourcePtr src, + bool backingChain); typedef int (*virSecurityDomainSetMemoryLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainMemoryDefPtr mem); @@ -171,9 +167,6 @@ struct _virSecurityDriver { virSecurityDomainSecurityVerify domainSecurityVerify; - virSecurityDomainSetDiskLabel domainSetSecurityDiskLabel; - virSecurityDomainRestoreDiskLabel domainRestoreSecurityDiskLabel; - virSecurityDomainSetImageLabel domainSetSecurityImageLabel; virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f6b4c2d5d5..5493f0f66b 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -418,10 +418,10 @@ virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, virDomainDiskDefPtr disk) { - if (mgr->drv->domainRestoreSecurityDiskLabel) { + if (mgr->drv->domainRestoreSecurityImageLabel) { int ret; virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityDiskLabel(mgr, vm, disk); + ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, disk->src, true); virObjectUnlock(mgr); return ret; } @@ -436,6 +436,7 @@ virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, * @mgr: security manager object * @vm: domain definition object * @src: disk source definition to operate on + * @backingChain: Restore labels also on backingChains of @src * * Removes security label from a single storage image. * @@ -444,12 +445,13 @@ virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingChain) { if (mgr->drv->domainRestoreSecurityImageLabel) { int ret; virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src); + ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src, backingChain); virObjectUnlock(mgr); return ret; } @@ -526,10 +528,10 @@ virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, virDomainDiskDefPtr disk) { - if (mgr->drv->domainSetSecurityDiskLabel) { + if (mgr->drv->domainSetSecurityImageLabel) { int ret; virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityDiskLabel(mgr, vm, disk); + ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, disk->src, true); virObjectUnlock(mgr); return ret; } @@ -544,6 +546,7 @@ virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, * @mgr: security manager object * @vm: domain definition object * @src: disk source definition to operate on + * @backingChain: set labels also on backing chain of @src * * Labels a single storage image with the configured security label. * @@ -552,12 +555,13 @@ virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingChain) { if (mgr->drv->domainSetSecurityImageLabel) { int ret; virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, src); + ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, src, backingChain); virObjectUnlock(mgr); return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index f7beb29f86..0207113b14 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -156,10 +156,12 @@ virSecurityManagerPtr* virSecurityManagerGetNested(virSecurityManagerPtr mgr); int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - virStorageSourcePtr src); + virStorageSourcePtr src, + bool backingChain); int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - virStorageSourcePtr src); + virStorageSourcePtr src, + bool backingChain); int virSecurityManagerSetMemoryLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index ff739f8199..21e668c169 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -55,14 +55,6 @@ virSecurityDriverGetDOINop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) return "0"; } -static int -virSecurityDomainRestoreDiskLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) -{ - return 0; -} - static int virSecurityDomainSetDaemonSocketLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr vm ATTRIBUTE_UNUSED) @@ -84,14 +76,6 @@ virSecurityDomainClearSocketLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } -static int -virSecurityDomainSetDiskLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) -{ - return 0; -} - static int virSecurityDomainRestoreHostdevLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr vm ATTRIBUTE_UNUSED, @@ -225,7 +209,8 @@ virSecurityGetBaseLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, static int virSecurityDomainRestoreImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def ATTRIBUTE_UNUSED, - virStorageSourcePtr src ATTRIBUTE_UNUSED) + virStorageSourcePtr src ATTRIBUTE_UNUSED, + bool backingChain ATTRIBUTE_UNUSED) { return 0; } @@ -233,7 +218,8 @@ virSecurityDomainRestoreImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED static int virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def ATTRIBUTE_UNUSED, - virStorageSourcePtr src ATTRIBUTE_UNUSED) + virStorageSourcePtr src ATTRIBUTE_UNUSED, + bool backingChain ATTRIBUTE_UNUSED) { return 0; } @@ -292,9 +278,6 @@ virSecurityDriver virSecurityDriverNop = { .domainSecurityVerify = virSecurityDomainVerifyNop, - .domainSetSecurityDiskLabel = virSecurityDomainSetDiskLabelNop, - .domainRestoreSecurityDiskLabel = virSecurityDomainRestoreDiskLabelNop, - .domainSetSecurityImageLabel = virSecurityDomainSetImageLabelNop, .domainRestoreSecurityImageLabel = virSecurityDomainRestoreImageLabelNop, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 5cdb839c13..106494ff3a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1771,20 +1771,11 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, } -static int -virSecuritySELinuxRestoreDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) -{ - return virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src, - false); -} - - static int virSecuritySELinuxRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingChain ATTRIBUTE_UNUSED) { return virSecuritySELinuxRestoreImageLabelInt(mgr, def, src, false); } @@ -1869,28 +1860,23 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, static int virSecuritySELinuxSetImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virStorageSourcePtr src) -{ - return virSecuritySELinuxSetImageLabelInternal(mgr, def, src, NULL); -} - - -static int -virSecuritySELinuxSetDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) - + virStorageSourcePtr src, + bool backingChain) { - virStorageSourcePtr next; + virStorageSourcePtr n; - for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) { - if (virSecuritySELinuxSetImageLabelInternal(mgr, def, next, disk->src) < 0) + for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, src) < 0) return -1; + + if (!backingChain) + break; } return 0; } + static int virSecuritySELinuxSetHostdevLabelHelper(const char *file, void *opaque) { @@ -3026,8 +3012,7 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr, def->disks[i]->dst); continue; } - if (virSecuritySELinuxSetDiskLabel(mgr, - def, def->disks[i]) < 0) + if (virSecuritySELinuxSetImageLabel(mgr, def, def->disks[i]->src, true) < 0) return -1; } /* XXX fixme process def->fss if relabel == true */ @@ -3441,9 +3426,6 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSecurityVerify = virSecuritySELinuxVerify, - .domainSetSecurityDiskLabel = virSecuritySELinuxSetDiskLabel, - .domainRestoreSecurityDiskLabel = virSecuritySELinuxRestoreDiskLabel, - .domainSetSecurityImageLabel = virSecuritySELinuxSetImageLabel, .domainRestoreSecurityImageLabel = virSecuritySELinuxRestoreImageLabel, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 3e60d5d2b7..e1c98a75e3 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -267,42 +267,6 @@ virSecurityStackReserveLabel(virSecurityManagerPtr mgr, } -static int -virSecurityStackSetDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainDiskDefPtr disk) -{ - virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); - virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; - - for (; item; item = item->next) { - if (virSecurityManagerSetDiskLabel(item->securityManager, vm, disk) < 0) - rc = -1; - } - - return rc; -} - - -static int -virSecurityStackRestoreDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainDiskDefPtr disk) -{ - virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); - virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; - - for (; item; item = item->next) { - if (virSecurityManagerRestoreDiskLabel(item->securityManager, vm, disk) < 0) - rc = -1; - } - - return rc; -} - - static int virSecurityStackSetHostdevLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, @@ -600,14 +564,16 @@ virSecurityStackGetBaseLabel(virSecurityManagerPtr mgr, int virtType) static int virSecurityStackSetImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingChain) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; int rc = 0; for (; item; item = item->next) { - if (virSecurityManagerSetImageLabel(item->securityManager, vm, src) < 0) + if (virSecurityManagerSetImageLabel(item->securityManager, vm, src, + backingChain) < 0) rc = -1; } @@ -617,7 +583,8 @@ virSecurityStackSetImageLabel(virSecurityManagerPtr mgr, static int virSecurityStackRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingChain) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; @@ -625,7 +592,7 @@ virSecurityStackRestoreImageLabel(virSecurityManagerPtr mgr, for (; item; item = item->next) { if (virSecurityManagerRestoreImageLabel(item->securityManager, - vm, src) < 0) + vm, src, backingChain) < 0) rc = -1; } @@ -816,9 +783,6 @@ virSecurityDriver virSecurityDriverStack = { .domainSecurityVerify = virSecurityStackVerify, - .domainSetSecurityDiskLabel = virSecurityStackSetDiskLabel, - .domainRestoreSecurityDiskLabel = virSecurityStackRestoreDiskLabel, - .domainSetSecurityImageLabel = virSecurityStackSetImageLabel, .domainRestoreSecurityImageLabel = virSecurityStackRestoreImageLabel, -- 2.20.1
On 1/23/19 11:10 AM, Peter Krempa wrote:
Security labelling of disks consists of labelling of the disk image
*labeling
itself and it's backing chain. Modify virSecurityManager[Set|Restore]ImageLabel to take a boolean flag that will label the full chain rather than the top image itself.
This allows to delete/unify some parts of the code and will also simplify callers in some cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_security.c | 6 ++-- src/security/security_apparmor.c | 24 +++------------ src/security/security_dac.c | 40 +++++++------------------ src/security/security_driver.h | 15 +++------- src/security/security_manager.c | 20 ++++++++----- src/security/security_manager.h | 6 ++-- src/security/security_nop.c | 25 +++------------- src/security/security_selinux.c | 42 ++++++++------------------- src/security/security_stack.c | 50 +++++--------------------------- 9 files changed, 60 insertions(+), 168 deletions(-)
I see two logical things happening... 1. Removing virSecurityDomain{Set|Restore}DiskLabel in favor of virSecurityDomain{Set|Restore}ImageLabel 2. Adding parameter to virSecurityManager{Set|Restore}ImageLabel. I think the parameter should be "unsigned int flags" instead of "bool backingChain"? The latter is too specific. Then of course the first flag defined is for backingChain. Also avoids some future change adding bool myNewFlagName to the API. I do see a few other API's use bool's, but does that mean they're right? Beyond that - seems odd to remove the backend/inside of the {Set|Restore}DiskLabel before replacing all the callers uses to {Set|Restore}ImageLabel first. I see the bisect problem is handled by changing virSecurityManager{Set|Restore}DiskLabel to call domain{Set|Restore}SecurityImageLabel instead. So, w/r/t commit message to "describe" what's happening consider indicating the short term usage of *ImageLabel for the *DiskLabel. The alternative is reordering patches, which I don't find necessary. Assuming usage of @flags and I'm confident you can make that alteration... So for the logic, Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
On Mon, Jan 28, 2019 at 09:26:45 -0500, John Ferlan wrote:
On 1/23/19 11:10 AM, Peter Krempa wrote:
Security labelling of disks consists of labelling of the disk image
*labeling
itself and it's backing chain. Modify virSecurityManager[Set|Restore]ImageLabel to take a boolean flag that will label the full chain rather than the top image itself.
This allows to delete/unify some parts of the code and will also simplify callers in some cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_security.c | 6 ++-- src/security/security_apparmor.c | 24 +++------------ src/security/security_dac.c | 40 +++++++------------------ src/security/security_driver.h | 15 +++------- src/security/security_manager.c | 20 ++++++++----- src/security/security_manager.h | 6 ++-- src/security/security_nop.c | 25 +++------------- src/security/security_selinux.c | 42 ++++++++------------------- src/security/security_stack.c | 50 +++++--------------------------- 9 files changed, 60 insertions(+), 168 deletions(-)
I see two logical things happening...
1. Removing virSecurityDomain{Set|Restore}DiskLabel in favor of virSecurityDomain{Set|Restore}ImageLabel
2. Adding parameter to virSecurityManager{Set|Restore}ImageLabel.
I think the parameter should be "unsigned int flags" instead of "bool
I'll use the correct type here.
backingChain"? The latter is too specific. Then of course the first flag defined is for backingChain. Also avoids some future change adding bool myNewFlagName to the API. I do see a few other API's use bool's, but does that mean they're right?
It will be somewhat verbose: +typedef enum { + VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN = 1 << 0; +} virSecurityDomainImageLabelFlags; + typedef int (*virSecurityDomainSetImageLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virStorageSourcePtr src, - bool backingChain); + virSecurityDomainImageLabelFlags flags); typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virStorageSourcePtr src, - bool backingChain); + virSecurityDomainImageLabelFlags flags; typedef int (*virSecurityDomainSetMemoryLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainMemoryDefPtr mem);
Beyond that - seems odd to remove the backend/inside of the {Set|Restore}DiskLabel before replacing all the callers uses to {Set|Restore}ImageLabel first. I see the bisect problem is handled by changing virSecurityManager{Set|Restore}DiskLabel to call domain{Set|Restore}SecurityImageLabel instead.
It's not only "bisect problem" but genuine replacement of the internals with a different internal impl without changing the callers. The disk labelling function becomes a shim which adds a parameter. This was done to limit scope change. This patch should in all callers besides the ones in Set|RestoreDisk label add the 'false' flag (or the equivalent.
So, w/r/t commit message to "describe" what's happening consider indicating the short term usage of *ImageLabel for the *DiskLabel. The alternative is reordering patches, which I don't find necessary.
Reordering is impossible without adding the parameter first or squashing them together, where it would drag in semantic changes from the places calling {Set|Restore}DiskLabel.
Assuming usage of @flags and I'm confident you can make that alteration... So for the logic,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
[...]
Allow callers use the new flag. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 4 ++-- src/qemu/qemu_security.c | 10 ++++++---- src/qemu/qemu_security.h | 6 ++++-- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9ec30099a1..2853337316 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9122,7 +9122,7 @@ qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(elem->path)); - if (qemuSecurityRestoreImageLabel(driver, vm, elem) < 0) + if (qemuSecurityRestoreImageLabel(driver, vm, elem, false) < 0) VIR_WARN("Unable to restore security label on %s", NULLSTR(elem->path)); if (qemuDomainNamespaceTeardownDisk(vm, elem) < 0) @@ -9173,7 +9173,7 @@ qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, if (qemuSetupImageCgroup(vm, elem) < 0) goto cleanup; - if (qemuSecuritySetImageLabel(driver, vm, elem) < 0) + if (qemuSecuritySetImageLabel(driver, vm, elem, false) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 4940195216..fed15e90e9 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -157,7 +157,8 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, int qemuSecuritySetImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingChain) { qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; @@ -170,7 +171,7 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, goto cleanup; if (virSecurityManagerSetImageLabel(driver->securityManager, - vm->def, src, false) < 0) + vm->def, src, backingChain) < 0) goto cleanup; if (virSecurityManagerTransactionCommit(driver->securityManager, @@ -187,7 +188,8 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, int qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool backingChain) { qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; @@ -200,7 +202,7 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, goto cleanup; if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, src, false) < 0) + vm->def, src, backingChain) < 0) goto cleanup; if (virSecurityManagerTransactionCommit(driver->securityManager, diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 5b4fe6eb8f..2a916f5169 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -44,11 +44,13 @@ int qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, int qemuSecuritySetImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - virStorageSourcePtr src); + virStorageSourcePtr src, + bool backingChain); int qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - virStorageSourcePtr src); + virStorageSourcePtr src, + bool backingChain); int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, -- 2.20.1
On 1/23/19 11:11 AM, Peter Krempa wrote:
Allow callers use the new flag.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 4 ++-- src/qemu/qemu_security.c | 10 ++++++---- src/qemu/qemu_security.h | 6 ++++-- 3 files changed, 12 insertions(+), 8 deletions(-)
Assuming previous comment/adjustment request to use int instead of flag.... The "false"'s change to 0 and 'bool backingChain' to unsigned int flags... Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Mon, Jan 28, 2019 at 09:27:51 -0500, John Ferlan wrote:
On 1/23/19 11:11 AM, Peter Krempa wrote:
Allow callers use the new flag.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 4 ++-- src/qemu/qemu_security.c | 10 ++++++---- src/qemu/qemu_security.h | 6 ++++-- 3 files changed, 12 insertions(+), 8 deletions(-)
Assuming previous comment/adjustment request to use int instead of flag.... The "false"'s change to 0 and 'bool backingChain' to unsigned int flags...
Passing in the flags of the security driver at this level would be weird. At this point I'll keep the flag in the function header and convert it to the appropriate flags. I don't really see a point in adding yet another set of flags for the qemu_security stuff.
The same can be achieved by using qemuSecurity[Set|Restore]ImageLabel. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 4 +-- src/qemu/qemu_security.c | 62 ---------------------------------------- src/qemu/qemu_security.h | 8 ------ 4 files changed, 3 insertions(+), 73 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fbc2a20915..025acec6af 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17190,7 +17190,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, disk->mirror->format != VIR_STORAGE_FILE_RAW && (qemuDomainNamespaceSetupDisk(vm, disk->src) < 0 || qemuSetupImageChainCgroup(vm, disk->src) < 0 || - qemuSecuritySetDiskLabel(driver, vm, disk) < 0)) + qemuSecuritySetImageLabel(driver, vm, disk->src, true) < 0)) goto cleanup; disk->src = oldsrc; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 000102ac3f..015f1837ab 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -113,7 +113,7 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, if (qemuDomainNamespaceSetupDisk(vm, disk->src) < 0) goto rollback_lock; - if (qemuSecuritySetDiskLabel(driver, vm, disk) < 0) + if (qemuSecuritySetImageLabel(driver, vm, disk->src, true) < 0) goto rollback_namespace; if (qemuSetupImageChainCgroup(vm, disk->src) < 0) @@ -127,7 +127,7 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, VIR_WARN("Unable to tear down cgroup access on %s", NULLSTR(virDomainDiskGetSource(disk))); rollback_label: - if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0) + if (qemuSecurityRestoreImageLabel(driver, vm, disk->src, true) < 0) VIR_WARN("Unable to restore security label on %s", NULLSTR(virDomainDiskGetSource(disk))); diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index fed15e90e9..c15ca24f21 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -92,68 +92,6 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, } -int -qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk) -{ - qemuDomainObjPrivatePtr priv = vm->privateData; - pid_t pid = -1; - int ret = -1; - - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) - pid = vm->pid; - - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) - goto cleanup; - - if (virSecurityManagerSetDiskLabel(driver->securityManager, - vm->def, - disk) < 0) - goto cleanup; - - if (virSecurityManagerTransactionCommit(driver->securityManager, - pid, priv->rememberOwner) < 0) - goto cleanup; - - ret = 0; - cleanup: - virSecurityManagerTransactionAbort(driver->securityManager); - return ret; -} - - -int -qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk) -{ - qemuDomainObjPrivatePtr priv = vm->privateData; - pid_t pid = -1; - int ret = -1; - - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) - pid = vm->pid; - - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) - goto cleanup; - - if (virSecurityManagerRestoreDiskLabel(driver->securityManager, - vm->def, - disk) < 0) - goto cleanup; - - if (virSecurityManagerTransactionCommit(driver->securityManager, - pid, priv->rememberOwner) < 0) - goto cleanup; - - ret = 0; - cleanup: - virSecurityManagerTransactionAbort(driver->securityManager); - return ret; -} - - int qemuSecuritySetImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 2a916f5169..546a66f284 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -34,14 +34,6 @@ void qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, bool migrated); -int qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk); - -int qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk); - int qemuSecuritySetImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src, -- 2.20.1
On 1/23/19 11:11 AM, Peter Krempa wrote:
The same can be achieved by using qemuSecurity[Set|Restore]ImageLabel.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 4 +-- src/qemu/qemu_security.c | 62 ---------------------------------------- src/qemu/qemu_security.h | 8 ------ 4 files changed, 3 insertions(+), 73 deletions(-)
Assuming previous comment/adjustment request to use int instead of flag.... The "true"'s change to some flag set to the new flag value Reviewed-by: John Ferlan <jferlan@redhat.com> John
Now that we have replacement in the form of the image labelling function we can drop the unnecessary functions by replacing all callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 -- src/lxc/lxc_controller.c | 3 +- src/lxc/lxc_driver.c | 4 +-- src/security/security_manager.c | 58 --------------------------------- src/security/security_manager.h | 6 ---- 5 files changed, 4 insertions(+), 69 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3d6306809..599b97569a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1354,7 +1354,6 @@ virSecurityManagerReleaseLabel; virSecurityManagerReserveLabel; virSecurityManagerRestoreAllLabel; virSecurityManagerRestoreChardevLabel; -virSecurityManagerRestoreDiskLabel; virSecurityManagerRestoreHostdevLabel; virSecurityManagerRestoreImageLabel; virSecurityManagerRestoreInputLabel; @@ -1365,7 +1364,6 @@ virSecurityManagerSetAllLabel; virSecurityManagerSetChardevLabel; virSecurityManagerSetChildProcessLabel; virSecurityManagerSetDaemonSocketLabel; -virSecurityManagerSetDiskLabel; virSecurityManagerSetHostdevLabel; virSecurityManagerSetImageFDLabel; virSecurityManagerSetImageLabel; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 2bec8846aa..790ff65b0e 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1932,7 +1932,8 @@ 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; - if (virSecurityManagerSetDiskLabel(securityDriver, ctrl->def, def) < 0) + if (virSecurityManagerSetImageLabel(securityDriver, ctrl->def, + def->src, true) < 0) goto cleanup; ret = 0; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index df15a0da50..f03c6af691 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3636,8 +3636,8 @@ lxcDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, virDomainDiskDefPtr def = data->def->data.disk; char *tmpsrc = def->src->path; def->src->path = data->file; - if (virSecurityManagerSetDiskLabel(data->driver->securityManager, - data->vm->def, def) < 0) { + if (virSecurityManagerSetImageLabel(data->driver->securityManager, + data->vm->def, def->src, true) < 0) { def->src->path = tmpsrc; goto cleanup; } diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 5493f0f66b..72081ac586 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -402,35 +402,6 @@ virSecurityManagerGetPrivileged(virSecurityManagerPtr mgr) } -/** - * virSecurityManagerRestoreDiskLabel: - * @mgr: security manager object - * @vm: domain definition object - * @disk: disk definition to operate on - * - * Removes security label from the source image of the disk. Note that this - * function doesn't restore labels on backing chain elements of @disk. - * - * Returns: 0 on success, -1 on error. - */ -int -virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainDiskDefPtr disk) -{ - if (mgr->drv->domainRestoreSecurityImageLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, disk->src, true); - virObjectUnlock(mgr); - return ret; - } - - virReportUnsupportedError(); - return -1; -} - - /** * virSecurityManagerRestoreImageLabel: * @mgr: security manager object @@ -512,35 +483,6 @@ virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, } -/** - * virSecurityManagerSetDiskLabel: - * @mgr: security manager object - * @vm: domain definition object - * @disk: disk definition to operate on - * - * Labels the disk image and all images in the backing chain with the configured - * security label. - * - * Returns: 0 on success, -1 on error. - */ -int -virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainDiskDefPtr disk) -{ - if (mgr->drv->domainSetSecurityImageLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, disk->src, true); - virObjectUnlock(mgr); - return ret; - } - - virReportUnsupportedError(); - return -1; -} - - /** * virSecurityManagerSetImageLabel: * @mgr: security manager object diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 0207113b14..8e1fb3b3c9 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -90,18 +90,12 @@ bool virSecurityManagerGetDefaultConfined(virSecurityManagerPtr mgr); bool virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr); bool virSecurityManagerGetPrivileged(virSecurityManagerPtr mgr); -int virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk); int virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm); int virSecurityManagerSetSocketLabel(virSecurityManagerPtr mgr, virDomainDefPtr def); int virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, virDomainDefPtr def); -int virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk); int virSecurityManagerRestoreHostdevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainHostdevDefPtr dev, -- 2.20.1
On 1/23/19 11:11 AM, Peter Krempa wrote:
Now that we have replacement in the form of the image labelling function
*labeling
we can drop the unnecessary functions by replacing all callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 -- src/lxc/lxc_controller.c | 3 +- src/lxc/lxc_driver.c | 4 +-- src/security/security_manager.c | 58 --------------------------------- src/security/security_manager.h | 6 ---- 5 files changed, 4 insertions(+), 69 deletions(-)
Similar to previous comments to use @flags... Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
Previously there weren't any suitable functions which would allow setting up host side of a full disk chain so we've opted to replace the 'src' in a virDomainDiskDef by the new image source. That is now no longer necessary so remove the munging. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 025acec6af..79a767288e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17144,7 +17144,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - virStorageSourcePtr oldsrc = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); if (!disk->mirror) { @@ -17180,21 +17179,15 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, * has already been labeled; but only necessary when we know for * sure that there is a backing chain. */ if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { - oldsrc = disk->src; - disk->src = disk->mirror; - if (qemuDomainDetermineDiskChain(driver, vm, disk, disk->mirror, true) < 0) goto cleanup; if (disk->mirror->format && disk->mirror->format != VIR_STORAGE_FILE_RAW && - (qemuDomainNamespaceSetupDisk(vm, disk->src) < 0 || - qemuSetupImageChainCgroup(vm, disk->src) < 0 || - qemuSecuritySetImageLabel(driver, vm, disk->src, true) < 0)) + (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 || + qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || + qemuSecuritySetImageLabel(driver, vm, disk->mirror, true) < 0)) goto cleanup; - - disk->src = oldsrc; - oldsrc = NULL; } /* Attempt the pivot. Record the attempt now, to prevent duplicate @@ -17222,9 +17215,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, } cleanup: - if (oldsrc) - disk->src = oldsrc; - virObjectUnref(cfg); return ret; } -- 2.20.1
On 1/23/19 11:11 AM, Peter Krempa wrote:
Previously there weren't any suitable functions which would allow setting up host side of a full disk chain so we've opted to replace the 'src' in a virDomainDiskDef by the new image source.
That is now no longer necessary so remove the munging.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Use the functions designed to deal with single images as the *Disk functions were just wrappers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 -- src/libxl/libxl_driver.c | 14 +++++++------- src/locking/domain_lock.c | 17 ----------------- src/locking/domain_lock.h | 8 -------- src/qemu/qemu_hotplug.c | 6 +++--- 5 files changed, 10 insertions(+), 37 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 599b97569a..ffabb66867 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1291,8 +1291,6 @@ virStreamInData; # locking/domain_lock.h -virDomainLockDiskAttach; -virDomainLockDiskDetach; virDomainLockImageAttach; virDomainLockImageDetach; virDomainLockLeaseAttach; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index e30c9891d2..0d8c5aec3a 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3050,9 +3050,9 @@ libxlDomainAttachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev) if (libxlMakeDisk(l_disk, &x_disk) < 0) goto cleanup; - if (virDomainLockDiskAttach(libxl_driver->lockManager, - "xen:///system", - vm, l_disk) < 0) + if (virDomainLockImageAttach(libxl_driver->lockManager, + "xen:///system", + vm, l_disk->src) < 0) goto cleanup; if ((ret = libxl_device_disk_add(cfg->ctx, vm->def->id, @@ -3060,8 +3060,8 @@ libxlDomainAttachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev) virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight failed to attach disk '%s'"), l_disk->dst); - if (virDomainLockDiskDetach(libxl_driver->lockManager, - vm, l_disk) < 0) { + if (virDomainLockImageDetach(libxl_driver->lockManager, + vm, l_disk->src) < 0) { VIR_WARN("Unable to release lock on %s", virDomainDiskGetSource(l_disk)); } @@ -3349,8 +3349,8 @@ libxlDomainDetachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev) goto cleanup; } - if (virDomainLockDiskDetach(libxl_driver->lockManager, - vm, l_disk) < 0) + if (virDomainLockImageDetach(libxl_driver->lockManager, + vm, l_disk->src) < 0) VIR_WARN("Unable to release lock on %s", virDomainDiskGetSource(l_disk)); diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 705b132457..d91ac83c45 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -281,15 +281,6 @@ int virDomainLockImageAttach(virLockManagerPluginPtr plugin, } -int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, - const char *uri, - virDomainObjPtr dom, - virDomainDiskDefPtr disk) -{ - return virDomainLockImageAttach(plugin, uri, dom, disk->src); -} - - int virDomainLockImageDetach(virLockManagerPluginPtr plugin, virDomainObjPtr dom, virStorageSourcePtr src) @@ -317,14 +308,6 @@ int virDomainLockImageDetach(virLockManagerPluginPtr plugin, } -int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, - virDomainObjPtr dom, - virDomainDiskDefPtr disk) -{ - return virDomainLockImageDetach(plugin, dom, disk->src); -} - - int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, const char *uri, virDomainObjPtr dom, diff --git a/src/locking/domain_lock.h b/src/locking/domain_lock.h index 027e93271a..02417b471b 100644 --- a/src/locking/domain_lock.h +++ b/src/locking/domain_lock.h @@ -42,14 +42,6 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, virDomainObjPtr dom, char **state); -int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, - const char *uri, - virDomainObjPtr dom, - virDomainDiskDefPtr disk); -int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, - virDomainObjPtr dom, - virDomainDiskDefPtr disk); - int virDomainLockImageAttach(virLockManagerPluginPtr plugin, const char *uri, virDomainObjPtr dom, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 015f1837ab..b08f443fbc 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -106,8 +106,8 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, goto rollback_cgroup; } - if (virDomainLockDiskAttach(driver->lockManager, cfg->uri, - vm, disk) < 0) + if (virDomainLockImageAttach(driver->lockManager, cfg->uri, + vm, disk->src) < 0) goto cleanup; if (qemuDomainNamespaceSetupDisk(vm, disk->src) < 0) @@ -137,7 +137,7 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, NULLSTR(virDomainDiskGetSource(disk))); rollback_lock: - if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) + if (virDomainLockImageDetach(driver->lockManager, vm, disk->src) < 0) VIR_WARN("Unable to release lock on %s", NULLSTR(virDomainDiskGetSource(disk))); -- 2.20.1
On 1/23/19 11:11 AM, Peter Krempa wrote:
Use the functions designed to deal with single images as the *Disk functions were just wrappers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 -- src/libxl/libxl_driver.c | 14 +++++++------- src/locking/domain_lock.c | 17 ----------------- src/locking/domain_lock.h | 8 -------- src/qemu/qemu_hotplug.c | 6 +++--- 5 files changed, 10 insertions(+), 37 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Rather than passing in a virStorageSource which would override the originally passed disk->src we can now drop passing in a disk completely as all functions called inside here require a virStorageSource. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 75 ++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 46 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b08f443fbc..19aed3ee8e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -68,37 +68,29 @@ unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5; /** - * qemuHotplugPrepareDiskAccess: + * qemuHotplugPrepareDiskSourceAccess: * @driver: qemu driver struct * @vm: domain object - * @disk: disk to prepare - * @overridesrc: Source different than @disk->src when necessary - * @teardown: Teardown the disk instead of adding it to a vm + * @src: Source to prepare + * @teardown: Teardown the access to @src instead of adding it to a vm * - * Setup the locks, cgroups and security permissions on a disk of a VM. - * If @overridesrc is specified the source struct is used instead of the - * one present in @disk. If @teardown is true, then the labels and cgroups - * are removed instead. + * Setup the locks, cgroups and security permissions on a disk source and it's + * backing chain. If @teardown is true, then the labels and cgroups are removed + * instead. * * Returns 0 on success and -1 on error. Reports libvirt error. */ static int -qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - virStorageSourcePtr overridesrc, - bool teardown) +qemuHotplugPrepareDiskSourceAccess(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src, + bool teardown) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + const char *srcstr = NULLSTR(src->path); int ret = -1; - virStorageSourcePtr origsrc = NULL; virErrorPtr orig_err = NULL; - if (overridesrc) { - origsrc = disk->src; - disk->src = overridesrc; - } - /* just tear down the disk access */ if (teardown) { virErrorPreserveLast(&orig_err); @@ -106,47 +98,38 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, goto rollback_cgroup; } - if (virDomainLockImageAttach(driver->lockManager, cfg->uri, - vm, disk->src) < 0) + if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, src) < 0) goto cleanup; - if (qemuDomainNamespaceSetupDisk(vm, disk->src) < 0) + if (qemuDomainNamespaceSetupDisk(vm, src) < 0) goto rollback_lock; - if (qemuSecuritySetImageLabel(driver, vm, disk->src, true) < 0) + if (qemuSecuritySetImageLabel(driver, vm, src, true) < 0) goto rollback_namespace; - if (qemuSetupImageChainCgroup(vm, disk->src) < 0) + if (qemuSetupImageChainCgroup(vm, src) < 0) goto rollback_label; ret = 0; goto cleanup; rollback_cgroup: - if (qemuTeardownImageChainCgroup(vm, disk->src) < 0) - VIR_WARN("Unable to tear down cgroup access on %s", - NULLSTR(virDomainDiskGetSource(disk))); + if (qemuTeardownImageChainCgroup(vm, src) < 0) + VIR_WARN("Unable to tear down cgroup access on %s", srcstr); rollback_label: - if (qemuSecurityRestoreImageLabel(driver, vm, disk->src, true) < 0) - VIR_WARN("Unable to restore security label on %s", - NULLSTR(virDomainDiskGetSource(disk))); + if (qemuSecurityRestoreImageLabel(driver, vm, src, true) < 0) + VIR_WARN("Unable to restore security label on %s", srcstr); rollback_namespace: - if (qemuDomainNamespaceTeardownDisk(vm, disk->src) < 0) - VIR_WARN("Unable to remove /dev entry for %s", - NULLSTR(virDomainDiskGetSource(disk))); + if (qemuDomainNamespaceTeardownDisk(vm, src) < 0) + VIR_WARN("Unable to remove /dev entry for %s", srcstr); rollback_lock: - if (virDomainLockImageDetach(driver->lockManager, vm, disk->src) < 0) - VIR_WARN("Unable to release lock on %s", - NULLSTR(virDomainDiskGetSource(disk))); + if (virDomainLockImageDetach(driver->lockManager, vm, src) < 0) + VIR_WARN("Unable to release lock on %s", srcstr); cleanup: - if (origsrc) - disk->src = origsrc; - virErrorRestore(&orig_err); - virObjectUnref(cfg); return ret; @@ -826,7 +809,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto cleanup; - if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) + if (qemuHotplugPrepareDiskSourceAccess(driver, vm, newsrc, false) < 0) goto cleanup; if (qemuHotplugAttachManagedPR(driver, vm, newsrc, QEMU_ASYNC_JOB_NONE) < 0) @@ -845,7 +828,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, /* remove the old source from shared device list */ disk->src = oldsrc; ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, oldsrc, true)); + ignore_value(qemuHotplugPrepareDiskSourceAccess(driver, vm, oldsrc, true)); /* media was changed, so we can remove the old media definition now */ virStorageSourceFree(oldsrc); @@ -860,7 +843,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (sharedAdded) ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, true)); + ignore_value(qemuHotplugPrepareDiskSourceAccess(driver, vm, newsrc, true)); } /* remove PR manager object if unneeded */ @@ -891,7 +874,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, char *devstr = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0) + if (qemuHotplugPrepareDiskSourceAccess(driver, vm, disk->src, false) < 0) goto cleanup; if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) @@ -954,7 +937,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, virDomainAuditDisk(vm, NULL, disk->src, "attach", false); error: - ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true)); + ignore_value(qemuHotplugPrepareDiskSourceAccess(driver, vm, disk->src, true)); goto cleanup; } @@ -4377,7 +4360,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, qemuDomainReleaseDeviceAddress(vm, &disk->info, virDomainDiskGetSource(disk)); /* tear down disk security access */ - qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true); + qemuHotplugPrepareDiskSourceAccess(driver, vm, disk->src, true); dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = disk; -- 2.20.1
On 1/23/19 11:11 AM, Peter Krempa wrote:
Rather than passing in a virStorageSource which would override the originally passed disk->src we can now drop passing in a disk completely as all functions called inside here require a virStorageSource.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 75 ++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 46 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Wed, Jan 23, 2019 at 05:11:05PM +0100, Peter Krempa wrote:
Rather than passing in a virStorageSource which would override the originally passed disk->src we can now drop passing in a disk completely as all functions called inside here require a virStorageSource.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 75 ++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 46 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b08f443fbc..19aed3ee8e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -68,37 +68,29 @@ unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
/** - * qemuHotplugPrepareDiskAccess: + * qemuHotplugPrepareDiskSourceAccess: * @driver: qemu driver struct * @vm: domain object - * @disk: disk to prepare - * @overridesrc: Source different than @disk->src when necessary - * @teardown: Teardown the disk instead of adding it to a vm + * @src: Source to prepare + * @teardown: Teardown the access to @src instead of adding it to a vm * - * Setup the locks, cgroups and security permissions on a disk of a VM. - * If @overridesrc is specified the source struct is used instead of the - * one present in @disk. If @teardown is true, then the labels and cgroups - * are removed instead. + * Setup the locks, cgroups and security permissions on a disk source and it's
*its
+ * backing chain. If @teardown is true, then the labels and cgroups are removed + * instead. * * Returns 0 on success and -1 on error. Reports libvirt error. */ static int
Jano
Be more sensible when setting labels of the target of a virDomainBlockCopy operation. Previously we'd relabel everything in case it's a copy job even if there's no unlabelled backing chain. Since we are also not sure whether the backing chain is shared we don't relabel the chain on completion of the blockjob. This certainly won't play nice with the image permission relabelling feature. While this does not fix the case where the image is reused and has backing chain it certainly sanitizes all the other cases. Later on it will also allow to do the correct thing in cases where only one layer was introduced. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 43 ++++++++++++++++++++--------------------- src/qemu/qemu_process.c | 22 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 79a767288e..2c2c0ce92e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17170,26 +17170,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, goto cleanup; } - /* For active commit, the mirror is part of the already labeled - * chain. For blockcopy, we previously labeled only the top-level - * image; but if the user is reusing an external image that - * includes a backing file, the pivot may result in qemu needing - * to open the entire backing chain, so we need to 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. */ - if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { - if (qemuDomainDetermineDiskChain(driver, vm, disk, disk->mirror, true) < 0) - goto cleanup; - - if (disk->mirror->format && - disk->mirror->format != VIR_STORAGE_FILE_RAW && - (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 || - qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || - qemuSecuritySetImageLabel(driver, vm, disk->mirror, true) < 0)) - goto cleanup; - } - /* Attempt the pivot. Record the attempt now, to prevent duplicate * attempts; but the actual disk change will be made when emitting * the event. @@ -17836,9 +17816,28 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, keepParentLabel) < 0) goto endjob; - if (qemuDomainDiskChainElementPrepare(driver, vm, mirror, false, true) < 0) { - qemuDomainDiskChainElementRevoke(driver, vm, mirror); + /* If reusing an external image that includes a backing file, the pivot may + * result in qemu needing to open the entire backing chain, so we need to + * label the full backing chain of the mirror instead of just the top image */ + if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT && + mirror->format >= VIR_STORAGE_FILE_BACKING && + qemuDomainDetermineDiskChain(driver, vm, disk, mirror, true) < 0) goto endjob; + + if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT && + virStorageSourceHasBacking(mirror)) { + /* note that we don't really know whether a part of the backing chain + * is shared so rolling this back is not as easy. Thus we do it only + * if there's a backing chain */ + if (qemuDomainNamespaceSetupDisk(vm, mirror) < 0 || + qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || + qemuSecuritySetImageLabel(driver, vm, disk->mirror, true) < 0) + goto endjob; + } else { + if (qemuDomainDiskChainElementPrepare(driver, vm, mirror, false, true) < 0) { + qemuDomainDiskChainElementRevoke(driver, vm, mirror); + goto endjob; + } } if (!(job = qemuBlockJobDiskNew(disk, QEMU_BLOCKJOB_TYPE_COPY, device))) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fb596d960f..c9e68397b6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7857,6 +7857,7 @@ qemuProcessRefreshLegacyBlockjob(void *payload, virDomainDiskDefPtr disk; qemuBlockJobDataPtr job; qemuBlockJobType jobtype = info->type; + qemuDomainObjPrivatePtr priv = vm->privateData; if (!(disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, jobname, jobname))) { VIR_DEBUG("could not find disk for block job '%s'", jobname); @@ -7878,8 +7879,29 @@ qemuProcessRefreshLegacyBlockjob(void *payload, disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; job->state = VIR_DOMAIN_BLOCK_JOB_READY; } + + /* Pre-blockdev block copy labelled the chain of the mirrored device + * just before pivoting. At that point it was no longer known whether + * it's even necessary (e.g. disk is being reused). This code fixes + * the labelling in case the job was started in a libvirt version + * which did not label the chain when the block copy is being started. + * Note that we can't do much on failure. */ + if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + if (qemuDomainDetermineDiskChain(priv->driver, vm, disk, + disk->mirror, true) < 0) + goto cleanup; + + if (disk->mirror->format && + disk->mirror->format != VIR_STORAGE_FILE_RAW && + (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 || + qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || + qemuSecuritySetImageLabel(priv->driver, vm, disk->mirror, + true) < 0)) + goto cleanup; + } } + cleanup: qemuBlockJobStartupFinalize(job); return 0; -- 2.20.1
On 1/23/19 11:11 AM, Peter Krempa wrote:
Be more sensible when setting labels of the target of a virDomainBlockCopy operation. Previously we'd relabel everything in case it's a copy job even if there's no unlabelled backing chain. Since we are also not sure whether the backing chain is shared we don't relabel the chain on completion of the blockjob. This certainly won't play nice with the image permission relabelling feature.
While this does not fix the case where the image is reused and has backing chain it certainly sanitizes all the other cases. Later on it will also allow to do the correct thing in cases where only one layer was introduced.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 43 ++++++++++++++++++++--------------------- src/qemu/qemu_process.c | 22 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 22 deletions(-)
[sorry - got sidetracked by a class...] I get to this point and started wondering - hmm... are there any assumptions or issues w/ [managed] save files? But I guess that what the changes in qemuProcessRefreshLegacyBlockjob will handle, right?
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 79a767288e..2c2c0ce92e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17170,26 +17170,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, goto cleanup; }
- /* For active commit, the mirror is part of the already labeled - * chain. For blockcopy, we previously labeled only the top-level - * image; but if the user is reusing an external image that - * includes a backing file, the pivot may result in qemu needing - * to open the entire backing chain, so we need to 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. */ - if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { - if (qemuDomainDetermineDiskChain(driver, vm, disk, disk->mirror, true) < 0) - goto cleanup; - - if (disk->mirror->format && - disk->mirror->format != VIR_STORAGE_FILE_RAW && - (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 || - qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || - qemuSecuritySetImageLabel(driver, vm, disk->mirror, true) < 0)) - goto cleanup; - } - /* Attempt the pivot. Record the attempt now, to prevent duplicate * attempts; but the actual disk change will be made when emitting * the event. @@ -17836,9 +17816,28 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, keepParentLabel) < 0) goto endjob;
- if (qemuDomainDiskChainElementPrepare(driver, vm, mirror, false, true) < 0) { - qemuDomainDiskChainElementRevoke(driver, vm, mirror); + /* If reusing an external image that includes a backing file, the pivot may + * result in qemu needing to open the entire backing chain, so we need to + * label the full backing chain of the mirror instead of just the top image */ + if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT && + mirror->format >= VIR_STORAGE_FILE_BACKING && + qemuDomainDetermineDiskChain(driver, vm, disk, mirror, true) < 0) goto endjob; + + if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT && + virStorageSourceHasBacking(mirror)) { + /* note that we don't really know whether a part of the backing chain + * is shared so rolling this back is not as easy. Thus we do it only + * if there's a backing chain */ + if (qemuDomainNamespaceSetupDisk(vm, mirror) < 0 || + qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || + qemuSecuritySetImageLabel(driver, vm, disk->mirror, true) < 0)
The words in the comment don't really tell me whether @mirror or @disk->mirror is expected to be passed along at least for the second two uses. Since you know the code better, I'll defer to your judgment. It seems logical, since IIUC the code is copying to @mirror and having/setting cgroup/imagelabel on disk->mirror would seem to be the correct step. Still want to make sure I'm reading things right.
+ goto endjob; + } else { + if (qemuDomainDiskChainElementPrepare(driver, vm, mirror, false, true) < 0) { + qemuDomainDiskChainElementRevoke(driver, vm, mirror); + goto endjob; + } }
if (!(job = qemuBlockJobDiskNew(disk, QEMU_BLOCKJOB_TYPE_COPY, device))) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fb596d960f..c9e68397b6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7857,6 +7857,7 @@ qemuProcessRefreshLegacyBlockjob(void *payload, virDomainDiskDefPtr disk; qemuBlockJobDataPtr job; qemuBlockJobType jobtype = info->type; + qemuDomainObjPrivatePtr priv = vm->privateData;
if (!(disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, jobname, jobname))) { VIR_DEBUG("could not find disk for block job '%s'", jobname); @@ -7878,8 +7879,29 @@ qemuProcessRefreshLegacyBlockjob(void *payload, disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; job->state = VIR_DOMAIN_BLOCK_JOB_READY; } + + /* Pre-blockdev block copy labelled the chain of the mirrored device + * just before pivoting. At that point it was no longer known whether + * it's even necessary (e.g. disk is being reused). This code fixes + * the labelling in case the job was started in a libvirt version + * which did not label the chain when the block copy is being started. + * Note that we can't do much on failure. */ + if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + if (qemuDomainDetermineDiskChain(priv->driver, vm, disk, + disk->mirror, true) < 0) + goto cleanup; + + if (disk->mirror->format && + disk->mirror->format != VIR_STORAGE_FILE_RAW && + (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 ||
This is essentially the former qemuDomainBlockPivot right? Lots of voodoo black magic taking place. Things seem right, so consider it... If you think a few more words of comments may help, then great... Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || + qemuSecuritySetImageLabel(priv->driver, vm, disk->mirror, + true) < 0)) + goto cleanup; + } }
+ cleanup: qemuBlockJobStartupFinalize(job);
return 0;
On Mon, Jan 28, 2019 at 13:00:03 -0500, John Ferlan wrote:
On 1/23/19 11:11 AM, Peter Krempa wrote:
Be more sensible when setting labels of the target of a virDomainBlockCopy operation. Previously we'd relabel everything in case it's a copy job even if there's no unlabelled backing chain. Since we are also not sure whether the backing chain is shared we don't relabel the chain on completion of the blockjob. This certainly won't play nice with the image permission relabelling feature.
While this does not fix the case where the image is reused and has backing chain it certainly sanitizes all the other cases. Later on it will also allow to do the correct thing in cases where only one layer was introduced.
I also forgot to note: The change is necessary as in case when -blockdev will be used we will need to hotplug the backing chain and thus labeling needs to be setup in advance and not only at the time of pivot. To avoid multiple code paths move the labeling now.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 43 ++++++++++++++++++++--------------------- src/qemu/qemu_process.c | 22 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 22 deletions(-)
[sorry - got sidetracked by a class...]
I get to this point and started wondering - hmm... are there any assumptions or issues w/ [managed] save files? But I guess that what the changes in qemuProcessRefreshLegacyBlockjob will handle, right?
Saving vms, migration and all the other stuff are forbidden when blockjobs are running as we can't restore them afterwards. I'm not sure whether it makes much sense to bother with restoring the job state in these cases as the use cases are really slim.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 79a767288e..2c2c0ce92e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17170,26 +17170,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, goto cleanup; }
- /* For active commit, the mirror is part of the already labeled - * chain. For blockcopy, we previously labeled only the top-level - * image; but if the user is reusing an external image that - * includes a backing file, the pivot may result in qemu needing - * to open the entire backing chain, so we need to 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. */ - if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { - if (qemuDomainDetermineDiskChain(driver, vm, disk, disk->mirror, true) < 0) - goto cleanup; - - if (disk->mirror->format && - disk->mirror->format != VIR_STORAGE_FILE_RAW && - (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 || - qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || - qemuSecuritySetImageLabel(driver, vm, disk->mirror, true) < 0)) - goto cleanup; - } - /* Attempt the pivot. Record the attempt now, to prevent duplicate * attempts; but the actual disk change will be made when emitting * the event. @@ -17836,9 +17816,28 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, keepParentLabel) < 0) goto endjob;
- if (qemuDomainDiskChainElementPrepare(driver, vm, mirror, false, true) < 0) { - qemuDomainDiskChainElementRevoke(driver, vm, mirror); + /* If reusing an external image that includes a backing file, the pivot may + * result in qemu needing to open the entire backing chain, so we need to + * label the full backing chain of the mirror instead of just the top image */ + if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT && + mirror->format >= VIR_STORAGE_FILE_BACKING && + qemuDomainDetermineDiskChain(driver, vm, disk, mirror, true) < 0) goto endjob; + + if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT && + virStorageSourceHasBacking(mirror)) { + /* note that we don't really know whether a part of the backing chain + * is shared so rolling this back is not as easy. Thus we do it only + * if there's a backing chain */ + if (qemuDomainNamespaceSetupDisk(vm, mirror) < 0 || + qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || + qemuSecuritySetImageLabel(driver, vm, disk->mirror, true) < 0)
The words in the comment don't really tell me whether @mirror or @disk->mirror is expected to be passed along at least for the second two uses. Since you know the code better, I'll defer to your judgment. It seems logical, since IIUC the code is copying to @mirror and having/setting cgroup/imagelabel on disk->mirror would seem to be the correct step. Still want to make sure I'm reading things right.
Actually, disk->mirror is a bug here as we setup/fill it only after the job is started. Passing mirror is the only correct thign to do. The comment only states that the namespace/label/cgroup setup makes sense only if the image has a backing chain.
+ goto endjob; + } else { + if (qemuDomainDiskChainElementPrepare(driver, vm, mirror, false, true) < 0) { + qemuDomainDiskChainElementRevoke(driver, vm, mirror); + goto endjob; + } }
if (!(job = qemuBlockJobDiskNew(disk, QEMU_BLOCKJOB_TYPE_COPY, device))) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fb596d960f..c9e68397b6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7857,6 +7857,7 @@ qemuProcessRefreshLegacyBlockjob(void *payload, virDomainDiskDefPtr disk; qemuBlockJobDataPtr job; qemuBlockJobType jobtype = info->type; + qemuDomainObjPrivatePtr priv = vm->privateData;
if (!(disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, jobname, jobname))) { VIR_DEBUG("could not find disk for block job '%s'", jobname); @@ -7878,8 +7879,29 @@ qemuProcessRefreshLegacyBlockjob(void *payload, disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; job->state = VIR_DOMAIN_BLOCK_JOB_READY; } + + /* Pre-blockdev block copy labelled the chain of the mirrored device + * just before pivoting. At that point it was no longer known whether + * it's even necessary (e.g. disk is being reused). This code fixes + * the labelling in case the job was started in a libvirt version + * which did not label the chain when the block copy is being started. + * Note that we can't do much on failure. */ + if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + if (qemuDomainDetermineDiskChain(priv->driver, vm, disk, + disk->mirror, true) < 0) + goto cleanup; + + if (disk->mirror->format && + disk->mirror->format != VIR_STORAGE_FILE_RAW && + (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 ||
This is essentially the former qemuDomainBlockPivot right?
No. qemuDomainBlockPivot makes qemu switch to the new image. This just sets up the labelling/namespaces/cgroups correctly for it to succeed once the pivot operation is attempted, but that's still separate and user controlled. As we want to do the labeling in advance now, we need to fix the jobs where it was not done before.
Lots of voodoo black magic taking place.
Things seem right, so consider it... If you think a few more words of comments may help, then great...
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
+ qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || + qemuSecuritySetImageLabel(priv->driver, vm, disk->mirror, + true) < 0)) + goto cleanup; + } }
+ cleanup: qemuBlockJobStartupFinalize(job);
return 0;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
John Ferlan -
Ján Tomko -
Peter Krempa