Merge changes from topic 'debuggerd_client'
* changes:
adb: use libdebuggerd_client.
debuggerd: add libdebuggerd_client.
diff --git a/adb/shell_service.cpp b/adb/shell_service.cpp
index b038fda..10b2502 100644
--- a/adb/shell_service.cpp
+++ b/adb/shell_service.cpp
@@ -160,9 +160,14 @@
pid_t pid() const { return pid_; }
// Sets up FDs, forks a subprocess, starts the subprocess manager thread,
- // and exec's the child. Returns false on failure.
+ // and exec's the child. Returns false and sets error on failure.
bool ForkAndExec(std::string* _Nonnull error);
+ // Start the subprocess manager thread. Consumes the subprocess, regardless of success.
+ // Returns false and sets error on failure.
+ static bool StartThread(std::unique_ptr<Subprocess> subprocess,
+ std::string* _Nonnull error);
+
private:
// Opens the file at |pts_name|.
int OpenPtyChildFd(const char* pts_name, unique_fd* error_sfd);
@@ -390,14 +395,19 @@
}
}
- if (!adb_thread_create(ThreadHandler, this)) {
+ D("subprocess parent: completed");
+ return true;
+}
+
+bool Subprocess::StartThread(std::unique_ptr<Subprocess> subprocess, std::string* error) {
+ Subprocess* raw = subprocess.release();
+ if (!adb_thread_create(ThreadHandler, raw)) {
*error =
android::base::StringPrintf("failed to create subprocess thread: %s", strerror(errno));
- kill(pid_, SIGKILL);
+ kill(raw->pid_, SIGKILL);
return false;
}
- D("subprocess parent: completed");
return true;
}
@@ -441,6 +451,7 @@
adb_thread_setname(android::base::StringPrintf(
"shell srvc %d", subprocess->local_socket_fd()));
+ D("passing data streams for PID %d", subprocess->pid());
subprocess->PassDataStreams();
D("deleting Subprocess for PID %d", subprocess->pid());
@@ -733,7 +744,7 @@
protocol == SubprocessProtocol::kNone ? "none" : "shell",
terminal_type, name);
- Subprocess* subprocess = new Subprocess(name, terminal_type, type, protocol);
+ auto subprocess = std::make_unique<Subprocess>(name, terminal_type, type, protocol);
if (!subprocess) {
LOG(ERROR) << "failed to allocate new subprocess";
return ReportError(protocol, "failed to allocate new subprocess");
@@ -742,11 +753,17 @@
std::string error;
if (!subprocess->ForkAndExec(&error)) {
LOG(ERROR) << "failed to start subprocess: " << error;
- delete subprocess;
return ReportError(protocol, error);
}
- D("subprocess creation successful: local_socket_fd=%d, pid=%d",
- subprocess->local_socket_fd(), subprocess->pid());
- return subprocess->local_socket_fd();
+ unique_fd local_socket(dup(subprocess->local_socket_fd()));
+ D("subprocess creation successful: local_socket_fd=%d, pid=%d", local_socket.get(),
+ subprocess->pid());
+
+ if (!Subprocess::StartThread(std::move(subprocess), &error)) {
+ LOG(ERROR) << "failed to start subprocess management thread: " << error;
+ return ReportError(protocol, error);
+ }
+
+ return local_socket.release();
}
diff --git a/debuggerd/tombstone.cpp b/debuggerd/tombstone.cpp
index fa983fa..dfdf29c 100644
--- a/debuggerd/tombstone.cpp
+++ b/debuggerd/tombstone.cpp
@@ -368,6 +368,7 @@
ALOGE("Cannot get siginfo for %d: %s\n", tid, strerror(errno));
}
+ ScopedBacktraceMapIteratorLock lock(map);
_LOG(log, logtype::MAPS, "\n");
if (!print_fault_address_marker) {
_LOG(log, logtype::MAPS, "memory map:\n");
diff --git a/healthd/Android.mk b/healthd/Android.mk
index 127f39e..deebed5 100644
--- a/healthd/Android.mk
+++ b/healthd/Android.mk
@@ -17,7 +17,7 @@
LOCAL_MODULE := libbatterymonitor
LOCAL_C_INCLUDES := $(LOCAL_PATH)/include
LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/include
-LOCAL_STATIC_LIBRARIES := libutils libbinder
+LOCAL_STATIC_LIBRARIES := libutils libbase libbinder
include $(BUILD_STATIC_LIBRARY)
include $(CLEAR_VARS)
@@ -60,7 +60,7 @@
LOCAL_C_INCLUDES := bootable/recovery $(LOCAL_PATH)/include
-LOCAL_STATIC_LIBRARIES := libbatterymonitor libbatteryservice libbinder
+LOCAL_STATIC_LIBRARIES := libbatterymonitor libbatteryservice libbinder libbase
ifneq ($(strip $(LOCAL_CHARGER_NO_UI)),true)
LOCAL_STATIC_LIBRARIES += libminui libpng libz
diff --git a/healthd/BatteryMonitor.cpp b/healthd/BatteryMonitor.cpp
index 69647de..d1c547d 100644
--- a/healthd/BatteryMonitor.cpp
+++ b/healthd/BatteryMonitor.cpp
@@ -28,6 +28,8 @@
#include <unistd.h>
#include <memory>
+#include <android-base/file.h>
+#include <android-base/strings.h>
#include <batteryservice/BatteryService.h>
#include <cutils/klog.h>
#include <cutils/properties.h>
@@ -121,34 +123,15 @@
return ret;
}
-int BatteryMonitor::readFromFile(const String8& path, char* buf, size_t size) {
- char *cp = NULL;
-
- if (path.isEmpty())
- return -1;
- int fd = open(path.string(), O_RDONLY, 0);
- if (fd == -1) {
- KLOG_ERROR(LOG_TAG, "Could not open '%s'\n", path.string());
- return -1;
+int BatteryMonitor::readFromFile(const String8& path, std::string* buf) {
+ if (android::base::ReadFileToString(String8::std_string(path), buf)) {
+ *buf = android::base::Trim(*buf);
}
-
- ssize_t count = TEMP_FAILURE_RETRY(read(fd, buf, size));
- if (count > 0)
- cp = (char *)memrchr(buf, '\n', count);
-
- if (cp)
- *cp = '\0';
- else
- buf[0] = '\0';
-
- close(fd);
- return count;
+ return buf->length();
}
BatteryMonitor::PowerSupplyType BatteryMonitor::readPowerSupplyType(const String8& path) {
- const int SIZE = 128;
- char buf[SIZE];
- int length = readFromFile(path, buf, SIZE);
+ std::string buf;
BatteryMonitor::PowerSupplyType ret;
struct sysfsStringEnumMap supplyTypeMap[] = {
{ "Unknown", ANDROID_POWER_SUPPLY_TYPE_UNKNOWN },
@@ -167,12 +150,12 @@
{ NULL, 0 },
};
- if (length <= 0)
+ if (readFromFile(path, &buf) <= 0)
return ANDROID_POWER_SUPPLY_TYPE_UNKNOWN;
- ret = (BatteryMonitor::PowerSupplyType)mapSysfsString(buf, supplyTypeMap);
+ ret = (BatteryMonitor::PowerSupplyType)mapSysfsString(buf.c_str(), supplyTypeMap);
if (ret < 0) {
- KLOG_WARNING(LOG_TAG, "Unknown power supply type '%s'\n", buf);
+ KLOG_WARNING(LOG_TAG, "Unknown power supply type '%s'\n", buf.c_str());
ret = ANDROID_POWER_SUPPLY_TYPE_UNKNOWN;
}
@@ -180,27 +163,23 @@
}
bool BatteryMonitor::getBooleanField(const String8& path) {
- const int SIZE = 16;
- char buf[SIZE];
-
+ std::string buf;
bool value = false;
- if (readFromFile(path, buf, SIZE) > 0) {
- if (buf[0] != '0') {
+
+ if (readFromFile(path, &buf) > 0)
+ if (buf[0] != '0')
value = true;
- }
- }
return value;
}
int BatteryMonitor::getIntField(const String8& path) {
- const int SIZE = 128;
- char buf[SIZE];
-
+ std::string buf;
int value = 0;
- if (readFromFile(path, buf, SIZE) > 0) {
- value = strtol(buf, NULL, 0);
- }
+
+ if (readFromFile(path, &buf) > 0)
+ value = std::stoi(buf.c_str(), NULL, 0);
+
return value;
}
@@ -241,18 +220,16 @@
props.batteryHealth = BATTERY_HEALTH_GOOD;
}
- const int SIZE = 128;
- char buf[SIZE];
- String8 btech;
+ std::string buf;
- if (readFromFile(mHealthdConfig->batteryStatusPath, buf, SIZE) > 0)
- props.batteryStatus = getBatteryStatus(buf);
+ if (readFromFile(mHealthdConfig->batteryStatusPath, &buf) > 0)
+ props.batteryStatus = getBatteryStatus(buf.c_str());
- if (readFromFile(mHealthdConfig->batteryHealthPath, buf, SIZE) > 0)
- props.batteryHealth = getBatteryHealth(buf);
+ if (readFromFile(mHealthdConfig->batteryHealthPath, &buf) > 0)
+ props.batteryHealth = getBatteryHealth(buf.c_str());
- if (readFromFile(mHealthdConfig->batteryTechnologyPath, buf, SIZE) > 0)
- props.batteryTechnology = String8(buf);
+ if (readFromFile(mHealthdConfig->batteryTechnologyPath, &buf) > 0)
+ props.batteryTechnology = String8(buf.c_str());
unsigned int i;
@@ -261,33 +238,31 @@
path.appendFormat("%s/%s/online", POWER_SUPPLY_SYSFS_PATH,
mChargerNames[i].string());
- if (readFromFile(path, buf, SIZE) > 0) {
- if (buf[0] != '0') {
- path.clear();
- path.appendFormat("%s/%s/type", POWER_SUPPLY_SYSFS_PATH,
- mChargerNames[i].string());
- switch(readPowerSupplyType(path)) {
- case ANDROID_POWER_SUPPLY_TYPE_AC:
- props.chargerAcOnline = true;
- break;
- case ANDROID_POWER_SUPPLY_TYPE_USB:
- props.chargerUsbOnline = true;
- break;
- case ANDROID_POWER_SUPPLY_TYPE_WIRELESS:
- props.chargerWirelessOnline = true;
- break;
- default:
- KLOG_WARNING(LOG_TAG, "%s: Unknown power supply type\n",
- mChargerNames[i].string());
- }
- path.clear();
- path.appendFormat("%s/%s/current_max", POWER_SUPPLY_SYSFS_PATH,
- mChargerNames[i].string());
- if (access(path.string(), R_OK) == 0) {
- int maxChargingCurrent = getIntField(path);
- if (props.maxChargingCurrent < maxChargingCurrent) {
- props.maxChargingCurrent = maxChargingCurrent;
- }
+ if (getIntField(path)) {
+ path.clear();
+ path.appendFormat("%s/%s/type", POWER_SUPPLY_SYSFS_PATH,
+ mChargerNames[i].string());
+ switch(readPowerSupplyType(path)) {
+ case ANDROID_POWER_SUPPLY_TYPE_AC:
+ props.chargerAcOnline = true;
+ break;
+ case ANDROID_POWER_SUPPLY_TYPE_USB:
+ props.chargerUsbOnline = true;
+ break;
+ case ANDROID_POWER_SUPPLY_TYPE_WIRELESS:
+ props.chargerWirelessOnline = true;
+ break;
+ default:
+ KLOG_WARNING(LOG_TAG, "%s: Unknown power supply type\n",
+ mChargerNames[i].string());
+ }
+ path.clear();
+ path.appendFormat("%s/%s/current_max", POWER_SUPPLY_SYSFS_PATH,
+ mChargerNames[i].string());
+ if (access(path.string(), R_OK) == 0) {
+ int maxChargingCurrent = getIntField(path);
+ if (props.maxChargingCurrent < maxChargingCurrent) {
+ props.maxChargingCurrent = maxChargingCurrent;
}
}
}
@@ -343,10 +318,9 @@
int BatteryMonitor::getChargeStatus() {
int result = BATTERY_STATUS_UNKNOWN;
if (!mHealthdConfig->batteryStatusPath.isEmpty()) {
- char buf[128];
- if (readFromFile(mHealthdConfig->batteryStatusPath, buf, sizeof(buf)) > 0) {
- result = getBatteryStatus(buf);
- }
+ std::string buf;
+ if (readFromFile(mHealthdConfig->batteryStatusPath, &buf) > 0)
+ result = getBatteryStatus(buf.c_str());
}
return result;
}
diff --git a/healthd/include/healthd/BatteryMonitor.h b/healthd/include/healthd/BatteryMonitor.h
index 440f2e4..8865a7d 100644
--- a/healthd/include/healthd/BatteryMonitor.h
+++ b/healthd/include/healthd/BatteryMonitor.h
@@ -55,7 +55,7 @@
int getBatteryStatus(const char* status);
int getBatteryHealth(const char* status);
- int readFromFile(const String8& path, char* buf, size_t size);
+ int readFromFile(const String8& path, std::string* buf);
PowerSupplyType readPowerSupplyType(const String8& path);
bool getBooleanField(const String8& path);
int getIntField(const String8& path);
diff --git a/include/backtrace/BacktraceMap.h b/include/backtrace/BacktraceMap.h
index 2373c45..b80045f 100644
--- a/include/backtrace/BacktraceMap.h
+++ b/include/backtrace/BacktraceMap.h
@@ -71,6 +71,12 @@
bool IsWritable(uintptr_t pc) { return GetFlags(pc) & PROT_WRITE; }
bool IsExecutable(uintptr_t pc) { return GetFlags(pc) & PROT_EXEC; }
+ // In order to use the iterators on this object, a caller must
+ // call the LockIterator and UnlockIterator function to guarantee
+ // that the data does not change while it's being used.
+ virtual void LockIterator() {}
+ virtual void UnlockIterator() {}
+
typedef std::deque<backtrace_map_t>::iterator iterator;
iterator begin() { return maps_.begin(); }
iterator end() { return maps_.end(); }
@@ -102,4 +108,18 @@
pid_t pid_;
};
+class ScopedBacktraceMapIteratorLock {
+public:
+ explicit ScopedBacktraceMapIteratorLock(BacktraceMap* map) : map_(map) {
+ map->LockIterator();
+ }
+
+ ~ScopedBacktraceMapIteratorLock() {
+ map_->UnlockIterator();
+ }
+
+private:
+ BacktraceMap* map_;
+};
+
#endif // _BACKTRACE_BACKTRACE_MAP_H
diff --git a/libbacktrace/BacktraceMap.cpp b/libbacktrace/BacktraceMap.cpp
index 19551dc..00c35b1 100644
--- a/libbacktrace/BacktraceMap.cpp
+++ b/libbacktrace/BacktraceMap.cpp
@@ -36,8 +36,8 @@
}
void BacktraceMap::FillIn(uintptr_t addr, backtrace_map_t* map) {
- for (BacktraceMap::const_iterator it = begin();
- it != end(); ++it) {
+ ScopedBacktraceMapIteratorLock lock(this);
+ for (BacktraceMap::const_iterator it = begin(); it != end(); ++it) {
if (addr >= it->start && addr < it->end) {
*map = *it;
return;
diff --git a/libbacktrace/UnwindMap.cpp b/libbacktrace/UnwindMap.cpp
index 34d79f9..af79562 100644
--- a/libbacktrace/UnwindMap.cpp
+++ b/libbacktrace/UnwindMap.cpp
@@ -14,6 +14,7 @@
* limitations under the License.
*/
+#include <pthread.h>
#include <stdint.h>
#include <stdlib.h>
#include <sys/types.h>
@@ -72,6 +73,7 @@
}
UnwindMapLocal::UnwindMapLocal() : UnwindMap(getpid()), map_created_(false) {
+ pthread_rwlock_init(&map_lock_, nullptr);
}
UnwindMapLocal::~UnwindMapLocal() {
@@ -82,9 +84,14 @@
}
bool UnwindMapLocal::GenerateMap() {
+ // Lock so that multiple threads cannot modify the maps data at the
+ // same time.
+ pthread_rwlock_wrlock(&map_lock_);
+
// It's possible for the map to be regenerated while this loop is occurring.
// If that happens, get the map again, but only try at most three times
// before giving up.
+ bool generated = false;
for (int i = 0; i < 3; i++) {
maps_.clear();
@@ -110,12 +117,17 @@
}
// Check to see if the map changed while getting the data.
if (ret != -UNW_EINVAL) {
- return true;
+ generated = true;
+ break;
}
}
- BACK_LOGW("Unable to generate the map.");
- return false;
+ pthread_rwlock_unlock(&map_lock_);
+
+ if (!generated) {
+ BACK_LOGW("Unable to generate the map.");
+ }
+ return generated;
}
bool UnwindMapLocal::Build() {
diff --git a/libbacktrace/UnwindMap.h b/libbacktrace/UnwindMap.h
index 111401f..f85b54a 100644
--- a/libbacktrace/UnwindMap.h
+++ b/libbacktrace/UnwindMap.h
@@ -17,6 +17,7 @@
#ifndef _LIBBACKTRACE_UNWIND_MAP_H
#define _LIBBACKTRACE_UNWIND_MAP_H
+#include <pthread.h>
#include <stdint.h>
#include <sys/types.h>
@@ -56,10 +57,15 @@
void FillIn(uintptr_t addr, backtrace_map_t* map) override;
+ void LockIterator() override { pthread_rwlock_rdlock(&map_lock_); }
+ void UnlockIterator() override { pthread_rwlock_unlock(&map_lock_); }
+
private:
bool GenerateMap();
bool map_created_;
+
+ pthread_rwlock_t map_lock_;
};
#endif // _LIBBACKTRACE_UNWIND_MAP_H
diff --git a/libbacktrace/backtrace_test.cpp b/libbacktrace/backtrace_test.cpp
index 7066c79..e25c8e9 100644
--- a/libbacktrace/backtrace_test.cpp
+++ b/libbacktrace/backtrace_test.cpp
@@ -895,6 +895,7 @@
std::unique_ptr<BacktraceMap> map(BacktraceMap::Create(pid));
// Basic test that verifies that the map is in the expected order.
+ ScopedBacktraceMapIteratorLock lock(map.get());
std::vector<map_test_t>::const_iterator test_it = test_maps.begin();
for (BacktraceMap::const_iterator it = map->begin(); it != map->end(); ++it) {
ASSERT_TRUE(test_it != test_maps.end());