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; };