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

[John Ferlan]

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.

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

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.

John

>      virLockManagerFree(lock);
>  
>      return ret;
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 0c672b05b1..9b1943daa6 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -796,7 +796,7 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
>          goto cleanup;
>  
>      if (fd &&
> -        (*fd = virNetClientDupFD(client, false)) < 0)
> +        (*fd = virNetClientDupFD(client)) < 0)
>          goto cleanup;
>  
>      if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) {
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index 40ed3fde6d..6b0ddbeaad 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -712,19 +712,12 @@ int virNetClientGetFD(virNetClientPtr client)
>  }
>  
>  
> -int virNetClientDupFD(virNetClientPtr client, bool cloexec)
> +int virNetClientDupFD(virNetClientPtr client)
>  {
>      int fd;
>  
>      virObjectLock(client);
> -
>      fd = virNetSocketDupFD(client->sock);
> -    if (!cloexec && fd >= 0 && virSetInherit(fd, true)) {
> -        virReportSystemError(errno, "%s",
> -                             _("Cannot disable close-on-exec flag"));
> -        VIR_FORCE_CLOSE(fd);
> -    }
> -
>      virObjectUnlock(client);
>      return fd;
>  }
> diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h
> index 9cf32091f5..3702f7fe5a 100644
> --- a/src/rpc/virnetclient.h
> +++ b/src/rpc/virnetclient.h
> @@ -95,7 +95,7 @@ void virNetClientSetCloseCallback(virNetClientPtr client,
>                                    virFreeCallback ff);
>  
>  int virNetClientGetFD(virNetClientPtr client);
> -int virNetClientDupFD(virNetClientPtr client, bool cloexec);
> +int virNetClientDupFD(virNetClientPtr client);
>  
>  bool virNetClientHasPassFD(virNetClientPtr client);
>  
>