[1/5] - [4/5] are straightforward ports from master [5/5] is a new one on top of them
I think I need review only for [5/5].
master commit 612d6b94116b477a3c48a5a161a1164cc4a2e176
Register signal instead of silly polling which even doesn't work in some cases (eg change ipv6 congig of a device to manual and switch it off and on -> only ipv6 config is showed) --- pyanaconda/ui/gui/spokes/network.py | 49 ++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 28 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/network.py b/pyanaconda/ui/gui/spokes/network.py index 051310e..a05e1a2 100644 --- a/pyanaconda/ui/gui/spokes/network.py +++ b/pyanaconda/ui/gui/spokes/network.py @@ -338,6 +338,8 @@ class NetworkControlBox(object):
def initialize(self): for device in self.client.get_devices(): + device.connect("notify::ip4-config", self.on_device_config_changed) + device.connect("notify::ip6-config", self.on_device_config_changed) self.add_device_to_list(device)
treeview = self.builder.get_object("treeview_devices") @@ -393,9 +395,12 @@ class NetworkControlBox(object): if new_state == NetworkManager.DeviceState.SECONDARIES: return self._refresh_carrier_info() - read_config_values = (new_state == NetworkManager.DeviceState.ACTIVATED) if device == self.selected_device(): - self.refresh_ui(device, read_config_values, new_state) + self.refresh_ui(device, new_state) + + def on_device_config_changed(self, device, *args): + if device == self.selected_device(): + self.refresh_ui(device)
def on_wireless_ap_changed_cb(self, combobox, *args): if self._updating_device: @@ -690,7 +695,7 @@ class NetworkControlBox(object): for row in rows_to_remove: del(row)
- def refresh_ui(self, device, read_config_values=True, state=None): + def refresh_ui(self, device, state=None):
if not device: notebook = self.builder.get_object("notebook_types") @@ -703,27 +708,12 @@ class NetworkControlBox(object): self._refresh_parent_vlanid(device) self._refresh_speed_hwaddr(device, state) self._refresh_ap(device, state) - if read_config_values: - num_of_tries = 3 - else: - num_of_tries = 0 - self._refresh_device_cfg((device, num_of_tries), state) + self._refresh_device_cfg(device, state)
- def _refresh_device_cfg(self, dev_tries, state): - device, num_of_tries = dev_tries - ipv4cfg = None - ipv6cfg = None + def _refresh_device_cfg(self, device, state):
- # We might need to wait for config objects to become available - if num_of_tries > 0: - ipv4cfg = device.get_ip4_config() - ipv6cfg = device.get_ip6_config() - if not ipv4cfg and not ipv6cfg: - GLib.timeout_add(300, - self._refresh_device_cfg, - (device, num_of_tries-1), - state) - return False + if state is None: + state = device.get_state()
dev_type = device.get_device_type() if dev_type == NetworkManager.DeviceType.ETHERNET: @@ -735,10 +725,14 @@ class NetworkControlBox(object): elif dev_type == NetworkManager.DeviceType.VLAN: dt = "wired"
- if state is None: - state = device.get_state() - if (ipv4cfg - and state == NetworkManager.DeviceState.ACTIVATED): + if state == NetworkManager.DeviceState.ACTIVATED: + ipv4cfg = device.get_ip4_config() + ipv6cfg = device.get_ip6_config() + else: + ipv4cfg = None + ipv6cfg = None + + if ipv4cfg: addr = socket.inet_ntoa(struct.pack('=L', ipv4cfg.get_addresses()[0].get_address())) self._set_device_info_value(dt, "ipv4", addr) @@ -764,8 +758,7 @@ class NetworkControlBox(object):
# TODO NM_GI_BUGS - segfaults on get_addres(), get_prefix() ipv6_addr = None - if (ipv6cfg - and state == NetworkManager.DeviceState.ACTIVATED): + if ipv6cfg: config = dbus.SystemBus().get_object(NM_SERVICE, ipv6cfg.get_path()) addr, prefix, gw = getNMObjProperty(config, ".IP6Config", "Addresses")[0]
master commit e0ba94b4c7abb6d1c7aa060a123eb1dbfecf15a5 --- pyanaconda/ui/gui/spokes/network.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/network.py b/pyanaconda/ui/gui/spokes/network.py index a05e1a2..eaee0f3 100644 --- a/pyanaconda/ui/gui/spokes/network.py +++ b/pyanaconda/ui/gui/spokes/network.py @@ -760,10 +760,13 @@ class NetworkControlBox(object): ipv6_addr = None if ipv6cfg: config = dbus.SystemBus().get_object(NM_SERVICE, ipv6cfg.get_path()) - addr, prefix, gw = getNMObjProperty(config, ".IP6Config", - "Addresses")[0] - ipv6_addr = nm_dbus_ay_to_ipv6(addr) - self._set_device_info_value(dt, "ipv6", ipv6_addr) + addr6_str = "" + for addr, _prefix, _gw in getNMObjProperty(config, ".IP6Config", "Addresses"): + ipv6_addr = nm_dbus_ay_to_ipv6(addr) + if not ipv6_addr.startswith("fe80:"): + addr6_str += "%s\n" % ipv6_addr + + self._set_device_info_value(dt, "ipv6", addr6_str.strip())
if ipv4cfg and ipv6_addr: self.builder.get_object("heading_%s_ipv4" % dt).set_label(_("IPv4 Address"))
master commit 7592d7f87427bbdd03422049e5bd221c5044b77f --- pyanaconda/ui/gui/spokes/network.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/network.py b/pyanaconda/ui/gui/spokes/network.py index eaee0f3..2d0c2c5 100644 --- a/pyanaconda/ui/gui/spokes/network.py +++ b/pyanaconda/ui/gui/spokes/network.py @@ -757,23 +757,22 @@ class NetworkControlBox(object): self._set_device_info_value(dt, "subnet", None)
# TODO NM_GI_BUGS - segfaults on get_addres(), get_prefix() - ipv6_addr = None + addr6_str = "" if ipv6cfg: config = dbus.SystemBus().get_object(NM_SERVICE, ipv6cfg.get_path()) - addr6_str = "" for addr, _prefix, _gw in getNMObjProperty(config, ".IP6Config", "Addresses"): ipv6_addr = nm_dbus_ay_to_ipv6(addr) if not ipv6_addr.startswith("fe80:"): addr6_str += "%s\n" % ipv6_addr
- self._set_device_info_value(dt, "ipv6", addr6_str.strip()) + self._set_device_info_value(dt, "ipv6", addr6_str.strip() or None)
- if ipv4cfg and ipv6_addr: + if ipv4cfg and addr6_str: self.builder.get_object("heading_%s_ipv4" % dt).set_label(_("IPv4 Address")) self.builder.get_object("heading_%s_ipv6" % dt).set_label(_("IPv6 Address")) elif ipv4cfg: self.builder.get_object("heading_%s_ipv4" % dt).set_label(_("IP Address")) - elif ipv6_addr: + elif addr6_str: self.builder.get_object("heading_%s_ipv6" % dt).set_label(_("IP Address"))
return False
master commit 612d6b94116b477a3c48a5a161a1164cc4a2e176
Device has to be in ACTIVATED state for IPXConfig to be valid.
1) use a recently added nm module function to get IP configuration, replacing python-dbus with GDBus here. Python-dbus would give us stale value.
2) Don't use get_cached_property of GDBus as it would give us stale value too. Instead use DBus.Properties interface call to get current values. --- pyanaconda/nm.py | 88 +++++++++++++++++++------------------ pyanaconda/ui/gui/spokes/network.py | 64 ++++++++++----------------- 2 files changed, 68 insertions(+), 84 deletions(-)
diff --git a/pyanaconda/nm.py b/pyanaconda/nm.py index f47cbe6..dd0df99 100644 --- a/pyanaconda/nm.py +++ b/pyanaconda/nm.py @@ -73,10 +73,27 @@ def _get_proxy(bus_type=Gio.BusType.SYSTEM, cancellable) return proxy
+def _get_property(object_path, prop, interface_name_suffix=""): + interface_name = "org.freedesktop.NetworkManager" + interface_name_suffix + proxy = _get_proxy(object_path=object_path, interface_name="org.freedesktop.DBus.Properties") + args = GLib.Variant('(ss)', (interface_name, prop)) + try: + prop = proxy.call_sync("Get", + args, + Gio.DBusCallFlags.NONE, + DEFAULT_DBUS_TIMEOUT, + None) + except GLib.GError as e: + if "org.freedesktop.DBus.Error.AccessDenied" in e.message: + return None + else: + raise + + return prop.unpack()[0] + def nm_state(): """Return state of NetworkManager""" - proxy = _get_proxy() - return proxy.get_cached_property("State").unpack() + return _get_property("/org/freedesktop/NetworkManager", "State")
# FIXME - use just GLOBAL? There is some connectivity checking # for GLOBAL in NM (nm_connectivity_get_connected), not sure if @@ -107,11 +124,11 @@ def nm_devices():
devices = devices.unpack()[0] for device in devices: - proxy = _get_proxy(object_path=device, interface_name="org.freedesktop.NetworkManager.Device") - device_type = proxy.get_cached_property("DeviceType").unpack() + device_type = _get_property(device, "DeviceType", ".Device") if device_type not in supported_device_types: continue - interfaces.append(proxy.get_cached_property("Interface").unpack()) + iface = _get_property(device, "Interface", ".Device") + interfaces.append(iface)
return interfaces
@@ -120,19 +137,15 @@ def nm_activated_devices():
interfaces = []
- proxy = _get_proxy() - active_connections = proxy.get_cached_property("ActiveConnections").unpack() + active_connections = _get_property("/org/freedesktop/NetworkManager", "ActiveConnections") for ac in active_connections: - proxy = _get_proxy(object_path=ac, interface_name="org.freedesktop.NetworkManager.Connection.Active") - state = proxy.get_cached_property("State") - if not state or state.unpack() != NetworkManager.ActiveConnectionState.ACTIVATED: - continue - devices = proxy.get_cached_property("Devices") - if not devices: + state = _get_property(ac, "State", ".Connection.Active") + if state != NetworkManager.ActiveConnectionState.ACTIVATED: continue - for device in devices.unpack(): - proxy = _get_proxy(object_path=device, interface_name="org.freedesktop.NetworkManager.Device") - interfaces.append(proxy.get_cached_property("Interface").unpack()) + devices = _get_property(ac, "Devices", ".Connection.Active") + for device in devices: + iface = _get_property(device, "Interface", ".Device") + interfaces.append(iface)
return interfaces
@@ -157,7 +170,7 @@ def _device_type_specific_interface(device): return iface return None
-def nm_device_property(name, property): +def nm_device_property(name, prop): """Return value of device NM property
Exceptions: @@ -181,21 +194,17 @@ def nm_device_property(name, property): raise
device = device.unpack()[0] - proxy = _get_proxy(object_path=device, interface_name="org.freedesktop.NetworkManager.Device")
- if property in proxy.get_cached_property_names(): - retval = proxy.get_cached_property(property).unpack() - else: + retval = _get_property(device, prop, ".Device") + if not retval: # Look in device type based interface interface = _device_type_specific_interface(device) if interface: - proxy = _get_proxy(object_path=device, interface_name=interface) - if property in proxy.get_cached_property_names(): - retval = proxy.get_cached_property(property).unpack() - else: - raise PropertyNotFoundError(property) + retval = _get_property(device, prop, interface[30:]) + if not retval: + raise PropertyNotFoundError(prop) else: - raise PropertyNotFoundError(property) + raise PropertyNotFoundError(prop)
return retval
@@ -284,21 +293,19 @@ def nm_device_ip_config(name, version=4): return []
if version == 4: - dbus_iface = "org.freedesktop.NetworkManager.IP4Config" - property = "Ip4Config" + dbus_iface = ".IP4Config" + prop= "Ip4Config" elif version == 6: - dbus_iface = "org.freedesktop.NetworkManager.IP6Config" - property = "Ip6Config" + dbus_iface = ".IP6Config" + prop= "Ip6Config" else: return []
- config = nm_device_property(name, property) + config = nm_device_property(name, prop) if config == "/": return []
- proxy = _get_proxy(object_path=config, interface_name=dbus_iface) - - addresses = proxy.get_cached_property("Addresses").unpack() + addresses = _get_property(config, "Addresses", dbus_iface) addr_list = [] for addr, prefix, gateway in addresses: # TODO - look for a library function (could have used IPy but byte order!) @@ -310,7 +317,7 @@ def nm_device_ip_config(name, version=4): gateway_str = nm_dbus_ay_to_ipv6(gateway) addr_list.append([addr_str, prefix, gateway_str])
- nameservers = proxy.get_cached_property("Nameservers").unpack() + nameservers = _get_property(config, "Nameservers", dbus_iface) ns_list = [] for ns in nameservers: # TODO - look for a library function @@ -322,7 +329,6 @@ def nm_device_ip_config(name, version=4):
return [addr_list, ns_list]
- def nm_ntp_servers_from_dhcp(): """Return a list of NTP servers that were specified the reply of the DHCP server or empty list if no NTP servers were returned. @@ -335,12 +341,10 @@ def nm_ntp_servers_from_dhcp(): for device in active_devices: # harvest NTP server addresses from DHCPv4 dhcp4_path = nm_device_property(device, "Dhcp4Config") - dhcp4_proxy = _get_proxy(object_path=dhcp4_path, - interface_name="org.freedesktop.NetworkManager.DHCP4Config") - options = dhcp4_proxy.get_cached_property("Options") - if options and 'ntp_servers' in options.unpack(): + options = _get_property(dhcp4_path, "Options", ".DHCP4Config") + if options and 'ntp_servers' in options: # NTP server addresses returned by DHCP are whitespace delimited - ntp_servers_string = options.unpack()["ntp_servers"] + ntp_servers_string = options["ntp_servers"] for ip in ntp_servers_string.split(" "): ntp_servers.append(ip)
diff --git a/pyanaconda/ui/gui/spokes/network.py b/pyanaconda/ui/gui/spokes/network.py index 2d0c2c5..ed8014d 100644 --- a/pyanaconda/ui/gui/spokes/network.py +++ b/pyanaconda/ui/gui/spokes/network.py @@ -44,7 +44,7 @@ from pyanaconda.ui.gui.utils import gtk_call_once, enlightbox from pyanaconda.ui.common import FirstbootSpokeMixIn
from pyanaconda import network -from pyanaconda.nm import nm_activated_devices, nm_device_setting_value, nm_dbus_ay_to_ipv6 +from pyanaconda.nm import nm_device_setting_value, nm_device_ip_config
# pylint: disable-msg=E0611 from gi.repository import GLib, GObject, Pango, Gio, NetworkManager, NMClient @@ -708,12 +708,9 @@ class NetworkControlBox(object): self._refresh_parent_vlanid(device) self._refresh_speed_hwaddr(device, state) self._refresh_ap(device, state) - self._refresh_device_cfg(device, state) + self._refresh_device_cfg(device)
- def _refresh_device_cfg(self, device, state): - - if state is None: - state = device.get_state() + def _refresh_device_cfg(self, device):
dev_type = device.get_device_type() if dev_type == NetworkManager.DeviceType.ETHERNET: @@ -725,45 +722,28 @@ class NetworkControlBox(object): elif dev_type == NetworkManager.DeviceType.VLAN: dt = "wired"
- if state == NetworkManager.DeviceState.ACTIVATED: - ipv4cfg = device.get_ip4_config() - ipv6cfg = device.get_ip6_config() - else: - ipv4cfg = None - ipv6cfg = None - - if ipv4cfg: - addr = socket.inet_ntoa(struct.pack('=L', - ipv4cfg.get_addresses()[0].get_address())) - self._set_device_info_value(dt, "ipv4", addr) - dnss = " ".join(socket.inet_ntoa(struct.pack('=L', addr)) - for addr in ipv4cfg.get_nameservers()) - self._set_device_info_value(dt, "dns", dnss) - gateway = socket.inet_ntoa(struct.pack('=L', - ipv4cfg.get_addresses()[0].get_gateway())) - self._set_device_info_value(dt, "route", gateway) - if dt == "wired": - prefix = ipv4cfg.get_addresses()[0].get_prefix() - nm_utils.nm_utils_ip4_prefix_to_netmask.argtypes = [ctypes.c_uint32] - nm_utils.nm_utils_ip4_prefix_to_netmask.restype = ctypes.c_uint32 - netmask = nm_utils.nm_utils_ip4_prefix_to_netmask(prefix) - netmask = socket.inet_ntoa(struct.pack('=L', netmask)) - self._set_device_info_value(dt, "subnet", netmask) + ipv4cfg = nm_device_ip_config(device.get_iface(), version=4) + ipv6cfg = nm_device_ip_config(device.get_iface(), version=6) + + if ipv4cfg and ipv4cfg[0]: + addr_str, prefix, gateway_str = ipv4cfg[0][0] + netmask_str = network.prefix2netmask(prefix) + dnss_str = " ".join(ipv4cfg[1]) else: - self._set_device_info_value(dt, "ipv4", None) - self._set_device_info_value(dt, "dns", None) - self._set_device_info_value(dt, "route", None) - if dt == "wired": - self._set_device_info_value(dt, "subnet", None) + addr_str = dnss_str = gateway_str = netmask_str = None + self._set_device_info_value(dt, "ipv4", addr_str) + self._set_device_info_value(dt, "dns", dnss_str) + self._set_device_info_value(dt, "route", gateway_str) + if dt == "wired": + self._set_device_info_value(dt, "subnet", netmask_str)
- # TODO NM_GI_BUGS - segfaults on get_addres(), get_prefix() addr6_str = "" - if ipv6cfg: - config = dbus.SystemBus().get_object(NM_SERVICE, ipv6cfg.get_path()) - for addr, _prefix, _gw in getNMObjProperty(config, ".IP6Config", "Addresses"): - ipv6_addr = nm_dbus_ay_to_ipv6(addr) - if not ipv6_addr.startswith("fe80:"): - addr6_str += "%s\n" % ipv6_addr + if ipv6cfg and ipv6cfg[0]: + for ipv6addr in ipv6cfg[0]: + addr_str, prefix, gateway_str = ipv6addr + # Do not display link-local addresses + if not addr_str.startswith("fe80:"): + addr6_str += "%s/%d\n" % (addr_str, prefix)
self._set_device_info_value(dt, "ipv6", addr6_str.strip() or None)
Fedora bz: #980576
Catch the infamous
DBusException: org.freedesktop.DBus.Error.UnknownMethod: Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn't exist
exception.
Object with path from devices's IPXConfig property is valid only if its device is in ACTIVATED state. --- pyanaconda/nm.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/pyanaconda/nm.py b/pyanaconda/nm.py index dd0df99..e91aa26 100644 --- a/pyanaconda/nm.py +++ b/pyanaconda/nm.py @@ -305,7 +305,15 @@ def nm_device_ip_config(name, version=4): if config == "/": return []
- addresses = _get_property(config, "Addresses", dbus_iface) + try: + addresses = _get_property(config, "Addresses", dbus_iface) + except GLib.GError as e: + # checking the device state (has to be ACTIVATED) is racy + if "org.freedesktop.DBus.Error.UnknownMethod" in e.message: + return [] + else: + raise + addr_list = [] for addr, prefix, gateway in addresses: # TODO - look for a library function (could have used IPy but byte order!) @@ -317,7 +325,15 @@ def nm_device_ip_config(name, version=4): gateway_str = nm_dbus_ay_to_ipv6(gateway) addr_list.append([addr_str, prefix, gateway_str])
- nameservers = _get_property(config, "Nameservers", dbus_iface) + try: + nameservers = _get_property(config, "Nameservers", dbus_iface) + except GLib.GError as e: + # checking the device state (has to be ACTIVATED) is racy + if "org.freedesktop.DBus.Error.UnknownMethod" in e.message: + return [] + else: + raise + ns_list = [] for ns in nameservers: # TODO - look for a library function
--- pyanaconda/nm.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/pyanaconda/nm.py b/pyanaconda/nm.py index e91aa26..24ca1aa 100644 --- a/pyanaconda/nm.py +++ b/pyanaconda/nm.py @@ -357,7 +357,14 @@ def nm_ntp_servers_from_dhcp(): for device in active_devices: # harvest NTP server addresses from DHCPv4 dhcp4_path = nm_device_property(device, "Dhcp4Config") - options = _get_property(dhcp4_path, "Options", ".DHCP4Config") + try: + options = _get_property(dhcp4_path, "Options", ".DHCP4Config") + except GLib.GError as e: + # checking the device state (has to be ACTIVATED) is racy + if "org.freedesktop.DBus.Error.UnknownMethod" in e.message: + options = None + else: + raise if options and 'ntp_servers' in options: # NTP server addresses returned by DHCP are whitespace delimited ntp_servers_string = options["ntp_servers"]
On Tue, 2013-09-10 at 12:05 +0200, Radek Vykydal wrote:
pyanaconda/nm.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/pyanaconda/nm.py b/pyanaconda/nm.py index e91aa26..24ca1aa 100644 --- a/pyanaconda/nm.py +++ b/pyanaconda/nm.py @@ -357,7 +357,14 @@ def nm_ntp_servers_from_dhcp(): for device in active_devices: # harvest NTP server addresses from DHCPv4 dhcp4_path = nm_device_property(device, "Dhcp4Config")
options = _get_property(dhcp4_path, "Options", ".DHCP4Config")
try:options = _get_property(dhcp4_path, "Options", ".DHCP4Config")except GLib.GError as e:# checking the device state (has to be ACTIVATED) is racyif "org.freedesktop.DBus.Error.UnknownMethod" in e.message:options = Noneelse:raise
We were discussing with Radek that it might be better to do the 'if "org.freedesktop.DBus.Error.UnknownMethod" in e.message:' check in the _get_property() method and raise some special "CannotCallGetError" instead that could be caught by the callers without doing the low-level check of GLib.GError's message.
Apart from this change all these look good to me.
On Tue, Sep 10, 2013 at 12:16:16PM +0200, Vratislav Podzimek wrote:
On Tue, 2013-09-10 at 12:05 +0200, Radek Vykydal wrote:
pyanaconda/nm.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/pyanaconda/nm.py b/pyanaconda/nm.py index e91aa26..24ca1aa 100644 --- a/pyanaconda/nm.py +++ b/pyanaconda/nm.py @@ -357,7 +357,14 @@ def nm_ntp_servers_from_dhcp(): for device in active_devices: # harvest NTP server addresses from DHCPv4 dhcp4_path = nm_device_property(device, "Dhcp4Config")
options = _get_property(dhcp4_path, "Options", ".DHCP4Config")
try:options = _get_property(dhcp4_path, "Options", ".DHCP4Config")except GLib.GError as e:# checking the device state (has to be ACTIVATED) is racyif "org.freedesktop.DBus.Error.UnknownMethod" in e.message:options = Noneelse:raiseWe were discussing with Radek that it might be better to do the 'if "org.freedesktop.DBus.Error.UnknownMethod" in e.message:' check in the _get_property() method and raise some special "CannotCallGetError" instead that could be caught by the callers without doing the low-level check of GLib.GError's message.
That sounds like a good idea to me.
anaconda-patches@lists.fedorahosted.org
-
Brian C. Lane -
Radek Vykydal -
Vratislav Podzimek