Merge pull request #468 from martin-belanger/code-review-improvements

code-review: fix bugs, modernize, and expand test coverage

Author: martin-belanger

.github/workflows/linters.yml

@@ -121,7 +121,7 @@ jobs:
 
       - name: "Ruff: lint"
         run: |
-          ruff check .build/stacctl .build/stacd .build/stafctl .build/stafd .build/stasadm .build/staslib
+          ruff check --diff .build/stacctl .build/stacd .build/stafctl .build/stafd .build/stasadm .build/staslib
 
       - name: "Ruff: format"
         if: always()

README.md

@@ -96,6 +96,22 @@ Required user-space dependencies include:
 
 This ensures consistency with other NVMe tools like `nvme-cli` and `libnvme`. Alternative values can be set in `/etc/stas/sys.conf`.
 
+## D-Bus Security and Trust
+
+Both *stafd* and *stacd* communicate over the **system D-Bus bus**. D-Bus policy
+files (installed under `/usr/share/dbus-1/system.d/`) control which callers are
+allowed to own or interact with the service names. Only processes running as
+**root** (or with explicit D-Bus policy permission) can invoke the service
+interfaces.
+
+*stacd* subscribes to D-Bus signals emitted by *stafd* (`log_pages_changed` and
+`dc_removed`). These signals arrive over the trusted system bus — *stacd* does
+not accept instructions from untrusted sources.
+
+The host credentials written to `/run/nvme-stas/` by the daemons (e.g. the
+last-known-config pickle file) are protected by root-only filesystem
+permissions. Do not grant untrusted users write access to that directory.
+
 ## Build, Install & Tests
 
 This Python project uses **Meson** as its build system:

stacctl.py

@@ -96,6 +96,10 @@ def ls(args):
     print(f'nvme-stas {defs.VERSION}')
     sys.exit(0)
 
+if not hasattr(ARGS, 'func'):
+    PARSER.print_usage()
+    sys.exit(1)
+
 try:
     ARGS.func(ARGS)
 except dasbus.error.DBusError:

stafctl.py

@@ -197,6 +197,10 @@ def adlp(args):
     print(f'nvme-stas {defs.VERSION}')
     sys.exit(0)
 
+if not hasattr(ARGS, 'func'):
+    PARSER.print_usage()
+    sys.exit(1)
+
 try:
     ARGS.func(ARGS)
 except dasbus.error.DBusError:

stasadm.py

@@ -62,6 +62,10 @@ def get_machine_app_specific(app_id):
     if not data:
         return None
 
+    # app_id is a public domain-separation constant (per systemd's design).
+    # It intentionally does NOT need to be secret: its only purpose is to
+    # make the derived ID unique to this application so that two different
+    # applications deriving from the same machine-id produce different values.
     hmac_obj = hmac.new(app_id, uuid.UUID(data).bytes, hashlib.sha256)
     id128_bytes = hmac_obj.digest()[0:16]
     return str(uuid.UUID(bytes=id128_bytes, version=4))
@@ -196,8 +200,8 @@ def get_parser():
     print(f'nvme-stas {defs.VERSION}')
     sys.exit(0)
 
-try:
-    ARGS.cmd(ARGS)
-except AttributeError as ex:
-    print(str(ex))
+if not hasattr(ARGS, 'cmd'):
     PARSER.print_usage()
+    sys.exit(1)
+
+ARGS.cmd(ARGS)

staslib/avahi.py

@@ -13,7 +13,6 @@
 import socket
 import typing
 import logging
-import functools
 import dasbus.error
 import dasbus.connection
 import dasbus.client.proxy
@@ -29,7 +28,7 @@ def _txt2dict(txt: list):
     the_dict = dict()
     for list_of_chars in txt:
         try:
-            string = functools.reduce(lambda accumulator, c: accumulator + chr(c), list_of_chars, '')
+            string = ''.join(chr(c) for c in list_of_chars)
             if string.isprintable():
                 key, val = string.split('=')
                 if key:  # Make sure the key is not an empty string
@@ -83,6 +82,8 @@ def __init__(self, values: list):
 
     def get_next(self):
         '''Get the next value (or last value if ceiling was reached)'''
+        if not self._values:
+            return 0
         value = self._values[self._index]
         if self._index >= 0:
             self._index += 1
@@ -146,7 +147,7 @@ def __init__(self, args, identified_cback):
 
     def info(self):
         '''Return debug info'''
-        info = self._data
+        info = dict(self._data)
         info['reachable'] = str(self._reachable)
         return info
 
@@ -417,7 +418,7 @@ def kick_start(self):
         self._kick_avahi_tmr.clear()
 
     def _remove_service(self, service_to_rm: typing.Tuple[int, int, str, str, str]):
-        service = self._services.pop(service_to_rm)
+        service = self._services.pop(service_to_rm, None)
         if service is not None:
             service.close()
 

staslib/conf.py

@@ -66,7 +66,8 @@ def _to_int(text):
 
 
 def _to_bool(text, positive='true'):
-    return _parse_single_val(text).lower() == positive
+    val = _parse_single_val(text)
+    return val is not None and val.lower() == positive
 
 
 def _to_ncc(text):
