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>
> ---
> 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.
Michal