mesos-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jpe...@apache.org
Subject [mesos] 02/02: Switched to `net::socketpair` in `ns::clone`.
Date Tue, 16 Oct 2018 23:30:47 GMT
This is an automated email from the ASF dual-hosted git repository.

jpeach pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git

commit 2ef75068d2583b9995cf944dab7ff0311b484f51
Author: James Peach <jpeach@apache.org>
AuthorDate: Tue Oct 16 16:30:24 2018 -0700

    Switched to `net::socketpair` in `ns::clone`.
    
    Started using to `net::socketpair` in `ns::clone` so that we don't
    have to deal with `SOCK_CLOEXEC` portability.
    
    Review: https://reviews.apache.org/r/63655/
---
 src/linux/ns.cpp | 69 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 30 deletions(-)

diff --git a/src/linux/ns.cpp b/src/linux/ns.cpp
index ffa9b65..4d16381 100644
--- a/src/linux/ns.cpp
+++ b/src/linux/ns.cpp
@@ -22,6 +22,7 @@
 #include <sys/wait.h>
 
 #include <cstring>
+#include <type_traits>
 #include <vector>
 
 #include <process/collect.hpp>
@@ -43,6 +44,7 @@
 
 #include <stout/os/exists.hpp>
 #include <stout/os/ls.hpp>
+#include <stout/os/socket.hpp>
 
 #include "common/status_utils.hpp"
 
@@ -268,6 +270,23 @@ Result<ino_t> getns(pid_t pid, const string& ns)
 }
 
 
