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

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

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3871 bytes --]

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(-)

diff --git a/autotests/util/hostapd.py b/autotests/util/hostapd.py
index 49047f38..0ed4e004 100644
--- a/autotests/util/hostapd.py
+++ b/autotests/util/hostapd.py
@@ -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):
         '''
-- 
2.21.1

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

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]

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(-)

diff --git a/autotests/testSAQuery/connection_test.py b/autotests/testSAQuery/connection_test.py
index 13c45ee9..de0fe2cd 100644
--- a/autotests/testSAQuery/connection_test.py
+++ b/autotests/testSAQuery/connection_test.py
@@ -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()
 
-- 
2.21.1

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

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

Hi James,

On 7/14/20 1:49 PM, James Prestwood wrote:
> 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(-)
> 

Both applied, thanks.

Regards,
-Denis