Merge changes I9ac3f420,Icff7679a into main

* changes:
  traced: improve signature of Start and keep backward compat
  Revert "tracing: clean up FD-passing when starting the service"

include/perfetto/ext/ipc/host.h

@@ -41,11 +41,14 @@ class Host {
  public:
   // Creates an instance and starts listening on the given |socket_name|.
   // Returns nullptr if listening on the socket fails.
-  // socket_name can be fd://123 for sockets pre-bound by init and passed as FD
-  // across exec in an env var.
   static std::unique_ptr<Host> CreateInstance(const char* socket_name,
                                               base::TaskRunner*);
 
+  // Like the above but takes a file descriptor to a pre-bound unix socket.
+  // Returns nullptr if listening on the socket fails.
+  static std::unique_ptr<Host> CreateInstance(base::ScopedSocketHandle,
+                                              base::TaskRunner*);
+
   // Creates a Host which is not backed by a POSIX listening socket.
   // Instead, it accepts sockets passed in via AdoptConnectedSocket_Fuchsia().
   // See go/fuchsetto for more details.

include/perfetto/ext/tracing/ipc/service_ipc_host.h

@@ -17,14 +17,13 @@
 #ifndef INCLUDE_PERFETTO_EXT_TRACING_IPC_SERVICE_IPC_HOST_H_
 #define INCLUDE_PERFETTO_EXT_TRACING_IPC_SERVICE_IPC_HOST_H_
 
+#include <list>
 #include <memory>
 
 #include "perfetto/base/export.h"
-#include "perfetto/ext/base/scoped_file.h"
 #include "perfetto/ext/base/unix_socket.h"
 #include "perfetto/ext/tracing/core/basic_types.h"
 #include "perfetto/ext/tracing/core/tracing_service.h"
-#include "perfetto/tracing/default_socket.h"
 
 namespace perfetto {
 namespace base {
@@ -35,6 +34,32 @@ namespace ipc {
 class Host;
 }  // namespace ipc
 
+// The argument passed to ServiceIPCHost::Start. Can be either:
+// 1. a socket name (e.g., "/dev/unix/socket" for AF_UNIX, "127.0.0.1:1234" for
+//    TCP, "vsock://1:1234")
+// 2. A FD of a pre-bound socket. This handles the case of Android in-tree
+//    builds where init creates the socket and passes the FD in env var
+//    (See perfetto.rc).
+// 3. A pre-existing ipc::Host object.
+struct ListenEndpoint {
+  explicit ListenEndpoint(const char* socket_name);
+  explicit ListenEndpoint(std::string socket_name);
+  explicit ListenEndpoint(base::ScopedSocketHandle);
+  explicit ListenEndpoint(std::unique_ptr<ipc::Host>);
+  ~ListenEndpoint();
+
+  // Allow move but not copy.
+  ListenEndpoint(ListenEndpoint&&) noexcept;
+  ListenEndpoint& operator=(ListenEndpoint&&);
+  ListenEndpoint(const ListenEndpoint&) noexcept = delete;
+  ListenEndpoint& operator=(const ListenEndpoint&) noexcept = delete;
+
+  // Only one of these is ever set.
+  std::string sock_name;
+  base::ScopedSocketHandle sock_handle;
+  std::unique_ptr<ipc::Host> ipc_host;
+};
+
 // Creates an instance of the service (business logic + UNIX socket transport).
 // Exposed to:
 //   The code in the tracing client that will host the service e.g., traced.
@@ -47,27 +72,32 @@ class PERFETTO_EXPORT_COMPONENT ServiceIPCHost {
       TracingService::InitOpts = {});
   virtual ~ServiceIPCHost();
 
-  // The overload to wrap the multi-value producer socket name in the
-  // single-value variant for compatibility in tests.
-  // The socket name can be fd://123 to pass a pre-bound socket. This is used
-  // when building as part of the Android tree, where init opens and binds the
-  // socket beore exec()-ing us.
-  bool Start(const char* producer_socket_name,
-             const char* consumer_socket_name) {
-    return Start(TokenizeProducerSockets(producer_socket_name),
-                 consumer_socket_name);
-  }
-
   // Start listening on the Producer & Consumer ports. Returns false in case of
   // failure (e.g., something else is listening on |socket_name|).
-  virtual bool Start(const std::vector<std::string>& producer_socket_names,
-                     const char* consumer_socket_name) = 0;
+  virtual bool Start(std::list<ListenEndpoint> producer_sockets,
+                     ListenEndpoint consumer_socket) = 0;
+
+  virtual TracingService* service() const = 0;
+
+  // The methods below are for API compatibility with other projects that use
+  // some of the old flavours of Start(), back in the days when we supported
+  // only one socket or fd.
+
+  // Like the above, but takes two file descriptors to already bound sockets.
+  // This is used when building as part of the Android tree, where init opens
+  // and binds the socket beore exec()-ing us.
+  // Note: An internal Google project uses this (b/390202952).
+  bool Start(base::ScopedSocketHandle producer_socket_fd,
+             base::ScopedSocketHandle consumer_socket_fd);
 
   // Allows callers to supply preconstructed Hosts.
-  virtual bool Start(std::unique_ptr<ipc::Host> producer_host,
-                     std::unique_ptr<ipc::Host> consumer_host) = 0;
+  bool Start(std::unique_ptr<ipc::Host> producer_host,
+             std::unique_ptr<ipc::Host> consumer_host);
 
-  virtual TracingService* service() const = 0;
+  // Used by tests. producer_socket_name can be a comma-separated list of N
+  // endpoints to listen onto.
+  bool Start(const char* producer_socket_names,
+             const char* consumer_socket_name);
 
  protected:
   ServiceIPCHost();

src/ipc/host_impl.cc

@@ -26,7 +26,6 @@
 #include "perfetto/base/task_runner.h"
 #include "perfetto/base/time.h"
 #include "perfetto/ext/base/crash_keys.h"
-#include "perfetto/ext/base/string_utils.h"
 #include "perfetto/ext/base/sys_types.h"
 #include "perfetto/ext/base/unix_socket.h"
 #include "perfetto/ext/base/utils.h"
@@ -124,34 +123,36 @@ std::unique_ptr<Host> Host::CreateInstance(const char* socket_name,
   return std::unique_ptr<Host>(std::move(host));
 }
 
+// static
+std::unique_ptr<Host> Host::CreateInstance(base::ScopedSocketHandle socket_fd,
+                                           base::TaskRunner* task_runner) {
+  std::unique_ptr<HostImpl> host(
+      new HostImpl(std::move(socket_fd), task_runner));
+  if (!host->sock() || !host->sock()->is_listening())
+    return nullptr;
+  return std::unique_ptr<Host>(std::move(host));
+}
+
 // static
 std::unique_ptr<Host> Host::CreateInstance_Fuchsia(
     base::TaskRunner* task_runner) {
   return std::unique_ptr<HostImpl>(new HostImpl(task_runner));
 }
 
-HostImpl::HostImpl(const char* socket_name, base::TaskRunner* task_runner)
+HostImpl::HostImpl(base::ScopedSocketHandle socket_fd,
+                   base::TaskRunner* task_runner)
     : task_runner_(task_runner), weak_ptr_factory_(this) {
   PERFETTO_DCHECK_THREAD(thread_checker_);
+  sock_ = base::UnixSocket::Listen(std::move(socket_fd), this, task_runner_,
+                                   kHostSockFamily, base::SockType::kStream);
+}
 
-  static constexpr char kFdPrefix[] = "fd://";
-  if (strncmp(socket_name, kFdPrefix, strlen(kFdPrefix)) == 0) {
-#if PERFETTO_BUILDFLAG(PERFETTO_OS_WIN)
-    PERFETTO_FATAL("%s is only supported on POSIX systems", socket_name);
-#else
-    auto fd_num = base::CStringToInt32(&socket_name[strlen(kFdPrefix)]);
-    if (!fd_num.has_value()) {
-      PERFETTO_FATAL("Failed to parse fd:// number from %s", socket_name);
-    }
-    sock_ = base::UnixSocket::Listen(base::ScopedFile(*fd_num), this,
-                                     task_runner_, base::SockFamily::kUnix,
-                                     base::SockType::kStream);
-#endif
-  } else {
-    sock_ = base::UnixSocket::Listen(socket_name, this, task_runner_,
-                                     base::GetSockFamily(socket_name),
-                                     base::SockType::kStream);
-  }
+HostImpl::HostImpl(const char* socket_name, base::TaskRunner* task_runner)
+    : task_runner_(task_runner), weak_ptr_factory_(this) {
+  PERFETTO_DCHECK_THREAD(thread_checker_);
+  sock_ = base::UnixSocket::Listen(socket_name, this, task_runner_,
+                                   base::GetSockFamily(socket_name),
+                                   base::SockType::kStream);
   if (!sock_) {
     PERFETTO_PLOG("Failed to create %s", socket_name);
   }

src/ipc/host_impl.h

@@ -38,10 +38,9 @@ constexpr uint32_t kDefaultIpcTxTimeoutMs = 10000;
 
 class HostImpl : public Host, public base::UnixSocket::EventListener {
  public:
-  // socket_name can be fd://123 for sockets pre-bound by init and passed as FD
-  // across exec in an env var.
   HostImpl(const char* socket_name, base::TaskRunner*);
-  explicit HostImpl(base::TaskRunner* task_runner);
+  HostImpl(base::ScopedSocketHandle, base::TaskRunner*);
+  HostImpl(base::TaskRunner* task_runner);
   ~HostImpl() override;
 
   // Host implementation.

src/traced/service/service.cc

@@ -150,21 +150,25 @@ int PERFETTO_EXPORT_ENTRYPOINT ServiceMain(int argc, char** argv) {
 #if PERFETTO_BUILDFLAG(PERFETTO_OS_WIN)
     PERFETTO_CHECK(false);
 #else
-    base::StackString<32> prod_sock("fd://%s", env_prod);
-    base::StackString<32> cons_sock("fd://%s", env_cons);
-    started = svc->Start({prod_sock.ToStdString()}, cons_sock.c_str());
+    ListenEndpoint consumer_ep(base::ScopedFile(atoi(env_cons)));
+    std::list<ListenEndpoint> producer_eps;
+    producer_eps.emplace_back(ListenEndpoint(base::ScopedFile(atoi(env_prod))));
+    started = svc->Start(std::move(producer_eps), std::move(consumer_ep));
 #endif
   } else {
-    auto producer_sockets = TokenizeProducerSockets(GetProducerSocket());
-    for (const auto& producer_socket : producer_sockets) {
-      remove(producer_socket.c_str());
+    std::list<ListenEndpoint> producer_eps;
+    auto producer_socket_names = TokenizeProducerSockets(GetProducerSocket());
+    for (const auto& producer_socket_name : producer_socket_names) {
+      remove(producer_socket_name.c_str());
+      producer_eps.emplace_back(ListenEndpoint(producer_socket_name));
     }
     remove(GetConsumerSocket());
-    started = svc->Start(producer_sockets, GetConsumerSocket());
+    started = svc->Start(std::move(producer_eps),
+                         ListenEndpoint(GetConsumerSocket()));
 
     if (!producer_socket_group.empty()) {
       auto status = base::OkStatus();
-      for (const auto& producer_socket : producer_sockets) {
+      for (const auto& producer_socket : producer_socket_names) {
         if (base::GetSockFamily(producer_socket.c_str()) !=
             base::SockFamily::kUnix) {
           // Socket permissions is only available to unix sockets.

src/tracing/ipc/service/BUILD.gn

@@ -37,6 +37,7 @@ source_set("service") {
   deps = [
     "..:common",
     "../../../../gn:default_deps",
+    "../../../../include/perfetto/tracing",
     "../../../../protos/perfetto/ipc",
     "../../../base",
     "../../core:core",

src/tracing/ipc/service/service_ipc_host_impl.cc

@@ -16,10 +16,13 @@
 
 #include "src/tracing/ipc/service/service_ipc_host_impl.h"
 
+#include <list>
+
 #include "perfetto/base/logging.h"
 #include "perfetto/base/task_runner.h"
 #include "perfetto/ext/ipc/host.h"
 #include "perfetto/ext/tracing/core/tracing_service.h"
+#include "perfetto/tracing/default_socket.h"
 #include "src/tracing/ipc/service/consumer_ipc_service.h"
 #include "src/tracing/ipc/service/producer_ipc_service.h"
 #include "src/tracing/ipc/service/relay_ipc_service.h"
@@ -34,8 +37,23 @@ namespace perfetto {
 
 namespace {
 constexpr uint32_t kProducerSocketTxTimeoutMs = 10;
+
+std::unique_ptr<ipc::Host> CreateIpcHost(base::TaskRunner* task_runner,
+                                         ListenEndpoint ep) {
+  if (!ep.sock_name.empty()) {
+    PERFETTO_DCHECK(!ep.sock_handle && !ep.ipc_host);
+    return ipc::Host::CreateInstance(ep.sock_name.c_str(), task_runner);
+  }
+  if (ep.sock_handle) {
+    PERFETTO_DCHECK(!ep.ipc_host);
+    return ipc::Host::CreateInstance(std::move(ep.sock_handle), task_runner);
+  }
+  PERFETTO_DCHECK(ep.ipc_host);
+  return std::move(ep.ipc_host);
 }
 
+}  // namespace
+
 // TODO(fmayer): implement per-uid connection limit (b/69093705).
 
 // Implements the publicly exposed factory method declared in
@@ -53,29 +71,16 @@ ServiceIPCHostImpl::ServiceIPCHostImpl(base::TaskRunner* task_runner,
 
 ServiceIPCHostImpl::~ServiceIPCHostImpl() {}
 
-bool ServiceIPCHostImpl::Start(
-    const std::vector<std::string>& producer_socket_names,
-    const char* consumer_socket_name) {
+bool ServiceIPCHostImpl::Start(std::list<ListenEndpoint> producer_sockets,
+                               ListenEndpoint consumer_socket) {
   PERFETTO_CHECK(!svc_);  // Check if already started.
 
   // Initialize the IPC transport.
-  for (const auto& producer_socket_name : producer_socket_names)
+  for (auto& sock : producer_sockets) {
     producer_ipc_ports_.emplace_back(
-        ipc::Host::CreateInstance(producer_socket_name.c_str(), task_runner_));
-  consumer_ipc_port_ =
-      ipc::Host::CreateInstance(consumer_socket_name, task_runner_);
-  return DoStart();
-}
-
-bool ServiceIPCHostImpl::Start(std::unique_ptr<ipc::Host> producer_host,
-                               std::unique_ptr<ipc::Host> consumer_host) {
-  PERFETTO_CHECK(!svc_);  // Check if already started.
-  PERFETTO_DCHECK(producer_host);
-  PERFETTO_DCHECK(consumer_host);
-
-  // Initialize the IPC transport.
-  producer_ipc_ports_.emplace_back(std::move(producer_host));
-  consumer_ipc_port_ = std::move(consumer_host);
+        CreateIpcHost(task_runner_, std::move(sock)));
+  }
+  consumer_ipc_port_ = CreateIpcHost(task_runner_, std::move(consumer_socket));
 
   return DoStart();
 }
@@ -151,4 +156,42 @@ void ServiceIPCHostImpl::Shutdown() {
 ServiceIPCHost::ServiceIPCHost() = default;
 ServiceIPCHost::~ServiceIPCHost() = default;
 
+// Definitions for ListenEndpoint, declared in service_ipc_host.h.
+ListenEndpoint::ListenEndpoint(const char* socket_name)
+    : sock_name(socket_name) {}
+ListenEndpoint::ListenEndpoint(std::string socket_name)
+    : sock_name(std::move(socket_name)) {}
+ListenEndpoint::ListenEndpoint(base::ScopedSocketHandle sh)
+    : sock_handle(std::move(sh)) {}
+ListenEndpoint::ListenEndpoint(std::unique_ptr<ipc::Host> ih)
+    : ipc_host(std::move(ih)) {}
+ListenEndpoint::ListenEndpoint(ListenEndpoint&&) noexcept = default;
+ListenEndpoint& ListenEndpoint::operator=(ListenEndpoint&&) = default;
+ListenEndpoint::~ListenEndpoint() = default;
+
+// Definitions for overloads of Start, declared in service_ipc_host.h.
+
+bool ServiceIPCHost::Start(const char* producer_socket_names,
+                           const char* consumer_socket_name) {
+  std::list<ListenEndpoint> eps;
+  for (const auto& sock_name : TokenizeProducerSockets(producer_socket_names)) {
+    eps.emplace_back(ListenEndpoint(sock_name));
+  }
+  return Start(std::move(eps), ListenEndpoint(consumer_socket_name));
+}
+
+bool ServiceIPCHost::Start(base::ScopedSocketHandle producer_socket_fd,
+                           base::ScopedSocketHandle consumer_socket_fd) {
+  std::list<ListenEndpoint> eps;
+  eps.emplace_back(ListenEndpoint(std::move(producer_socket_fd)));
+  return Start(std::move(eps), ListenEndpoint(std::move(consumer_socket_fd)));
+}
+
+bool ServiceIPCHost::Start(std::unique_ptr<ipc::Host> producer_host,
+                           std::unique_ptr<ipc::Host> consumer_host) {
+  std::list<ListenEndpoint> eps;
+  eps.emplace_back(ListenEndpoint(std::move(producer_host)));
+  return Start(std::move(eps), ListenEndpoint(std::move(consumer_host)));
+}
+
 }  // namespace perfetto

src/tracing/ipc/service/service_ipc_host_impl.h

@@ -38,10 +38,8 @@ class ServiceIPCHostImpl : public ServiceIPCHost {
   ~ServiceIPCHostImpl() override;
 
   // ServiceIPCHost implementation.
-  bool Start(const std::vector<std::string>& producer_socket_names,
-             const char* consumer_socket_name) override;
-  bool Start(std::unique_ptr<ipc::Host> producer_host,
-             std::unique_ptr<ipc::Host> consumer_host) override;
+  bool Start(std::list<ListenEndpoint> producer_sockets,
+             ListenEndpoint consumer_socket) override;
 
   TracingService* service() const override;