From e8e8cf3f9509808b23506597d23212ac972d1393 Mon Sep 17 00:00:00 2001 From: James Hawkins Date: Tue, 22 Mar 2016 14:13:06 -0700 Subject: [PATCH] bootstat: Track record inconsistencies w/ a debug metric, bootstat_mtime_matches_content. Fixed a file descriptor leak while I was in here. Bug: 27550578 Change-Id: I8e252e4f5bb3c4e2ae96a1560fbb32ae636722a0 (cherry picked from commit 6f28299d0d10a7c397d563dc8293dacf6acadc04) --- bootstat/Android.mk | 17 +++--------- bootstat/boot_event_record_store.cpp | 36 +++++++++++++++++++++++- bootstat/bootstat.cpp | 25 ++++------------- bootstat/histogram_logger.cpp | 41 ++++++++++++++++++++++++++++ bootstat/histogram_logger.h | 26 ++++++++++++++++++ 5 files changed, 111 insertions(+), 34 deletions(-) create mode 100644 bootstat/histogram_logger.cpp create mode 100644 bootstat/histogram_logger.h diff --git a/bootstat/Android.mk b/bootstat/Android.mk index 3d027527f..630094102 100644 --- a/bootstat/Android.mk +++ b/bootstat/Android.mk @@ -21,6 +21,7 @@ bootstat_c_includes := external/gtest/include bootstat_lib_src_files := \ boot_event_record_store.cpp \ event_log_list_builder.cpp \ + histogram_logger.cpp \ uptime_parser.cpp \ bootstat_src_files := \ @@ -41,17 +42,13 @@ bootstat_cflags := \ -Wextra \ -Werror \ -bootstat_cppflags := \ - -Wno-non-virtual-dtor \ - -bootstat_debug_cflags := \ - $(bootstat_cflags) \ - -UNDEBUG \ - # 524291 corresponds to sysui_histogram, from # frameworks/base/core/java/com/android/internal/logging/EventLogTags.logtags bootstat_cflags += -DHISTOGRAM_LOG_TAG=524291 +bootstat_debug_cflags := \ + $(bootstat_cflags) \ + -UNDEBUG \ # bootstat static library # ----------------------------------------------------------------------------- @@ -60,7 +57,6 @@ include $(CLEAR_VARS) LOCAL_MODULE := libbootstat LOCAL_CFLAGS := $(bootstat_cflags) -LOCAL_CPPFLAGS := $(bootstat_cppflags) LOCAL_C_INCLUDES := $(bootstat_c_includes) LOCAL_SHARED_LIBRARIES := $(bootstat_shared_libs) LOCAL_SRC_FILES := $(bootstat_lib_src_files) @@ -76,7 +72,6 @@ include $(CLEAR_VARS) LOCAL_MODULE := libbootstat_debug LOCAL_CFLAGS := $(bootstat_cflags) -LOCAL_CPPFLAGS := $(bootstat_debug_cppflags) LOCAL_C_INCLUDES := $(bootstat_c_includes) LOCAL_SHARED_LIBRARIES := $(bootstat_shared_libs) LOCAL_SRC_FILES := $(bootstat_lib_src_files) @@ -92,7 +87,6 @@ include $(CLEAR_VARS) LOCAL_MODULE := libbootstat_host_debug LOCAL_CFLAGS := $(bootstat_debug_cflags) -LOCAL_CPPFLAGS := $(bootstat_cppflags) LOCAL_C_INCLUDES := $(bootstat_c_includes) LOCAL_SHARED_LIBRARIES := $(bootstat_shared_libs) LOCAL_SRC_FILES := $(bootstat_lib_src_files) @@ -108,7 +102,6 @@ include $(CLEAR_VARS) LOCAL_MODULE := bootstat LOCAL_CFLAGS := $(bootstat_cflags) -LOCAL_CPPFLAGS := $(bootstat_cppflags) LOCAL_C_INCLUDES := $(bootstat_c_includes) LOCAL_SHARED_LIBRARIES := $(bootstat_shared_libs) LOCAL_STATIC_LIBRARIES := libbootstat @@ -126,7 +119,6 @@ include $(CLEAR_VARS) LOCAL_MODULE := bootstat_tests LOCAL_CFLAGS := $(bootstat_tests_cflags) -LOCAL_CPPFLAGS := $(bootstat_cppflags) LOCAL_SHARED_LIBRARIES := $(bootstat_shared_libs) LOCAL_STATIC_LIBRARIES := libbootstat_debug libgmock LOCAL_SRC_FILES := $(bootstat_test_src_files) @@ -142,7 +134,6 @@ include $(CLEAR_VARS) LOCAL_MODULE := bootstat_tests LOCAL_CFLAGS := $(bootstat_tests_cflags) -LOCAL_CPPFLAGS := $(bootstat_cppflags) LOCAL_SHARED_LIBRARIES := $(bootstat_shared_libs) LOCAL_STATIC_LIBRARIES := libbootstat_host_debug libgmock_host LOCAL_SRC_FILES := $(bootstat_test_src_files) diff --git a/bootstat/boot_event_record_store.cpp b/bootstat/boot_event_record_store.cpp index 40254f866..5d1fae90b 100644 --- a/bootstat/boot_event_record_store.cpp +++ b/bootstat/boot_event_record_store.cpp @@ -21,9 +21,11 @@ #include #include #include +#include #include #include #include +#include "histogram_logger.h" #include "uptime_parser.h" namespace { @@ -42,6 +44,20 @@ bool ParseRecordEventTime(const std::string& path, int32_t* uptime) { } *uptime = file_stat.st_mtime; + + // The following code (till function exit) is a debug test to ensure the + // validity of the file mtime value, i.e., to check that the record file + // mtime values are not changed once set. + // TODO(jhawkins): Remove this code. + std::string content; + if (!android::base::ReadFileToString(path, &content)) { + PLOG(ERROR) << "Failed to read " << path; + return false; + } + + int32_t value = std::stoi(content); + bootstat::LogHistogram("bootstat_mtime_matches_content", value == *uptime); + return true; } @@ -61,8 +77,20 @@ void BootEventRecordStore::AddBootEvent(const std::string& event) { void BootEventRecordStore::AddBootEventWithValue( const std::string& event, int32_t value) { std::string record_path = GetBootEventPath(event); - if (creat(record_path.c_str(), S_IRUSR | S_IWUSR) == -1) { + int record_fd = creat(record_path.c_str(), S_IRUSR | S_IWUSR); + if (record_fd == -1) { PLOG(ERROR) << "Failed to create " << record_path; + return; + } + + // Writing the value as content in the record file is a debug measure to + // ensure the validity of the file mtime value, i.e., to check that the record + // file mtime values are not changed once set. + // TODO(jhawkins): Remove this block. + if (!android::base::WriteStringToFd(std::to_string(value), record_fd)) { + PLOG(ERROR) << "Failed to write value to " << record_path; + close(record_fd); + return; } // Fill out the stat structure for |record_path| in order to get the atime to @@ -70,6 +98,8 @@ void BootEventRecordStore::AddBootEventWithValue( struct stat file_stat; if (stat(record_path.c_str(), &file_stat) == -1) { PLOG(ERROR) << "Failed to read " << record_path; + close(record_fd); + return; } // Set the |modtime| of the file to store the value of the boot event while @@ -77,7 +107,11 @@ void BootEventRecordStore::AddBootEventWithValue( struct utimbuf times = {/* actime */ file_stat.st_atime, /* modtime */ value}; if (utime(record_path.c_str(), ×) == -1) { PLOG(ERROR) << "Failed to set mtime for " << record_path; + close(record_fd); + return; } + + close(record_fd); } bool BootEventRecordStore::GetBootEvent( diff --git a/bootstat/bootstat.cpp b/bootstat/bootstat.cpp index 4c8c8b64c..360447f81 100644 --- a/bootstat/bootstat.cpp +++ b/bootstat/bootstat.cpp @@ -32,26 +32,11 @@ #include #include "boot_event_record_store.h" #include "event_log_list_builder.h" +#include "histogram_logger.h" #include "uptime_parser.h" namespace { -// Builds an EventLog buffer named |event| containing |data| and writes -// the log into the Tron histogram logs. -void LogBootEvent(const std::string& event, int32_t data) { - LOG(INFO) << "Logging boot metric: " << event << " " << data; - - EventLogListBuilder log_builder; - log_builder.Append(event); - log_builder.Append(data); - - std::unique_ptr log; - size_t size; - log_builder.Release(&log, &size); - - android_bWriteLog(HISTOGRAM_LOG_TAG, log.get(), size); -} - // Scans the boot event record store for record files and logs each boot event // via EventLog. void LogBootEvents() { @@ -59,7 +44,7 @@ void LogBootEvents() { auto events = boot_event_store.GetAllBootEvents(); for (auto i = events.cbegin(); i != events.cend(); ++i) { - LogBootEvent(i->first, i->second); + bootstat::LogHistogram(i->first, i->second); } } @@ -202,10 +187,10 @@ void RecordFactoryReset() { static const char* factory_reset_current_time = "factory_reset_current_time"; if (current_time_utc < 0) { // UMA does not display negative values in buckets, so convert to positive. - LogBootEvent(factory_reset_current_time, std::abs(current_time_utc)); + bootstat::LogHistogram(factory_reset_current_time, std::abs(current_time_utc)); return; } else { - LogBootEvent(factory_reset_current_time, current_time_utc); + bootstat::LogHistogram(factory_reset_current_time, current_time_utc); } // The factory_reset boot event does not exist after the device is reset, so @@ -221,7 +206,7 @@ void RecordFactoryReset() { // Calculate and record the difference in time between now and the // factory_reset time. time_t factory_reset_utc = record.second; - LogBootEvent("factory_reset_record_value", factory_reset_utc); + bootstat::LogHistogram("factory_reset_record_value", factory_reset_utc); time_t time_since_factory_reset = difftime(current_time_utc, factory_reset_utc); boot_event_store.AddBootEventWithValue("time_since_factory_reset", diff --git a/bootstat/histogram_logger.cpp b/bootstat/histogram_logger.cpp new file mode 100644 index 000000000..e3aad28eb --- /dev/null +++ b/bootstat/histogram_logger.cpp @@ -0,0 +1,41 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "histogram_logger.h" + +#include +#include +#include +#include +#include "event_log_list_builder.h" + +namespace bootstat { + +void LogHistogram(const std::string& event, int32_t data) { + LOG(INFO) << "Logging histogram: " << event << " " << data; + + EventLogListBuilder log_builder; + log_builder.Append(event); + log_builder.Append(data); + + std::unique_ptr log; + size_t size; + log_builder.Release(&log, &size); + + android_bWriteLog(HISTOGRAM_LOG_TAG, log.get(), size); +} + +} // namespace bootstat \ No newline at end of file diff --git a/bootstat/histogram_logger.h b/bootstat/histogram_logger.h new file mode 100644 index 000000000..60c7776a6 --- /dev/null +++ b/bootstat/histogram_logger.h @@ -0,0 +1,26 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include + +namespace bootstat { + +// Builds an EventLog buffer named |event| containing |data| and writes +// the log into the Tron histogram logs. +void LogHistogram(const std::string& event, int32_t data); + +} // namespace bootstat \ No newline at end of file