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

[John Ferlan]

On 09/04/2018 07:53 AM, Michal Privoznik wrote:
> 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 at 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.
> 

No need to apologize. The problem is complicated, tough to design, and
presents some interesting challenges with the various interactions.
Hopefully between your chat w/ mkletzan and this review you got feedback
that was helpful. Going forward when there is some assumed knowledge,
don't be shy in noting that in code comments.

In thinking more about a path related transaction solution - I begin to
wonder about interactions for/from iSCSI or LVM "devices" on a local
system that in the long run resolve to the same file. Is the fnctl using
one or the other path "hold true" and get managed somewhere underneath
libvirt in such a way that we don't have to "know" or "check" that
there's multiple paths the same thing.

John