Re: [PATCH] Fix check for device state when reading its IPXConfig (#1001776)

[Radek Vykydal]

On 09/09/2013 10:23 AM, Vratislav Podzimek wrote:
> On Mon, 2013-09-09 at 09:41 +0200, Radek Vykydal wrote:
> > Device has to be in ACTIVATED state for IPXConfig to be valid.
> > use a recently added nm module function to get IP configuration, replacing
> >
> > python-dbus with GDBus here. Python-dbus would give us stale value.
> > 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                    | 77 ++++++++++++++++++++-----------------
> >   pyanaconda/ui/gui/spokes/network.py | 64 +++++++++++-------------------
> >   2 files changed, 63 insertions(+), 78 deletions(-)
> >
> > diff --git a/pyanaconda/nm.py b/pyanaconda/nm.py
> > index 1109d6e..4f0923d 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]
>
> I'd suggest using dbus_get_property_safe_sync from
> pyanaconda/safe_dbus.py. From what I recall using proxy.call_sync may
> cause troubles if it was used in a non-main thread. The _get_property
> function could stay almost the same, it should just handle exceptions
> raised from the safe_dbus module.

For now I'd assume that calling a method on a proxy synchronously is
safe, as the doc says:

"A GDBusProxy 
https://developer.gnome.org/gio/2.37/GDBusProxy.htmlinstance can be 
used from multiple threads but note that all signals (e.g. "g-signal" 
https://developer.gnome.org/gio/2.37/GDBusProxy.html#GDBusProxy-g-signal, 
"g-properties-changed" 
https://developer.gnome.org/gio/2.37/GDBusProxy.html#GDBusProxy-g-properties-changedand 
"notify" 
https://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.html#GObject-notify) 
are emitted in the thread-default main loop 
https://developer.gnome.org/glib/unstable/glib-The-Main-Event-Loop.html#g-main-context-push-thread-defaultof 
the thread where the instance was constructed."

and I'll get back to dbus_get_property_safe_sync or to Glib guys if
I hit similar issue. In this bz just calling DBus.Properties method instead
using the properties cached by the proxy seems to fix the problem.

(Checking the device state and then getting the config is racy anyway,
but this is another story with the race condition being harder to hit.)

I've peeked into the gdbus code and gdbus_call_proxy_sync is calling
g_dbus_connection_call_sync 
http://fossies.org/dox/glib-2.36.4/gdbusconnection_8c.html#a0140b7433268c65bd4c812d112316a66(called 
by safe_dbus) so the only
difference seems to be the connection object being used. With proxy
in nm.py the connection object seems to by created each time, while
in keyboard.py you are re-using a connection created in __init__ of
LocaledWrapper. Also as you said in your case the connection is
autostarting its service which is not the case with NM soperhaps
this might really be the problem I don't hit with NM service.
> > 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
> >
> > @@ -181,18 +194,14 @@ def nm_device_property(name, prop):
> >           raise
> >   device = device.unpack()[0]
> >
> > proxy = _get_proxy(object_path=device, interface_name="org.freedesktop.NetworkManager.Device")
> >
> > if prop in proxy.get_cached_property_names():
> >    retval = proxy.get_cached_property(prop).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 prop in proxy.get_cached_property_names():
> >            retval = proxy.get_cached_property(prop).unpack()
> >        else:
> >        retval = _get_property(device, prop, interface[30:])
> >        if not retval:
> >             raise PropertyNotFoundError(prop)
> >     else:
> >         raise PropertyNotFoundError(prop)
> >
> > @@ -284,10 +293,10 @@ def nm_device_ip_config(name, version=4):
> >           return []
> >   if version == 4:
> >    dbus_iface = "org.freedesktop.NetworkManager.IP4Config"
> >    dbus_iface = ".IP4Config"
> >     prop= "Ip4Config"
> > elif version == 6:
> >    dbus_iface = "org.freedesktop.NetworkManager.IP6Config"
> >    dbus_iface = ".IP6Config"
> >     prop= "Ip6Config"
> > else:
> >     return []
> >
> > @@ -296,9 +305,7 @@ def nm_device_ip_config(name, version=4):
> >       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,16 +341,15 @@ 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)
> >
> >     # NetworkManager does not request NTP/SNTP options for DHCP6
> >
> > print ntp_servers
>
> Debugging print?

Yes, thanks

Radek