[PATCH] osmo-gsm-tester[master]: fix: refresh dbus object when interfaces have changed

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Thu May 25 01:05:33 UTC 2017


Review at  https://gerrit.osmocom.org/2733

fix: refresh dbus object when interfaces have changed

This solves the KeyError problems when we attempt to use new Interfaces that
have come up. The solution is to get a fresh pydbus object when interfaces have
been added.

Another key solution is to not completely discard and unregister all signals
every time. This is racy and may cause signals getting lost. If an interface
was not removed, it is not harmful to have it subscribed using an older pydbus
object. These older objects may linger until the specific signal subscriptions
are disconnected. It is important to fetch a new dbus object for subscribing to
signals on interfaces that have just been added.

Put signal subscription and property watching in a separate class
ModemDbusInteraction. This class may also be used without signals or a modem
config, in anticipation of the IMSI discovery patch that's coming up.

Related: OS#2233
Change-Id: Ia36b881c25976d7e69dbb587317dd139169ce3d9
---
M src/osmo_gsm_tester/bts_sysmo.py
M src/osmo_gsm_tester/ofono_client.py
2 files changed, 198 insertions(+), 98 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-gsm-tester refs/changes/33/2733/1

diff --git a/src/osmo_gsm_tester/bts_sysmo.py b/src/osmo_gsm_tester/bts_sysmo.py
index 771d303..038ef7a 100644
--- a/src/osmo_gsm_tester/bts_sysmo.py
+++ b/src/osmo_gsm_tester/bts_sysmo.py
@@ -60,10 +60,10 @@
             self.remote_dir = util.Dir(SysmoBts.REMOTE_DIR)
             self.remote_inst = util.Dir(self.remote_dir.child(os.path.basename(str(self.inst))))
 
-            self.run_remote('rm-remote-dir', ('test', '!', '-d', SysmoBts.REMOTE_DIR, '||', 'rm', '-rf', SysmoBts.REMOTE_DIR))
-            self.run_remote('mk-remote-dir', ('mkdir', '-p', SysmoBts.REMOTE_DIR))
-            self.run_local('scp-inst-to-sysmobts',
-                ('scp', '-r', str(self.inst), '%s@%s:%s' % (self.remote_user, self.remote_addr, str(self.remote_inst))))
+            #self.run_remote('rm-remote-dir', ('test', '!', '-d', SysmoBts.REMOTE_DIR, '||', 'rm', '-rf', SysmoBts.REMOTE_DIR))
+            #self.run_remote('mk-remote-dir', ('mkdir', '-p', SysmoBts.REMOTE_DIR))
+            #self.run_local('scp-inst-to-sysmobts',
+            #    ('scp', '-r', str(self.inst), '%s@%s:%s' % (self.remote_user, self.remote_addr, str(self.remote_inst))))
 
             remote_run_dir = self.remote_dir.child(SysmoBts.BTS_SYSMO_BIN)
             self.run_remote('mk-remote-run-dir', ('mkdir', '-p', remote_run_dir))
diff --git a/src/osmo_gsm_tester/ofono_client.py b/src/osmo_gsm_tester/ofono_client.py
index 1ff98a9..3e5ec63 100644
--- a/src/osmo_gsm_tester/ofono_client.py
+++ b/src/osmo_gsm_tester/ofono_client.py
@@ -73,6 +73,169 @@
     root = systembus_get('/')
     return sorted(root.GetModems())
 
