From 951efd6b29515ca368b47d7f746682912a4c61fd Mon Sep 17 00:00:00 2001 From: Mike Degatano Date: Fri, 23 Sep 2022 04:28:16 -0400 Subject: [PATCH] Replace __del__ with explicit disconnect (#3892) --- supervisor/dbus/interface.py | 4 -- supervisor/dbus/network/__init__.py | 9 +++ supervisor/dbus/network/connection.py | 69 ++++++++++++++------- supervisor/dbus/network/interface.py | 39 ++++++++---- supervisor/dbus/network/setting/__init__.py | 4 +- supervisor/dbus/network/wireless.py | 22 ++++--- tests/dbus/network/test_connection.py | 47 ++++++++++++++ tests/dbus/network/test_interface.py | 26 ++++++++ tests/dbus/network/test_network_manager.py | 16 ++++- tests/dbus/network/test_wireless.py | 40 +++++++++--- 10 files changed, 222 insertions(+), 54 deletions(-) create mode 100644 tests/dbus/network/test_connection.py diff --git a/supervisor/dbus/interface.py b/supervisor/dbus/interface.py index ddd370d01..10981690f 100644 --- a/supervisor/dbus/interface.py +++ b/supervisor/dbus/interface.py @@ -35,10 +35,6 @@ class DBusInterface(ABC): """Return True, if they is connected to D-Bus.""" return self.dbus is not None - def __del__(self) -> None: - """Disconnect on delete.""" - self.disconnect() - async def connect(self, bus: MessageBus) -> None: """Connect to D-Bus.""" self.dbus = await DBus.connect(bus, self.bus_name, self.object_path) diff --git a/supervisor/dbus/network/__init__.py b/supervisor/dbus/network/__init__.py index cdc6c2cf4..8427943d3 100644 --- a/supervisor/dbus/network/__init__.py +++ b/supervisor/dbus/network/__init__.py @@ -1,4 +1,5 @@ """Network Manager implementation for DBUS.""" +import asyncio import logging from typing import Any @@ -210,6 +211,14 @@ class NetworkManager(DBusInterfaceProxy): interfaces[interface.name] = interface + # Disconnect removed devices + for device in set(curr_devices.keys()) - set( + self.properties[DBUS_ATTR_DEVICES] + ): + asyncio.get_event_loop().run_in_executor( + None, curr_devices[device].disconnect + ) + self._interfaces = interfaces def disconnect(self) -> None: diff --git a/supervisor/dbus/network/connection.py b/supervisor/dbus/network/connection.py index 0bb8d49e9..a1f02c85f 100644 --- a/supervisor/dbus/network/connection.py +++ b/supervisor/dbus/network/connection.py @@ -1,5 +1,6 @@ """Connection object for Network Manager.""" +import asyncio from typing import Any from supervisor.dbus.network.setting import NetworkSetting @@ -77,16 +78,40 @@ class NetworkConnection(DBusInterfaceProxy): """Return settings.""" return self._settings + @settings.setter + def settings(self, settings: NetworkSetting | None) -> None: + """Set settings.""" + if self._settings and self._settings is not settings: + asyncio.get_event_loop().run_in_executor(None, self._settings.disconnect) + + self._settings = settings + @property def ipv4(self) -> IpConfiguration | None: """Return a ip4 configuration object for the connection.""" return self._ipv4 + @ipv4.setter + def ipv4(self, ipv4: IpConfiguration | None) -> None: + """Set ipv4 configuration.""" + if self._ipv4 and self._ipv4 is not ipv4: + asyncio.get_event_loop().run_in_executor(None, self._ipv4.disconnect) + + self._ipv4 = ipv4 + @property def ipv6(self) -> IpConfiguration | None: """Return a ip6 configuration object for the connection.""" return self._ipv6 + @ipv6.setter + def ipv6(self, ipv6: IpConfiguration | None) -> None: + """Set ipv6 configuration.""" + if self._ipv6 and self._ipv6 is not ipv6: + asyncio.get_event_loop().run_in_executor(None, self._ipv6.disconnect) + + self._ipv6 = ipv6 + @dbus_connected async def update(self, changed: dict[str, Any] | None = None) -> None: """Update connection information.""" @@ -102,46 +127,44 @@ class NetworkConnection(DBusInterfaceProxy): # IPv4 if not changed or DBUS_ATTR_IP4CONFIG in changed: if ( - self._ipv4 - and self._ipv4.is_connected - and self._ipv4.object_path == self.properties[DBUS_ATTR_IP4CONFIG] + self.ipv4 + and self.ipv4.is_connected + and self.ipv4.object_path == self.properties[DBUS_ATTR_IP4CONFIG] ): - await self._ipv4.update() + await self.ipv4.update() elif self.properties[DBUS_ATTR_IP4CONFIG] != DBUS_OBJECT_BASE: - self._ipv4 = IpConfiguration(self.properties[DBUS_ATTR_IP4CONFIG]) - await self._ipv4.connect(self.dbus.bus) + self.ipv4 = IpConfiguration(self.properties[DBUS_ATTR_IP4CONFIG]) + await self.ipv4.connect(self.dbus.bus) else: - self._ipv4 = None + self.ipv4 = None # IPv6 if not changed or DBUS_ATTR_IP6CONFIG in changed: if ( - self._ipv6 - and self._ipv6.is_connected - and self._ipv6.object_path == self.properties[DBUS_ATTR_IP6CONFIG] + self.ipv6 + and self.ipv6.is_connected + and self.ipv6.object_path == self.properties[DBUS_ATTR_IP6CONFIG] ): - await self._ipv6.update() + await self.ipv6.update() elif self.properties[DBUS_ATTR_IP6CONFIG] != DBUS_OBJECT_BASE: - self._ipv6 = IpConfiguration( - self.properties[DBUS_ATTR_IP6CONFIG], False - ) - await self._ipv6.connect(self.dbus.bus) + self.ipv6 = IpConfiguration(self.properties[DBUS_ATTR_IP6CONFIG], False) + await self.ipv6.connect(self.dbus.bus) else: - self._ipv6 = None + self.ipv6 = None # Settings if not changed or DBUS_ATTR_CONNECTION in changed: if ( - self._settings - and self._settings.is_connected - and self._settings.object_path == self.properties[DBUS_ATTR_CONNECTION] + self.settings + and self.settings.is_connected + and self.settings.object_path == self.properties[DBUS_ATTR_CONNECTION] ): - await self._settings.reload() + await self.settings.reload() elif self.properties[DBUS_ATTR_CONNECTION] != DBUS_OBJECT_BASE: - self._settings = NetworkSetting(self.properties[DBUS_ATTR_CONNECTION]) - await self._settings.connect(self.dbus.bus) + self.settings = NetworkSetting(self.properties[DBUS_ATTR_CONNECTION]) + await self.settings.connect(self.dbus.bus) else: - self._settings = None + self.settings = None def disconnect(self) -> None: """Disconnect from D-Bus.""" diff --git a/supervisor/dbus/network/interface.py b/supervisor/dbus/network/interface.py index 6269498c8..81714bd2a 100644 --- a/supervisor/dbus/network/interface.py +++ b/supervisor/dbus/network/interface.py @@ -1,5 +1,6 @@ """NetworkInterface object for Network Manager.""" +import asyncio from typing import Any from dbus_fast.aio.message_bus import MessageBus @@ -51,7 +52,7 @@ class NetworkInterface(DBusInterfaceProxy): @property @dbus_property - def type(self) -> int: + def type(self) -> DeviceType: """Return interface type.""" return self.properties[DBUS_ATTR_DEVICE_TYPE] @@ -72,6 +73,14 @@ class NetworkInterface(DBusInterfaceProxy): """Return the connection used for this interface.""" return self._connection + @connection.setter + def connection(self, connection: NetworkConnection | None) -> None: + """Set connection for interface.""" + if self._connection and self._connection is not connection: + asyncio.get_event_loop().run_in_executor(None, self._connection.disconnect) + + self._connection = connection + @property def settings(self) -> NetworkSetting | None: """Return the connection settings used for this interface.""" @@ -82,6 +91,14 @@ class NetworkInterface(DBusInterfaceProxy): """Return the wireless data for this interface.""" return self._wireless + @wireless.setter + def wireless(self, wireless: NetworkWireless | None) -> None: + """Set wireless for interface.""" + if self._wireless and self._wireless is not wireless: + asyncio.get_event_loop().run_in_executor(None, self._wireless.disconnect) + + self._wireless = wireless + async def connect(self, bus: MessageBus) -> None: """Connect to D-Bus.""" return await super().connect(bus) @@ -98,29 +115,29 @@ class NetworkInterface(DBusInterfaceProxy): # If active connection exists if not changed or DBUS_ATTR_ACTIVE_CONNECTION in changed: if ( - self._connection - and self._connection.is_connected - and self._connection.object_path + self.connection + and self.connection.is_connected + and self.connection.object_path == self.properties[DBUS_ATTR_ACTIVE_CONNECTION] ): await self.connection.update() elif self.properties[DBUS_ATTR_ACTIVE_CONNECTION] != DBUS_OBJECT_BASE: - self._connection = NetworkConnection( + self.connection = NetworkConnection( self.properties[DBUS_ATTR_ACTIVE_CONNECTION] ) - await self._connection.connect(self.dbus.bus) + await self.connection.connect(self.dbus.bus) else: - self._connection = None + self.connection = None # Wireless if not changed or DBUS_ATTR_DEVICE_TYPE in changed: if self.type != DeviceType.WIRELESS: - self._wireless = None + self.wireless = None elif self.wireless and self.wireless.is_connected: - await self._wireless.update() + await self.wireless.update() else: - self._wireless = NetworkWireless(self.object_path) - await self._wireless.connect(self.dbus.bus) + self.wireless = NetworkWireless(self.object_path) + await self.wireless.connect(self.dbus.bus) def disconnect(self) -> None: """Disconnect from D-Bus.""" diff --git a/supervisor/dbus/network/setting/__init__.py b/supervisor/dbus/network/setting/__init__.py index 6d71f5d13..c57fe03fa 100644 --- a/supervisor/dbus/network/setting/__init__.py +++ b/supervisor/dbus/network/setting/__init__.py @@ -169,7 +169,9 @@ class NetworkSetting(DBusInterface): def disconnect(self) -> None: """Disconnect from D-Bus.""" - self.dbus.Settings.Connection.off_updated(self.reload) + if self.is_connected: + self.dbus.Settings.Connection.off_updated(self.reload) + super().disconnect() @dbus_connected diff --git a/supervisor/dbus/network/wireless.py b/supervisor/dbus/network/wireless.py index 6a523ccd6..8b636d115 100644 --- a/supervisor/dbus/network/wireless.py +++ b/supervisor/dbus/network/wireless.py @@ -37,6 +37,14 @@ class NetworkWireless(DBusInterfaceProxy): """Return details about active connection.""" return self._active + @active.setter + def active(self, active: NetworkWirelessAP | None) -> None: + """Set active wireless AP.""" + if self._active and self._active is not active: + asyncio.get_event_loop().run_in_executor(None, self._active.disconnect) + + self._active = active + @dbus_connected async def request_scan(self) -> None: """Request a new AP scan.""" @@ -63,16 +71,16 @@ class NetworkWireless(DBusInterfaceProxy): # Get details from current active if not changed or DBUS_ATTR_ACTIVE_ACCESSPOINT in changed: if ( - self._active - and self._active.is_connected - and self._active.object_path + self.active + and self.active.is_connected + and self.active.object_path == self.properties[DBUS_ATTR_ACTIVE_ACCESSPOINT] ): - await self._active.update() + await self.active.update() elif self.properties[DBUS_ATTR_ACTIVE_ACCESSPOINT] != DBUS_OBJECT_BASE: - self._active = NetworkWirelessAP( + self.active = NetworkWirelessAP( self.properties[DBUS_ATTR_ACTIVE_ACCESSPOINT] ) - await self._active.connect(self.dbus.bus) + await self.active.connect(self.dbus.bus) else: - self._active = None + self.active = None diff --git a/tests/dbus/network/test_connection.py b/tests/dbus/network/test_connection.py new file mode 100644 index 000000000..49dcfccba --- /dev/null +++ b/tests/dbus/network/test_connection.py @@ -0,0 +1,47 @@ +"""Test connection object.""" + +import asyncio + +from supervisor.dbus.network import NetworkManager + +from tests.common import fire_property_change_signal +from tests.const import TEST_INTERFACE + + +async def test_old_ipv4_disconnect(network_manager: NetworkManager): + """Test old ipv4 disconnects on ipv4 change.""" + connection = network_manager.interfaces[TEST_INTERFACE].connection + ipv4 = connection.ipv4 + assert ipv4.is_connected is True + + fire_property_change_signal(connection, {"Ip4Config": "/"}) + await asyncio.sleep(0) + + assert connection.ipv4 is None + assert ipv4.is_connected is False + + +async def test_old_ipv6_disconnect(network_manager: NetworkManager): + """Test old ipv6 disconnects on ipv6 change.""" + connection = network_manager.interfaces[TEST_INTERFACE].connection + ipv6 = connection.ipv6 + assert ipv6.is_connected is True + + fire_property_change_signal(connection, {"Ip6Config": "/"}) + await asyncio.sleep(0) + + assert connection.ipv6 is None + assert ipv6.is_connected is False + + +async def test_old_settings_disconnect(network_manager: NetworkManager): + """Test old settings disconnects on settings change.""" + connection = network_manager.interfaces[TEST_INTERFACE].connection + settings = connection.settings + assert settings.is_connected is True + + fire_property_change_signal(connection, {"Connection": "/"}) + await asyncio.sleep(0) + + assert connection.settings is None + assert settings.is_connected is False diff --git a/tests/dbus/network/test_interface.py b/tests/dbus/network/test_interface.py index 7a8654645..a4d8f1737 100644 --- a/tests/dbus/network/test_interface.py +++ b/tests/dbus/network/test_interface.py @@ -57,3 +57,29 @@ async def test_network_interface_wlan(network_manager: NetworkManager): interface = network_manager.interfaces[TEST_INTERFACE_WLAN] assert interface.name == TEST_INTERFACE_WLAN assert interface.type == DeviceType.WIRELESS + + +async def test_old_connection_disconnect(network_manager: NetworkManager): + """Test old connection disconnects on connection change.""" + interface = network_manager.interfaces[TEST_INTERFACE] + connection = interface.connection + assert connection.is_connected is True + + fire_property_change_signal(interface, {"ActiveConnection": "/"}) + await asyncio.sleep(0) + + assert interface.connection is None + assert connection.is_connected is False + + +async def test_old_wireless_disconnect(network_manager: NetworkManager): + """Test old wireless disconnects on type change.""" + interface = network_manager.interfaces[TEST_INTERFACE_WLAN] + wireless = interface.wireless + assert wireless.is_connected is True + + fire_property_change_signal(interface, {"DeviceType": DeviceType.ETHERNET}) + await asyncio.sleep(0) + + assert interface.wireless is None + assert wireless.is_connected is False diff --git a/tests/dbus/network/test_network_manager.py b/tests/dbus/network/test_network_manager.py index fbe6b8e41..4aab26c46 100644 --- a/tests/dbus/network/test_network_manager.py +++ b/tests/dbus/network/test_network_manager.py @@ -11,7 +11,7 @@ from supervisor.exceptions import HostNotSupportedError from .setting.test_init import SETTINGS_WITH_SIGNATURE from tests.common import fire_property_change_signal -from tests.const import TEST_INTERFACE +from tests.const import TEST_INTERFACE, TEST_INTERFACE_WLAN # pylint: disable=protected-access @@ -93,3 +93,17 @@ async def test_add_and_activate_connection( "/org/freedesktop/NetworkManager-org.freedesktop.NetworkManager.AddAndActivateConnection", "/org/freedesktop/NetworkManager/Settings/1-org.freedesktop.NetworkManager.Settings.Connection.GetSettings", ] + + +async def test_removed_devices_disconnect(network_manager: NetworkManager): + """Test removed devices are disconnected.""" + wlan = network_manager.interfaces[TEST_INTERFACE_WLAN] + assert wlan.is_connected is True + + fire_property_change_signal( + network_manager, {"Devices": ["/org/freedesktop/NetworkManager/Devices/1"]} + ) + await asyncio.sleep(0) + + assert TEST_INTERFACE_WLAN not in network_manager.interfaces + assert wlan.is_connected is False diff --git a/tests/dbus/network/test_wireless.py b/tests/dbus/network/test_wireless.py index e525c8f8f..4a995e53d 100644 --- a/tests/dbus/network/test_wireless.py +++ b/tests/dbus/network/test_wireless.py @@ -4,32 +4,39 @@ import asyncio from supervisor.dbus.network import NetworkManager from tests.common import fire_property_change_signal +from tests.const import TEST_INTERFACE_WLAN async def test_wireless(network_manager: NetworkManager): """Test wireless properties.""" - assert network_manager.interfaces["wlan0"].wireless.active is None + assert network_manager.interfaces[TEST_INTERFACE_WLAN].wireless.active is None fire_property_change_signal( - network_manager.interfaces["wlan0"].wireless, + network_manager.interfaces[TEST_INTERFACE_WLAN].wireless, {"ActiveAccessPoint": "/org/freedesktop/NetworkManager/AccessPoint/43099"}, ) await asyncio.sleep(0) assert ( - network_manager.interfaces["wlan0"].wireless.active.mac == "E4:57:40:A9:D7:DE" + network_manager.interfaces[TEST_INTERFACE_WLAN].wireless.active.mac + == "E4:57:40:A9:D7:DE" ) fire_property_change_signal( - network_manager.interfaces["wlan0"].wireless, {}, ["ActiveAccessPoint"] + network_manager.interfaces[TEST_INTERFACE_WLAN].wireless, + {}, + ["ActiveAccessPoint"], ) await asyncio.sleep(0) - assert network_manager.interfaces["wlan0"].wireless.active is None + assert network_manager.interfaces[TEST_INTERFACE_WLAN].wireless.active is None async def test_request_scan(network_manager: NetworkManager, dbus: list[str]): """Test request scan.""" dbus.clear() - assert await network_manager.interfaces["wlan0"].wireless.request_scan() is None + assert ( + await network_manager.interfaces[TEST_INTERFACE_WLAN].wireless.request_scan() + is None + ) assert dbus == [ "/org/freedesktop/NetworkManager/Devices/3-org.freedesktop.NetworkManager.Device.Wireless.RequestScan" ] @@ -39,7 +46,7 @@ async def test_get_all_access_points(network_manager: NetworkManager, dbus: list """Test get all access points.""" dbus.clear() accesspoints = await network_manager.interfaces[ - "wlan0" + TEST_INTERFACE_WLAN ].wireless.get_all_accesspoints() assert len(accesspoints) == 2 assert accesspoints[0].mac == "E4:57:40:A9:D7:DE" @@ -49,3 +56,22 @@ async def test_get_all_access_points(network_manager: NetworkManager, dbus: list assert dbus == [ "/org/freedesktop/NetworkManager/Devices/3-org.freedesktop.NetworkManager.Device.Wireless.GetAllAccessPoints" ] + + +async def test_old_active_ap_disconnects(network_manager: NetworkManager): + """Test old access point disconnects on active ap change.""" + wireless = network_manager.interfaces[TEST_INTERFACE_WLAN].wireless + fire_property_change_signal( + wireless, + {"ActiveAccessPoint": "/org/freedesktop/NetworkManager/AccessPoint/43099"}, + ) + await asyncio.sleep(0) + + active = wireless.active + assert active.is_connected is True + + fire_property_change_signal(wireless, {"ActiveAccessPoint": "/"}) + await asyncio.sleep(0) + + assert wireless.active is None + assert active.is_connected is False