On 08/17/2018 08:38 PM, John Ferlan wrote: > > > On 08/14/2018 07:19 AM, Michal Privoznik wrote: >> Fortunately, we have qemu wrappers so it's sufficient to put >> lock/unlock call only there. >> > > Kind of sparse... Maybe a few words related to what benefit locking the > metadata provides. There is a danger in all this too if the unlock does > fail since metadata locks are wait type locks any subsequent lock will wait. > >> Signed-off-by: Michal Privoznik <mpriv...@redhat.com> >> --- >> src/qemu/qemu_security.c | 107 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 107 insertions(+) >> >> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c >> index af3be42854..527563947c 100644 >> --- a/src/qemu/qemu_security.c >> +++ b/src/qemu/qemu_security.c >> @@ -26,6 +26,7 @@ >> #include "qemu_domain.h" >> #include "qemu_security.h" >> #include "virlog.h" >> +#include "locking/domain_lock.h" >> >> #define VIR_FROM_THIS VIR_FROM_QEMU >> >> @@ -39,6 +40,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, >> { >> int ret = -1; >> qemuDomainObjPrivatePtr priv = vm->privateData; >> + bool locked = false; >> + >> + if (virDomainLockMetadataLock(driver->lockManager, vm) < 0) >> + goto cleanup; >> + >> + locked = true; >> >> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && >> virSecurityManagerTransactionStart(driver->securityManager) < 0) >> @@ -55,9 +62,17 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, >> vm->pid) < 0) >> goto cleanup; >> >> + locked = false; > > Technically not true yet. Also , I think two such attempts with the > second eliciting the VIR_WARN is perfectly reasonable (IOW: on failure > of the following goto cleanup with locked == true, try again, and spew > that message). > > This repeats a few times, but I'll note just once.
I don't think it is safe to unlock something twice if we've locked it just once. > >> + >> + if (virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) >> + goto cleanup; >> + >> ret = 0; >> cleanup: >> virSecurityManagerTransactionAbort(driver->securityManager); >> + if (locked && >> + virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) >> + VIR_WARN("unable to release metadata lock"); > > Should we add the @stdin_path and @vm->pid and/or vm->name here? It may > help some day. Similar comments for other new VIR_WARN's, but the > parameters change... Ah, good point. Okay. > >> return ret; >> } >> >> @@ -68,6 +83,10 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, >> bool migrated) >> { >> qemuDomainObjPrivatePtr priv = vm->privateData; >> + bool unlock = true; >> + >> + if (virDomainLockMetadataLock(driver->lockManager, vm) < 0) >> + unlock = false; >> >> /* In contrast to qemuSecuritySetAllLabel, do not use >> * secdriver transactions here. This function is called from >> @@ -79,6 +98,10 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, >> vm->def, >> migrated, >> priv->chardevStdioLogd); >> + >> + if (unlock && >> + virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) >> + VIR_WARN("unable to release metadata lock"); >> } >> >> >> @@ -88,6 +111,12 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, >> virDomainDiskDefPtr disk) >> { >> int ret = -1; >> + bool locked = false; >> + >> + if (virDomainLockMetadataDiskLock(driver->lockManager, vm, disk) < 0) >> + goto cleanup; >> + >> + locked = true; >> >> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && >> virSecurityManagerTransactionStart(driver->securityManager) < 0) >> @@ -103,9 +132,17 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, >> vm->pid) < 0) >> goto cleanup; >> >> + locked = false; >> + >> + if (virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) >> + goto cleanup; >> + >> ret = 0; >> cleanup: >> virSecurityManagerTransactionAbort(driver->securityManager); >> + if (locked && >> + virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) >> + VIR_WARN("unable to release disk metadata lock"); >> return ret; >> } >> >> @@ -116,6 +153,12 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, >> virDomainDiskDefPtr disk) >> { >> int ret = -1; >> + bool locked = false; >> + >> + if (virDomainLockMetadataDiskLock(driver->lockManager, vm, disk) < 0) >> + goto cleanup; >> + >> + locked = true; >> >> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && >> virSecurityManagerTransactionStart(driver->securityManager) < 0) >> @@ -131,9 +174,17 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, >> vm->pid) < 0) >> goto cleanup; >> >> + locked = false; >> + >> + if (virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) >> + goto cleanup; >> + >> ret = 0; >> cleanup: >> virSecurityManagerTransactionAbort(driver->securityManager); >> + if (locked && >> + virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) >> + VIR_WARN("unable to release disk metadata lock"); >> return ret; >> } >> >> @@ -144,6 +195,12 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, >> virStorageSourcePtr src) >> { >> int ret = -1; >> + bool locked = false; >> + >> + if (virDomainLockMetadataImageLock(driver->lockManager, vm, src) < 0) >> + goto cleanup; >> + >> + locked = true; >> >> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && >> virSecurityManagerTransactionStart(driver->securityManager) < 0) >> @@ -159,9 +216,17 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, >> vm->pid) < 0) >> goto cleanup; >> >> + locked = false; >> + >> + if (virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) >> + goto cleanup; >> + >> ret = 0; >> cleanup: >> virSecurityManagerTransactionAbort(driver->securityManager); >> + if (locked && >> + virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) >> + VIR_WARN("unable to release image metadata lock"); >> return ret; >> } >> >> @@ -172,6 +237,12 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, >> virStorageSourcePtr src) >> { >> int ret = -1; >> + bool locked = false; >> + >> + if (virDomainLockMetadataImageLock(driver->lockManager, vm, src) < 0) >> + goto cleanup; >> + >> + locked = true; >> >> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && >> virSecurityManagerTransactionStart(driver->securityManager) < 0) >> @@ -187,9 +258,17 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, >> vm->pid) < 0) >> goto cleanup; >> >> + locked = false; >> + >> + if (virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) >> + goto cleanup; >> + >> ret = 0; >> cleanup: >> virSecurityManagerTransactionAbort(driver->securityManager); >> + if (locked && >> + virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) >> + VIR_WARN("unable to release image metadata lock"); >> return ret; >> } >> > > Just below here is HostdevLabel processing... For a SCSI host device > that would seem to cover the type of resource described in the cover > letter to patch6 - at least for iSCSI LUNs (and of course vHBA LUNs). > > Need to look at the hostdev->mode and hostdev->source.subsys.type and > dig into the subsys.u.scsi to get to the src->path. Oh, this was the reason I made VIR_WARN() so sparse. I did not wanted to get into business of getting paths from all the complicated structs we have. I'll just put vm->def->name and vm->pid into the warn message. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list