From 9dbd22b555613755e81e8be5fa5aadde2d7dd687 Mon Sep 17 00:00:00 2001 From: Liav A Date: Sun, 25 Jun 2023 13:45:56 +0300 Subject: [PATCH] SystemServer: Make decision on whether to enable a service more readable This change ensures that code in the Service class doesn't try to check the g_system_mode variable, but instead is asked on whether it supports a given system mode string value. Also, don't assume that we should create sockets for any new Service instance, but instead do that only if the Service should run in the current system mode. --- Userland/Services/SystemServer/Service.cpp | 9 +-- Userland/Services/SystemServer/Service.h | 5 +- Userland/Services/SystemServer/main.cpp | 85 +++++++++++++++++----- 3 files changed, 72 insertions(+), 27 deletions(-) diff --git a/Userland/Services/SystemServer/Service.cpp b/Userland/Services/SystemServer/Service.cpp index effa696d5c..1491b3c3d5 100644 --- a/Userland/Services/SystemServer/Service.cpp +++ b/Userland/Services/SystemServer/Service.cpp @@ -359,16 +359,13 @@ Service::Service(Core::ConfigFile const& config, StringView name) ErrorOr> Service::try_create(Core::ConfigFile const& config, StringView name) { - auto service = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) Service(config, name))); - if (service->is_enabled()) - TRY(service->setup_sockets()); - return service; + return TRY(adopt_nonnull_ref_or_enomem(new (nothrow) Service(config, name))); } -bool Service::is_enabled() const +bool Service::is_enabled_for_system_mode(StringView mode) const { extern DeprecatedString g_system_mode; - return m_system_modes.contains_slow(g_system_mode); + return m_system_modes.contains_slow(mode); } ErrorOr Service::determine_account(int fd) diff --git a/Userland/Services/SystemServer/Service.h b/Userland/Services/SystemServer/Service.h index d89d2fc628..5134286456 100644 --- a/Userland/Services/SystemServer/Service.h +++ b/Userland/Services/SystemServer/Service.h @@ -20,11 +20,13 @@ public: static ErrorOr> try_create(Core::ConfigFile const& config, StringView name); ~Service(); - bool is_enabled() const; + bool is_enabled_for_system_mode(StringView) const; ErrorOr activate(); // Note: This is a `status` as in POSIX's wait syscall, not an exit-code. ErrorOr did_exit(int status); + ErrorOr setup_sockets(); + static Service* find_by_pid(pid_t); private: @@ -90,7 +92,6 @@ private: int m_restart_attempts { 0 }; ErrorOr setup_socket(SocketDescriptor&); - ErrorOr setup_sockets(); void setup_notifier(); ErrorOr handle_socket_connection(); }; diff --git a/Userland/Services/SystemServer/main.cpp b/Userland/Services/SystemServer/main.cpp index 13547f7802..58862d3a39 100644 --- a/Userland/Services/SystemServer/main.cpp +++ b/Userland/Services/SystemServer/main.cpp @@ -30,7 +30,10 @@ #include #include -DeprecatedString g_system_mode = "graphical"; +static constexpr StringView text_system_mode = "text"sv; +static constexpr StringView selftest_system_mode = "self-test"sv; +static constexpr StringView graphical_system_mode = "graphical"sv; +DeprecatedString g_system_mode = graphical_system_mode; Vector> g_services; // NOTE: This handler ensures that the destructor of g_services is called. @@ -68,8 +71,8 @@ static ErrorOr determine_system_mode() { ArmedScopeGuard declare_text_mode_on_failure([&] { // Note: Only if the mode is not set to self-test, degrade it to text mode. - if (g_system_mode != "self-test") - g_system_mode = "text"; + if (g_system_mode != selftest_system_mode) + g_system_mode = text_system_mode; }); auto file_or_error = Core::File::open("/sys/kernel/system_mode"sv, Core::File::OpenMode::Read); @@ -482,6 +485,26 @@ static ErrorOr create_tmp_semaphore_directory() return {}; } +static ErrorOr activate_services(Core::ConfigFile const& config) +{ + for (auto const& name : config.groups()) { + auto service = TRY(Service::try_create(config, name)); + if (service->is_enabled_for_system_mode(g_system_mode)) { + TRY(service->setup_sockets()); + g_services.append(move(service)); + } + } + // After we've set them all up, activate them! + dbgln("Activating {} services...", g_services.size()); + for (auto& service : g_services) { + dbgln_if(SYSTEMSERVER_DEBUG, "Activating {}", service->name()); + if (auto result = service->activate(); result.is_error()) + dbgln("{}: {}", service->name(), result.release_error()); + } + + return {}; +} + static ErrorOr reopen_base_file_descriptors() { // Note: We open the /dev/null device and set file descriptors 0, 1, 2 to it @@ -498,6 +521,42 @@ static ErrorOr reopen_base_file_descriptors() return {}; } +static ErrorOr activate_base_services_based_on_system_mode() +{ + if (g_system_mode == graphical_system_mode) { + bool found_gpu_device = false; + for (int attempt = 0; attempt < 10; attempt++) { + struct stat file_state; + int rc = lstat("/dev/gpu/connector0", &file_state); + if (rc == 0) { + found_gpu_device = true; + break; + } + sleep(1); + } + if (!found_gpu_device) { + dbgln("WARNING: No device nodes at /dev/gpu/ directory after 10 seconds. This is probably a sign of disabled graphics functionality."); + dbgln("To cope with this, graphical mode will not be enabled."); + g_system_mode = text_system_mode; + } + } + + // Read our config and instantiate services. + // This takes care of setting up sockets. + auto config = TRY(Core::ConfigFile::open_for_system("SystemServer")); + TRY(activate_services(*config)); + return {}; +} + +static ErrorOr activate_user_services_based_on_system_mode() +{ + // Read our config and instantiate services. + // This takes care of setting up sockets. + auto config = TRY(Core::ConfigFile::open_for_app("SystemServer")); + TRY(activate_services(*config)); + return {}; +} + ErrorOr serenity_main(Main::Arguments arguments) { bool user = false; @@ -525,22 +584,10 @@ ErrorOr serenity_main(Main::Arguments arguments) event_loop.register_signal(SIGCHLD, sigchld_handler); event_loop.register_signal(SIGTERM, sigterm_handler); - // Read our config and instantiate services. - // This takes care of setting up sockets. - auto config = (user) - ? TRY(Core::ConfigFile::open_for_app("SystemServer")) - : TRY(Core::ConfigFile::open_for_system("SystemServer")); - for (auto const& name : config->groups()) { - auto service = TRY(Service::try_create(*config, name)); - if (service->is_enabled()) - g_services.append(move(service)); - } - - // After we've set them all up, activate them! - dbgln("Activating {} services...", g_services.size()); - for (auto& service : g_services) { - if (auto result = service->activate(); result.is_error()) - dbgln("{}: {}", service->name(), result.release_error()); + if (!user) { + TRY(activate_base_services_based_on_system_mode()); + } else { + TRY(activate_user_services_based_on_system_mode()); } return event_loop.exec();