Re: [libvirt] [PATCH v3 22/28] security_manager: Introduce metadata locking APIs

On 08/27/2018 04:08 AM, Michal Privoznik wrote: milliseconds ^^^^^^^^^^^ consistent at least ;-) or # define METADATA_LOCK_WAIT_MAX (100 * METADATA_LOCK_SLEEP_MAX) Could use a few words of wisdom here - it's not necessary self documenting. > +int > +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr, > + const char *path) > +{ > + virSecurityManagerLockPtr lock = mgr->lock; > + unsigned long long now; > + unsigned long long then; > + int ret = -1; > + VIR_DEBUG("mgr=%p path=%s lock=%p", mgr, path, lock); > + if (!lock) > + return 0; I'm still wondering how this could be true... If this happens and we return 0, couldn't the caller have a false sense of security? > + virObjectLock(lock); > + while (lock->pathLocked) { Someone already operating on the thing. virCondWaitUntil perhaps? If we get here, but considering the previous patch where something else "force"'d the CondSignal, then patchLocked == false now... So if there were more than 1 waiter what's going to happen next... Should this fail? Should that force code set a flag or something to indicate everyone start walking the plank? > + } > + if (!lock->lock && > + !(lock->lock = virSecurityManagerNewLockManager(lock))) > + goto cleanup; Finally we're getting lock->lock filled in, knew it would happen some day! > + if (virLockManagerAddResource(lock->lock, > + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA, > + path, 0, NULL, 0) < 0) > + goto cleanup; > + if (virTimeMillisNowRaw(&now) < 0) { > + virReportSystemError(errno, "%s", > + _("Unable to get system time")); > + goto cleanup; > + } > + then = now + METADATA_LOCK_WAIT_MAX; > + while (1) { > + uint32_t s; > + int rc; > + rc = virLockManagerAcquire(lock->lock, NULL, > + VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN, > + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL); > + if (!rc) > + break; > + if (rc < 0) { > + virErrorPtr err = virGetLastError(); Coverity notes that @err can be NULL at this point and thus the subsequent accesses won't be happen Consider: virLastErrorIsSystemErrno Consider: virGetLastErrorCode > + /* Some regular error. Exit now. */ > + goto cleanup; > + } > + /* Proceed to waiting & retry. */ > + } > + if (now >= then) Might be nice to add a timeout error message... > + goto cleanup; > + s = virRandomInt(METADATA_LOCK_SLEEP_MAX) + 1; > + if (now + s > then) > + s = then - now; > + usleep(1000 * s); > + if (virTimeMillisNowRaw(&now) < 0) { > + virReportSystemError(errno, "%s", > + _("Unable to get system time")); > + goto cleanup; > + } Does this really need to be all that complicated? What about using virTimeBackOff{Start|Wait} > + } > + lock->pathLocked = true; Yay, been waiting for this one too ;-) Should this code grab/save the current error message if (ret < 0) so that nothing overwrites it in the subsequent calls? Coverity also notes that by checking lock->lock here But not here... means it's possible the above blindly derefs lock->lock eventually in virLockManagerCloseConn Beyond that why are we calling virLockManagerClearResources if we have acquired the lock? > + virObjectUnlock(lock); > + return ret; > +} > +int > +virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr, > + const char *path) > +{ > + virSecurityManagerLockPtr lock = mgr->lock; > + int ret = -1; > + VIR_DEBUG("mgr=%p path=%s lock=%p", mgr, path, lock); > + if (!lock) > + return 0; Sigh. > + virObjectLock(lock); > + /* Shouldn't happen, but doesn't hurt to check. */ > + if (!lock->lock) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("unlock mismatch")); > + goto cleanup; > + } > + if (virLockManagerAddResource(lock->lock, > + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA, > + path, 0, NULL, 0) < 0) > + goto cleanup; Shouldn't the resource already be added? If we didn't clear the resources above, then we wouldn't need this would we? I could be missing something subtle... > + if (virLockManagerRelease(lock->lock, NULL, > + VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN) < 0) > + goto cleanup; > + lock->pathLocked = false; > + virCondSignal(&lock->cond); > + virLockManagerClearResources(lock->lock, 0); This would seemingly happen after successful Release wouldn't it? I Add a resource, I lock a resource, I use a resource, I unlock a resource, I clear a resource. John > + if (ret < 0) > + virSecurityManagerLockCloseConnLocked(lock, true); > + virObjectUnlock(lock); > + return ret; > +} > diff --git a/src/security/security_manager.h b/src/security/security_manager.h > index c589b8808d..d6f36272eb 100644 > --- a/src/security/security_manager.h > +++ b/src/security/security_manager.h > @@ -198,4 +198,18 @@ int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr, > int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr, > virDomainDefPtr vm); > > +/* Ideally, these APIs wouldn't be here and the security manager > + * would call lock and unlock from these APIs above just before > + * calling corresponding callback from the driver. However, that > + * means we would have to dig out paths from all the possible > + * devices that APIs above handle which effectively means > + * duplicating code from the driver (which has to do it already > + * anyway). > + * Therefore, have these APIs and let the driver call them when > + * needed. */ > +int virSecurityManagerMetadataLock(virSecurityManagerPtr mgr, > + const char *path); > +int virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr, > + const char *path); > #endif /* VIR_SECURITY_MANAGER_H__ */ >