[refactor] use VPNManagment in composition instead of inheritance

We make an ugly step for that moving the VPNStatus into VPNManagement.
Let's iterate on it a second time to clean up that.

- Related: #8942

3 files changed, 67 insertions, 70 deletions

diff --git a/src/leap/bitmask/vpn/_management.py b/src/leap/bitmask/vpn/_management.py
index e86b3e5b..c06c1d91 100644
--- a/src/leap/bitmask/vpn/_management.py
+++ b/src/leap/bitmask/vpn/_management.py
@@ -53,9 +53,7 @@ class VPNManagement(object):
         self._host = None
         self._port = None
 
-        self._last_state = None
-        self._last_status = None
-        self._status = None
+        self._watcher = None
         self._logs = {}
 
     def set_connection(self, host, port):
@@ -75,24 +73,6 @@ class VPNManagement(object):
     def is_connected(self):
         return bool(self._tn)
 
-    def connect(self):
-        if not self._host or not self._port:
-            raise ImproperlyConfigured('Connection is not configured')
-
-        try:
-            self._tn = UDSTelnet(self._host, self._port)
-            self._tn.read_eager()
-
-        except Exception as e:
-            self.log.warn('Could not connect to OpenVPN yet: %r' % (e,))
-            self._tn = None
-
-        if self._tn:
-            return True
-        else:
-            self.log.error('Error while connecting to management!')
-            return False
-
     def connect_retry(self, retry=0, max_retries=None):
         """
         Attempts to connect to a management interface, and retries
@@ -109,11 +89,29 @@ class VPNManagement(object):
             return
 
         if not self.aborted and not self.is_connected():
-            self.connect()
+            self._connect()
             reactor.callLater(
                 self.CONNECTION_RETRY_TIME,
                 self.connect_retry, retry + 1, max_retries)
 
+    def _connect(self):
+        if not self._host or not self._port:
+            raise ImproperlyConfigured('Connection is not configured')
+
+        try:
+            self._tn = UDSTelnet(self._host, self._port)
+            self._tn.read_eager()
+
+        except Exception as e:
+            self.log.warn('Could not connect to OpenVPN yet: %r' % (e,))
+            self._tn = None
+
+        if self._tn:
+            return True
+        else:
+            self.log.error('Error while connecting to management!')
+            return False
+
     def process_log(self):
         if not self._watcher or not self._tn:
             return
@@ -191,10 +189,9 @@ class VPNManagement(object):
         self._tn.get_socket().close()
         self._tn = None
 
-    def _parse_state_and_notify(self, output):
+    def _parse_state(self, output):
         """
-        Parses the output of the state command, and trigger a state transition
-        when the state changes.
+        Parses the output of the state command.
 
         :param output: list of lines that the state command printed as
                        its output
@@ -216,17 +213,11 @@ class VPNManagement(object):
                 except ValueError:
                     self.log.debug('Could not parse %s' % parts)
 
-            state = status_step
-            if state != self._last_state:
-                # XXX this status object is the vpn status observer
-                if self._status:
-                    self._status.set_status(state, None)
-                self._last_state = state
+            return status_step
 
-    def _parse_status_and_notify(self, output):
+    def _parse_status(self, output):
         """
-        Parses the output of the status command and emits
-        status_changed signal when the status changes.
+        Parses the output of the status command.
 
         :param output: list of lines that the status command printed
                        as its output
@@ -260,27 +251,25 @@ class VPNManagement(object):
             elif text == "TUN/TAP write bytes":
                 tun_tap_write = value  # upload
 
-        traffic_status = (tun_tap_read, tun_tap_write)
-        if traffic_status != self._last_status:
-            if self._status:
-                self._status.set_traffic_status(traffic_status)
-            self._last_status = traffic_status
+        return (tun_tap_read, tun_tap_write)
 
     def get_state(self):
         """
         Notifies the gui of the output of the state command over
         the openvpn management interface.
         """
-        if self.is_connected():
-            return self._parse_state_and_notify(self._send_command("state"))
+        if not self.is_connected():
+            return ""
+        return self._parse_state(self._send_command("state"))
 
-    def get_status(self):
+    def get_traffic_status(self):
         """
         Notifies the gui of the output of the status command over
         the openvpn management interface.
         """
-        if self.is_connected():
-            return self._parse_status_and_notify(self._send_command("status"))
+        if not self.is_connected():
+            return (None, None)
+        return self._parse_status(self._send_command("status"))
 
     def terminate(self, shutdown=False):
         """
@@ -380,7 +369,7 @@ class VPNManagement(object):
                 port = cmdline[index + 2]
                 self.log.debug("Trying to connect to %s:%s"
                                % (host, port))
-                self.connect()
+                self._connect()
 
                 # XXX this has a problem with connections to different
                 # remotes. So the reconnection will only work when we are
diff --git a/src/leap/bitmask/vpn/_status.py b/src/leap/bitmask/vpn/_status.py
index 6bd9c7c9..5d41a535 100644
--- a/src/leap/bitmask/vpn/_status.py
+++ b/src/leap/bitmask/vpn/_status.py
@@ -74,9 +74,10 @@ class VPNStatus(object):
         elif status in self._CONNECTED:
             status = "on"
 
