Re: [libvirt] [PATCH 3/4] virLockManagerLockDaemonAcquire: Duplicate client FD with CLOEXEC flag
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(a)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);