@@ -77,7 +78,7 @@ def _to_ncc(text):
 
 
 def _to_ip_family(text):
-    return tuple((4 if text == 'ipv4' else 6 for text in _parse_single_val(text).split('+')))
+    return tuple((4 if token == 'ipv4' else 6 for token in _parse_single_val(text).split('+')))
 
 
 # ******************************************************************************
@@ -86,6 +87,11 @@ class OrderedMultisetDict(dict):
     and allow multiple configuration parameters with the same key. The
     result is a list of values, where values are sorted by the order they
     appear in the file.
+
+    Note: configparser internally joins repeated keys with '\\n' when
+    strict=False. __getitem__ splits on '\\n' to reconstruct the list, so
+    callers always receive a list of strings rather than a single newline-
+    joined string.
     '''
 
     def __setitem__(self, key, value):
@@ -214,12 +220,6 @@ class SvcConf(metaclass=singleton.Singleton):
                 'convert': _parse_list,
                 'default': [],
             },
-            ### BEGIN: LEGACY SECTION TO BE REMOVED ###
-            'blacklist': {
-                'convert': _parse_list,
-                'default': [],
-            },
-            ### END: LEGACY SECTION TO BE REMOVED ###
         },
     }
 
@@ -316,6 +316,10 @@ def persistent_connections(self):
         section = 'Discovery controller connection management'
         option = 'persistent-connections'
 
+        # Use ignore_default=True so we can distinguish "not set in file" from
+        # "set to false". The per-daemon default (stafd vs stacd) differs and is
+        # held in self._defaults rather than in OPTION_CHECKER, so we fall back
+        # to that dict explicitly.
         value = self.get_option(section, option, ignore_default=True)
         if value is not None:
             return value
@@ -379,11 +383,6 @@ def get_excluded(self):
         }
         '''
         controller_list = self.get_option('Controllers', 'exclude')
-
-        # 2022-09-20: Look for "blacklist". This is for backwards compatibility
-        # with releases 1.0 to 1.1.x. This is to be phased out (i.e. remove by 2024)
-        controller_list += self.get_option('Controllers', 'blacklist')
-
         excluded = [_parse_controller(controller) for controller in controller_list]
         for controller in excluded:
             controller.pop('host-traddr', None)  # remove host-traddr
@@ -544,8 +543,11 @@ def hostnqn(self):
         except FileNotFoundError as ex:
             sys.exit(f'Error reading mandatory Host NQN (see stasadm --help): {ex}')
 
-        if value is not None and not value.startswith('nqn.'):
-            sys.exit(f'Error Host NQN "{value}" should start with "nqn."')
+        if value is not None:
+            if not value.startswith('nqn.'):
+                sys.exit(f'Error Host NQN "{value}" should start with "nqn."')
+            if len(value) > 223:
+                sys.exit(f'Error Host NQN is too long ({len(value)} chars, max 223 per NVMe spec)')
 
         return value
 

staslib/ctrl.py

@@ -170,7 +170,8 @@ def _on_ctrl_removed(self, udev_obj):
         self._ctrl = None
         self._device = None
         self._connect_attempts = 0
-        self._retry_connect_tmr.start()
+        # Reset the retry interval to fast since this is effectively a fresh start
+        self._retry_connect_tmr.start(self.FAST_CONNECT_RETRY_PERIOD_SEC)
 
     def _get_cfg(self):
         '''Get configuration parameters. These may either come from the [Global]

staslib/gutil.py

@@ -57,7 +57,16 @@ def stop(self):
             self._source = None
 
     def start(self, new_interval_sec: float = -1.0):
-        '''@brief Start (or restart) timer'''
+        '''@brief Start (or restart) timer.
+
+        If the timer is not yet running, create and attach a new GLib source
+        that will fire after @new_interval_sec (or the previously set interval
+        if @new_interval_sec is negative).
+
+        If the timer is already running, reschedule it to fire @new_interval_sec
+        from NOW (i.e. the deadline is reset relative to the current monotonic
+        clock, not the original start time).
+        '''
         if new_interval_sec >= 0:
             self._interval_sec = float(new_interval_sec)
 

staslib/iputil.py

@@ -136,9 +136,9 @@ def ip_equal(ip1, ip2):
     @param ip1: IPv4Address or IPv6Address object
     @param ip2: IPv4Address or IPv6Address object
     '''
-    if not isinstance(ip1, ipaddress._BaseAddress):
+    if not isinstance(ip1, (ipaddress.IPv4Address, ipaddress.IPv6Address)):
         return False
-    if not isinstance(ip2, ipaddress._BaseAddress):
+    if not isinstance(ip2, (ipaddress.IPv4Address, ipaddress.IPv6Address)):
         return False
 
     if ip1.version == 4 and ip2.version == 6:
@@ -256,7 +256,7 @@ def get_interface(ifaces: dict, src_addr):
     @param ifaces: Interface info previously returned by @net_if_addrs()
     @param src_addr: IPv4Address or IPv6Address object
     '''
-    if not isinstance(src_addr, ipaddress._BaseAddress):
+    if not isinstance(src_addr, (ipaddress.IPv4Address, ipaddress.IPv6Address)):
         return ''
 
     for iface, addr_map in ifaces.items():