Skip to contentSkip to conversation

auto-t: fix hostapd.ungraceful_restart

PATCH In review James Prestwood 2 participants 3 messages Jul 14, 2020 – Jul 15, 2020

lore
Share & tools
lore
#0 lore raw

PATCH 1/2 auto-t: fix hostapd.ungraceful_restart

Restarting hostapd from python was actually leaking memory and causing the hostapd object to stay referenced in python. The GLib timeout in wait_for_event was the ultimate cause, but this had no come to light because no tests restarted hostapd then used wait_for_event.

In addition, any use of wait_for_event after a restart would cause an exception because the event socket was never re-attached after hostapd restarted.

Now we properly clean up the timeout in wait_for_event and re-initialize the hostapd object on restart.

---
 autotests/util/hostapd.py | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)
autotests/util/hostapd.py +29 -10
index 49047f38..0ed4e004 100644
@@ -33,7 +33,7 @@ hostapd_map = {ifname: intf for wname, wiphy in wiphy_map.items()
         if wiphy.use == 'hostapd'}
 
 class HostapdCLI:
-    def __init__(self, interface=None, config=None):
+    def _init_hostapd(self, interface=None, config=None):
         global ctrl_count
 
         if not interface and not config:
@@ -54,7 +54,8 @@ class HostapdCLI:
         self.cmdline = 'hostapd_cli -p"' + self.socket_path + '" -i"' + \
                 self.ifname + '"'
 
-        self._hostapd_restarted = False
+        if not hasattr(self, '_hostapd_restarted'):
+            self._hostapd_restarted = False
 
         self.local_ctrl = '/tmp/hostapd_' + str(os.getpid()) + '_' + \
                             str(ctrl_count)
@@ -67,6 +68,9 @@ class HostapdCLI:
 
         ctrl_count = ctrl_count + 1
 
+    def __init__(self, interface=None, config=None):
+        self._init_hostapd(interface, config)
+
     def wait_for_event(self, event, timeout=10):
         global mainloop
         self._wait_timed_out = False
@@ -84,6 +88,7 @@ class HostapdCLI:
             while self._data_available(0.25):
                 data = self.ctrl_sock.recv(4096).decode('utf-8')
                 if event in data:
+                    GLib.source_remove(timeout)
                     return data
 
             if self._wait_timed_out:
@@ -108,11 +113,20 @@ class HostapdCLI:
 
         raise Exception('timeout waiting for control response')
 
-    def __del__(self):
+    def _del_hostapd(self, force=False):
+        self.ctrl_sock.close()
+
         if self._hostapd_restarted:
-            os.system('killall hostapd')
+            if force:
+                os.system('killall -9 hostapd')
+            else:
+                os.system('killall hostapd')
 
-        self.ctrl_sock.close()
+            os.system('ifconfig %s down' % self.ifname)
+            os.system('ifconfig %s up' % self.ifname)
+
+    def __del__(self):
+        self._del_hostapd()
 
     def wps_push_button(self):
         os.system(self.cmdline + ' wps_pbc')
@@ -193,15 +207,20 @@ class HostapdCLI:
         '''
             Ungracefully kill and restart hostapd
         '''
+        # set flag so hostapd can be killed after the test
+        self._hostapd_restarted = True
         intf = hostapd_map[self.ifname]
-        os.system('killall -9 hostapd')
-        os.system('ifconfig %s down' % intf.name)
-        os.system('ifconfig %s up' % intf.name)
+
+        self._del_hostapd(force=True)
+
         os.system('hostapd -g %s -i %s %s &' %
                   (intf.ctrl_interface, intf.name, intf.config))
 
-        # set flag so hostapd can be killed after the test
-        self._hostapd_restarted = True
+        # Give hostapd a second to start and initialize the control interface
+        time.sleep(1)
+
+        # New hostapd process, so re-init
+        self._init_hostapd(intf)
 
     def req_beacon(self, addr, request):
         '''
Signature · 1 line
2.21.1
#1 lore raw

PATCH 2/2 auto-t: make testSAQuery more reliable

First, looking for DeviceState.connected gives a much better indication if we are actually connected vs the connected property on the network object. Second, its good practice to also check that hostapd sees that the station is connected.

---
 autotests/testSAQuery/connection_test.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
autotests/testSAQuery/connection_test.py +5 -3
index 13c45ee9..de0fe2cd 100644
@@ -13,7 +13,7 @@ from hostapd import HostapdCLI, hostapd_map
 class Test(unittest.TestCase):
 
     def test_connection_success(self):
-        hostapd = HostapdCLI(list(hostapd_map.values())[0])
+        hostapd = HostapdCLI(config='ssidCCMP.conf')
 
         wd = IWD()
 
@@ -53,8 +53,10 @@ class Test(unittest.TestCase):
 
         # IWD should now try and re-connect to the AP
 
-        condition = 'obj.connected'
-        wd.wait_for_object_condition(ordered_network.network_object, condition)
+        condition = 'obj.state == DeviceState.connected'
+        wd.wait_for_object_condition(device, condition)
+
+        hostapd.wait_for_event('AP-STA-CONNECTED')
 
         device.disconnect()
 
Signature · 1 line
2.21.1
#2 lore raw
In reply to James Prestwood

Hi James,

Quoted 10 lines · James Prestwood

Restarting hostapd from python was actually leaking memory and causing the hostapd object to stay referenced in python. The GLib timeout in wait_for_event was the ultimate cause, but this had no come to light because no tests restarted hostapd then used wait_for_event.

In addition, any use of wait_for_event after a restart would cause an exception because the event socket was never re-attached after hostapd restarted.

Quoted 6 lines

Now we properly clean up the timeout in wait_for_event and re-initialize the hostapd object on restart.

---
  autotests/util/hostapd.py | 39 +++++++++++++++++++++++++++++----------
  1 file changed, 29 insertions(+), 10 deletions(-)

Both applied, thanks.

Regards,
-Denis