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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

The same can be achieved by using qemuSecurity[Set|Restore]ImageLabel. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

Use the functions designed to deal with single images as the *Disk functions were just wrappers. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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 Wed, Jan 23, 2019 at 05:11:05PM +0100, Peter Krempa wrote: *its Jano

On 1/23/19 11:11 AM, Peter Krempa wrote: [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? 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. 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(a)redhat.com&gt; John