cvd start always runs cvd_internal_start in the daemon mode Self-restarting is complicated by the non-daemon mode launches. As long as the cvd server launches a Cuttlefish device, it always runs the device in the daemon mode. Thus, if the user specifies non-daemon mode explicitly, cvd refuses the request and guides the user to use the daemon mode. If the user does not specify the flag, cvd runs Cuttlefish in daemon mode. Bug: 306506449 Test: launch_cvd --nodaemon Test: launch_cvd --daemon=INVALID Test: launch_cvd --daemon=false Test: launch_cvd --daemon=true Test: launch_cvd Change-Id: I2b4c7987c0f74b7c6f4f2b84a0321d86ae34e842
diff --git a/host/commands/cvd/server_command/Android.bp b/host/commands/cvd/server_command/Android.bp index 0ae8c7f..a2cbcb2 100644 --- a/host/commands/cvd/server_command/Android.bp +++ b/host/commands/cvd/server_command/Android.bp
@@ -59,18 +59,3 @@ ], defaults: ["cvd_lib_defaults"], } - -cc_test_host { - name: "libcvd_sub_commands_test", - srcs: [ - "start_test.cpp", - ], - test_options: { - unit_test: true, - }, - static_libs: [ - "libcvd_sub_commands", - "libgmock", - ], - defaults: ["cvd_and_fetch_cvd_defaults"], -}
diff --git a/host/commands/cvd/server_command/start.cpp b/host/commands/cvd/server_command/start.cpp index 46a8158..e4010f2 100644 --- a/host/commands/cvd/server_command/start.cpp +++ b/host/commands/cvd/server_command/start.cpp
@@ -164,8 +164,7 @@ * response. */ Result<cvd::Response> PostStartExecutionActions( - selector::GroupCreationInfo& group_creation_info, const uid_t uid, - const bool is_daemonized); + selector::GroupCreationInfo& group_creation_info, const uid_t uid); Result<void> AcloudCompatActions( const selector::GroupCreationInfo& group_creation_info, const RequestWithStdio& request); @@ -505,9 +504,7 @@ return start_bin; } -Result<bool> IsDaemonModeFlag(const cvd_common::Args& args) { - bool flag_set = false; - bool is_daemon = true; +static Result<void> ConsumeDaemonModeFlag(cvd_common::Args& args) { Flag flag = Flag() .Alias({FlagAliasMode::kFlagPrefix, "-daemon="}) @@ -516,29 +513,38 @@ .Alias({FlagAliasMode::kFlagExact, "--daemon"}) .Alias({FlagAliasMode::kFlagExact, "-nodaemon"}) .Alias({FlagAliasMode::kFlagExact, "--nodaemon"}) - .Setter([&is_daemon, - &flag_set](const FlagMatch& match) -> Result<void> { - flag_set = true; + .Setter([](const FlagMatch& match) -> Result<void> { + static constexpr char kPossibleCmds[] = + "\"cvd start\" or \"launch_cvd\""; if (match.key == match.value) { - is_daemon = match.key.find("no") == std::string::npos; + CF_EXPECTF(match.key.find("no") == std::string::npos, + "--nodaemon is not supported by {}", kPossibleCmds); return {}; } CF_EXPECTF(match.value.find(",") == std::string::npos, - "{} had a comma", match.value); - static constexpr std::string_view kFalseStrings[] = {"n", "no", - "false"}; - for (const auto& falseString : kFalseStrings) { - if (android::base::EqualsIgnoreCase(falseString, match.value)) { - is_daemon = false; + "{} had a comma that is not allowed", match.value); + static constexpr std::string_view kValidFalseStrings[] = {"n", "no", + "false"}; + static constexpr std::string_view kValidTrueStrings[] = {"y", "yes", + "true"}; + for (const auto& true_string : kValidTrueStrings) { + if (android::base::EqualsIgnoreCase(true_string, match.value)) { + return {}; } } - // Allow `cvd_internal_start` to produce its own error for other - // invalid strings. - return {}; + for (const auto& false_string : kValidFalseStrings) { + CF_EXPECTF( + !android::base::EqualsIgnoreCase(false_string, match.value), + "\"{}{} was given and is not supported by {}", match.key, + match.value, kPossibleCmds); + } + return CF_ERRF( + "Invalid --daemon option: {}{}. {} supports only " + "\"--daemon=true\"", + match.key, match.value, kPossibleCmds); }); - auto args_copy = args; - CF_EXPECT(ParseFlags({flag}, args_copy)); - return flag_set && is_daemon; + CF_EXPECT(ParseFlags({flag}, args)); + return {}; } // For backward compatibility, we add extra symlink in system wide home @@ -643,7 +649,8 @@ CF_EXPECT(Contains(supported_commands_, subcmd), "subcmd should be start but is " << subcmd); const bool is_help = CF_EXPECT(IsHelpSubcmd(subcmd_args)); - const bool is_daemon = CF_EXPECT(IsDaemonModeFlag(subcmd_args)); + CF_EXPECT(ConsumeDaemonModeFlag(subcmd_args)); + subcmd_args.push_back("--daemon=true"); std::optional<selector::GroupCreationInfo> group_creation_info; if (!is_help) { @@ -682,12 +689,6 @@ return ResponseFromSiginfo(infop); } - // For backward compatibility, we add extra symlink in system wide home - // when HOME is NOT overridden and selector flags are NOT given. - if (group_creation_info->is_default_group) { - CF_EXPECT(CreateSymlinks(*group_creation_info)); - } - // make acquire interrupt_lock inside. auto acloud_compat_action_result = AcloudCompatActions(*group_creation_info, request); @@ -697,7 +698,7 @@ LOG(ERROR) << "AcloudCompatActions() failed" << " but continue as they are minor errors."; } - return PostStartExecutionActions(*group_creation_info, uid, is_daemon); + return PostStartExecutionActions(*group_creation_info, uid); } static constexpr char kCollectorFailure[] = R"( @@ -733,21 +734,15 @@ } Result<cvd::Response> CvdStartCommandHandler::PostStartExecutionActions( - selector::GroupCreationInfo& group_creation_info, const uid_t uid, - const bool is_daemonized) { + selector::GroupCreationInfo& group_creation_info, const uid_t uid) { auto infop = CF_EXPECT(subprocess_waiter_.Wait()); if (infop.si_code != CLD_EXITED || infop.si_status != EXIT_SUCCESS) { - if (is_daemonized) { - // run_cvd processes may be still running in background - // the order of the following operations should be kept - auto reset_response = CF_EXPECT(CvdResetGroup(group_creation_info)); - instance_manager_.RemoveInstanceGroup(uid, group_creation_info.home); - if (reset_response.status().code() != cvd::Status::OK) { - return reset_response; - } - } else { - // run_cvd processes are not running - instance_manager_.RemoveInstanceGroup(uid, group_creation_info.home); + // run_cvd processes may be still running in background + // the order of the following operations should be kept + auto reset_response = CF_EXPECT(CvdResetGroup(group_creation_info)); + instance_manager_.RemoveInstanceGroup(uid, group_creation_info.home); + if (reset_response.status().code() != cvd::Status::OK) { + return reset_response; } } auto final_response = ResponseFromSiginfo(infop); @@ -755,14 +750,19 @@ final_response.status().code() != cvd::Status::OK) { return final_response; } - if (is_daemonized) { - // If not daemonized, reaching here means the instance group terminated. - // Thus, it's enough to release the file lock in the destructor. - // If daemonized, reaching here means the group started successfully - // As the destructor will release the file lock, the instance lock - // files must be marked as used - MarkLockfilesInUse(group_creation_info); + // If not daemonized, reaching here means the instance group terminated. + // Thus, it's enough to release the file lock in the destructor. + // If daemonized, reaching here means the group started successfully + // As the destructor will release the file lock, the instance lock + // files must be marked as used + MarkLockfilesInUse(group_creation_info); + + // For backward compatibility, we add extra symlink in system wide home + // when HOME is NOT overridden and selector flags are NOT given. + if (group_creation_info.is_default_group) { + CF_EXPECT(CreateSymlinks(group_creation_info)); } + // group_creation_info is nullopt only if is_help is false return FillOutNewInstanceInfo(std::move(final_response), group_creation_info); }
diff --git a/host/commands/cvd/server_command/start.h b/host/commands/cvd/server_command/start.h index 515691d..58d18cc 100644 --- a/host/commands/cvd/server_command/start.h +++ b/host/commands/cvd/server_command/start.h
@@ -25,8 +25,6 @@ namespace cuttlefish { -Result<bool> IsDaemonModeFlag(const cvd_common::Args& args); - std::unique_ptr<CvdServerHandler> NewCvdStartCommandHandler( InstanceManager& instance_manager, HostToolTargetManager& host_tool_target_manager,
diff --git a/host/commands/cvd/server_command/start_test.cpp b/host/commands/cvd/server_command/start_test.cpp deleted file mode 100644 index 6b32f52..0000000 --- a/host/commands/cvd/server_command/start_test.cpp +++ /dev/null
@@ -1,42 +0,0 @@ -/* - * Copyright (C) 2023 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -// - -#include "host/commands/cvd/server_command/start.h" - -#include <gmock/gmock-matchers.h> -#include <gtest/gtest.h> - -#include "common/libs/utils/result_matchers.h" - -namespace cuttlefish { - -TEST(CvdStart, DaemonFlag) { - ASSERT_THAT(IsDaemonModeFlag({}), IsOkAndValue(false)); - ASSERT_THAT(IsDaemonModeFlag({"--daemon"}), IsOkAndValue(true)); - ASSERT_THAT(IsDaemonModeFlag({"--nodaemon"}), IsOkAndValue(false)); - ASSERT_THAT(IsDaemonModeFlag({"--daemon=y"}), IsOkAndValue(true)); - ASSERT_THAT(IsDaemonModeFlag({"--daemon=TRUE"}), IsOkAndValue(true)); - ASSERT_THAT(IsDaemonModeFlag({"--daemon=true"}), IsOkAndValue(true)); - ASSERT_THAT(IsDaemonModeFlag({"--daemon=false"}), IsOkAndValue(false)); - ASSERT_THAT(IsDaemonModeFlag({"--daemon=FALSE"}), IsOkAndValue(false)); - ASSERT_THAT(IsDaemonModeFlag({"--daemon=no"}), IsOkAndValue(false)); - - ASSERT_THAT(IsDaemonModeFlag({"--daemon=true,false"}), IsError()); - ASSERT_THAT(IsDaemonModeFlag({"--daemon=hello"}), IsOkAndValue(true)); -} - -} // namespace cuttlefish