From 3217c5c7d9612d3984af7384940ed0949b900b6a Mon Sep 17 00:00:00 2001 From: Michael Scott Date: Sun, 5 Jun 2016 11:20:13 -0700 Subject: [PATCH] batterymonitor: simplify readFromFile and use std::string buffers In readFromFile() when a newline is not found in the data, we reset the initial character of the buffer to \0, but leave the count as is (something >0 in this case). Later in getBooleanField() we could erroneously treat a response as "true" because count would be >0 and the initial value of buf would be != '0' (set to \0 in this case). To fixup error paths such as this, we can simplify readFromFile by using android::base functions: ReadFromFileString() and Trim(). NOTES: - Converted char * buffers used with readFromFile to std::string - Removed unused variable btech from BatteryMonitor::update Testing Done: - Build healthd and recovery for angler device - Confirm that known values are being read correctly from kernel sysfs. Change-Id: I238bbff097543767f352aa084bf0acbc1324baca Signed-off-by: Michael Scott --- healthd/Android.mk | 4 +- healthd/BatteryMonitor.cpp | 134 +++++++++-------------- healthd/include/healthd/BatteryMonitor.h | 2 +- 3 files changed, 57 insertions(+), 83 deletions(-) diff --git a/healthd/Android.mk b/healthd/Android.mk index 127f39e90..deebed5a5 100644 --- a/healthd/Android.mk +++ b/healthd/Android.mk @@ -17,7 +17,7 @@ LOCAL_SRC_FILES := BatteryMonitor.cpp 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 @@ endif 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 69647de26..d1c547df9 100644 --- a/healthd/BatteryMonitor.cpp +++ b/healthd/BatteryMonitor.cpp @@ -28,6 +28,8 @@ #include #include +#include +#include #include #include #include @@ -121,34 +123,15 @@ int BatteryMonitor::getBatteryHealth(const char* status) { 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 @@ BatteryMonitor::PowerSupplyType BatteryMonitor::readPowerSupplyType(const String { 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 @@ BatteryMonitor::PowerSupplyType BatteryMonitor::readPowerSupplyType(const String } 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 @@ bool BatteryMonitor::update(void) { 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 @@ bool BatteryMonitor::update(void) { 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 @@ bool BatteryMonitor::update(void) { 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 440f2e44c..8865a7d82 100644 --- a/healthd/include/healthd/BatteryMonitor.h +++ b/healthd/include/healthd/BatteryMonitor.h @@ -55,7 +55,7 @@ class BatteryMonitor { 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);