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@xxxxxxxxxx>
> ---
> 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);
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
