[libvirt] [PATCH 3/4] virLockManagerLockDaemonAcquire: Duplicate client FD with CLOEXEC flag

[Michal Privoznik]

On 09/25/2018 06:29 PM, John Ferlan wrote:
> 
> 
> On 9/21/18 5:29 AM, Michal Privoznik wrote:
>> There is one caller (virSecurityManagerMetadataLock) which
>> duplicates the connection FD and wants to have the flag set.
>> However, trying to set the flag after dup() is not safe as
>> another thread might fork() meanwhile. Therefore, switch to
>> duplicating with the flag set and only let callers refine this
>> later.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/locking/domain_lock.c       | 18 ++++++++++++++++++
>>  src/locking/lock_driver_lockd.c |  2 +-
>>  src/rpc/virnetclient.c          |  9 +--------
>>  src/rpc/virnetclient.h          |  2 +-
>>  4 files changed, 21 insertions(+), 10 deletions(-)
>>
> 
>> diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
>> index 705b132457..db20fa86a3 100644
>> --- a/src/locking/domain_lock.c
>> +++ b/src/locking/domain_lock.c
>> @@ -188,6 +188,24 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin,
>>      ret = virLockManagerAcquire(lock, NULL, flags,
>>                                  dom->def->onLockFailure, fd);
>>  
>> +    if (ret >= 0 && fd && *fd >= 0 && virSetInherit(*fd, true) < 0) {
> 
> Not quite sure 'how' ret > 0, but I'll go with it considering other
> consumers perform the same check.

Yeah, maybe there's no way for ret > 0 right now. I am just trying to be
more future proof and follow our (perhaps unwritten) rule on return
values: a negative value means error, zero or positive value means success.

> 
>> +        int saved_errno = errno;
>> +        virErrorPtr origerr;
>> +
>> +        virErrorPreserveLast(&origerr);
>> +
>> +        if (virLockManagerRelease(lock, NULL, 0) < 0)
>> +            VIR_WARN("Unable to release acquired resourced in cleanup path");
> 
> *resource(s)
> 
>> +
>> +        virErrorRestore(&origerr);
>> +        errno = saved_errno;
>> +
>> +        virReportSystemError(errno, "%s",
>> +                             _("Cannot disable close-on-exec flag"));
>> +
>> +        ret = -1;
>> +    }
>> +
> 
> OK so this perhaps *is* the only thing you're after. Discounting patch2,
> you get a dup()'d socket and you then remove the CLOEXEC from it.

Actually, I am trying to fix leaking FD when doing metadata locking.

> 
> The rest of the patch doesn't seem to matter.  Perhaps some day there is
> a virNetClientDupFD consumer that wants to pass @true, then they have to
> add back all that you removed.

That should never happen (TM). If somebody will try to revert these two
patches they will have to deal with FD leaks in some way. I'd be
interested to see how they do that.

Michal