Always set interface down in object destruction

We would like to reason about our interfaces as being up
when they are configured and ready for use, and down otherwise.
This helps when switching modes. For example, when setting up a SoftAP,
you can expect that the interface starts in a down state, and
transitions to up when hostapd is ready for business.  Note that we
cannot do this after firmware reload, because this may cause driver
de-initialization (see c#6 in b/31205821).

Bug: 31337216
Test: Integration tests pass

Change-Id: I03a71ea623e29ef9b023d97afc81cf836ebfb1ff

diff --git a/Android.mk b/Android.mk
index 2472b7e..f520e67 100644 (file)
--- a/Android.mk
+++ b/Android.mk
@@ -173,7 +173,8 @@ LOCAL_SHARED_LIBRARIES := \
     libbase \
     libbinder \
     libcutils \
-    libutils
+    libutils \
+    libwifi-system
 LOCAL_STATIC_LIBRARIES := \
     libgmock \
     libwificond_ipc \

diff --git a/ap_interface_impl.cpp b/ap_interface_impl.cpp
index 0bbbb85..e7f0df9 100644 (file)
--- a/ap_interface_impl.cpp
+++ b/ap_interface_impl.cpp
@@ -48,6 +48,7 @@ ApInterfaceImpl::ApInterfaceImpl(const string& interface_name,
 
 ApInterfaceImpl::~ApInterfaceImpl() {
   binder_->NotifyImplDead();
+  if_tool_->SetUpState(interface_name_.c_str(), false);
 }
 
 sp<IApInterface> ApInterfaceImpl::GetBinder() const {

diff --git a/client_interface_impl.cpp b/client_interface_impl.cpp
index 20b04ec..6011b79 100644 (file)
--- a/client_interface_impl.cpp
+++ b/client_interface_impl.cpp
@@ -29,6 +29,7 @@
 
 using android::net::wifi::IClientInterface;
 using android::sp;
+using android::wifi_system::InterfaceTool;
 using android::wifi_system::SupplicantManager;
 
 using namespace std::placeholders;
@@ -43,12 +44,14 @@ ClientInterfaceImpl::ClientInterfaceImpl(
     const std::string& interface_name,
     uint32_t interface_index,
     const std::vector<uint8_t>& interface_mac_addr,
+    InterfaceTool* if_tool,
     SupplicantManager* supplicant_manager,
     NetlinkUtils* netlink_utils,
     ScanUtils* scan_utils)
     : interface_name_(interface_name),
       interface_index_(interface_index),
       interface_mac_addr_(interface_mac_addr),
+      if_tool_(if_tool),
       supplicant_manager_(supplicant_manager),
       netlink_utils_(netlink_utils),
       scan_utils_(scan_utils),
@@ -64,6 +67,7 @@ ClientInterfaceImpl::~ClientInterfaceImpl() {
   binder_->NotifyImplDead();
   DisableSupplicant();
   scan_utils_->UnsubscribeScanResultNotification(interface_index_);
+  if_tool_->SetUpState(interface_name_.c_str(), false);
 }
 
 sp<android::net::wifi::IClientInterface> ClientInterfaceImpl::GetBinder() const {

diff --git a/client_interface_impl.h b/client_interface_impl.h
index a73252c..efeccfe 100644 (file)
--- a/client_interface_impl.h
+++ b/client_interface_impl.h
@@ -21,6 +21,7 @@
 
 #include <android-base/macros.h>
 #include <utils/StrongPointer.h>
+#include <wifi_system/interface_tool.h>
 #include <wifi_system/supplicant_manager.h>
 
 #include "android/net/wifi/IClientInterface.h"
@@ -44,6 +45,7 @@ class ClientInterfaceImpl {
       const std::string& interface_name,
       uint32_t interface_index,
       const std::vector<uint8_t>& interface_mac_addr,
+      android::wifi_system::InterfaceTool* if_tool,
       android::wifi_system::SupplicantManager* supplicant_manager,
       NetlinkUtils* netlink_utils,
       ScanUtils* scan_utils);
@@ -68,6 +70,7 @@ class ClientInterfaceImpl {
   const std::string interface_name_;
   const uint32_t interface_index_;
   const std::vector<uint8_t> interface_mac_addr_;
+  android::wifi_system::InterfaceTool* const if_tool_;
   android::wifi_system::SupplicantManager* const supplicant_manager_;
   NetlinkUtils* const netlink_utils_;
   ScanUtils* const scan_utils_;

diff --git a/server.cpp b/server.cpp
index 9eef6f4..61cfb21 100644 (file)
--- a/server.cpp
+++ b/server.cpp
@@ -121,6 +121,7 @@ Status Server::createClientInterface(sp<IClientInterface>* created_interface) {
       interface_name,
       interface_index,
       interface_mac_addr,
+      if_tool_.get(),
       supplicant_manager_.get(),
       netlink_utils_,
       scan_utils_));

diff --git a/tests/ap_interface_impl_unittest.cpp b/tests/ap_interface_impl_unittest.cpp
index 8b266e9..6ed328e 100644 (file)
--- a/tests/ap_interface_impl_unittest.cpp
+++ b/tests/ap_interface_impl_unittest.cpp
@@ -80,6 +80,7 @@ TEST_F(ApInterfaceImplTest, ShouldReportStopSuccess) {
   EXPECT_CALL(*if_tool_, SetUpState(StrEq(kTestInterfaceName), false))
       .WillOnce(Return(true));
   EXPECT_TRUE(ap_interface_.StopHostapd());
+  testing::Mock::VerifyAndClearExpectations(if_tool_.get());
 }
 
 TEST_F(ApInterfaceImplTest, ShouldRejectInvalidConfig) {

diff --git a/tests/client_interface_impl_unittest.cpp b/tests/client_interface_impl_unittest.cpp
index 7dd0f23..960667b 100644 (file)
--- a/tests/client_interface_impl_unittest.cpp
+++ b/tests/client_interface_impl_unittest.cpp
@@ -20,6 +20,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include <wifi_system/supplicant_manager.h>
+#include <wifi_system_test/mock_interface_tool.h>
 #include <wifi_system_test/mock_supplicant_manager.h>
 
 #include "wificond/client_interface_impl.h"
@@ -27,6 +28,7 @@
 #include "wificond/tests/mock_netlink_utils.h"
 #include "wificond/tests/mock_scan_utils.h"
 
+using android::wifi_system::MockInterfaceTool;
 using android::wifi_system::MockSupplicantManager;
 using android::wifi_system::SupplicantManager;
 using std::unique_ptr;
@@ -53,6 +55,7 @@ class ClientInterfaceImplTest : public ::testing::Test {
         kTestInterfaceName,
         kTestInterfaceIndex,
         vector<uint8_t>{0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+        if_tool_.get(),
         supplicant_manager_.get(),
         &netlink_utils_,
         &scan_utils_});
@@ -66,6 +69,8 @@ class ClientInterfaceImplTest : public ::testing::Test {
         UnsubscribeScanResultNotification(kTestInterfaceIndex));
   }
 
+  unique_ptr<NiceMock<MockInterfaceTool>> if_tool_{
+      new NiceMock<MockInterfaceTool>};
   unique_ptr<NiceMock<MockSupplicantManager>> supplicant_manager_{
       new NiceMock<MockSupplicantManager>};
   unique_ptr<NiceMock<MockNetlinkManager>> netlink_manager_{

diff --git a/tests/integration/ap_interface_test.cpp b/tests/integration/ap_interface_test.cpp
index 9f24268..29206d0 100644 (file)
--- a/tests/integration/ap_interface_test.cpp
+++ b/tests/integration/ap_interface_test.cpp
@@ -18,6 +18,7 @@
 
 #include <gtest/gtest.h>
 #include <utils/StrongPointer.h>
+#include <wifi_system/interface_tool.h>
 
 #include "android/net/wifi/IApInterface.h"
 #include "android/net/wifi/IWificond.h"
@@ -25,10 +26,12 @@
 
 using android::net::wifi::IApInterface;
 using android::net::wifi::IWificond;
+using android::wifi_system::InterfaceTool;
 using android::wificond::tests::integration::HostapdIsDead;
 using android::wificond::tests::integration::HostapdIsRunning;
 using android::wificond::tests::integration::ScopedDevModeWificond;
 using android::wificond::tests::integration::WaitForTrue;
+using std::string;
 using std::vector;
 
 namespace android {
@@ -56,6 +59,17 @@ TEST(ApInterfaceTest, CanCreateApInterfaces) {
   EXPECT_TRUE(service->createApInterface(&ap_interface).isOk());
   EXPECT_NE(nullptr, ap_interface.get());
 
+  // The interface should start out down.
+  string if_name;
+  EXPECT_TRUE(ap_interface->getInterfaceName(&if_name).isOk());
+  EXPECT_TRUE(!if_name.empty());
+  InterfaceTool if_tool;
+  EXPECT_FALSE(if_tool.GetUpState(if_name.c_str()));
+
+  // Mark the interface as up, just to test that we mark it down on teardown.
+  EXPECT_TRUE(if_tool.SetUpState(if_name.c_str(), true));
+  EXPECT_TRUE(if_tool.GetUpState(if_name.c_str()));
+
   // We should not be able to create two AP interfaces.
   sp<IApInterface> ap_interface2;
   EXPECT_TRUE(service->createApInterface(&ap_interface2).isOk());
@@ -63,6 +77,7 @@ TEST(ApInterfaceTest, CanCreateApInterfaces) {
 
   // We can tear down the created interface.
   EXPECT_TRUE(service->tearDownInterfaces().isOk());
+  EXPECT_FALSE(if_tool.GetUpState(if_name.c_str()));
 }
 
 // TODO: b/30311493 this test fails because hostapd fails to set the driver
@@ -74,6 +89,13 @@ TEST(ApInterfaceTest, CanStartStopHostapd) {
   EXPECT_TRUE(service->createApInterface(&ap_interface).isOk());
   ASSERT_NE(nullptr, ap_interface.get());
 
+  // Interface should start out down.
+  string if_name;
+  EXPECT_TRUE(ap_interface->getInterfaceName(&if_name).isOk());
+  EXPECT_TRUE(!if_name.empty());
+  InterfaceTool if_tool;
+  EXPECT_FALSE(if_tool.GetUpState(if_name.c_str()));
+
   bool wrote_config = false;
   EXPECT_TRUE(ap_interface->writeHostapdConfig(
       vector<uint8_t>(kValidSsid, kValidSsid + sizeof(kValidSsid) - 1),
@@ -99,12 +121,18 @@ TEST(ApInterfaceTest, CanStartStopHostapd) {
     //      can leave the driver in a poor state.
     // The latter points to an obvious race, where we cannot fully clean up the
     // driver on quick transitions.
-    sleep(1);
+    auto InterfaceIsUp = [&if_tool, &if_name] () {
+      return if_tool.GetUpState(if_name.c_str());
+    };
+    EXPECT_TRUE(WaitForTrue(InterfaceIsUp, kHostapdStartupTimeoutSeconds))
+        << "Failed on iteration " << iteration;
     EXPECT_TRUE(HostapdIsRunning()) << "Failed on iteration " << iteration;
 
     bool hostapd_stopped = false;
     EXPECT_TRUE(ap_interface->stopHostapd(&hostapd_stopped).isOk());
     EXPECT_TRUE(hostapd_stopped);
+    EXPECT_FALSE(if_tool.GetUpState(if_name.c_str()));
+
 
     EXPECT_TRUE(WaitForTrue(HostapdIsDead, kHostapdDeathTimeoutSeconds))
         << "Failed on iteration " << iteration;

diff --git a/tests/integration/client_interface_test.cpp b/tests/integration/client_interface_test.cpp
index 46ef655..6177f17 100644 (file)
--- a/tests/integration/client_interface_test.cpp
+++ b/tests/integration/client_interface_test.cpp
@@ -14,10 +14,12 @@
  * limitations under the License.
  */
 
+#include <string>
 #include <vector>
 
 #include <gtest/gtest.h>
 #include <utils/StrongPointer.h>
+#include <wifi_system/interface_tool.h>
 
 #include "android/net/wifi/IClientInterface.h"
 #include "android/net/wifi/IWificond.h"
@@ -25,10 +27,12 @@
 
 using android::net::wifi::IClientInterface;
 using android::net::wifi::IWificond;
+using android::wifi_system::InterfaceTool;
 using android::wificond::tests::integration::ScopedDevModeWificond;
 using android::wificond::tests::integration::SupplicantIsDead;
 using android::wificond::tests::integration::SupplicantIsRunning;
 using android::wificond::tests::integration::WaitForTrue;
+using std::string;
 using std::vector;
 
 namespace android {
@@ -49,6 +53,17 @@ TEST(ClientInterfaceTest, CanCreateClientInterfaces) {
   EXPECT_TRUE(service->createClientInterface(&client_interface).isOk());
   EXPECT_NE(nullptr, client_interface.get());
 
+  // The interface should start out down.
+  string if_name;
+  EXPECT_TRUE(client_interface->getInterfaceName(&if_name).isOk());
+  EXPECT_TRUE(!if_name.empty());
+  InterfaceTool if_tool;
+  EXPECT_FALSE(if_tool.GetUpState(if_name.c_str()));
+
+  // Mark the interface as up, just to test that we mark it down on teardown.
+  EXPECT_TRUE(if_tool.SetUpState(if_name.c_str(), true));
+  EXPECT_TRUE(if_tool.GetUpState(if_name.c_str()));
+
   // We should not be able to create two client interfaces.
   sp<IClientInterface> client_interface2;
   EXPECT_TRUE(service->createClientInterface(&client_interface2).isOk());
@@ -56,6 +71,7 @@ TEST(ClientInterfaceTest, CanCreateClientInterfaces) {
 
   // We can tear down the created interface.
   EXPECT_TRUE(service->tearDownInterfaces().isOk());
+  EXPECT_FALSE(if_tool.GetUpState(if_name.c_str()));
 }
 
 TEST(ClientInterfaceTest, CanStartStopSupplicant) {