-        self._status = status
-        self.errcode = errcode
-        emit_async(catalog.VPN_STATUS_CHANGED)
+        if self._status != status:
+            self._status = status
+            self.errcode = errcode
+            emit_async(catalog.VPN_STATUS_CHANGED)
 
     def get_traffic_status(self):
         down = None
@@ -87,7 +88,8 @@ class VPNStatus(object):
             up = bytes2human(self._traffic_up)
         return {'down': down, 'up': up}
 
-    def set_traffic_status(self, status):
-        up, down = status
-        self._traffic_up = up
-        self._traffic_down = down
+    def set_traffic_status(self, up, down):
+        if up != self._traffic_up or down != self._traffic_down:
+            self._traffic_up = up
+            self._traffic_down = down
+            emit_async(catalog.VPN_STATUS_CHANGED)
diff --git a/src/leap/bitmask/vpn/process.py b/src/leap/bitmask/vpn/process.py
index daf3576b..aaf990c1 100644
--- a/src/leap/bitmask/vpn/process.py
+++ b/src/leap/bitmask/vpn/process.py
@@ -29,8 +29,8 @@ from twisted.internet import error as internet_error
 from twisted.logger import Logger
 
 from leap.bitmask.vpn.utils import get_vpn_launcher
-from leap.bitmask.vpn import _status
-from leap.bitmask.vpn import _management
+from leap.bitmask.vpn._status import VPNStatus
+from leap.bitmask.vpn._management import VPNManagement
 
 from leap.bitmask.vpn.launchers import darwin
 from leap.bitmask.vpn.constants import IS_MAC, IS_LINUX
@@ -40,7 +40,7 @@ from leap.bitmask.vpn.constants import IS_MAC, IS_LINUX
 OPENVPN_VERBOSITY = 1
 
 
-class _VPNProcess(protocol.ProcessProtocol, _management.VPNManagement):
+class _VPNProcess(protocol.ProcessProtocol):
 
     """
     A ProcessProtocol class that can be used to spawn a process that will
@@ -76,16 +76,15 @@ class _VPNProcess(protocol.ProcessProtocol, _management.VPNManagement):
                              openvpn invocation
         :type openvpn_verb: int
         """
-        # TODO handle management as a component
-        _management.VPNManagement.__init__(self)
-        self.set_connection(socket_host, socket_port)
+        self._management = VPNManagement()
+        self._management.set_connection(socket_host, socket_port)
+        self._host = socket_host
+        self._port = socket_port
 
         self._vpnconfig = vpnconfig
         self._providerconfig = providerconfig
         self._launcher = get_vpn_launcher()
 
-        self._last_state = None
-        self._last_status = None
         self._alive = False
 
         # XXX use flags, maybe, instead of passing
@@ -93,8 +92,8 @@ class _VPNProcess(protocol.ProcessProtocol, _management.VPNManagement):
         self._openvpn_verb = openvpn_verb
         self._restartfun = restartfun
 
-        self._status = _status.VPNStatus()
-        self.set_watcher(self._status)
+        self._status = VPNStatus()
+        self._management.set_watcher(self._status)
 
         self.restarting = True
         self._remotes = remotes
@@ -110,10 +109,6 @@ class _VPNProcess(protocol.ProcessProtocol, _management.VPNManagement):
     def traffic_status(self):
         return self._status.get_traffic_status()
 
-    @traffic_status.setter
-    def traffic_status(self, value):
-        self._status.set_traffic_status(value)
-
     # processProtocol methods
 
     def connectionMade(self):
@@ -122,7 +117,7 @@ class _VPNProcess(protocol.ProcessProtocol, _management.VPNManagement):
         """
         self._alive = True
         self.aborted = False
-        self.connect_retry(max_retries=10)
+        self._management.connect_retry(max_retries=10)
 
     def outReceived(self, data):
         """
@@ -183,18 +178,20 @@ class _VPNProcess(protocol.ProcessProtocol, _management.VPNManagement):
         Polls connection status.
         """
         if self._alive:
-            self.get_status()
+            up, down = self._management.get_traffic_status()
+            self._status.set_traffic_status(up, down)
 
     def pollState(self):
         """
         Polls connection state.
         """
         if self._alive:
-            self.get_state()
+            state = self._management.get_state()
+            self._status.set_status(state, None)
 
     def pollLog(self):
         if self._alive:
-            self.process_log()
+            self._management.process_log()
 
     # launcher
 
@@ -243,8 +240,17 @@ class _VPNProcess(protocol.ProcessProtocol, _management.VPNManagement):
         # filter out ports since we don't need that info
         return [gateway for gateway, port in gateways_ports]
 
+    def get_openvpn_process(self):
+        return self._management.get_openvpn_process()
+
     # shutdown
 
+    def stop_if_already_running(self):
+        return self._management.stop_if_already_running()
+
+    def terminate(self, shutdown=False):
+        self._management.terminate(shutdown)
+
     def killProcess(self):
         """
         Sends the KILL signal to the running process.