From 95a30d82015865271f5b8a57401ed3526a609f0e Mon Sep 17 00:00:00 2001 From: "A. Cody Schuffelen" Date: Mon, 6 Jul 2026 14:00:20 -0700 Subject: [PATCH] Return a Result from Subprocess::Wait The return value was only ever 0 or -1 based on the result of waitid. Using a Result avoids the output argument in favor of using the return value to produce the outcome of the function call. There were some callers that ignored the return value of Wait that now check it for errors as well. Bug: b/531812844 --- .../cuttlefish/common/libs/utils/BUILD.bazel | 1 + .../common/libs/utils/subprocess.cpp | 25 ++++++++----------- .../cuttlefish/common/libs/utils/subprocess.h | 2 +- .../commands/cvd/cli/commands/display.cpp | 4 +-- .../host/commands/cvd/cli/commands/env.cpp | 4 +-- .../commands/cvd/cli/commands/snapshot.cpp | 4 +-- .../commands/cvd/instances/local_instance.cpp | 3 +-- .../host/commands/cvd/utils/BUILD.bazel | 1 - .../commands/cvd/utils/subprocess_waiter.cpp | 9 ++----- .../libs/process_monitor/process_monitor.cc | 10 ++++---- 10 files changed, 26 insertions(+), 37 deletions(-) diff --git a/base/cvd/cuttlefish/common/libs/utils/BUILD.bazel b/base/cvd/cuttlefish/common/libs/utils/BUILD.bazel index 10c218fa026..3aacc4e97f4 100644 --- a/base/cvd/cuttlefish/common/libs/utils/BUILD.bazel +++ b/base/cvd/cuttlefish/common/libs/utils/BUILD.bazel @@ -345,6 +345,7 @@ cf_cc_library( "//cuttlefish/common/libs/fs", "//cuttlefish/common/libs/utils:contains", "//cuttlefish/common/libs/utils:files", + "//cuttlefish/posix:strerror", "//cuttlefish/result", "//libbase", "@abseil-cpp//absl/log", diff --git a/base/cvd/cuttlefish/common/libs/utils/subprocess.cpp b/base/cvd/cuttlefish/common/libs/utils/subprocess.cpp index 6a6eccc57c2..33549aab6aa 100644 --- a/base/cvd/cuttlefish/common/libs/utils/subprocess.cpp +++ b/base/cvd/cuttlefish/common/libs/utils/subprocess.cpp @@ -52,6 +52,7 @@ #include "cuttlefish/common/libs/utils/contains.h" #include "cuttlefish/common/libs/utils/files.h" +#include "cuttlefish/posix/strerror.h" extern char** environ; @@ -207,22 +208,19 @@ int Subprocess::Wait() { } return retval; } -int Subprocess::Wait(siginfo_t* infop, int options) { - if (pid_ < 0) { - LOG(ERROR) - << "Attempt to wait on invalid pid(has it been waited on already?): " - << pid_; - return -1; - } - *infop = {}; - auto retval = TEMP_FAILURE_RETRY(waitid(P_PID, pid_, infop, options)); +Result Subprocess::Wait(int options) { + CF_EXPECT_GE(pid_, 0, + "Attempt to wait on invalid pid(has it been waited on already?"); + siginfo_t infop = {}; + int retval = TEMP_FAILURE_RETRY(waitid(P_PID, pid_, &infop, options)); // We don't want to wait twice for the same process - bool exited = infop->si_code == CLD_EXITED || infop->si_code == CLD_DUMPED; + bool exited = infop.si_code == CLD_EXITED || infop.si_code == CLD_DUMPED; bool reaped = !(options & WNOWAIT); if (exited && reaped) { pid_ = -1; } - return retval; + CF_EXPECT_EQ(retval, 0, StrError(errno)); + return infop; } static Result SendSignalImpl(const int signal, const pid_t pid, @@ -581,10 +579,7 @@ Result Execute(std::vector command, Subprocess subprocess = cmd.Start(std::move(subprocess_options)); CF_EXPECT(subprocess.Started(), "Subprocess failed to start."); - siginfo_t info; - CF_EXPECT_EQ(subprocess.Wait(&info, wait_options), 0); - - return info; + return CF_EXPECT(subprocess.Wait(wait_options)); } } // namespace cuttlefish diff --git a/base/cvd/cuttlefish/common/libs/utils/subprocess.h b/base/cvd/cuttlefish/common/libs/utils/subprocess.h index bf3036876f9..8a1422f4231 100644 --- a/base/cvd/cuttlefish/common/libs/utils/subprocess.h +++ b/base/cvd/cuttlefish/common/libs/utils/subprocess.h @@ -75,7 +75,7 @@ class Subprocess { // successfully, non-zero otherwise. int Wait(); // Same as waitid(2) - int Wait(siginfo_t* infop, int options); + Result Wait(int options); // Whether the command started successfully. It only says whether the call to // fork() succeeded or not, it says nothing about exec or successful // completion of the command, that's what Wait is for. diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/display.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/display.cpp index 10125240bb7..dedb5b885d3 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/display.cpp +++ b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/display.cpp @@ -127,8 +127,8 @@ Result CvdDisplayCommandHandler::Handle(const CommandRequest& request) { Command command = CF_EXPECT(BuildCommand(instance_manager_, request, subcmd_args, env)); - siginfo_t infop; // NOLINT(misc-include-cleaner) - command.Start().Wait(&infop, WEXITED); + // NOLINTNEXTLINE(misc-include-cleaner) + siginfo_t infop = CF_EXPECT(command.Start().Wait(WEXITED)); CF_EXPECT(CheckProcessExitedNormally(infop)); return {}; diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/env.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/env.cpp index 434bc32d107..35edfdc1838 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/env.cpp +++ b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/env.cpp @@ -105,8 +105,8 @@ Result CvdEnvCommandHandler::Handle(const CommandRequest& request) { ? CF_EXPECT(HelpCommand(request)) : CF_EXPECT(NonHelpCommand(instance_manager_, request)); - siginfo_t infop; // NOLINT(misc-include-cleaner) - command.Start().Wait(&infop, WEXITED); + // NOLINTNEXTLINE(misc-include-cleaner) + siginfo_t infop = CF_EXPECT(command.Start().Wait(WEXITED)); CF_EXPECT(CheckProcessExitedNormally(infop)); return {}; diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/snapshot.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/snapshot.cpp index 3345b176de8..c7ffbf76a3b 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/snapshot.cpp +++ b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/snapshot.cpp @@ -93,8 +93,8 @@ Result CvdSnapshotCommandHandler::Handle(const CommandRequest& request) { Command command = CF_EXPECT(GenerateCommand(request, subcmd, subcmd_args, request.Env())); - siginfo_t infop; // NOLINT(misc-include-cleaner) - command.Start().Wait(&infop, WEXITED); + // NOLINTNEXTLINE(misc-include-cleaner) + siginfo_t infop = CF_EXPECT(command.Start().Wait(WEXITED)); CF_EXPECT(CheckProcessExitedNormally(infop)); return {}; diff --git a/base/cvd/cuttlefish/host/commands/cvd/instances/local_instance.cpp b/base/cvd/cuttlefish/host/commands/cvd/instances/local_instance.cpp index c188dbe5b16..32913bd1128 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/instances/local_instance.cpp +++ b/base/cvd/cuttlefish/host/commands/cvd/instances/local_instance.cpp @@ -146,8 +146,7 @@ Result LocalInstance::PressPowerBtnLegacy() { VLOG(0) << "Executing: " << cmd.ToString(); - siginfo_t infop; - cmd.Start().Wait(&infop, WEXITED); + siginfo_t infop = CF_EXPECT(cmd.Start().Wait(WEXITED)); CF_EXPECT(CheckProcessExitedNormally(infop)); return {}; diff --git a/base/cvd/cuttlefish/host/commands/cvd/utils/BUILD.bazel b/base/cvd/cuttlefish/host/commands/cvd/utils/BUILD.bazel index 4930c16ba53..fb98dca562f 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/utils/BUILD.bazel +++ b/base/cvd/cuttlefish/host/commands/cvd/utils/BUILD.bazel @@ -55,7 +55,6 @@ cf_cc_library( copts = COPTS + ["-Werror=sign-compare"], deps = [ "//cuttlefish/common/libs/utils:subprocess", - "//cuttlefish/posix:strerror", "//cuttlefish/result", ], ) diff --git a/base/cvd/cuttlefish/host/commands/cvd/utils/subprocess_waiter.cpp b/base/cvd/cuttlefish/host/commands/cvd/utils/subprocess_waiter.cpp index 41961602184..b8a10427a5b 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/utils/subprocess_waiter.cpp +++ b/base/cvd/cuttlefish/host/commands/cvd/utils/subprocess_waiter.cpp @@ -19,7 +19,6 @@ #include #include "cuttlefish/common/libs/utils/subprocess.h" -#include "cuttlefish/posix/strerror.h" #include "cuttlefish/result/result.h" namespace cuttlefish { @@ -38,17 +37,13 @@ Result SubprocessWaiter::Wait() { CF_EXPECT(!interrupted_, "Interrupted"); CF_EXPECT(subprocess_.has_value()); - siginfo_t infop{}; - interrupt_lock.unlock(); // This blocks until the process exits, but doesn't reap it. - auto result = subprocess_->Wait(&infop, WEXITED | WNOWAIT); - CF_EXPECTF(result != -1, "Lost track of subprocess pid: {}", StrError(errno)); + siginfo_t infop = CF_EXPECT(subprocess_->Wait(WEXITED | WNOWAIT)); interrupt_lock.lock(); // Perform a reaping wait on the process (which should already have exited). - result = subprocess_->Wait(&infop, WEXITED); - CF_EXPECT(result != -1, "Lost track of subprocess pid"); + infop = CF_EXPECT(subprocess_->Wait(WEXITED)); // The double wait avoids a race around the kernel reusing pids. Waiting // with WNOWAIT won't cause the child process to be reaped, so the kernel // won't reuse the pid until the Wait call below, and any kill signals won't diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc index cff2ec3c038..a5ae6603ba7 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc +++ b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc @@ -159,14 +159,14 @@ Result StopSubprocesses(std::vector& monitored) { LOG(WARNING) << "Error in stopping \"" << it.cmd->GetShortName() << "\""; return false; } - siginfo_t infop; - auto success = it.proc->Wait(&infop, WEXITED); - if (success < 0) { - LOG(WARNING) << "Failed to wait for process " << it.cmd->GetShortName(); + Result infop = it.proc->Wait(WEXITED); + if (!infop.ok()) { + LOG(WARNING) << "Failed to wait for process " << it.cmd->GetShortName() + << ": " << infop.error(); return false; } if (stop_result == StopperResult::kStopCrash) { - LogSubprocessExit(it.cmd->GetShortName(), infop); + LogSubprocessExit(it.cmd->GetShortName(), *infop); } return true; };