update_engine: pipe stderr individually in SycnronousExec
Currently, SyncronousExec pipes stderr to stdout which is fine but not
ideal. Specially we have an issue with vpd_get_value script that exits
with 0 even with underlying failures. This is problematic, because we
get the combined stdout/stderr of the command and since the exit code is
0 we assume the output is correct. Then, we create the XML request based
on this output but with stderr combined (too much junk) as the value of
an XML attribute. This causes update failure. Fortunately, vpd_get_value
correctly separates its children's stderr and stdout. So as long as we
don't combine both stdout and stderr into one stream, this error wil not
happen again anymore.
Also a few other nitpicks in this CL:
- Constructing the command for shutdown using simpler syntax.
- Logging the command before running it for all external subprocess runs.
BUG=chromium:1010306
TEST=sudo FEATURES=test emerge update_engine
Change-Id: Ia620afed814e4fe9ba24b1a0ad01680481c6ba7c
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/1901886
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Andrew Lassalle <lassalle@chromium.org>
Reviewed-by: Jae Hoon Kim <kimjae@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
diff --git a/hardware_chromeos.cc b/hardware_chromeos.cc
index dd21c1b..de1d7c0 100644
--- a/hardware_chromeos.cc
+++ b/hardware_chromeos.cc
@@ -87,14 +87,16 @@
// shell command. Returns true on success.
int GetVpdValue(string key, string* result) {
int exit_code = 0;
- string value;
+ string value, error;
vector<string> cmd = {"vpd_get_value", key};
if (!chromeos_update_engine::Subprocess::SynchronousExec(
- cmd, &exit_code, &value) ||
+ cmd, &exit_code, &value, &error) ||
exit_code) {
LOG(ERROR) << "Failed to get vpd key for " << value
- << " with exit code: " << exit_code;
+ << " with exit code: " << exit_code << " and error: " << error;
return false;
+ } else if (!error.empty()) {
+ LOG(INFO) << "vpd_get_value succeeded but with following errors: " << error;
}
base::TrimWhitespaceASCII(value, base::TRIM_ALL, &value);
@@ -198,13 +200,14 @@
}
string HardwareChromeOS::GetECVersion() const {
- string input_line;
+ string input_line, error;
int exit_code = 0;
vector<string> cmd = {"/usr/sbin/mosys", "-k", "ec", "info"};
- bool success = Subprocess::SynchronousExec(cmd, &exit_code, &input_line);
- if (!success || exit_code) {
- LOG(ERROR) << "Unable to read ec info from mosys (" << exit_code << ")";
+ if (!Subprocess::SynchronousExec(cmd, &exit_code, &input_line, &error) ||
+ exit_code != 0) {
+ LOG(ERROR) << "Unable to read EC info from mosys with exit code: "
+ << exit_code << " and error: " << error;
return "";
}
@@ -353,22 +356,28 @@
bool HardwareChromeOS::SetFirstActiveOmahaPingSent() {
int exit_code = 0;
- string output;
+ string output, error;
vector<string> vpd_set_cmd = {
"vpd", "-i", "RW_VPD", "-s", string(kActivePingKey) + "=1"};
- if (!Subprocess::SynchronousExec(vpd_set_cmd, &exit_code, &output) ||
+ if (!Subprocess::SynchronousExec(vpd_set_cmd, &exit_code, &output, &error) ||
exit_code) {
LOG(ERROR) << "Failed to set vpd key for " << kActivePingKey
- << " with exit code: " << exit_code << " with error: " << output;
+ << " with exit code: " << exit_code << " with output: " << output
+ << " and error: " << error;
return false;
+ } else if (!error.empty()) {
+ LOG(INFO) << "vpd succeeded but with error logs: " << error;
}
vector<string> vpd_dump_cmd = {"dump_vpd_log", "--force"};
- if (!Subprocess::SynchronousExec(vpd_dump_cmd, &exit_code, &output) ||
+ if (!Subprocess::SynchronousExec(vpd_dump_cmd, &exit_code, &output, &error) ||
exit_code) {
LOG(ERROR) << "Failed to cache " << kActivePingKey << " using dump_vpd_log"
- << " with exit code: " << exit_code << " with error: " << output;
+ << " with exit code: " << exit_code << " with output: " << output
+ << " and error: " << error;
return false;
+ } else if (!error.empty()) {
+ LOG(INFO) << "dump_vpd_log succeeded but with error logs: " << error;
}
return true;
}