From 6f92e238b055eaefb525bd107430403add7871d3 Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Tue, 23 Sep 2025 11:54:03 -0700 Subject: [PATCH] Refactor P2pCLusterPcpHandler and WifiLan. PiperOrigin-RevId: 810525937 --- .../implementation/mediums/wifi_lan.cc | 91 +++++++++++-------- connections/implementation/mediums/wifi_lan.h | 12 ++- .../implementation/mediums/wifi_lan_test.cc | 71 ++++++--------- .../implementation/p2p_cluster_pcp_handler.cc | 37 ++------ 4 files changed, 100 insertions(+), 111 deletions(-) diff --git a/connections/implementation/mediums/wifi_lan.cc b/connections/implementation/mediums/wifi_lan.cc index 1277df5f..24bf2341 100644 --- a/connections/implementation/mediums/wifi_lan.cc +++ b/connections/implementation/mediums/wifi_lan.cc @@ -83,7 +83,8 @@ bool WifiLan::IsAvailable() const { bool WifiLan::IsAvailableLocked() const { return medium_.IsValid(); } ErrorOr WifiLan::StartAdvertising(const std::string& service_id, - NsdServiceInfo& nsd_service_info) { + NsdServiceInfo& nsd_service_info, + AcceptedConnectionCallback callback) { MutexLock lock(&mutex_); if (!IsAvailableLocked()) { @@ -103,28 +104,25 @@ ErrorOr WifiLan::StartAdvertising(const std::string& service_id, << "Failed to WifiLan advertise because we're already advertising."; return {Error(OperationResultCode::CLIENT_WIFI_LAN_DUPLICATE_ADVERTISING)}; } - - if (!IsAcceptingConnectionsLocked(service_id)) { - LOG(INFO) << "Failed to turn on WifiLan advertising with nsd_service_info=" - << &nsd_service_info - << ", service_name=" << nsd_service_info.GetServiceName() - << ", service_id=" << service_id - << ". Should accept connections before advertising."; - return {Error(OperationResultCode:: - CLIENT_DUPLICATE_ACCEPTING_LAN_CONNECTION_REQUEST)}; - } - nsd_service_info.SetServiceType(GenerateServiceType(service_id)); const auto& it = server_sockets_.find(service_id); if (it != server_sockets_.end()) { - nsd_service_info.SetIPAddress(it->second.GetIPAddress()); nsd_service_info.SetPort(it->second.GetPort()); + } else { + int port = 0; + ErrorOr port_result = StartAcceptingConnectionsLocked( + service_id, port, std::move(callback)); + if (port_result.has_error()) { + return {port_result.error()}; + } + nsd_service_info.SetPort(port_result.value()); } if (!medium_.StartAdvertising(nsd_service_info)) { LOG(INFO) << "Failed to turn on WifiLan advertising with nsd_service_info=" << &nsd_service_info << ", service_name=" << nsd_service_info.GetServiceName() << ", service_id=" << service_id; + StopAcceptingConnectionsLocked(service_id); return {Error( OperationResultCode::CONNECTIVITY_WIFI_LAN_START_ADVERTISING_FAILURE)}; } @@ -228,35 +226,12 @@ bool WifiLan::IsDiscoveringLocked(const std::string& service_id) { return discovering_info_.Existed(service_id); } -ErrorOr WifiLan::StartAcceptingConnections( - const std::string& service_id, AcceptedConnectionCallback callback) { - MutexLock lock(&mutex_); - - if (service_id.empty()) { - LOG(INFO) << "Refusing to start accepting WifiLan connections; " - "service_id is empty."; - return {Error(OperationResultCode::NEARBY_LOCAL_CLIENT_STATE_WRONG)}; - } - - if (!IsAvailableLocked()) { - LOG(INFO) << "Can't start accepting WifiLan connections [service_id=" - << service_id << "]; WifiLan not available."; - return {Error( - OperationResultCode::MEDIUM_UNAVAILABLE_WIFI_AWARE_NOT_AVAILABLE)}; - } - - if (IsAcceptingConnectionsLocked(service_id)) { - LOG(INFO) << "Refusing to start accepting WifiLan connections [service=" - << service_id - << "]; WifiLan server is already in-progress with the same name."; - return {Error(OperationResultCode:: - CLIENT_DUPLICATE_ACCEPTING_LAN_CONNECTION_REQUEST)}; - } - +ErrorOr WifiLan::StartAcceptingConnectionsLocked( + const std::string& service_id, int port, + AcceptedConnectionCallback callback) { auto port_range = medium_.GetDynamicPortRange(); // Generate an exact port here on server socket; if platform doesn't provide // range of port then assign 0 to let platform decide it. - int port = 0; if (port_range.has_value() && (port_range->first > 0 && port_range->first <= 65535 && port_range->second > 0 && port_range->second <= 65535 && @@ -289,6 +264,7 @@ ErrorOr WifiLan::StartAcceptingConnections( } }); } + port = owned_server_socket.GetPort(); // Start the accept loop on a dedicated thread - this stays alive and // listening for new incoming connections until StopAcceptingConnections() is // invoked. @@ -358,12 +334,49 @@ ErrorOr WifiLan::StartAcceptingConnections( } }); + return {port}; +} + +ErrorOr WifiLan::StartAcceptingConnections( + const std::string& service_id, AcceptedConnectionCallback callback) { + MutexLock lock(&mutex_); + + if (service_id.empty()) { + LOG(INFO) << "Refusing to start accepting WifiLan connections; " + "service_id is empty."; + return {Error(OperationResultCode::NEARBY_LOCAL_CLIENT_STATE_WRONG)}; + } + + if (!IsAvailableLocked()) { + LOG(INFO) << "Can't start accepting WifiLan connections [service_id=" + << service_id << "]; WifiLan not available."; + return {Error( + OperationResultCode::MEDIUM_UNAVAILABLE_WIFI_AWARE_NOT_AVAILABLE)}; + } + + if (IsAcceptingConnectionsLocked(service_id)) { + LOG(INFO) << "Refusing to start accepting WifiLan connections [service=" + << service_id + << "]; WifiLan server is already in-progress with the same name."; + return {Error(OperationResultCode:: + CLIENT_DUPLICATE_ACCEPTING_LAN_CONNECTION_REQUEST)}; + } + ErrorOr port = StartAcceptingConnectionsLocked(service_id, /*port=*/0, + std::move(callback)) + .has_error(); + if (port.has_error()) { + return {port.error()}; + } return {true}; } bool WifiLan::StopAcceptingConnections(const std::string& service_id) { MutexLock lock(&mutex_); + return StopAcceptingConnectionsLocked(service_id); +} + +bool WifiLan::StopAcceptingConnectionsLocked(const std::string& service_id) { if (service_id.empty()) { LOG(INFO) << "Unable to stop accepting WifiLan connections because " "the service_id is empty."; diff --git a/connections/implementation/mediums/wifi_lan.h b/connections/implementation/mediums/wifi_lan.h index 428a4e83..08f454e2 100644 --- a/connections/implementation/mediums/wifi_lan.h +++ b/connections/implementation/mediums/wifi_lan.h @@ -55,7 +55,8 @@ class WifiLan { // then enables WifiLan advertising. // Returns true, if NsdServiceInfo is successfully set, and false otherwise. ErrorOr StartAdvertising(const std::string& service_id, - NsdServiceInfo& nsd_service_info) + NsdServiceInfo& nsd_service_info, + AcceptedConnectionCallback callback) ABSL_LOCKS_EXCLUDED(mutex_); // Disables WifiLan advertising. @@ -183,6 +184,15 @@ class WifiLan { bool IsAcceptingConnectionsLocked(const std::string& service_id) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); + // Returns the port number of the server socket if successful, otherwise + // returns an error. + ErrorOr StartAcceptingConnectionsLocked( + const std::string& service_id, int port, + AcceptedConnectionCallback callback) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); + bool StopAcceptingConnectionsLocked(const std::string& service_id) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); + // Generates mDNS type. std::string GenerateServiceType(const std::string& service_id); diff --git a/connections/implementation/mediums/wifi_lan_test.cc b/connections/implementation/mediums/wifi_lan_test.cc index ae938144..e7af5823 100644 --- a/connections/implementation/mediums/wifi_lan_test.cc +++ b/connections/implementation/mediums/wifi_lan_test.cc @@ -74,17 +74,16 @@ TEST_P(WifiLanTest, CanConnect) { CountDownLatch accept_latch(1); WifiLanSocket socket_for_server; - EXPECT_TRUE(wifi_lan_server.StartAcceptingConnections( - service_id, [&](const std::string& service_id, WifiLanSocket socket) { - socket_for_server = std::move(socket); - accept_latch.CountDown(); - })); - NsdServiceInfo nsd_service_info; nsd_service_info.SetServiceName(service_info_name); nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey), endpoint_info_name); - wifi_lan_server.StartAdvertising(service_id, nsd_service_info); + wifi_lan_server.StartAdvertising( + service_id, nsd_service_info, + [&](const std::string& service_id, WifiLanSocket socket) { + socket_for_server = std::move(socket); + accept_latch.CountDown(); + }); NsdServiceInfo discovered_service_info; wifi_lan_client.StartDiscovery( @@ -138,17 +137,16 @@ TEST_P(WifiLanTest, CanConnectWithMultiplex) { CountDownLatch accept_latch(1); WifiLanSocket socket_for_server; - EXPECT_TRUE(wifi_lan_server.StartAcceptingConnections( - service_id, [&](const std::string& service_id, WifiLanSocket socket) { - socket_for_server = std::move(socket); - accept_latch.CountDown(); - })); - NsdServiceInfo nsd_service_info; nsd_service_info.SetServiceName(service_info_name); nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey), endpoint_info_name); - wifi_lan_server.StartAdvertising(service_id, nsd_service_info); + wifi_lan_server.StartAdvertising( + service_id, nsd_service_info, + [&](const std::string& service_id, WifiLanSocket socket) { + socket_for_server = std::move(socket); + accept_latch.CountDown(); + }); WifiLanSocket socket_for_client; SingleThreadExecutor client_executor; @@ -204,17 +202,16 @@ TEST_P(WifiLanTest, CanCancelConnect) { CountDownLatch accept_latch(1); WifiLanSocket socket_for_server; - EXPECT_TRUE(wifi_lan_server.StartAcceptingConnections( - service_id, [&](const std::string& service_id, WifiLanSocket socket) { - socket_for_server = std::move(socket); - accept_latch.CountDown(); - })); - NsdServiceInfo nsd_service_info; nsd_service_info.SetServiceName(service_info_name); nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey), endpoint_info_name); - wifi_lan_server.StartAdvertising(service_id, nsd_service_info); + wifi_lan_server.StartAdvertising( + service_id, nsd_service_info, + [&](const std::string& service_id, WifiLanSocket socket) { + socket_for_server = std::move(socket); + accept_latch.CountDown(); + }); NsdServiceInfo discovered_service_info; wifi_lan_client.StartDiscovery( @@ -262,16 +259,15 @@ TEST_P(WifiLanTest, CanConnectWithIpAddressAndPort) { CountDownLatch accept_latch(1); WifiLanSocket socket_for_server; - EXPECT_TRUE(wifi_lan_server.StartAcceptingConnections( - service_id, [&](const std::string& service_id, WifiLanSocket socket) { + NsdServiceInfo nsd_service_info; + nsd_service_info.SetServiceName(std::string(kServiceInfoName)); + EXPECT_TRUE(wifi_lan_server.StartAdvertising( + service_id, nsd_service_info, + [&](const std::string& service_id, WifiLanSocket socket) { socket_for_server = std::move(socket); accept_latch.CountDown(); })); - NsdServiceInfo nsd_service_info; - nsd_service_info.SetServiceName(std::string(kServiceInfoName)); - EXPECT_TRUE(wifi_lan_server.StartAdvertising(service_id, nsd_service_info)); - auto server_credentials = wifi_lan_server.GetCredentials(service_id); ASSERT_FALSE(server_credentials.first.empty()); ASSERT_NE(server_credentials.second, 0); @@ -309,13 +305,11 @@ TEST_F(WifiLanTest, CanStartAdvertising) { std::string service_info_name(kServiceInfoName); std::string endpoint_info_name(kEndpointName); - EXPECT_TRUE(wifi_lan_a.StartAcceptingConnections(service_id, {})); - NsdServiceInfo nsd_service_info; nsd_service_info.SetServiceName(service_info_name); nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey), endpoint_info_name); - EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id, nsd_service_info)); + EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id, nsd_service_info, {})); EXPECT_TRUE(wifi_lan_a.StopAdvertising(service_id)); env_.Stop(); } @@ -329,9 +323,6 @@ TEST_F(WifiLanTest, CanStartMultipleAdvertising) { std::string service_info_name_2("ServiceInfoName_1"); std::string endpoint_info_name(kEndpointName); - EXPECT_TRUE(wifi_lan_a.StartAcceptingConnections(service_id_1, {})); - EXPECT_TRUE(wifi_lan_a.StartAcceptingConnections(service_id_2, {})); - NsdServiceInfo nsd_service_info_1; nsd_service_info_1.SetServiceName(service_info_name_1); nsd_service_info_1.SetTxtRecord(std::string(kEndpointInfoKey), @@ -340,8 +331,10 @@ TEST_F(WifiLanTest, CanStartMultipleAdvertising) { nsd_service_info_2.SetServiceName(service_info_name_2); nsd_service_info_2.SetTxtRecord(std::string(kEndpointInfoKey), endpoint_info_name); - EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id_1, nsd_service_info_1)); - EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id_2, nsd_service_info_2)); + EXPECT_TRUE( + wifi_lan_a.StartAdvertising(service_id_1, nsd_service_info_1, {})); + EXPECT_TRUE( + wifi_lan_a.StartAdvertising(service_id_2, nsd_service_info_2, {})); EXPECT_TRUE(wifi_lan_a.StopAdvertising(service_id_1)); EXPECT_TRUE(wifi_lan_a.StopAdvertising(service_id_2)); EXPECT_TRUE(wifi_lan_a.StopAcceptingConnections(service_id_1)); @@ -400,13 +393,11 @@ TEST_F(WifiLanTest, CanAdvertiseThatOtherMediumDiscover) { }, }); - EXPECT_TRUE(wifi_lan_a.StartAcceptingConnections(service_id, {})); - NsdServiceInfo nsd_service_info; nsd_service_info.SetServiceName(service_info_name); nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey), endpoint_info_name); - EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id, nsd_service_info)); + EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id, nsd_service_info, {})); EXPECT_TRUE(discovered_latch.Await(kWaitDuration).result()); EXPECT_TRUE(wifi_lan_a.StopAdvertising(service_id)); EXPECT_TRUE(lost_latch.Await(kWaitDuration).result()); @@ -424,13 +415,11 @@ TEST_F(WifiLanTest, CanDiscoverThatOtherMediumAdvertise) { CountDownLatch discovered_latch(1); CountDownLatch lost_latch(1); - EXPECT_TRUE(wifi_lan_b.StartAcceptingConnections(service_id, {})); - NsdServiceInfo nsd_service_info; nsd_service_info.SetServiceName(service_info_name); nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey), endpoint_info_name); - wifi_lan_b.StartAdvertising(service_id, nsd_service_info); + wifi_lan_b.StartAdvertising(service_id, nsd_service_info, {}); EXPECT_TRUE(wifi_lan_a.StartDiscovery( service_id, DiscoveredServiceCallback{ diff --git a/connections/implementation/p2p_cluster_pcp_handler.cc b/connections/implementation/p2p_cluster_pcp_handler.cc index a71647b6..047bc673 100644 --- a/connections/implementation/p2p_cluster_pcp_handler.cc +++ b/connections/implementation/p2p_cluster_pcp_handler.cc @@ -3171,31 +3171,6 @@ ErrorOr P2pClusterPcpHandler::StartWifiLanAdvertising( // request comes in very quickly. LOG(INFO) << "P2pClusterPcpHandler::StartWifiLanAdvertising: service=" << service_id << ": start"; - if (!wifi_lan_medium_.IsAcceptingConnections(service_id)) { - ErrorOr wifi_lan_result = wifi_lan_medium_.StartAcceptingConnections( - service_id, - absl::bind_front( - &P2pClusterPcpHandler::WifiLanConnectionAcceptedHandler, this, - client, local_endpoint_id, local_endpoint_info.AsStringView(), - NearbyDevice::Type::kConnectionsDevice)); - if (wifi_lan_result.has_error()) { - LOG(WARNING) - << "In StartWifiLanAdvertising(" - << absl::BytesToHexString(local_endpoint_info.data()) - << "), client=" << client->GetClientId() - << " failed to start listening for incoming WifiLan connections " - "to service_id=" - << service_id; - return {Error(wifi_lan_result.error().operation_result_code().value())}; - } - VLOG(1) << "In StartWifiLanAdvertising(" - << absl::BytesToHexString(local_endpoint_info.data()) - << "), client=" << client->GetClientId() - << " started listening for incoming WifiLan connections " - "to service_id = " - << service_id; - } - // Generate a WifiLanServiceInfo with which to become WifiLan discoverable. // TODO(b/169550050): Implement UWBAddress. const ByteArray service_id_hash = @@ -3220,7 +3195,6 @@ ErrorOr P2pClusterPcpHandler::StartWifiLanAdvertising( << absl::BytesToHexString(service_id_hash.data()) << ", endpoint_info=" << absl::BytesToHexString(local_endpoint_info.data()) << "}."; - wifi_lan_medium_.StopAcceptingConnections(service_id); return { Error(OperationResultCode::NEARBY_WIFI_LAN_ADVERTISE_TO_BYTES_FAILURE)}; } @@ -3231,15 +3205,18 @@ ErrorOr P2pClusterPcpHandler::StartWifiLanAdvertising( << nsd_service_info.GetServiceName() << " with service_id=" << service_id; - ErrorOr wifi_lan_result = - wifi_lan_medium_.StartAdvertising(service_id, nsd_service_info); + ErrorOr wifi_lan_result = wifi_lan_medium_.StartAdvertising( + service_id, nsd_service_info, + absl::bind_front(&P2pClusterPcpHandler::WifiLanConnectionAcceptedHandler, + this, client, local_endpoint_id, + local_endpoint_info.AsStringView(), + NearbyDevice::Type::kConnectionsDevice)); if (wifi_lan_result.has_error()) { - LOG(INFO) << "In StartWifiLanAdvertising(" + LOG(WARNING) << "In StartWifiLanAdvertising(" << absl::BytesToHexString(local_endpoint_info.data()) << "), client=" << client->GetClientId() << " couldn't advertise with WifiLanServiceInfo " << nsd_service_info.GetServiceName(); - wifi_lan_medium_.StopAcceptingConnections(service_id); return {Error(wifi_lan_result.error().operation_result_code().value())}; } VLOG(1) << "In StartWifiLanAdvertising("