+class ModemDbusInteraction(log.Origin):
+    '''Work around inconveniences specific to pydbus and ofono.
+    ofono adds and removes DBus interfaces and notifies about them.
+    Upon changes we need a fresh pydbus object to benefit from that.
+    Watching the interfaces change is optional; be sure to call
+    watch_interfaces() if you'd like to have signals subscribed.
+    '''
+
+    def __init__(self, modem_path):
+        self.modem_path = modem_path
+        self.set_name(self.modem_path)
+        self.set_log_category(log.C_BUS)
+        self.watch_props_subscription = None
+        self._dbus_obj = None
+        self.interfaces = set()
+
+        # A dict listing signal handlers to connect, e.g.
+        # { I_SMS: ( ('IncomingMessage', self._on_incoming_message), ), }
+        self.required_signals = {}
+
+        # A dict collecting subscription tokens for connected signal handlers.
+        # { I_SMS: ( token1, token2, ... ), }
+        self.connected_signals = util.listdict()
+
+    def __del__(self):
+        self.unwatch_interfaces()
+        for interface_name in list(self.connected_signals.keys()):
+            self.remove_signals(interface_name)
+
+    def get_new_dbus_obj(self):
+        return systembus_get(self.modem_path)
+
+    def dbus_obj(self):
+        if self._dbus_obj is None:
+            self._dbus_obj = self.get_new_dbus_obj()
+        return self._dbus_obj
+
+    def interface(self, interface_name):
+        return self.dbus_obj()[interface_name]
+
+    def signal(self, interface_name, signal):
+        return getattr(self.interface(interface_name), signal)
+
+    def watch_interfaces(self):
+        self.unwatch_interfaces()
+        # Note: we are watching the properties on a get_new_dbus_obj() that is
+        # separate from the one used to interact with interfaces.  We need to
+        # refresh the pydbus object to interact with Interfaces that have newly
+        # appeared, but exchanging the DBus object to watch Interfaces being
+        # enabled and disabled is racy: we may skip some removals and
+        # additions. Hence do not exchange this DBus object. We don't even
+        # need to store the dbus object used for this, we will not touch it
+        # again. We only store the signal subscription.
+        self.watch_props_subscription = dbus_connect(self.get_new_dbus_obj().PropertyChanged,
+                                                     self.on_property_change)
+        self.on_interfaces_change(self.properties().get('Interfaces'))
+
+    def unwatch_interfaces(self):
+        if self.watch_props_subscription is None:
+            return
+        self.watch_props_subscription.disconnect()
+        self.watch_props_subscription = None
+
+    def on_property_change(self, name, value):
+        if name == 'Interfaces':
+            self.on_interfaces_change(value)
+
+    def on_interfaces_change(self, interfaces_now):
+        # First some logging.
+        now = set(interfaces_now)
+        additions = now - self.interfaces
+        removals = self.interfaces - now
+        self.interfaces = now
+        if not (additions or removals):
+            # nothing changed.
+            return
+
+        if additions:
+            self.dbg('interface enabled:', ', '.join(sorted(additions)))
+
+        if removals:
+            self.dbg('interface disabled:', ', '.join(sorted(removals)))
+
+        # The dbus object is now stale and needs refreshing before we
+        # access the next interface function.
+        self._dbus_obj = None
+
+        # If an interface disappeared, disconnect the signal handlers for it.
+        # Even though we're going to use a fresh dbus object for new
+        # subscriptions, we will still keep active subscriptions alive on the
+        # old dbus object which will linger, associated with the respective
+        # signal subscription.
+        for removed in removals:
+            self.remove_signals(removed)
+
+        # Connect signals for added interfaces.
+        for interface_name in additions:
+            self.connect_signals(interface_name)
+
+    def remove_signals(self, interface_name):
+        got = self.connected_signals.get(interface_name, [])
+
+        if not got:
+            return
+
+        self.dbg('Disconnecting', len(got), 'signals for', interface_name)
+        for subscription in self.connected_signals.get(interface_name, []):
+            subscription.disconnect()
+
+    def connect_signals(self, interface_name):
+        # If an interface was added, it must not have existed before. For
+        # paranoia, make sure we have no handlers for those.
+        self.remove_signals(interface_name)
+
+        want = self.required_signals.get(interface_name, [])
+        if not want:
+            return
+
+        self.dbg('Connecting', len(want), 'signals for', interface_name)
+        for signal, cb in self.required_signals.get(interface_name, []):
+            subscription = dbus_connect(self.signal(interface_name, signal), cb)
+            self.connected_signals.add(interface_name, subscription)
+
+    def has_interface(self, *interface_names):
+        try:
+            for interface_name in interface_names:
+                self.interface(interface_name)
+            result = True
+        except KeyError:
+            result = False
+        self.dbg('has_interface(%s) ==' % (', '.join(interface_names)), result)
+        return result
+
+    def properties(self, iface=I_MODEM):
+        return self.dbus_obj()[iface].GetProperties()
+
+    def property_is(self, name, val, iface=I_MODEM):
+        is_val = self.properties(iface).get(name)
+        self.dbg(name, '==', is_val, ', is', val, '->', is_val is not None and is_val == val)
+        return is_val is not None and is_val == val
+
+    def set_bool(self, name, bool_val, iface=I_MODEM):
+        # to make sure any pending signals are received before we send out more DBus requests
+        event_loop.poll()
+
+        val = bool(bool_val)
+        self.log('Setting', name, val)
+        self.interface(iface).SetProperty(name, Variant('b', val))
+
+        event_loop.wait(self, self.property_is, name, bool_val)
+
+    def set_powered(self, on=True):
+        self.set_bool('Powered', on)
+
+    def set_online(self, on=True):
+        self.set_bool('Online', on)
+
+    def is_powered(self):
+        return self.property_is('Powered', True)
+
+    def is_online(self, *args, **kwargs):
+        return self.property_is('Online', True)
+
 
 class Modem(log.Origin):
     'convenience for ofono Modem interaction'
