Re: [libvirt] [PATCH v3 21/28] security_manager: Introduce virSecurityManagerLockCloseConn

On 08/31/2018 08:42 PM, John Ferlan wrote: > > > On 08/27/2018 04:08 AM, Michal Privoznik wrote: >> This is basically just a wrapper over virLockManagerCloseConn() >> so that no connection is left open when it shouldn't be. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/security/security_manager.c | 75 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 75 insertions(+) >> >> diff --git a/src/security/security_manager.c b/src/security/security_manager.c >> index caaff1f703..2238c75a5c 100644 >> --- a/src/security/security_manager.c >> +++ b/src/security/security_manager.c >> @@ -91,6 +91,56 @@ virSecurityManagerLockDispose(void *obj) >> } >> >> >> +static void >> +virSecurityManagerLockCloseConnLocked(virSecurityManagerLockPtr lock, >> + bool force) >> +{ >> + int rc; >> + >> + if (!lock) >> + return; > > Not possible - if this is true you may just want to abort or let the > following code fail miserably. The definition of the API is that it's > called when @lock is locked! > >> + >> + while (!force && >> + lock->pathLocked) { > > No need to split && across 2 lines. Still waiting for @pathLocked = > true ;-) > > >> + if (virCondWait(&lock->cond, &lock->parent.lock) < 0) { >> + VIR_WARN("Unable to wait on metadata condition"); >> + return; >> + } >> + } >> + >> + rc = virLockManagerCloseConn(lock->lock, 0); > > still haven't filled in lock->lock either > >> + if (rc < 0) >> + return; >> + if (rc > 0) >> + lock->lock = NULL; >> + >> + if (force) { >> + /* We've closed the connection. Wake up anybody who might be > > s/the/our/ > >> + * waiting. */ >> + lock->pathLocked = false; >> + virCondSignal(&lock->cond); >> + } >> +} >> + >> + >> +static void >> +virSecurityManagerLockCloseConn(virSecurityManagerLockPtr lock) >> +{ >> + if (!lock) >> + return; > > So this is lock->lock from a few patches ago. > >> + >> + virObjectLock(lock); >> + >> + if (!lock->lock) > > Still not seeing lock->lock, but maybe the next patch exposes that part. > >> + goto cleanup; >> + >> + virSecurityManagerLockCloseConnLocked(lock, false); >> + >> + cleanup: >> + virObjectUnlock(lock); >> +} >> + >> + > > I'm staring blankly at the rest of changes wondering, hmmmm... what am I > missing. > > Beyond that if the mgr->drv->*(mgr) isn't called, then should we call > the CloseConn. I'm not seeing why it's called in the first place! Because of the connection sharing (KEEP_OPEN flags) both AcquireResources and ReleaseResources will either use previously opened connection OR open a new one AND leave it open. This is important - that after the last ReleaseResource there is still a connection open to virtlockd. ReleaseResouce can't close the connection because it can't possibly know whether there is new acquire/release call pair waiting or not. It's security driver who knows that. And thus, the last one should close the connection. View from another angle, when relabelling a device we may end up relabelling one, two or even more paths. For instance: virSecurityManagerDomainSetPathLabel() relabels just one path, virSecurityManagerSetChardevLabel() or virSecurityManagerSetHostdevLabel() can relabel one, or many paths, and finally, virSecurityManagerSetAllLabel() will definitely relabel plenty of paths. Having said that, it's clear now that ReleaseResouce can't know if the path its releasing is the last one in the queue and thus if the connection can be closed. But the API knows that. So after security manager API has called the callback it knows no other path needs relabelling (i.e. acquire+release) and thus the connection can be closed. Except if there is not another thread that is already talking to virlockd and locking/unlocking paths for some other domain. Hence the connection refcounting in one of the previous patches. Yes, this is very complicated design. And if possible I'd like to simplify it. But I'm not successful on that front, sorry.