From bd603f76a0008cb5943a649af8a59785be21fa25 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Mon, 6 Apr 2020 14:21:55 -0700 Subject: Harder failures for lazy service clients. Detect when hwservicemanager state is inconsistent at runtime. Bug: 140310064 Test: hidl_lazy_test Change-Id: I891368d7c4ef346b521c81a77e02214df50a5511 --- transport/HidlLazyUtils.cpp | 65 ++++++++++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 22 deletions(-) (limited to 'transport/HidlLazyUtils.cpp') diff --git a/transport/HidlLazyUtils.cpp b/transport/HidlLazyUtils.cpp index 08ed676..b943c99 100644 --- a/transport/HidlLazyUtils.cpp +++ b/transport/HidlLazyUtils.cpp @@ -29,15 +29,27 @@ namespace details { using ::android::hidl::base::V1_0::IBase; class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCallback { - public: - ClientCounterCallback() : mNumConnectedServices(0) {} + public: + ClientCounterCallback() {} bool addRegisteredService(const sp& service, const std::string& name); - protected: + protected: Return onClients(const sp& service, bool clients) override; - private: + private: + struct Service { + sp service; + std::string name; + bool clients = false; + }; + + /** + * Looks up service that is guaranteed to be registered (service from + * onClients). + */ + Service& assertRegisteredService(const sp& service); + /** * Registers or re-registers services. Returns whether successful. */ @@ -49,15 +61,6 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall */ void tryShutdown(); - /** - * Counter of the number of services that currently have at least one client. - */ - size_t mNumConnectedServices; - - struct Service { - sp service; - std::string name; - }; /** * Number of services that have been registered. */ @@ -65,13 +68,13 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall }; class LazyServiceRegistrarImpl { - public: + public: LazyServiceRegistrarImpl() : mClientCallback(new ClientCounterCallback) {} status_t registerService(const sp<::android::hidl::base::V1_0::IBase>& service, const std::string& name); - private: + private: sp mClientCallback; }; @@ -86,6 +89,17 @@ bool ClientCounterCallback::addRegisteredService(const sp& service, return success; } +ClientCounterCallback::Service& ClientCounterCallback::assertRegisteredService( + const sp& service) { + for (Service& registered : mRegisteredServices) { + if (registered.service != service) continue; + return registered; + } + LOG(FATAL) << "Got callback on service " << getDescriptor(service.get()) + << " which we did not register."; + __builtin_unreachable(); +} + bool ClientCounterCallback::registerService(const sp& service, const std::string& name) { auto manager = hardware::defaultServiceManager1_2(); @@ -114,17 +128,24 @@ bool ClientCounterCallback::registerService(const sp& service, const std: */ Return ClientCounterCallback::onClients(const sp<::android::hidl::base::V1_0::IBase>& service, bool clients) { - if (clients) { - mNumConnectedServices++; - } else { - mNumConnectedServices--; + Service& registered = assertRegisteredService(service); + if (registered.clients == clients) { + LOG(FATAL) << "Process already thought " << getDescriptor(service.get()) << "/" + << registered.name << " had clients: " << registered.clients + << " but hwservicemanager has notified has clients: " << clients; + } + registered.clients = clients; + + size_t numWithClients = 0; + for (const Service& registered : mRegisteredServices) { + if (registered.clients) numWithClients++; } - LOG(INFO) << "Process has " << mNumConnectedServices << " (of " << mRegisteredServices.size() + LOG(INFO) << "Process has " << numWithClients << " (of " << mRegisteredServices.size() << " available) client(s) in use after notification " << getDescriptor(service.get()) - << " has clients: " << clients; + << "/" << registered.name << " has clients: " << clients; - if (mNumConnectedServices == 0) { + if (numWithClients == 0) { tryShutdown(); } -- cgit v1.2.3 From 365a0f99089dd155a0aa864590e6f9194a3cd83e Mon Sep 17 00:00:00 2001 From: Peter Kalauskas Date: Wed, 10 Jun 2020 11:43:01 -0700 Subject: Add LOG_TAG to HidlLazyUtils Set LOG_TAG in HidlLazyUtils to make it easier to debug. Previously HidlLazyUtils inherited log tag of HAL, making it difficult to know if HAL was using HidlLazyUtils or its own onClients callback. This matches behavior of AidlLazyServiceRegistrar, which also sets LOG_TAG. Bug: 157451814 Test: atest hidl_lazy_test, logcat -s 'HidlLazyUtils:*' Change-Id: I1340fa18b5352da39e54dee786ff48a32c3c39a1 --- transport/HidlLazyUtils.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'transport/HidlLazyUtils.cpp') diff --git a/transport/HidlLazyUtils.cpp b/transport/HidlLazyUtils.cpp index b943c99..d00b461 100644 --- a/transport/HidlLazyUtils.cpp +++ b/transport/HidlLazyUtils.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#define LOG_TAG "HidlLazyUtils" + #include #include -- cgit v1.2.3 From 57341bb19f7782cefb3f762bad6601fbd635df5e Mon Sep 17 00:00:00 2001 From: Amos Bianchi Date: Wed, 23 Dec 2020 11:11:37 -0800 Subject: Add active services count callback to lazy HALs. Additionally, add methods to tryUnregister/reRegister services. Bug: 176240491 Test: atest hidl_lazy_test Change-Id: I9e67315e1308d0ce7370bf7d293d74f9c081a3c4 --- transport/HidlLazyUtils.cpp | 105 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 88 insertions(+), 17 deletions(-) (limited to 'transport/HidlLazyUtils.cpp') diff --git a/transport/HidlLazyUtils.cpp b/transport/HidlLazyUtils.cpp index d00b461..ce37dd0 100644 --- a/transport/HidlLazyUtils.cpp +++ b/transport/HidlLazyUtils.cpp @@ -36,6 +36,13 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall bool addRegisteredService(const sp& service, const std::string& name); + bool tryUnregister(); + + void reRegister(); + + void setActiveServicesCountCallback( + const std::function& activeServicesCountCallback); + protected: Return onClients(const sp& service, bool clients) override; @@ -44,6 +51,8 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall sp service; std::string name; bool clients = false; + // Used to keep track of unregistered services to allow re-registry + bool registered = true; }; /** @@ -67,6 +76,11 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall * Number of services that have been registered. */ std::vector mRegisteredServices; + + /** + * Callback for reporting the number of services with clients. + */ + std::function mActiveServicesCountCallback; }; class LazyServiceRegistrarImpl { @@ -75,6 +89,10 @@ class LazyServiceRegistrarImpl { status_t registerService(const sp<::android::hidl::base::V1_0::IBase>& service, const std::string& name); + bool tryUnregister(); + void reRegister(); + void setActiveServicesCountCallback( + const std::function& activeServicesCountCallback); private: sp mClientCallback; @@ -147,48 +165,75 @@ Return ClientCounterCallback::onClients(const sp<::android::hidl::base::V1 << " available) client(s) in use after notification " << getDescriptor(service.get()) << "/" << registered.name << " has clients: " << clients; - if (numWithClients == 0) { + bool handledInCallback = false; + if (mActiveServicesCountCallback != nullptr) { + handledInCallback = mActiveServicesCountCallback(numWithClients); + } + + // If there is no callback defined or the callback did not handle this + // client count change event, try to shutdown the process if its services + // have no clients. + if (!handledInCallback && numWithClients == 0) { tryShutdown(); } return Status::ok(); } -void ClientCounterCallback::tryShutdown() { - LOG(INFO) << "Trying to exit HAL. No clients in use for any service in process."; - +bool ClientCounterCallback::tryUnregister() { auto manager = hardware::defaultServiceManager1_2(); - auto unRegisterIt = mRegisteredServices.begin(); - for (; unRegisterIt != mRegisteredServices.end(); ++unRegisterIt) { - auto& entry = (*unRegisterIt); - + for (Service& entry : mRegisteredServices) { const std::string descriptor = getDescriptor(entry.service.get()); bool success = manager->tryUnregister(descriptor, entry.name, entry.service); if (!success) { LOG(INFO) << "Failed to unregister HAL " << descriptor << "/" << entry.name; - break; + return false; } - } - if (unRegisterIt == mRegisteredServices.end()) { - LOG(INFO) << "Unregistered all clients and exiting"; - exit(EXIT_SUCCESS); + // Mark the entry unregistered, but do not remove it (may still be re-registered) + entry.registered = false; } - for (auto reRegisterIt = mRegisteredServices.begin(); reRegisterIt != unRegisterIt; - reRegisterIt++) { - auto& entry = (*reRegisterIt); + return true; +} + +void ClientCounterCallback::reRegister() { + for (Service& entry : mRegisteredServices) { + // re-register entry if not already registered + if (entry.registered) { + continue; + } - // re-register entry if (!registerService(entry.service, entry.name)) { // Must restart. Otherwise, clients will never be able to get ahold of this service. LOG(FATAL) << "Bad state: could not re-register " << getDescriptor(entry.service.get()); } + + entry.registered = true; } } +void ClientCounterCallback::tryShutdown() { + LOG(INFO) << "Trying to exit HAL. No clients in use for any service in process."; + + if (tryUnregister()) { + LOG(INFO) << "Unregistered all clients and exiting"; + exit(EXIT_SUCCESS); + } + + // At this point, we failed to unregister some of the services, leaving the + // server in an inconsistent state. Re-register all services that were + // unregistered by tryUnregister(). + reRegister(); +} + +void ClientCounterCallback::setActiveServicesCountCallback( + const std::function& activeServicesCountCallback) { + mActiveServicesCountCallback = activeServicesCountCallback; +} + status_t LazyServiceRegistrarImpl::registerService( const sp<::android::hidl::base::V1_0::IBase>& service, const std::string& name) { if (!mClientCallback->addRegisteredService(service, name)) { @@ -198,6 +243,19 @@ status_t LazyServiceRegistrarImpl::registerService( return ::android::OK; } +bool LazyServiceRegistrarImpl::tryUnregister() { + return mClientCallback->tryUnregister(); +} + +void LazyServiceRegistrarImpl::reRegister() { + mClientCallback->reRegister(); +} + +void LazyServiceRegistrarImpl::setActiveServicesCountCallback( + const std::function& activeServicesCountCallback) { + mClientCallback->setActiveServicesCountCallback(activeServicesCountCallback); +} + } // namespace details LazyServiceRegistrar::LazyServiceRegistrar() { @@ -214,5 +272,18 @@ status_t LazyServiceRegistrar::registerService( return mImpl->registerService(service, name); } +bool LazyServiceRegistrar::tryUnregister() { + return mImpl->tryUnregister(); +} + +void LazyServiceRegistrar::reRegister() { + mImpl->reRegister(); +} + +void LazyServiceRegistrar::setActiveServicesCountCallback( + const std::function& activeServicesCountCallback) { + mImpl->setActiveServicesCountCallback(activeServicesCountCallback); +} + } // namespace hardware } // namespace android -- cgit v1.2.3 From fb9186324e1987ab59779bddc54266b0acdcf676 Mon Sep 17 00:00:00 2001 From: Amos Bianchi Date: Wed, 20 Jan 2021 17:41:02 -0800 Subject: Change callback argument to boolean. Bug: 176240491 Test: atest hidl_lazy_test Change-Id: If909f292288ecf0d521b0eaf93f5517fafaf3969 --- transport/HidlLazyUtils.cpp | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) (limited to 'transport/HidlLazyUtils.cpp') diff --git a/transport/HidlLazyUtils.cpp b/transport/HidlLazyUtils.cpp index ce37dd0..be7470f 100644 --- a/transport/HidlLazyUtils.cpp +++ b/transport/HidlLazyUtils.cpp @@ -40,8 +40,7 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall void reRegister(); - void setActiveServicesCountCallback( - const std::function& activeServicesCountCallback); + void setActiveServicesCallback(const std::function& activeServicesCallback); protected: Return onClients(const sp& service, bool clients) override; @@ -80,7 +79,12 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall /** * Callback for reporting the number of services with clients. */ - std::function mActiveServicesCountCallback; + std::function mActiveServicesCallback; + + /** + * Previous value passed to the active services callback. + */ + std::optional mPreviousHasClients; }; class LazyServiceRegistrarImpl { @@ -91,8 +95,7 @@ class LazyServiceRegistrarImpl { const std::string& name); bool tryUnregister(); void reRegister(); - void setActiveServicesCountCallback( - const std::function& activeServicesCountCallback); + void setActiveServicesCallback(const std::function& activeServicesCallback); private: sp mClientCallback; @@ -166,8 +169,12 @@ Return ClientCounterCallback::onClients(const sp<::android::hidl::base::V1 << "/" << registered.name << " has clients: " << clients; bool handledInCallback = false; - if (mActiveServicesCountCallback != nullptr) { - handledInCallback = mActiveServicesCountCallback(numWithClients); + if (mActiveServicesCallback != nullptr) { + bool hasClients = numWithClients != 0; + if (hasClients != mPreviousHasClients) { + handledInCallback = mActiveServicesCallback(hasClients); + mPreviousHasClients = hasClients; + } } // If there is no callback defined or the callback did not handle this @@ -229,9 +236,9 @@ void ClientCounterCallback::tryShutdown() { reRegister(); } -void ClientCounterCallback::setActiveServicesCountCallback( - const std::function& activeServicesCountCallback) { - mActiveServicesCountCallback = activeServicesCountCallback; +void ClientCounterCallback::setActiveServicesCallback( + const std::function& activeServicesCallback) { + mActiveServicesCallback = activeServicesCallback; } status_t LazyServiceRegistrarImpl::registerService( @@ -251,9 +258,9 @@ void LazyServiceRegistrarImpl::reRegister() { mClientCallback->reRegister(); } -void LazyServiceRegistrarImpl::setActiveServicesCountCallback( - const std::function& activeServicesCountCallback) { - mClientCallback->setActiveServicesCountCallback(activeServicesCountCallback); +void LazyServiceRegistrarImpl::setActiveServicesCallback( + const std::function& activeServicesCallback) { + mClientCallback->setActiveServicesCallback(activeServicesCallback); } } // namespace details @@ -280,9 +287,9 @@ void LazyServiceRegistrar::reRegister() { mImpl->reRegister(); } -void LazyServiceRegistrar::setActiveServicesCountCallback( - const std::function& activeServicesCountCallback) { - mImpl->setActiveServicesCountCallback(activeServicesCountCallback); +void LazyServiceRegistrar::setActiveServicesCallback( + const std::function& activeServicesCallback) { + mImpl->setActiveServicesCallback(activeServicesCallback); } } // namespace hardware -- cgit v1.2.3