From 2e1a2316cfd175941902d8282d11cad8e465f9ec Mon Sep 17 00:00:00 2001 From: Chris Manton Date: Tue, 21 Jan 2020 11:34:00 -0800 Subject: [PATCH] gd: Fix for l2cap connection on failure Included method and parameter naming updates to reflect functionality Bug: 140778599 Test: bt_headless with failing params Change-Id: I5ed4e2769f7c8f6f78a92af76545e3a0e41582d1 --- gd/shim/il2cap.h | 12 ++++---- gd/shim/l2cap.cc | 82 +++++++++++++++++++++++++++---------------------- gd/shim/l2cap.h | 8 ++--- main/shim/l2cap.cc | 11 +++---- main/shim/l2cap.h | 3 +- main/shim/l2cap_test.cc | 15 ++++++--- main/shim/stub/stack.cc | 12 ++++---- main/shim/stub/stack.h | 8 ++--- 8 files changed, 82 insertions(+), 69 deletions(-) diff --git a/gd/shim/il2cap.h b/gd/shim/il2cap.h index 933a3a792..8761957f9 100644 --- a/gd/shim/il2cap.h +++ b/gd/shim/il2cap.h @@ -28,17 +28,17 @@ namespace bluetooth { namespace shim { using ConnectionClosedCallback = std::function; -using ConnectionOpenCallback = std::function; -using ConnectionFailedCallback = ConnectionOpenCallback; +using ConnectionCompleteCallback = + std::function; using ReadDataReadyCallback = std::function data)>; struct IL2cap { - virtual void RegisterService(uint16_t psm, bool use_ertm, uint16_t mtu, ConnectionOpenCallback on_open, - std::promise completed) = 0; + virtual void RegisterService(uint16_t psm, bool use_ertm, uint16_t mtu, ConnectionCompleteCallback on_complete, + std::promise registered) = 0; virtual void UnregisterService(uint16_t psm) = 0; - virtual void CreateConnection(uint16_t psm, const std::string address, ConnectionOpenCallback on_open, - std::promise completed) = 0; + virtual void CreateConnection(uint16_t psm, const std::string address, ConnectionCompleteCallback on_complete, + std::promise created) = 0; virtual void CloseConnection(uint16_t cid) = 0; virtual void SetReadDataReadyCallback(uint16_t cid, ReadDataReadyCallback on_data_ready) = 0; diff --git a/gd/shim/l2cap.cc b/gd/shim/l2cap.cc index 427e4e3df..9db698008 100644 --- a/gd/shim/l2cap.cc +++ b/gd/shim/l2cap.cc @@ -52,6 +52,10 @@ constexpr char kModuleName[] = "shim::L2cap"; constexpr ConnectionInterfaceDescriptor kInvalidConnectionInterfaceDescriptor = 0; constexpr ConnectionInterfaceDescriptor kStartConnectionInterfaceDescriptor = 64; constexpr ConnectionInterfaceDescriptor kMaxConnections = UINT16_MAX - kStartConnectionInterfaceDescriptor - 1; + +constexpr bool kConnectionFailed = false; +constexpr bool kConnectionOpened = true; + } // namespace using ServiceInterfaceCallback = @@ -180,22 +184,23 @@ struct ConnectionInterfaceManager { return cid_to_interface_map_.size(); } - void OnConnectionChanged(ConnectionOpenCallback on_open, hci::Address address, l2cap::Psm psm, - ConnectionInterfaceDescriptor cid) { - on_open(address.ToString(), static_cast(psm), static_cast(cid)); + void OnConnectionChanged(ConnectionCompleteCallback on_complete, hci::Address address, l2cap::Psm psm, + ConnectionInterfaceDescriptor cid, uint16_t status) { + on_complete(address.ToString(), static_cast(psm), static_cast(cid), status); } - void ConnectionOpened(ConnectionOpenCallback on_open, l2cap::Psm psm, ConnectionInterfaceDescriptor cid) { + void ConnectionOpened(ConnectionCompleteCallback on_complete, l2cap::Psm psm, ConnectionInterfaceDescriptor cid) { hci::Address address = cid_to_interface_map_[cid]->GetRemoteAddress(); LOG_DEBUG("Connection opened address:%s psm:%hd cid:%hd", address.ToString().c_str(), psm, cid); - handler_->Post(common::BindOnce(&ConnectionInterfaceManager::OnConnectionChanged, common::Unretained(this), on_open, - address, psm, cid)); + handler_->Post(common::BindOnce(&ConnectionInterfaceManager::OnConnectionChanged, common::Unretained(this), + on_complete, address, psm, cid, kConnectionOpened)); } - void ConnectionFailed(ConnectionFailedCallback on_failed, hci::Address address, l2cap::Psm psm) { + void ConnectionFailed(ConnectionCompleteCallback on_complete, hci::Address address, l2cap::Psm psm, + ConnectionInterfaceDescriptor cid) { LOG_DEBUG("Connection failed address:%s psm:%hd", address.ToString().c_str(), psm); handler_->Post(common::BindOnce(&ConnectionInterfaceManager::OnConnectionChanged, common::Unretained(this), - std::move(on_failed), address, psm, kInvalidConnectionInterfaceDescriptor)); + std::move(on_complete), address, psm, cid, kConnectionFailed)); } ConnectionInterfaceManager(os::Handler* handler); @@ -287,16 +292,16 @@ bool ConnectionInterfaceManager::Write(ConnectionInterfaceDescriptor cid, std::u class PendingConnection { public: PendingConnection(ConnectionInterfaceManager* connection_interface_manager, ConnectionInterfaceDescriptor cid, - l2cap::Psm psm, hci::Address address, ConnectionOpenCallback on_open, - std::promise completed, std::function deleter) + l2cap::Psm psm, hci::Address address, ConnectionCompleteCallback on_complete, + std::function deleter) : connection_interface_manager_(connection_interface_manager), cid_(cid), psm_(psm), address_(address), - on_open_(std::move(on_open)), completed_(std::move(completed)), deleter_(deleter) {} + on_complete_(std::move(on_complete)), deleter_(deleter) {} void OnConnectionOpen(std::unique_ptr channel) { LOG_DEBUG("Local initiated connection is open to device:%s for psm:%hd", address_.ToString().c_str(), psm_); ASSERT_LOG(address_ == channel->GetDevice(), " Expected remote device does not match actual remote device"); connection_interface_manager_->AddConnection(cid_, std::move(channel)); - connection_interface_manager_->ConnectionOpened(std::move(on_open_), psm_, cid_); + connection_interface_manager_->ConnectionOpened(std::move(on_complete_), psm_, cid_); deleter_(); } @@ -318,8 +323,7 @@ class PendingConnection { l2cap::ConnectionResponseResultText(result.l2cap_connection_response_result).c_str()); break; } - completed_.set_value(kInvalidConnectionInterfaceDescriptor); - connection_interface_manager_->ConnectionFailed(std::move(on_open_), address_, psm_); + connection_interface_manager_->ConnectionFailed(std::move(on_complete_), address_, psm_, cid_); connection_interface_manager_->FreeConnectionInterfaceDescriptor(cid_); deleter_(); } @@ -329,7 +333,7 @@ class PendingConnection { const ConnectionInterfaceDescriptor cid_; const l2cap::Psm psm_; const hci::Address address_; - ConnectionOpenCallback on_open_; + ConnectionCompleteCallback on_complete_; std::promise completed_; std::function deleter_; }; @@ -337,17 +341,17 @@ class PendingConnection { class ServiceInterface { public: ServiceInterface(ConnectionInterfaceManager* connection_interface_manager, l2cap::Psm psm, - ConnectionOpenCallback on_open, std::promise completed) - : connection_interface_manager_(connection_interface_manager), psm_(psm), on_open_(on_open), - completed_(std::move(completed)) {} + ConnectionCompleteCallback on_complete, std::promise registered) + : connection_interface_manager_(connection_interface_manager), psm_(psm), on_complete_(on_complete), + registered_(std::move(registered)) {} void OnRegistrationComplete(l2cap::classic::DynamicChannelManager::RegistrationResult result, std::unique_ptr service) { ASSERT(service_ == nullptr); ASSERT(psm_ == service->GetPsm()); - LOG_DEBUG("Registration is complete for psm:%hd", psm_); + LOG_DEBUG("Service is registered for psm:%hd", psm_); service_ = std::move(service); - completed_.set_value(); + registered_.set_value(); } void OnConnectionOpen(std::unique_ptr channel) { @@ -355,7 +359,7 @@ class ServiceInterface { psm_); ConnectionInterfaceDescriptor cid = connection_interface_manager_->AllocateConnectionInterfaceDescriptor(); connection_interface_manager_->AddConnection(cid, std::move(channel)); - connection_interface_manager_->ConnectionOpened(std::move(on_open_), psm_, cid); + connection_interface_manager_->ConnectionOpened(on_complete_, psm_, cid); } l2cap::SecurityPolicy GetSecurityPolicy() const { @@ -374,8 +378,8 @@ class ServiceInterface { private: ConnectionInterfaceManager* connection_interface_manager_; const l2cap::Psm psm_; - ConnectionOpenCallback on_open_; - std::promise completed_; + ConnectionCompleteCallback on_complete_; + std::promise registered_; std::unique_ptr service_; @@ -384,11 +388,11 @@ class ServiceInterface { struct L2cap::impl { void RegisterService(l2cap::Psm psm, l2cap::classic::DynamicChannelConfigurationOption option, - ConnectionOpenCallback on_open, std::promise completed); + ConnectionCompleteCallback on_complete, std::promise registered); void UnregisterService(l2cap::Psm psm); - void CreateConnection(l2cap::Psm psm, hci::Address address, ConnectionOpenCallback on_open, - std::promise completed); + void CreateConnection(l2cap::Psm psm, hci::Address address, ConnectionCompleteCallback on_complete, + std::promise created); void CloseConnection(ConnectionInterfaceDescriptor cid); void SetReadDataReadyCallback(ConnectionInterfaceDescriptor cid, ReadDataReadyCallback on_data_ready); @@ -445,11 +449,11 @@ void L2cap::impl::Dump(int fd) { } void L2cap::impl::RegisterService(l2cap::Psm psm, l2cap::classic::DynamicChannelConfigurationOption option, - ConnectionOpenCallback on_open, std::promise completed) { + ConnectionCompleteCallback on_complete, std::promise registered) { ASSERT(psm_to_service_interface_map_.find(psm) == psm_to_service_interface_map_.end()); auto service_interface = - std::make_shared(&connection_interface_manager_, psm, on_open, std::move(completed)); + std::make_shared(&connection_interface_manager_, psm, on_complete, std::move(registered)); psm_to_service_interface_map_.emplace(psm, service_interface); service_interface->RegisterService( @@ -466,14 +470,18 @@ void L2cap::impl::UnregisterService(l2cap::Psm psm) { psm_to_service_interface_map_.erase(psm); } -void L2cap::impl::CreateConnection(l2cap::Psm psm, hci::Address address, ConnectionOpenCallback on_open, - std::promise completed) { +void L2cap::impl::CreateConnection(l2cap::Psm psm, hci::Address address, ConnectionCompleteCallback on_complete, + std::promise created) { ConnectionInterfaceDescriptor cid = connection_interface_manager_.AllocateConnectionInterfaceDescriptor(); - completed.set_value(cid); + created.set_value(cid); + + if (cid == kInvalidConnectionInterfaceDescriptor) { + LOG_WARN("No resources to create a connection"); + return; + } auto pending_connection = std::make_shared( - &connection_interface_manager_, cid, psm, address, std::move(on_open), std::move(completed), - [this, address, psm]() { + &connection_interface_manager_, cid, psm, address, std::move(on_complete), [this, address, psm]() { ASSERT(!endpoint_to_pending_connection_map_[HashEndpoint(address, psm)].empty()); endpoint_to_pending_connection_map_[HashEndpoint(address, psm)].pop_front(); if (endpoint_to_pending_connection_map_[HashEndpoint(address, psm)].empty()) { @@ -511,7 +519,7 @@ void L2cap::impl::SendLoopbackResponse(std::function function) { function(); } -void L2cap::RegisterService(uint16_t raw_psm, bool use_ertm, uint16_t mtu, ConnectionOpenCallback on_open, +void L2cap::RegisterService(uint16_t raw_psm, bool use_ertm, uint16_t mtu, ConnectionCompleteCallback on_complete, std::promise completed) { l2cap::Psm psm{raw_psm}; l2cap::classic::DynamicChannelConfigurationOption option; @@ -521,7 +529,7 @@ void L2cap::RegisterService(uint16_t raw_psm, bool use_ertm, uint16_t mtu, Conne } option.incoming_mtu = mtu; GetHandler()->Post(common::BindOnce(&L2cap::impl::RegisterService, common::Unretained(pimpl_.get()), psm, option, - on_open, std::move(completed))); + on_complete, std::move(completed))); } void L2cap::UnregisterService(uint16_t raw_psm) { @@ -529,14 +537,14 @@ void L2cap::UnregisterService(uint16_t raw_psm) { GetHandler()->Post(common::Bind(&L2cap::impl::UnregisterService, common::Unretained(pimpl_.get()), psm)); } -void L2cap::CreateConnection(uint16_t raw_psm, const std::string address_string, ConnectionOpenCallback on_open, +void L2cap::CreateConnection(uint16_t raw_psm, const std::string address_string, ConnectionCompleteCallback on_complete, std::promise completed) { l2cap::Psm psm{raw_psm}; hci::Address address; hci::Address::FromString(address_string, address); GetHandler()->Post(common::BindOnce(&L2cap::impl::CreateConnection, common::Unretained(pimpl_.get()), psm, address, - on_open, std::move(completed))); + on_complete, std::move(completed))); } void L2cap::CloseConnection(uint16_t raw_cid) { diff --git a/gd/shim/l2cap.h b/gd/shim/l2cap.h index 552d487bd..5f0c7014b 100644 --- a/gd/shim/l2cap.h +++ b/gd/shim/l2cap.h @@ -29,12 +29,12 @@ namespace shim { class L2cap : public bluetooth::Module, public bluetooth::shim::IL2cap { public: - void RegisterService(uint16_t psm, bool use_ertm, uint16_t mtu, ConnectionOpenCallback on_open, - std::promise completed) override; + void RegisterService(uint16_t psm, bool use_ertm, uint16_t mtu, ConnectionCompleteCallback on_complete, + std::promise registered) override; void UnregisterService(uint16_t psm) override; - void CreateConnection(uint16_t psm, const std::string address_string, ConnectionOpenCallback on_open, - std::promise completed) override; + void CreateConnection(uint16_t psm, const std::string address_string, ConnectionCompleteCallback on_complete, + std::promise created) override; void CloseConnection(uint16_t cid) override; void SetReadDataReadyCallback(uint16_t cid, ReadDataReadyCallback on_data_ready) override; diff --git a/main/shim/l2cap.cc b/main/shim/l2cap.cc index 575e317c8..e2396fcb7 100644 --- a/main/shim/l2cap.cc +++ b/main/shim/l2cap.cc @@ -244,7 +244,7 @@ uint16_t bluetooth::shim::legacy::L2cap::CreateConnection( std::bind( &bluetooth::shim::legacy::L2cap::OnLocalInitiatedConnectionCreated, this, std::placeholders::_1, std::placeholders::_2, - std::placeholders::_3), + std::placeholders::_3, std::placeholders::_4), std::move(connect_completed)); uint16_t cid = completed.get(); @@ -264,17 +264,16 @@ uint16_t bluetooth::shim::legacy::L2cap::CreateConnection( } void bluetooth::shim::legacy::L2cap::OnLocalInitiatedConnectionCreated( - std::string string_address, uint16_t psm, uint16_t cid) { + std::string string_address, uint16_t psm, uint16_t cid, bool connected) { LOG_DEBUG(LOG_TAG, "Sending connection confirm to the upper stack but really " "a connection to %s has already been done cid:%hd", string_address.c_str(), cid); - uint16_t status = kConnectionFail; - if (cid != kInvalidConnectionInterfaceDescriptor) { + if (connected) { SetDownstreamCallbacks(cid); - status = kConnectionSuccess; } - Classic().Callbacks(psm)->pL2CA_ConnectCfm_Cb(cid, status); + Classic().Callbacks(psm)->pL2CA_ConnectCfm_Cb( + cid, connected ? (kConnectionSuccess) : (kConnectionFail)); } bool bluetooth::shim::legacy::L2cap::Write(uint16_t cid, BT_HDR* bt_hdr) { diff --git a/main/shim/l2cap.h b/main/shim/l2cap.h index 65b2759f8..a57f5bac7 100644 --- a/main/shim/l2cap.h +++ b/main/shim/l2cap.h @@ -68,7 +68,8 @@ class L2cap { bool Write(uint16_t cid, BT_HDR* bt_hdr); void OnLocalInitiatedConnectionCreated(std::string string_address, - uint16_t psm, uint16_t cid); + uint16_t psm, uint16_t cid, + bool connected); void OnRemoteInitiatedConnectionCreated(std::string string_addresss, uint16_t psm, uint16_t cid); diff --git a/main/shim/l2cap_test.cc b/main/shim/l2cap_test.cc index 7708ae2ae..3db605ab6 100644 --- a/main/shim/l2cap_test.cc +++ b/main/shim/l2cap_test.cc @@ -227,7 +227,8 @@ TEST_F(L2capTest, CreateConnection_ConfigRequest) { CHECK(cid != 0); // Simulate a successful connection response - l2cap_->OnLocalInitiatedConnectionCreated("11:22:33:44:55:66", kPsm, kCid); + l2cap_->OnLocalInitiatedConnectionCreated("11:22:33:44:55:66", kPsm, kCid, + true); CHECK(cnt_.L2caConnectCfmCb == 1); CHECK(l2cap_->ConfigRequest(cid, nullptr)); @@ -244,7 +245,8 @@ TEST_F(L2capTest, CreateConnection_ConfigResponse) { CHECK(cid != 0); // Simulate a successful connection response - l2cap_->OnLocalInitiatedConnectionCreated("11:22:33:44:55:66", kPsm, kCid); + l2cap_->OnLocalInitiatedConnectionCreated("11:22:33:44:55:66", kPsm, kCid, + true); CHECK(cnt_.L2caConnectCfmCb == 1); CHECK(l2cap_->ConfigResponse(cid, nullptr)); @@ -261,7 +263,8 @@ TEST_F(L2capTest, CreateConnection_DisconnectRequest) { CHECK(cid != 0); // Simulate a successful connection response - l2cap_->OnLocalInitiatedConnectionCreated("11:22:33:44:55:66", kPsm, kCid); + l2cap_->OnLocalInitiatedConnectionCreated("11:22:33:44:55:66", kPsm, kCid, + true); CHECK(cnt_.L2caConnectCfmCb == 1); CHECK(l2cap_->DisconnectRequest(cid)); @@ -278,7 +281,8 @@ TEST_F(L2capTest, CreateConnection_DisconnectResponse) { CHECK(cid != 0); // Simulate a successful connection response - l2cap_->OnLocalInitiatedConnectionCreated("11:22:33:44:55:66", kPsm, kCid); + l2cap_->OnLocalInitiatedConnectionCreated("11:22:33:44:55:66", kPsm, kCid, + true); CHECK(cnt_.L2caConnectCfmCb == 1); CHECK(l2cap_->DisconnectResponse(cid)); @@ -295,7 +299,8 @@ TEST_F(L2capTest, CreateConnection_WithHandshake) { CHECK(cid != 0); // Simulate a successful connection response - l2cap_->OnLocalInitiatedConnectionCreated("11:22:33:44:55:66", kPsm, kCid); + l2cap_->OnLocalInitiatedConnectionCreated("11:22:33:44:55:66", kPsm, kCid, + true); CHECK(cnt_.L2caConnectCfmCb == 1); CHECK(l2cap_->ConfigRequest(cid, nullptr) == true); diff --git a/main/shim/stub/stack.cc b/main/shim/stub/stack.cc index 73bbd6ce0..c82abab42 100644 --- a/main/shim/stub/stack.cc +++ b/main/shim/stub/stack.cc @@ -36,9 +36,9 @@ bluetooth::shim::IStack* bluetooth::shim::GetGabeldorscheStack() { void TestGdShimL2cap::RegisterService( uint16_t psm, bool use_ertm, uint16_t mtu, - bluetooth::shim::ConnectionOpenCallback on_open, - std::promise completed) { - completed.set_value(); + bluetooth::shim::ConnectionCompleteCallback on_complete, + std::promise registered) { + registered.set_value(); registered_service_.insert(psm); } @@ -48,9 +48,9 @@ void TestGdShimL2cap::UnregisterService(uint16_t psm) { void TestGdShimL2cap::CreateConnection( uint16_t psm, const std::string address, - bluetooth::shim::ConnectionOpenCallback on_open, - std::promise completed) { - completed.set_value(cid_); + bluetooth::shim::ConnectionCompleteCallback on_complete, + std::promise created) { + created.set_value(cid_); } void TestGdShimL2cap::CloseConnection(uint16_t cid) {} diff --git a/main/shim/stub/stack.h b/main/shim/stub/stack.h index a2672cce2..ae9a126e8 100644 --- a/main/shim/stub/stack.h +++ b/main/shim/stub/stack.h @@ -29,12 +29,12 @@ class TestGdShimL2cap : public bluetooth::shim::IL2cap { std::set registered_service_; void RegisterService(uint16_t psm, bool use_ertm, uint16_t mtu, - bluetooth::shim::ConnectionOpenCallback on_open, - std::promise completed) override; + bluetooth::shim::ConnectionCompleteCallback on_complete, + std::promise registered) override; void UnregisterService(uint16_t psm); void CreateConnection(uint16_t psm, const std::string address, - bluetooth::shim::ConnectionOpenCallback on_open, - std::promise completed) override; + bluetooth::shim::ConnectionCompleteCallback on_complete, + std::promise created) override; void CloseConnection(uint16_t cid); void SetReadDataReadyCallback( uint16_t cid, -- 2.11.0