Revert "cvd start will always run devices in daemon mode"
This reverts commit f2133376486b66ffec788549f166987d7fa94bb2.
Bug: 312643212
Change-Id: I3ea19bdb9cdf26b0a4e5862e4d187b82882dfc67
diff --git a/host/commands/cvd/server_command/Android.bp b/host/commands/cvd/server_command/Android.bp
index 837c808..8bcf7f3 100644
--- a/host/commands/cvd/server_command/Android.bp
+++ b/host/commands/cvd/server_command/Android.bp
@@ -56,3 +56,18 @@
],
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 84492b9..c9858b3 100644
--- a/host/commands/cvd/server_command/start.cpp
+++ b/host/commands/cvd/server_command/start.cpp
@@ -30,9 +30,9 @@
#include <regex>
#include <sstream>
#include <string>
-#include <string_view>
#include <thread>
+#include <android-base/parseint.h>
#include <android-base/strings.h>
#include "common/libs/fs/shared_fd.h"
@@ -149,7 +149,7 @@
*/
Result<cvd::Response> PostStartExecutionActions(
std::optional<selector::GroupCreationInfo>& group_creation_info,
- const uid_t uid);
+ const uid_t uid, const bool is_daemonized);
Result<void> AcloudCompatActions(
const selector::GroupCreationInfo& group_creation_info,
const RequestWithStdio& request);
@@ -489,7 +489,9 @@
return start_bin;
}
-static Result<void> ConsumeDaemonModeFlag(cvd_common::Args& args) {
+Result<bool> IsDaemonModeFlag(const cvd_common::Args& args) {
+ bool flag_set = false;
+ bool is_daemon = true;
Flag flag =
Flag()
.Alias({FlagAliasMode::kFlagPrefix, "-daemon="})
@@ -498,26 +500,29 @@
.Alias({FlagAliasMode::kFlagExact, "--daemon"})
.Alias({FlagAliasMode::kFlagExact, "-nodaemon"})
.Alias({FlagAliasMode::kFlagExact, "--nodaemon"})
- .Setter([](const FlagMatch& match) -> Result<void> {
+ .Setter([&is_daemon,
+ &flag_set](const FlagMatch& match) -> Result<void> {
+ flag_set = true;
if (match.key == match.value) {
+ is_daemon = match.key.find("no") == std::string::npos;
return {};
}
- std::vector<std::string> values =
- android::base::Split(match.value, ",");
- const std::vector<std::string_view> kFalseStrings{"n", "no",
- "false"};
- const std::vector<std::string_view> kTrueStrings{"y", "yes",
- "true"};
- for (const auto& value : values) {
- CF_EXPECTF(Contains(kTrueStrings, value) ||
- Contains(kFalseStrings, value),
- "{} is not a valid value for --daemon=", match.value);
+ 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;
+ }
}
+ // Allow `cvd_internal_start` to produce its own error for other
+ // invalid strings.
return {};
});
- CF_EXPECT(ParseFlags({flag}, args));
- args.push_back("--daemon=true");
- return {};
+ auto args_copy = args;
+ CF_EXPECT(ParseFlags({flag}, args_copy));
+ return flag_set && is_daemon;
}
// For backward compatibility, we add extra symlink in system wide home
@@ -652,7 +657,7 @@
CF_EXPECT(Contains(supported_commands_, subcmd),
"subcmd should be start but is " << subcmd);
const bool is_help = HasHelpOpts(subcmd_args);
- CF_EXPECT(ConsumeDaemonModeFlag(subcmd_args));
+ const bool is_daemon = CF_EXPECT(IsDaemonModeFlag(subcmd_args));
std::optional<selector::GroupCreationInfo> group_creation_info;
if (!is_help) {
@@ -706,12 +711,12 @@
LOG(ERROR) << "AcloudCompatActions() failed"
<< " but continue as they are minor errors.";
}
- return PostStartExecutionActions(group_creation_info, uid);
+ return PostStartExecutionActions(group_creation_info, uid, is_daemon);
}
Result<cvd::Response> CvdStartCommandHandler::PostStartExecutionActions(
std::optional<selector::GroupCreationInfo>& group_creation_info,
- const uid_t uid) {
+ const uid_t uid, const bool is_daemonized) {
auto infop = CF_EXPECT(subprocess_waiter_.Wait());
if (infop.si_code != CLD_EXITED || infop.si_status != EXIT_SUCCESS) {
// perhaps failed in launch
@@ -722,10 +727,14 @@
final_response.status().code() != cvd::Status::OK) {
return final_response;
}
-
- MarkLockfilesInUse(*group_creation_info);
- LOG(INFO) << "The Cuttlefish devices are now always daemonized";
-
+ 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);
+ }
// 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_test.cpp b/host/commands/cvd/server_command/start_test.cpp
new file mode 100644
index 0000000..6b32f52
--- /dev/null
+++ b/host/commands/cvd/server_command/start_test.cpp
@@ -0,0 +1,42 @@
+/*
+ * 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