@@ -83,14 +246,28 @@
         self.conf = conf
         self.path = conf.get('path')
         self.set_name(self.path)
-        self.set_log_category(log.C_BUS)
-        self._dbus_obj = None
-        self._interfaces = set()
-        self._connected_signals = util.listdict()
+        self.set_log_category(log.C_TST)
         self.sms_received_list = []
-        # init interfaces and connect to signals:
-        self.dbus_obj()
-        event_loop.poll()
+        self.dbus = ModemDbusInteraction(self.path)
+        self.dbus.required_signals = {
+                I_SMS: ( ('IncomingMessage', self._on_incoming_message), ),
+            }
+        self.dbus.watch_interfaces()
+
+    def properties(self, *args, **kwargs):
+        return self.dbus.properties(*args, **kwargs)
+
+    def set_powered(self, *args, **kwargs):
+        return self.dbus.set_powered(*args, **kwargs)
+
+    def set_online(self, *args, **kwargs):
+        return self.dbus.set_online(*args, **kwargs)
+
+    def is_powered(self, *args, **kwargs):
+        return self.dbus.is_powered(*args, **kwargs)
+
+    def is_online(self, *args, **kwargs):
+        return self.dbus.is_online(*args, **kwargs)
 
     def set_msisdn(self, msisdn):
         self.msisdn = msisdn
@@ -105,107 +282,30 @@
     def ki(self):
         return self.conf.get('ki')
 