+// Helper for closing a container of file descriptors.
+template <
+  typename Iterable,
+  typename = typename std::enable_if<
+    std::is_same<typename Iterable::value_type, int>::value>::type>
+static void close(const Iterable& fds)
+{
+  int errsav = errno;
+
+  foreach (int fd, fds) {
+    ::close(fd); // Need to call the async-signal safe version.
+  }
+
+  errno = errsav;
+}
+
+
 Try<pid_t> clone(
     pid_t target,
     int nstypes,
@@ -324,13 +343,6 @@ Try<pid_t> clone(
   // File descriptors keyed by the (parent) namespace we are entering.
   hashmap<int, int> fds = {};
 
-  // Helper for closing a list of file descriptors.
-  auto close = [](const std::vector<int>& fds) {
-    foreach (int fd, fds) {
-      ::close(fd); // Need to call the async-signal safe version.
-    }
-  };
-
   // NOTE: we do all of this ahead of time so we can be async signal
   // safe after calling fork below.
   for (size_t i = 0; i < NAMESPACES; i++) {
@@ -353,10 +365,10 @@ Try<pid_t> clone(
   // `sockets[0]` and the child socket is `sockets[1]`. Note that both
   // sockets are both read/write but currently only the parent reads
   // and the child writes.
-  int sockets[2] = {-1, -1};
-  if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) != 0) {
+  Try<std::array<int_fd, 2>> sockets = net::socketpair(AF_UNIX, SOCK_STREAM,
0);
+  if (sockets.isError()) {
     close(fds.values());
-    return ErrnoError("Failed to create Unix domain socket");
+    return Error("Failed to create Unix domain socket: " + sockets.error());
   }
 
   // Need to set SO_PASSCRED option in order to receive credentials
@@ -365,12 +377,10 @@ Try<pid_t> clone(
   // receiving, not also for sending.
   const int value = 1;
   const socklen_t size = sizeof(value);
-  if (setsockopt(sockets[0], SOL_SOCKET, SO_PASSCRED, &value, size) == -1) {
-    Error error = ErrnoError("Failed to set socket option SO_PASSCRED");
+  if (setsockopt(sockets->at(0), SOL_SOCKET, SO_PASSCRED, &value, size) == -1) {
     close(fds.values());
-    ::close(sockets[0]);
-    ::close(sockets[1]);
-    return error;
+    close(sockets.get());
+    return ErrnoError("Failed to set socket option SO_PASSCRED");
   }
 
   // NOTE: to determine the pid of the final process executing the
@@ -424,17 +434,16 @@ Try<pid_t> clone(
   if (child < 0) {
     stack->deallocate();
     close(fds.values());
-    ::close(sockets[0]);
-    ::close(sockets[1]);
+    close(sockets.get());
     return ErrnoError();
   } else if (child > 0) {
     // Parent.
     stack->deallocate();
 
     close(fds.values());
-    ::close(sockets[1]);
+    ::close(sockets->at(1));
 
-    ssize_t length = recvmsg(sockets[0], &message, 0);
+    ssize_t length = recvmsg(sockets->at(0), &message, 0);
 
     // TODO(benh): Note that whenever we 'kill(child, SIGKILL)' below
     // we don't guarantee cleanup! It's possible that the
@@ -447,17 +456,17 @@ Try<pid_t> clone(
       // (which might die on it's own trying to write to the
       // socket).
       Error error = ErrnoError("Failed to receive");
-      ::close(sockets[0]);
+      ::close(sockets->at(0));
       kill(child, SIGKILL);
       return error;
     } else if (length == 0) {
       // Socket closed, child must have died, but kill anyway.
-      ::close(sockets[0]);
+      ::close(sockets->at(0));
       kill(child, SIGKILL);
       return Error("Failed to receive: Socket closed");
     }
 
-    ::close(sockets[0]);
+    ::close(sockets->at(0));
 
     // Extract pid.
     if (CMSG_FIRSTHDR(&message) == nullptr ||
@@ -501,7 +510,7 @@ Try<pid_t> clone(
     return pid;
   } else {
     // Child.
-    ::close(sockets[0]);
+    ::close(sockets->at(0));
 
     // Loop through and 'setns' into all of the parent namespaces that
     // have been requested.
@@ -511,7 +520,7 @@ Try<pid_t> clone(
         ASSERT(namespaces[i].nstype & nstypes);
         if (::setns(fd.get(), namespaces[i].nstype) < 0) {
           close(fds.values());
-          ::close(sockets[1]);
+          ::close(sockets->at(1));
           _exit(EXIT_FAILURE);
         }
       }
@@ -542,17 +551,17 @@ Try<pid_t> clone(
             std::memcpy(
                 CMSG_DATA(CMSG_FIRSTHDR(&message)), &cred, sizeof(ucred));
 
-            if (sendmsg(sockets[1], &message, 0) == -1) {
+            if (sendmsg(sockets->at(1), &message, 0) == -1) {
               // Failed to send the pid back to the parent!
               _exit(EXIT_FAILURE);
             }
 
-            ::close(sockets[1]);
+            ::close(sockets->at(1));
 
             return f();
           });
 
-      ::close(sockets[1]);
+      ::close(sockets->at(1));
 
       // TODO(benh): Kill ourselves with an exit status that we can
       // decode above to determine why `clone` failed.
@@ -563,7 +572,7 @@ Try<pid_t> clone(
     os::Stack grandchildStack(os::Stack::DEFAULT_SIZE);
 
     if (!grandchildStack.allocate()) {
-      ::close(sockets[1]);
+      ::close(sockets->at(1));
       _exit(EXIT_FAILURE);
     }
 
@@ -590,11 +599,11 @@ Try<pid_t> clone(
 
     if (grandchild < 0) {
       // TODO(benh): Exit with `errno` in order to capture `fork` error?
-      ::close(sockets[1]);
+      ::close(sockets->at(1));
       _exit(EXIT_FAILURE);
     } else if (grandchild > 0) {
       // Still the (first) child.
-      ::close(sockets[1]);
+      ::close(sockets->at(1));
 
       // Need to reap the grandchild and then just exit since we're no
       // longer necessary. Technically when the grandchild exits it'll


Mime
View raw message