[Reland] "[devcoordinator] Limit use of fshost's loader service"
This is a reland of 8e288427c87350b017096cda98e88b95038c0181
Reland reason: Thought to be the cause of GI roller failure
but was confirmed to be a different change.
Original change's description:
> [devcoordinator] Limit use of fshost's loader service
>
> Clarify which things use and rely on the fshost loader service today,
> rather than swapping out devcoordinator's default loader for it and
> having things inherit it through FDIO_SPAWN_DEFAULT_LDSVC.
>
> Bug: 34633
>
> Change-Id: I3a1ac3b5e08f02dd15c3b6c930f84edb895f6546
Bug: 34633
Change-Id: I2a06e986317030349c1d1bd34363bcfdcf9464c1
diff --git a/zircon/system/core/devmgr/devcoordinator/BUILD.gn b/zircon/system/core/devmgr/devcoordinator/BUILD.gn
index 4b94b03..4f85f0d 100644
--- a/zircon/system/core/devmgr/devcoordinator/BUILD.gn
+++ b/zircon/system/core/devmgr/devcoordinator/BUILD.gn
@@ -67,6 +67,7 @@
"$zx/system/fidl/fuchsia-device-manager:c",
"$zx/system/fidl/fuchsia-device-manager:llcpp",
"$zx/system/fidl/fuchsia-driver-test:c",
+ "$zx/system/fidl/fuchsia-ldsvc:llcpp",
"$zx/system/fidl/fuchsia-io:c",
"$zx/system/fidl/fuchsia-mem:c",
"$zx/system/ulib/async-loop:async-loop-cpp",
diff --git a/zircon/system/core/devmgr/devcoordinator/coordinator.cc b/zircon/system/core/devmgr/devcoordinator/coordinator.cc
index 49fd5d72..60fbc2b 100644
--- a/zircon/system/core/devmgr/devcoordinator/coordinator.cc
+++ b/zircon/system/core/devmgr/devcoordinator/coordinator.cc
@@ -361,21 +361,11 @@
return ZX_OK;
}
-static zx_status_t dc_launch_devhost(Devhost* host, DevhostLoaderService* loader_service,
+static zx_status_t dc_launch_devhost(Devhost* host, const LoaderServiceConnector& loader_connector,
const char* devhost_bin, const char* name,
const char* const* env, zx_handle_t hrpc,
const zx::resource& root_resource,
zx::unowned_job devhost_job) {
- zx::channel loader_connection;
- if (loader_service != nullptr) {
- zx_status_t status = loader_service->Connect(&loader_connection);
- if (status != ZX_OK) {
- log(ERROR, "devcoordinator: failed to use loader service: %s\n",
- zx_status_get_string(status));
- return status;
- }
- }
-
// Give devhosts the root resource if we have it (in tests, we may not)
// TODO: limit root resource to root devhost only
zx::resource resource;
@@ -425,15 +415,17 @@
};
}
- uint32_t spawn_flags = FDIO_SPAWN_CLONE_ENVIRON;
- if (loader_connection.is_valid()) {
- actions[actions_count++] = fdio_spawn_action_t{
- .action = FDIO_SPAWN_ACTION_ADD_HANDLE,
- .h = {.id = PA_HND(PA_LDSVC_LOADER, 0), .handle = loader_connection.release()},
- };
- } else {
- spawn_flags |= FDIO_SPAWN_DEFAULT_LDSVC;
+ zx::channel loader_connection;
+ status = loader_connector(&loader_connection);
+ if (status != ZX_OK) {
+ log(ERROR, "devcoordinator: failed to get devhost loader connection: %s\n",
+ zx_status_get_string(status));
+ return status;
}
+ actions[actions_count++] = fdio_spawn_action_t{
+ .action = FDIO_SPAWN_ACTION_ADD_HANDLE,
+ .h = {.id = PA_HND(PA_LDSVC_LOADER, 0), .handle = loader_connection.release()},
+ };
ZX_ASSERT(actions_count <= kMaxActions);
zx::process proc;
@@ -443,8 +435,8 @@
devhost_bin,
nullptr,
};
- status = fdio_spawn_etc(devhost_job->get(), spawn_flags, argv[0], argv, env, actions_count,
- actions, proc.reset_and_get_address(), err_msg);
+ status = fdio_spawn_etc(devhost_job->get(), FDIO_SPAWN_CLONE_ENVIRON, argv[0], argv, env,
+ actions_count, actions, proc.reset_and_get_address(), err_msg);
if (status != ZX_OK) {
log(ERROR, "devcoordinator: launch devhost '%s': failed: %s: %s\n", name,
zx_status_get_string(status), err_msg);
@@ -478,10 +470,9 @@
fbl::Vector<const char*> env;
boot_args().Collect("driver.", &env);
env.push_back(nullptr);
- status =
- dc_launch_devhost(dh.get(), loader_service_, get_devhost_bin(config_.asan_drivers), name,
- env.get(), hrpc.release(), root_resource(),
- zx::unowned_job(config_.devhost_job));
+ status = dc_launch_devhost(dh.get(), loader_service_connector_,
+ get_devhost_bin(config_.asan_drivers), name, env.get(), hrpc.release(),
+ root_resource(), zx::unowned_job(config_.devhost_job));
if (status != ZX_OK) {
zx_handle_close(dh->hrpc());
return status;
diff --git a/zircon/system/core/devmgr/devcoordinator/coordinator.h b/zircon/system/core/devmgr/devcoordinator/coordinator.h
index be4d36e..1e36e22 100644
--- a/zircon/system/core/devmgr/devcoordinator/coordinator.h
+++ b/zircon/system/core/devmgr/devcoordinator/coordinator.h
@@ -114,6 +114,8 @@
bool suspend_fallback;
};
+using LoaderServiceConnector = fit::function<zx_status_t(zx::channel*)>;
+
class Coordinator {
public:
Coordinator(const Coordinator&) = delete;
@@ -205,8 +207,9 @@
bool system_available() const { return system_available_; }
void set_system_available(bool system_available) { system_available_ = system_available; }
bool system_loaded() const { return system_loaded_; }
- void set_loader_service(DevhostLoaderService* loader_service) {
- loader_service_ = loader_service;
+
+ void set_loader_service_connector(LoaderServiceConnector loader_service_connector) {
+ loader_service_connector_ = std::move(loader_service_connector);
}
fbl::DoublyLinkedList<Driver*, Driver::Node>& drivers() { return drivers_; }
@@ -247,7 +250,7 @@
bool launched_first_devhost_ = false;
bool system_available_ = false;
bool system_loaded_ = false;
- DevhostLoaderService* loader_service_ = nullptr;
+ LoaderServiceConnector loader_service_connector_;
// Services offered to the rest of the system.
svc::Outgoing outgoing_services_;
diff --git a/zircon/system/core/devmgr/devcoordinator/main.cc b/zircon/system/core/devmgr/devcoordinator/main.cc
index 5aee442..134218e 100644
--- a/zircon/system/core/devmgr/devcoordinator/main.cc
+++ b/zircon/system/core/devmgr/devcoordinator/main.cc
@@ -6,6 +6,7 @@
#include <fcntl.h>
#include <fuchsia/boot/c/fidl.h>
#include <fuchsia/hardware/virtioconsole/llcpp/fidl.h>
+#include <fuchsia/ldsvc/llcpp/fidl.h>
#include <getopt.h>
#include <lib/async-loop/cpp/loop.h>
#include <lib/devmgr-launcher/processargs.h>
@@ -17,6 +18,7 @@
#include <lib/fdio/spawn.h>
#include <lib/fdio/unsafe.h>
#include <lib/fdio/watcher.h>
+#include <lib/fit/optional.h>
#include <lib/zx/debuglog.h>
#include <lib/zx/event.h>
#include <lib/zx/port.h>
@@ -31,7 +33,6 @@
#include <unistd.h>
#include <zircon/boot/image.h>
#include <zircon/device/vfs.h>
-#include <zircon/dlfcn.h>
#include <zircon/process.h>
#include <zircon/processargs.h>
#include <zircon/status.h>
@@ -63,7 +64,7 @@
constexpr char kRootJobPath[] = "/bootsvc/" fuchsia_boot_RootJob_Name;
constexpr char kRootResourcePath[] = "/bootsvc/" fuchsia_boot_RootResource_Name;
-struct {
+struct GlobalHandles {
// The handle used to transmit messages to appmgr.
zx::channel appmgr_client;
@@ -83,6 +84,12 @@
// The handle used by device_name_provider to serve incoming requests.
zx::channel device_name_provider_server;
+ // Handle to the loader service hosted in fshost, which allows loading from /boot and /system
+ // rather than specific packages.
+ // This isn't actually "optional", it's just initialized later.
+ // TODO(ZX-4860): Delete this once all dependencies have been removed.
+ fit::optional<llcpp::fuchsia::ldsvc::Loader::SyncClient> fshost_ldsvc;
+
zx::job svc_job;
zx::job fuchsia_job;
zx::channel svchost_outgoing;
@@ -92,7 +99,28 @@
// Used to bind the svchost to the virtual-console binary to provide fidl
// services.
zx::channel virtcon_fidl;
-} g_handles;
+
+ GlobalHandles() : fshost_ldsvc(zx::channel()) {}
+};
+
+static GlobalHandles g_handles;
+
+// TODO(ZX-4860): DEPRECATED. Do not add new dependencies on the fshost loader service!
+zx_status_t clone_fshost_ldsvc(zx::channel* loader) {
+ // Only valid to call this after fshost_ldsvc has been wired up.
+ ZX_ASSERT_MSG(g_handles.fshost_ldsvc.has_value(), "clone_fshost_ldsvc called too early");
+
+ zx::channel remote;
+ zx_status_t status = zx::channel::create(0, loader, &remote);
+ if (status != ZX_OK) {
+ return status;
+ }
+ auto result = g_handles.fshost_ldsvc->Clone(std::move(remote));
+ if (result.status() != ZX_OK) {
+ return result.status();
+ }
+ return result.Unwrap()->rv;
+}
// Wait for the requested file. Its parent directory must exist.
zx_status_t wait_for_file(const char* path, zx::time deadline) {
@@ -138,8 +166,17 @@
if (cmd != nullptr) {
auto args = devmgr::ArgumentVector::FromCmdline(cmd);
args.Print("autorun");
- devmgr::devmgr_launch(g_handles.svc_job, name, args.argv(), nullptr, -1, nullptr, nullptr, 0,
- nullptr, FS_ALL);
+
+ zx::channel ldsvc;
+ zx_status_t status = clone_fshost_ldsvc(&ldsvc);
+ if (status != ZX_OK) {
+ fprintf(stderr, "devcoordinator: failed to clone fshost loader for console: %d\n", status);
+ return;
+ }
+
+ devmgr::devmgr_launch_with_loader(g_handles.svc_job, name, zx::vmo(), std::move(ldsvc),
+ args.argv(), nullptr, -1, nullptr, nullptr, 0, nullptr,
+ FS_ALL);
}
}
@@ -251,17 +288,25 @@
struct stat s;
if (!appmgr_started && stat(argv_appmgr[0], &s) == 0) {
+ zx::channel ldsvc;
+ zx_status_t status = clone_fshost_ldsvc(&ldsvc);
+ if (status != ZX_OK) {
+ fprintf(stderr, "devcoordinator: failed to clone fshost loader for appmgr: %d\n", status);
+ continue;
+ }
+
unsigned int appmgr_hnd_count = 0;
zx_handle_t appmgr_hnds[2] = {};
uint32_t appmgr_ids[2] = {};
if (g_handles.appmgr_server.is_valid()) {
- assert(appmgr_hnd_count < fbl::count_of(appmgr_hnds));
+ ZX_ASSERT(appmgr_hnd_count < fbl::count_of(appmgr_hnds));
appmgr_hnds[appmgr_hnd_count] = g_handles.appmgr_server.release();
appmgr_ids[appmgr_hnd_count] = PA_DIRECTORY_REQUEST;
appmgr_hnd_count++;
}
- devmgr::devmgr_launch(g_handles.fuchsia_job, "appmgr", argv_appmgr, nullptr, -1, appmgr_hnds,
- appmgr_ids, appmgr_hnd_count, nullptr, FS_FOR_APPMGR);
+ devmgr::devmgr_launch_with_loader(g_handles.fuchsia_job, "appmgr", zx::vmo(),
+ std::move(ldsvc), argv_appmgr, nullptr, -1, appmgr_hnds,
+ appmgr_ids, appmgr_hnd_count, nullptr, FS_FOR_APPMGR);
appmgr_started = true;
}
if (!autorun_started) {
@@ -348,10 +393,18 @@
}
}
+ zx::channel ldsvc;
+ status = clone_fshost_ldsvc(&ldsvc);
+ if (status != ZX_OK) {
+ fprintf(stderr, "devcoordinator: failed to clone fshost loader for console: %d\n", status);
+ return 1;
+ }
+
const char* argv_sh[] = {"/boot/bin/sh", nullptr};
zx::process proc;
- status = devmgr::devmgr_launch(g_handles.svc_job, "sh:console", argv_sh, envp, fd.release(),
- nullptr, nullptr, 0, &proc, FS_ALL);
+ status = devmgr::devmgr_launch_with_loader(g_handles.svc_job, "sh:console", zx::vmo(),
+ std::move(ldsvc), argv_sh, envp, fd.release(),
+ nullptr, nullptr, 0, &proc, FS_ALL);
if (status != ZX_OK) {
printf("devcoordinator: failed to launch console shell (%s)\n", zx_status_get_string(status));
return 1;
@@ -697,17 +750,19 @@
zx_handle_t handles[ZX_CHANNEL_MAX_MSG_HANDLES];
uint32_t types[fbl::count_of(handles)];
size_t n = 0;
- zx_handle_t ldsvc;
- // Pass "fs_root", and ldsvc handles to fshost.
- if (zx_channel_create(0, g_handles.fs_root.reset_and_get_address(), &handles[n]) == ZX_OK) {
+ // Pass server ends of fs_root and ldsvc handles to fshost.
+ zx::channel fs_root_server;
+ if (zx::channel::create(0, &g_handles.fs_root, &fs_root_server) == ZX_OK) {
+ handles[n] = fs_root_server.release();
types[n++] = PA_HND(PA_USER0, 0);
}
- if (zx_channel_create(0, &ldsvc, &handles[n]) == ZX_OK) {
+ zx::channel ldsvc_client, ldsvc_server;
+ if (zx::channel::create(0, &ldsvc_client, &ldsvc_server) == ZX_OK) {
+ handles[n] = ldsvc_server.release();
types[n++] = PA_HND(PA_USER0, 2);
- } else {
- ldsvc = ZX_HANDLE_INVALID;
}
+ g_handles.fshost_ldsvc.emplace(std::move(ldsvc_client));
// The "public directory" of the fshost service.
handles[n] = fshost_server.release();
@@ -765,9 +820,6 @@
devmgr::devmgr_launch(g_handles.svc_job, "fshost", args.get(), env.get(), -1, handles, types, n,
nullptr, FS_BOOT | FS_DEV | FS_SVC);
-
- // switch to system loader service provided by fshost
- zx_handle_close(dl_set_loader_service(ldsvc));
}
void devmgr_vfs_init(devmgr::Coordinator* coordinator, const devmgr::DevmgrArgs& devmgr_args,
@@ -1226,7 +1278,23 @@
if (status != ZX_OK) {
return 1;
}
- coordinator.set_loader_service(loader_service.get());
+ coordinator.set_loader_service_connector(
+ [loader_service = std::move(loader_service)](zx::channel* c) {
+ zx_status_t status = loader_service->Connect(c);
+ if (status != ZX_OK) {
+ log(ERROR, "devcoordinator: failed to add devhost loader connection: %s\n",
+ zx_status_get_string(status));
+ }
+ return status;
+ });
+ } else {
+ coordinator.set_loader_service_connector([](zx::channel* c) {
+ zx_status_t status = clone_fshost_ldsvc(c);
+ if (status != ZX_OK) {
+ fprintf(stderr, "devcoordinator: failed to clone fshost loader for devhost: %d\n", status);
+ }
+ return status;
+ });
}
for (const char* path : devmgr_args.driver_search_paths) {
diff --git a/zircon/system/ulib/inspect/BUILD.gn b/zircon/system/ulib/inspect/BUILD.gn
index 92391a8..9057995 100644
--- a/zircon/system/ulib/inspect/BUILD.gn
+++ b/zircon/system/ulib/inspect/BUILD.gn
@@ -37,8 +37,8 @@
]
deps = [
+ "$zx/system/ulib/fdio",
"$zx/system/ulib/fit",
- "$zx/system/ulib/syslog",
"$zx/system/ulib/zx",
]
}
diff --git a/zircon/system/ulib/kcounter/BUILD.gn b/zircon/system/ulib/kcounter/BUILD.gn
index f0bee94..e9cc67d 100644
--- a/zircon/system/ulib/kcounter/BUILD.gn
+++ b/zircon/system/ulib/kcounter/BUILD.gn
@@ -18,7 +18,6 @@
"$zx/system/ulib/fzl",
"$zx/system/ulib/inspect",
"$zx/system/ulib/svc",
- "$zx/system/ulib/syslog",
"$zx/system/ulib/zircon",
"$zx/system/ulib/zx",
]
diff --git a/zircon/system/ulib/kcounter/provider.cc b/zircon/system/ulib/kcounter/provider.cc
index 19a027d..578ef29 100644
--- a/zircon/system/ulib/kcounter/provider.cc
+++ b/zircon/system/ulib/kcounter/provider.cc
@@ -5,7 +5,6 @@
#include <fuchsia/kernel/c/fidl.h>
#include <lib/fidl-async/bind.h>
#include <lib/kcounter/provider.h>
-#include <lib/syslog/global.h>
#include <lib/zx/vmo.h>
#include <string.h>
#include <zircon/status.h>
diff --git a/zircon/system/ulib/zxtest/README.md b/zircon/system/ulib/zxtest/README.md
index 6b6cfb8..8358832 100644
--- a/zircon/system/ulib/zxtest/README.md
+++ b/zircon/system/ulib/zxtest/README.md
@@ -15,6 +15,23 @@
* For feature requests, please describe the use case and the problems the missing feature is causing.
+## zxtest Dependencies
+
+zxtest requires some basic libc I/O functions. Typically, this is provided by
+including an fdio dependency in the test target that uses zxtest. However, the
+"core tests" in ``//zircon/system/utest/core`` provide their own libc I/O
+functions since the core tests can't use fdio. (This is why the zxtest library
+itself cannot depend on fdio.)
+
+Most other tests should ensure that they separately depend on fdio. If your test
+crashes and you see ``libc_io_functions_not_implemented_use_fdio_instead`` at
+the top of the stack trace, this is why.
+
+**TODO**: Consider splitting "zxtest" into "zxtest" and "zxtest-core", where the
+former includes fdio but the latter doesn't. This would help avoid the
+dependency weirdness where fdio has to be separately included by most users even
+though it's really needed by zxtest.
+
## Key Differences from 'Google Test'
* Limited set of dependencies.