-    def _dbus_set_bool(self, name, bool_val, iface=I_MODEM):
-        # to make sure any pending signals are received before we send out more DBus requests
-        event_loop.poll()
-
-        val = bool(bool_val)
-        self.log('Setting', name, val)
-        self.dbus_obj()[iface].SetProperty(name, Variant('b', val))
-
-        event_loop.wait(self, self.property_is, name, bool_val)
-
-    def property_is(self, name, val):
-        is_val = self.properties().get(name)
-        self.dbg(name, '==', is_val)
-        return is_val is not None and is_val == val
-
-    def set_powered(self, on=True):
-        self._dbus_set_bool('Powered', on)
-
-    def set_online(self, on=True):
-        self._dbus_set_bool('Online', on)
-
-    def dbus_obj(self):
-        if self._dbus_obj is not None:
-            return self._dbus_obj
-        self._dbus_obj = systembus_get(self.path)
-        dbus_connect(self._dbus_obj.PropertyChanged, self._on_property_change)
-        self._on_interfaces_change(self.properties().get('Interfaces'))
-        return self._dbus_obj
-
-    def properties(self, iface=I_MODEM):
-        return self.dbus_obj()[iface].GetProperties()
-
-    def _on_property_change(self, name, value):
-        if name == 'Interfaces':
-            self._on_interfaces_change(value)
-
-    def _on_interfaces_change(self, interfaces_now):
-        now = set(interfaces_now)
-        additions = now - self._interfaces
-        removals = self._interfaces - now
-        self._interfaces = now
-        for iface in removals:
-            self._on_interface_disabled(iface)
-        for iface in additions:
-            self._on_interface_enabled(iface)
-
-    def _disconnect(self, interface_name):
-        subscriptions = self._connected_signals.pop(interface_name, [])
-        if subscriptions:
-            self.dbg('Disconnecting', len(subscriptions), 'signals from', interface_name)
-        for subscription in subscriptions:
-            subscription.disconnect()
-
-    def _on_interface_enabled(self, interface_name):
-        self.dbg('Interface enabled:', interface_name)
-
-        all_wanted_conns = {
-                I_SMS: ( ('IncomingMessage', self._on_incoming_message), ),
-            }
-
-        want = all_wanted_conns.get(interface_name)
-        if not want:
-            return
-
-        # sanity
-        self._disconnect(interface_name)
-
-        self.dbg('Connecting', len(want), 'signals to', interface_name)
-        for signal, cb in want:
-            dbus_iface = getattr(self.dbus_obj()[interface_name], signal)
-            self._connected_signals.add(interface_name, dbus_connect(dbus_iface, cb))
-
-    def _on_interface_disabled(self, interface_name):
-        self.dbg('Interface disabled:', interface_name)
-        self._disconnect(interface_name)
-
-    def has_interface(self, name):
-        return name in self._interfaces
-
     def connect(self, nitb):
         'set the modem up to connect to MCC+MNC from NITB config'
         self.log('connect to', nitb)
-        prepowered = self.properties().get('Powered')
-        if prepowered is not None and prepowered:
+        if self.is_powered():
+            self.dbg('is powered')
             self.set_online(False)
             self.set_powered(False)
+            event_loop.wait(self, lambda: not self.dbus.has_interface(I_NETREG, I_SMS), timeout=10)
         self.set_powered()
         self.set_online()
-        if not self.has_interface(I_NETREG):
-            self.log('No %r interface, hoping that the modem connects by itself' % I_NETREG)
-        else:
-            self.log('Use of %r interface not implemented yet, hoping that the modem connects by itself' % I_NETREG)
+        event_loop.wait(self, self.dbus.has_interface, I_NETREG, I_SMS, timeout=10)
 
     def sms_send(self, to_msisdn, *tokens):
+        # shim: if the caller passed a modem object instead of an MSISDN string
         if hasattr(to_msisdn, 'msisdn'):
             to_msisdn = to_msisdn.msisdn
+            # and maybe we can tickle some more info out of it?
+            if hasattr(to_msisdn, 'name'):
+                tokens.append('to ' + to_msisdn.name())
         sms = Sms(self.msisdn, to_msisdn, 'from ' + self.name(), *tokens)
         self.log('sending sms to MSISDN', to_msisdn, sms=sms)
-        if not self.has_interface(I_SMS):
+        if not self.dbus.has_interface(I_SMS):
             raise RuntimeError('Modem cannot send SMS, interface not active: %r' % I_SMS)
-        mm = self.dbus_obj()[I_SMS]
+        mm = self.dbus.interface(I_SMS)
         mm.SendMessage(to_msisdn, str(sms))
         return sms
 

-- 
To view, visit https://gerrit.osmocom.org/2733
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia36b881c25976d7e69dbb587317dd139169ce3d9
Gerrit-PatchSet: 1
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>



More information about the gerrit-log mailing list