EventHub: Reimplement sysfsNodeChanged
There were several bugs in the old implementation of sysfsNodeChanged.
There are unfortunately no existing unit tests for EventHub, so the old
code and the new code are not tested exhaustively.
Issues addressed:
- The old code would concurrently modify mOpeningDevices while iterating
through it, making the code difficult to reason about, particularly
since calling openDeviceLocked() from the iteration would add an
additional opening device.
- The old code was improperly erasing items from mOpeningDevices.
vector::erase() returns the next iterator position after the removal,
and when progressing to the next for loop iteration, the iterator is
incremented again, leading to a missed element.
- Each call to isChanged() involves several syscalls, and the old code
would perform the check multiple times on the same AssociatedDevice
object. Note that for each changed sysfs node, the sysfs node is
reloaded yet again for each openDeviceLocked() call, which this CL
does not address.
Bug: 245989146
Test: atest SonyDualshock4BluetoothTest --iterations
Test: Presubmit
Flag: EXEMPT refactor/bug fix
Change-Id: Iaec9079d8c888310f6d8e496b9dd9df231aff228
diff --git a/services/inputflinger/reader/EventHub.cpp b/services/inputflinger/reader/EventHub.cpp
index cc105bd..3c8b6f5 100644
--- a/services/inputflinger/reader/EventHub.cpp
+++ b/services/inputflinger/reader/EventHub.cpp
@@ -1654,10 +1654,6 @@
lightInfos(readLightsConfiguration(sysfsRootPath)),
layoutInfo(readLayoutConfiguration(sysfsRootPath)) {}
-bool EventHub::AssociatedDevice::isChanged() const {
- return AssociatedDevice(sysfsRootPath) != *this;
-}
-
std::string EventHub::AssociatedDevice::dump() const {
return StringPrintf("path=%s, numBatteries=%zu, numLight=%zu", sysfsRootPath.c_str(),
batteryInfos.size(), lightInfos.size());
@@ -2652,33 +2648,56 @@
void EventHub::sysfsNodeChanged(const std::string& sysfsNodePath) {
std::scoped_lock _l(mLock);
- // Check in opening devices
- for (auto it = mOpeningDevices.begin(); it != mOpeningDevices.end(); it++) {
- std::unique_ptr<Device>& device = *it;
- if (device->associatedDevice &&
- sysfsNodePath.find(device->associatedDevice->sysfsRootPath.string()) !=
- std::string::npos &&
- device->associatedDevice->isChanged()) {
- it = mOpeningDevices.erase(it);
- openDeviceLocked(device->path);
+ // Testing whether a sysfs node changed involves several syscalls, so use a cache to avoid
+ // testing the same node multiple times.
+ std::map<std::shared_ptr<const AssociatedDevice>, bool /*changed*/> testedDevices;
+ auto isAssociatedDeviceChanged = [&testedDevices, &sysfsNodePath](const Device& dev) {
+ if (!dev.associatedDevice) {
+ return false;
+ }
+ if (auto testedIt = testedDevices.find(dev.associatedDevice);
+ testedIt != testedDevices.end()) {
+ return testedIt->second;
+ }
+ // Cache miss
+ if (sysfsNodePath.find(dev.associatedDevice->sysfsRootPath.string()) == std::string::npos) {
+ testedDevices.emplace(dev.associatedDevice, false);
+ return false;
+ }
+ auto reloadedDevice = AssociatedDevice(dev.associatedDevice->sysfsRootPath);
+ const bool changed = *dev.associatedDevice != reloadedDevice;
+ testedDevices.emplace(dev.associatedDevice, changed);
+ return changed;
+ };
+
+ std::set<Device*> devicesToClose;
+ std::set<std::string /*path*/> devicesToOpen;
+
+ // Check in opening devices. If its associated device changed,
+ // the device should be removed from mOpeningDevices and needs to be opened again.
+ std::erase_if(mOpeningDevices, [&](const auto& dev) {
+ if (isAssociatedDeviceChanged(*dev)) {
+ devicesToOpen.emplace(dev->path);
+ return true;
+ }
+ return false;
+ });
+
+ // Check in already added device. If its associated device changed,
+ // the device needs to be re-opened.
+ for (const auto& [id, dev] : mDevices) {
+ if (isAssociatedDeviceChanged(*dev)) {
+ devicesToOpen.emplace(dev->path);
+ devicesToClose.emplace(dev.get());
}
}
- // Check in already added device
- std::vector<Device*> devicesToReopen;
- for (const auto& [id, device] : mDevices) {
- if (device->associatedDevice &&
- sysfsNodePath.find(device->associatedDevice->sysfsRootPath.string()) !=
- std::string::npos &&
- device->associatedDevice->isChanged()) {
- devicesToReopen.push_back(device.get());
- }
- }
- for (const auto& device : devicesToReopen) {
+ for (auto* device : devicesToClose) {
closeDeviceLocked(*device);
- openDeviceLocked(device->path);
}
- devicesToReopen.clear();
+ for (const auto& path : devicesToOpen) {
+ openDeviceLocked(path);
+ }
}
void EventHub::createVirtualKeyboardLocked() {
diff --git a/services/inputflinger/reader/include/EventHub.h b/services/inputflinger/reader/include/EventHub.h
index 8109dae..31ac63f 100644
--- a/services/inputflinger/reader/include/EventHub.h
+++ b/services/inputflinger/reader/include/EventHub.h
@@ -626,7 +626,6 @@
std::unordered_map<int32_t /*lightId*/, RawLightInfo> lightInfos;
std::optional<RawLayoutInfo> layoutInfo;
- bool isChanged() const;
bool operator==(const AssociatedDevice&) const = default;
bool operator!=(const AssociatedDevice&) const = default;
std::string dump() const;