Extract IncidentHeaderProto to a separated file for statsd to consume
Create a HeaderSection to deal with header protos which is more testable
Bug: 65422268
Test: unit tested
Change-Id: Icdcbeded8bc963940a8f9e503cb65a9a247ca5b2
diff --git a/cmds/incidentd/src/Reporter.cpp b/cmds/incidentd/src/Reporter.cpp
index 722bd4f..11347e2 100644
--- a/cmds/incidentd/src/Reporter.cpp
+++ b/cmds/incidentd/src/Reporter.cpp
@@ -17,8 +17,6 @@
#define LOG_TAG "incidentd"
#include "Reporter.h"
-#include "io_util.h"
-#include "protobuf.h"
#include "report_directory.h"
#include "section_list.h"
@@ -52,6 +50,12 @@
{
}
+bool
+ReportRequest::ok()
+{
+ return fd >= 0 && err == NO_ERROR;
+}
+
// ================================================================================
ReportRequestSet::ReportRequestSet()
:mRequests(),
@@ -117,6 +121,7 @@
status_t err = NO_ERROR;
bool needMainFd = false;
int mainFd = -1;
+ HeaderSection headers;
// See if we need the main file
for (ReportRequestSet::iterator it=batch.begin(); it!=batch.end(); it++) {
@@ -129,7 +134,7 @@
// Create the directory
if (!isTest) err = create_directory(mIncidentDirectory);
if (err != NO_ERROR) {
- goto done;
+ goto DONE;
}
// If there are too many files in the directory (for whatever reason),
@@ -140,7 +145,7 @@
// Open the file.
err = create_file(&mainFd);
if (err != NO_ERROR) {
- goto done;
+ goto DONE;
}
// Add to the set
@@ -155,24 +160,7 @@
}
// Write the incident headers
- for (ReportRequestSet::iterator it=batch.begin(); it!=batch.end(); it++) {
- const sp<ReportRequest> request = (*it);
- const vector<vector<int8_t>>& headers = request->args.headers();
-
- for (vector<vector<int8_t>>::const_iterator buf=headers.begin(); buf!=headers.end();
- buf++) {
- int fd = request->fd >= 0 ? request->fd : mainFd;
-
- uint8_t buffer[20];
- uint8_t* p = write_length_delimited_tag_header(buffer, FIELD_ID_INCIDENT_HEADER,
- buf->size());
- write_all(fd, buffer, p-buffer);
-
- write_all(fd, (uint8_t const*)buf->data(), buf->size());
- // If there was an error now, there will be an error later and we will remove
- // it from the list then.
- }
- }
+ headers.Execute(&batch);
// For each of the report fields, see if we need it, and if so, execute the command
// and report to those that care that we're doing it.
@@ -193,7 +181,7 @@
if (err != NO_ERROR) {
ALOGW("Incident section %s (%d) failed. Stopping report.",
(*section)->name.string(), id);
- goto done;
+ goto DONE;
}
// Notify listener of starting
@@ -207,7 +195,7 @@
}
}
-done:
+DONE:
// Close the file.
if (mainFd >= 0) {
close(mainFd);
diff --git a/cmds/incidentd/src/Reporter.h b/cmds/incidentd/src/Reporter.h
index b74cc42..2615c62 100644
--- a/cmds/incidentd/src/Reporter.h
+++ b/cmds/incidentd/src/Reporter.h
@@ -40,6 +40,8 @@
ReportRequest(const IncidentReportArgs& args,
const sp<IIncidentReportStatusListener> &listener, int fd);
virtual ~ReportRequest();
+
+ bool ok(); // returns true if the request is ok for write.
};
// ================================================================================
diff --git a/cmds/incidentd/src/Section.cpp b/cmds/incidentd/src/Section.cpp
index 0895926..6f052de 100644
--- a/cmds/incidentd/src/Section.cpp
+++ b/cmds/incidentd/src/Section.cpp
@@ -39,7 +39,7 @@
const char* INCIDENT_HELPER = "/system/bin/incident_helper";
static pid_t
-forkAndExecuteIncidentHelper(const int id, const char* name, Fpipe& p2cPipe, Fpipe& c2pPipe)
+fork_execute_incident_helper(const int id, const char* name, Fpipe& p2cPipe, Fpipe& c2pPipe)
{
const char* ihArgs[] { INCIDENT_HELPER, "-s", String8::format("%d", id).string(), NULL };
@@ -74,14 +74,14 @@
}
// ================================================================================
-static status_t killChild(pid_t pid) {
+static status_t kill_child(pid_t pid) {
int status;
kill(pid, SIGKILL);
if (waitpid(pid, &status, 0) == -1) return -1;
return WIFEXITED(status) == 0 ? NO_ERROR : -WEXITSTATUS(status);
}
-static status_t waitForChild(pid_t pid) {
+static status_t wait_child(pid_t pid) {
int status;
bool died = false;
// wait for child to report status up to 1 seconds
@@ -90,13 +90,12 @@
// sleep for 0.2 second
nanosleep(&WAIT_INTERVAL_NS, NULL);
}
- if (!died) return killChild(pid);
+ if (!died) return kill_child(pid);
return WIFEXITED(status) == 0 ? NO_ERROR : -WEXITSTATUS(status);
}
-
// ================================================================================
static const Privacy*
-GetPrivacyOfSection(int id)
+get_privacy_of_section(int id)
{
if (id < 0) return NULL;
int i=0;
@@ -109,33 +108,27 @@
return NULL;
}
+// ================================================================================
static status_t
-WriteToRequest(const int id, const int fd, EncodedBuffer& buffer)
+write_section_header(int fd, int sectionId, size_t size)
{
- if (buffer.size() == 0) return NO_ERROR;
-
- status_t err = NO_ERROR;
uint8_t buf[20];
- uint8_t *p = write_length_delimited_tag_header(buf, id, buffer.size());
- err = write_all(fd, buf, p-buf);
- if (err == NO_ERROR) {
- err = buffer.flush(fd);
- }
- return err;
+ uint8_t *p = write_length_delimited_tag_header(buf, sectionId, size);
+ return write_all(fd, buf, p-buf);
}
static status_t
-WriteToReportRequests(const int id, const FdBuffer& buffer, ReportRequestSet* requests)
+write_report_requests(const int id, const FdBuffer& buffer, ReportRequestSet* requests)
{
status_t err = -EBADF;
- EncodedBuffer encodedBuffer(buffer, GetPrivacyOfSection(id));
+ EncodedBuffer encodedBuffer(buffer, get_privacy_of_section(id));
int writeable = 0;
// The streaming ones, group requests by spec in order to save unnecessary strip operations
map<PrivacySpec, vector<sp<ReportRequest>>> requestsBySpec;
for (ReportRequestSet::iterator it = requests->begin(); it != requests->end(); it++) {
sp<ReportRequest> request = *it;
- if (!request->args.containsSection(id) || request->fd < 0 || request->err != NO_ERROR) {
+ if (!request->ok() || !request->args.containsSection(id)) {
continue; // skip invalid request
}
PrivacySpec spec = new_spec_from_args(request->args.dest());
@@ -150,12 +143,11 @@
for (vector<sp<ReportRequest>>::iterator it = mit->second.begin(); it != mit->second.end(); it++) {
sp<ReportRequest> request = *it;
- err = WriteToRequest(id, request->fd, encodedBuffer);
- if (err != NO_ERROR) {
- request->err = err;
- } else {
- writeable++;
- }
+ err = write_section_header(request->fd, id, encodedBuffer.size());
+ if (err != NO_ERROR) { request->err = err; continue; }
+ err = encodedBuffer.flush(request->fd);
+ if (err != NO_ERROR) { request->err = err; continue; }
+ writeable++;
ALOGD("Section %d flushed %zu bytes to fd %d with spec %d", id, encodedBuffer.size(), request->fd, spec.dest);
}
encodedBuffer.clear();
@@ -165,21 +157,25 @@
if (requests->mainFd() >= 0) {
err = encodedBuffer.strip(get_default_dropbox_spec());
if (err != NO_ERROR) return err; // the buffer data is corrupted.
+ if (encodedBuffer.size() == 0) goto DONE;
- err = WriteToRequest(id, requests->mainFd(), encodedBuffer);
- if (err != NO_ERROR) {
- requests->setMainFd(-1);
- } else {
- writeable++;
- }
+ err = write_section_header(requests->mainFd(), id, encodedBuffer.size());
+ if (err != NO_ERROR) { requests->setMainFd(-1); goto DONE; }
+ err = encodedBuffer.flush(requests->mainFd());
+ if (err != NO_ERROR) { requests->setMainFd(-1); goto DONE; }
+ writeable++;
+ ALOGD("Section %d flushed %zu bytes to dropbox %d", id, encodedBuffer.size(), requests->mainFd());
}
+
+DONE:
// only returns error if there is no fd to write to.
return writeable > 0 ? NO_ERROR : err;
}
// ================================================================================
Section::Section(int i, const int64_t timeoutMs)
- :id(i), timeoutMs(timeoutMs)
+ :id(i),
+ timeoutMs(timeoutMs)
{
}
@@ -188,8 +184,41 @@
}
// ================================================================================
+HeaderSection::HeaderSection()
+ :Section(FIELD_ID_INCIDENT_HEADER, 0)
+{
+}
+
+HeaderSection::~HeaderSection()
+{
+}
+
+status_t
+HeaderSection::Execute(ReportRequestSet* requests) const
+{
+ for (ReportRequestSet::iterator it=requests->begin(); it!=requests->end(); it++) {
+ const sp<ReportRequest> request = *it;
+ const vector<vector<int8_t>>& headers = request->args.headers();
+
+ for (vector<vector<int8_t>>::const_iterator buf=headers.begin(); buf!=headers.end(); buf++) {
+ if (buf->empty()) continue;
+
+ // So the idea is only requests with negative fd are written to dropbox file.
+ int fd = request->fd >= 0 ? request->fd : requests->mainFd();
+ write_section_header(fd, FIELD_ID_INCIDENT_HEADER, buf->size());
+ write_all(fd, (uint8_t const*)buf->data(), buf->size());
+ // If there was an error now, there will be an error later and we will remove
+ // it from the list then.
+ }
+ }
+ return NO_ERROR;
+}
+
+// ================================================================================
FileSection::FileSection(int id, const char* filename, const int64_t timeoutMs)
- : Section(id, timeoutMs), mFilename(filename) {
+ :Section(id, timeoutMs),
+ mFilename(filename)
+{
name = filename;
}
@@ -215,7 +244,7 @@
return -errno;
}
- pid_t pid = forkAndExecuteIncidentHelper(this->id, this->name.string(), p2cPipe, c2pPipe);
+ pid_t pid = fork_execute_incident_helper(this->id, this->name.string(), p2cPipe, c2pPipe);
if (pid == -1) {
ALOGW("FileSection '%s' failed to fork", this->name.string());
return -errno;
@@ -227,11 +256,11 @@
if (readStatus != NO_ERROR || buffer.timedOut()) {
ALOGW("FileSection '%s' failed to read data from incident helper: %s, timedout: %s, kill: %s",
this->name.string(), strerror(-readStatus), buffer.timedOut() ? "true" : "false",
- strerror(-killChild(pid)));
+ strerror(-kill_child(pid)));
return readStatus;
}
- status_t ihStatus = waitForChild(pid);
+ status_t ihStatus = wait_child(pid);
if (ihStatus != NO_ERROR) {
ALOGW("FileSection '%s' abnormal child process: %s", this->name.string(), strerror(-ihStatus));
return ihStatus;
@@ -239,7 +268,7 @@
ALOGD("FileSection '%s' wrote %zd bytes in %d ms", this->name.string(), buffer.size(),
(int)buffer.durationMs());
- status_t err = WriteToReportRequests(this->id, buffer, requests);
+ status_t err = write_report_requests(this->id, buffer, requests);
if (err != NO_ERROR) {
ALOGW("FileSection '%s' failed writing: %s", this->name.string(), strerror(-err));
return err;
@@ -396,7 +425,7 @@
// Write the data that was collected
ALOGD("WorkerThreadSection '%s' wrote %zd bytes in %d ms", name.string(), buffer.size(),
(int)buffer.durationMs());
- err = WriteToReportRequests(this->id, buffer, requests);
+ err = write_report_requests(this->id, buffer, requests);
if (err != NO_ERROR) {
ALOGW("WorkerThreadSection '%s' failed writing: '%s'", this->name.string(), strerror(-err));
return err;
@@ -433,7 +462,7 @@
}
CommandSection::CommandSection(int id, const int64_t timeoutMs, const char* command, ...)
- : Section(id, timeoutMs)
+ :Section(id, timeoutMs)
{
va_list args;
va_start(args, command);
@@ -442,7 +471,7 @@
}
CommandSection::CommandSection(int id, const char* command, ...)
- : Section(id)
+ :Section(id)
{
va_list args;
va_start(args, command);
@@ -484,7 +513,7 @@
ALOGW("CommandSection '%s' failed in executing command: %s", this->name.string(), strerror(errno));
_exit(err); // exit with command error code
}
- pid_t ihPid = forkAndExecuteIncidentHelper(this->id, this->name.string(), cmdPipe, ihPipe);
+ pid_t ihPid = fork_execute_incident_helper(this->id, this->name.string(), cmdPipe, ihPipe);
if (ihPid == -1) {
ALOGW("CommandSection '%s' failed to fork", this->name.string());
return -errno;
@@ -496,14 +525,14 @@
ALOGW("CommandSection '%s' failed to read data from incident helper: %s, "
"timedout: %s, kill command: %s, kill incident helper: %s",
this->name.string(), strerror(-readStatus), buffer.timedOut() ? "true" : "false",
- strerror(-killChild(cmdPid)), strerror(-killChild(ihPid)));
+ strerror(-kill_child(cmdPid)), strerror(-kill_child(ihPid)));
return readStatus;
}
// TODO: wait for command here has one trade-off: the failed status of command won't be detected until
// buffer timeout, but it has advatage on starting the data stream earlier.
- status_t cmdStatus = waitForChild(cmdPid);
- status_t ihStatus = waitForChild(ihPid);
+ status_t cmdStatus = wait_child(cmdPid);
+ status_t ihStatus = wait_child(ihPid);
if (cmdStatus != NO_ERROR || ihStatus != NO_ERROR) {
ALOGW("CommandSection '%s' abnormal child processes, return status: command: %s, incident helper: %s",
this->name.string(), strerror(-cmdStatus), strerror(-ihStatus));
@@ -512,7 +541,7 @@
ALOGD("CommandSection '%s' wrote %zd bytes in %d ms", this->name.string(), buffer.size(),
(int)buffer.durationMs());
- status_t err = WriteToReportRequests(this->id, buffer, requests);
+ status_t err = write_report_requests(this->id, buffer, requests);
if (err != NO_ERROR) {
ALOGW("CommandSection '%s' failed writing: %s", this->name.string(), strerror(-err));
return err;
diff --git a/cmds/incidentd/src/Section.h b/cmds/incidentd/src/Section.h
index 9cfd696..0a1e03e 100644
--- a/cmds/incidentd/src/Section.h
+++ b/cmds/incidentd/src/Section.h
@@ -45,6 +45,18 @@
};
/**
+ * Section that generates incident headers.
+ */
+class HeaderSection : public Section
+{
+public:
+ HeaderSection();
+ virtual ~HeaderSection();
+
+ virtual status_t Execute(ReportRequestSet* requests) const;
+};
+
+/**
* Section that reads in a file.
*/
class FileSection : public Section
diff --git a/cmds/incidentd/tests/Section_test.cpp b/cmds/incidentd/tests/Section_test.cpp
index f2aee8f..25b05b2 100644
--- a/cmds/incidentd/tests/Section_test.cpp
+++ b/cmds/incidentd/tests/Section_test.cpp
@@ -39,6 +39,8 @@
using ::testing::internal::CaptureStdout;
using ::testing::internal::GetCapturedStdout;
+// NOTICE: this test requires /system/bin/incident_helper is installed.
+
class SimpleListener : public IIncidentReportStatusListener
{
public:
@@ -54,7 +56,43 @@
virtual IBinder* onAsBinder() override { return nullptr; };
};
-// NOTICE: this test requires /system/bin/incident_helper is installed.
+TEST(SectionTest, HeaderSection) {
+ TemporaryFile output2;
+ HeaderSection hs;
+ ReportRequestSet requests;
+
+ IncidentReportArgs args1, args2;
+ args1.addSection(1);
+ args1.addSection(2);
+ args2.setAll(true);
+
+ vector<int8_t> head1;
+ head1.push_back('a');
+ head1.push_back('x');
+ head1.push_back('e');
+
+ vector<int8_t> head2;
+ head2.push_back('p');
+ head2.push_back('u');
+ head2.push_back('p');
+
+ args1.addHeader(head1);
+ args1.addHeader(head2);
+ args2.addHeader(head2);
+
+ requests.add(new ReportRequest(args1, new SimpleListener(), -1));
+ requests.add(new ReportRequest(args2, new SimpleListener(), output2.fd));
+ requests.setMainFd(STDOUT_FILENO);
+
+ string content;
+ CaptureStdout();
+ ASSERT_EQ(NO_ERROR, hs.Execute(&requests));
+ EXPECT_THAT(GetCapturedStdout(), StrEq("\n\x3" "axe\n\x03pup"));
+
+ EXPECT_TRUE(ReadFileToString(output2.path, &content));
+ EXPECT_THAT(content, StrEq("\n\x03pup"));
+}
+
TEST(SectionTest, FileSection) {
TemporaryFile tf;
FileSection fs(REVERSE_PARSER, tf.path);