Merge "Standardize cvd server response handler signatures."

2 files changed, 72 insertions, 68 deletions

diff --git a/host/commands/cvd/proto/cvd_server.proto b/host/commands/cvd/proto/cvd_server.proto
index 91194bc54..6fb97c083 100644
--- a/host/commands/cvd/proto/cvd_server.proto
+++ b/host/commands/cvd/proto/cvd_server.proto
@@ -23,6 +23,7 @@ message Status {
   enum Code {
     OK = 0;
     FAILED_PRECONDITION = 9;
+    INTERNAL = 13;
   }
 
   Code code = 1;
diff --git a/host/commands/cvd/server.cc b/host/commands/cvd/server.cc
index 299a254e3..6765c0c22 100644
--- a/host/commands/cvd/server.cc
+++ b/host/commands/cvd/server.cc
@@ -84,6 +84,12 @@ const std::map<std::string, std::string> CommandToBinaryMap = {
     {"clear", kClearBin},
     {"fleet", kFleetBin}};
 
+struct RequestWithStdio {
+  cvd::Request request;
+  SharedFD in, out, err;
+  std::optional<SharedFD> extra;
+};
+
 class CvdServer {
  public:
   void ServerLoop(const SharedFD& server) {
@@ -96,44 +102,39 @@ class CvdServer {
       } else if (read_set.IsSet(server)) {
         auto client = SharedFD::Accept(*server);
         while (true) {
-          Result<void> result = {};
-          auto request_with_stdio = GetRequest(client);
-          if (!request_with_stdio.ok()) {
+          auto request = GetRequest(client);
+          if (!request.ok()) {
             client->Close();
             break;
           }
-          auto request = request_with_stdio->request;
-          auto in = request_with_stdio->in;
-          auto out = request_with_stdio->out;
-          auto err = request_with_stdio->err;
-          auto extra = request_with_stdio->extra;
-          switch (request.contents_case()) {
+          Result<cvd::Response> response;
+          switch (request->request.contents_case()) {
             case cvd::Request::ContentsCase::CONTENTS_NOT_SET:
               // No more messages from this client.
               client->Close();
               break;
             case cvd::Request::ContentsCase::kVersionRequest:
-              result = GetVersion(client);
+              response = GetVersion(*request);
               break;
             case cvd::Request::ContentsCase::kShutdownRequest:
-              if (!extra) {
-                result =
-                    CF_ERR("Missing extra ShareFD for shutdown write_pipe");
-              } else {
-                result = Shutdown(client, request.shutdown_request(), out, err,
-                                  *extra);
-              }
+              response = Shutdown(*request);
               break;
             case cvd::Request::ContentsCase::kCommandRequest:
-              result = HandleCommand(client, request.command_request(), in, out,
-                                     err);
+              response = HandleCommand(*request);
               break;
             default:
-              result = CF_ERR("Unknown request in cvd_server.");
+              response = CF_ERR("Unknown request in cvd_server.");
               break;
           }
-          if (!result.ok()) {
-            LOG(ERROR) << result.error();
+          if (response.ok()) {
+            SendResponse(client, *response);
+          } else {
+            LOG(ERROR) << response.error();
+            cvd::Response error_response;
+            error_response.mutable_status()->set_code(cvd::Status::INTERNAL);
+            *error_response.mutable_status()->mutable_message() =
+                response.error().message();
+            SendResponse(client, error_response);
             client->Close();
           }
         }
@@ -141,7 +142,9 @@ class CvdServer {
     }
   }
 
-  Result<void> GetVersion(const SharedFD& client) const {
+  Result<cvd::Response> GetVersion(const RequestWithStdio& request) const {
+    CF_EXPECT(request.request.contents_case() ==
+              cvd::Request::ContentsCase::kVersionRequest);
     cvd::Response response;
     response.mutable_version_response()->mutable_version()->set_major(
         cvd::kVersionMajor);
@@ -150,21 +153,25 @@ class CvdServer {
     response.mutable_version_response()->mutable_version()->set_build(
         android::build::GetBuildNumber());
     response.mutable_status()->set_code(cvd::Status::OK);
-    return SendResponse(client, response);
+    return response;
   }
 
-  Result<void> Shutdown(const SharedFD& client,
-                        const cvd::ShutdownRequest& request,
-                        const SharedFD& out, const SharedFD& err,
-                        const SharedFD& write_pipe) {
+  Result<cvd::Response> Shutdown(const RequestWithStdio& request) {
+    CF_EXPECT(request.request.contents_case() ==
+              cvd::Request::ContentsCase::kShutdownRequest);
     cvd::Response response;
     response.mutable_shutdown_response();
 
-    if (request.clear()) {
-      *response.mutable_status() = CvdClear(out, err);
-      if (response.status().code() != cvd::Status::OK) {
-        return SendResponse(client, response);
-      }
+    if (!request.extra) {
+      response.mutable_status()->set_code(cvd::Status::FAILED_PRECONDITION);
+      response.mutable_status()->set_message(
+          "Missing extra SharedFD for shutdown");
+      return response;
+    }
+
+    if (request.request.shutdown_request().clear()) {
+      *response.mutable_status() = CvdClear(request.out, request.err);
+      return response;
     }
 
     if (!assemblies_.empty()) {
@@ -172,37 +179,37 @@ class CvdServer {
       response.mutable_status()->set_message(
           "Cannot shut down cvd_server while devices are being tracked. "
           "Try `cvd kill-server`.");
-      return SendResponse(client, response);
+      return response;
     }
 
-    // Intentionally leak the write_pipe fd so that it only closes
+    // Intentionally leak the extra fd so that it only closes
     // when this process fully exits.
-    write_pipe->UNMANAGED_Dup();
+    (*request.extra)->UNMANAGED_Dup();
 
-    WriteAll(out, "Stopping the cvd_server.\n");
+    WriteAll(request.out, "Stopping the cvd_server.\n");
     running_ = false;
+
     response.mutable_status()->set_code(cvd::Status::OK);
-    return SendResponse(client, response);
+    return response;
   }
 
-  Result<void> HandleCommand(const SharedFD& client,
-                             const cvd::CommandRequest& request,
-                             const SharedFD& in, const SharedFD& out,
-                             const SharedFD& err) {
+  Result<cvd::Response> HandleCommand(const RequestWithStdio& request) {
+    CF_EXPECT(request.request.contents_case() ==
+              cvd::Request::ContentsCase::kCommandRequest);
     cvd::Response response;
     response.mutable_command_response();
 
-    if (request.args_size() == 0) {
+    if (request.request.command_request().args_size() == 0) {
       // No command to handle
       response.mutable_status()->set_code(cvd::Status::FAILED_PRECONDITION);
       response.mutable_status()->set_message("No args passed to HandleCommand");
-      return SendResponse(client, response);
+      return response;
     }
 
     std::vector<Flag> flags;
 
     std::vector<std::string> args;
-    for (const std::string& arg : request.args()) {
+    for (const std::string& arg : request.request.command_request().args()) {
       args.push_back(arg);
     }
 
@@ -243,20 +250,21 @@ class CvdServer {
 
     CHECK(ParseFlags(flags, args));
 
-    auto host_artifacts_path = request.env().find("ANDROID_HOST_OUT");
-    if (host_artifacts_path == request.env().end()) {
+    auto host_artifacts_path =
+        request.request.command_request().env().find("ANDROID_HOST_OUT");
+    if (host_artifacts_path == request.request.command_request().env().end()) {
       response.mutable_status()->set_code(cvd::Status::FAILED_PRECONDITION);
       response.mutable_status()->set_message(
           "Missing ANDROID_HOST_OUT in client environment.");
-      return SendResponse(client, response);
+      return response;
     }
 
     if (bin == kHelpBin) {
       // Handle `cvd help`
       if (args.empty()) {
-        WriteAll(out, kHelpMessage);
+        WriteAll(request.out, kHelpMessage);
         response.mutable_status()->set_code(cvd::Status::OK);
-        return SendResponse(client, response);
+        return response;
       }
 
       // Certain commands have no detailed help text.
@@ -264,20 +272,20 @@ class CvdServer {
       auto it = CommandToBinaryMap.find(args[0]);
       if (it == CommandToBinaryMap.end() ||
           builtins.find(args[0]) != builtins.end()) {
-        WriteAll(out, kHelpMessage);
+        WriteAll(request.out, kHelpMessage);
         response.mutable_status()->set_code(cvd::Status::OK);
-        return SendResponse(client, response);
+        return response;
       }
 
       // Handle `cvd help <subcommand>` by calling the subcommand with --help.
       bin = it->second;
       args_copy.push_back("--help");
     } else if (bin == kClearBin) {
-      *response.mutable_status() = CvdClear(out, err);
-      return SendResponse(client, response);
+      *response.mutable_status() = CvdClear(request.out, request.err);
+      return response;
     } else if (bin == kFleetBin) {
-      *response.mutable_status() = CvdFleet(out);
-      return SendResponse(client, response);
+      *response.mutable_status() = CvdFleet(request.out);
+      return response;
     } else if (bin == kStartBin) {
       // Track this assembly_dir in the fleet.
       AssemblyInfo info;
@@ -292,27 +300,28 @@ class CvdServer {
 
     // Set CuttlefishConfig path based on assembly dir,
     // used by subcommands when locating the CuttlefishConfig.
-    if (request.env().count(kCuttlefishConfigEnvVarName) == 0) {
+    if (request.request.command_request().env().count(
+            kCuttlefishConfigEnvVarName) == 0) {
       auto config_path = GetCuttlefishConfigPath(assembly_dir);
       if (config_path) {
         command.AddEnvironmentVariable(kCuttlefishConfigEnvVarName,
                                        *config_path);
       }
     }
-    for (auto& it : request.env()) {
+    for (auto& it : request.request.command_request().env()) {
       command.AddEnvironmentVariable(it.first, it.second);
     }
 
     // Redirect stdin, stdout, stderr back to the cvd client
-    command.RedirectStdIO(Subprocess::StdIOChannel::kStdIn, in);
-    command.RedirectStdIO(Subprocess::StdIOChannel::kStdOut, out);
-    command.RedirectStdIO(Subprocess::StdIOChannel::kStdErr, err);
+    command.RedirectStdIO(Subprocess::StdIOChannel::kStdIn, request.in);
+    command.RedirectStdIO(Subprocess::StdIOChannel::kStdOut, request.out);
+    command.RedirectStdIO(Subprocess::StdIOChannel::kStdErr, request.err);
     SubprocessOptions options;
     options.ExitWithParent(false);
     command.Start(options);
 
     response.mutable_status()->set_code(cvd::Status::OK);
-    return SendResponse(client, response);
+    return response;
   }
 
  private:
@@ -323,12 +332,6 @@ class CvdServer {
   std::map<AssemblyDir, AssemblyInfo> assemblies_;
   bool running_ = true;
 
-  struct RequestWithStdio {
-    cvd::Request request;
-    SharedFD in, out, err;
-    std::optional<SharedFD> extra;
-  };
-
   std::optional<std::string> GetCuttlefishConfigPath(
       const std::string& assembly_dir) const {
     std::string assembly_dir_realpath;