From dea4a481ea25e446272b8220f866aaa8ede1dcb8 Mon Sep 17 00:00:00 2001 From: Jake Hunsaker Date: Tue, 21 Jun 2022 14:43:30 -0400 Subject: [PackageManager] Make pkg_by_name() more predictable As highlighted in #1817, `pkg_by_name()` could provide unpredictable results, when using wildcards. As such, limited this method to only returning package info for exact package name matches. In turn, change `Plugin.is_installed()` to leverage `PackageManager.all_pkgs_by_name()` which does explicitly support wildcards and returns information on _all_ matching packages, not just the last one found. In so doing, clean up the `PackageManager` design to use a new `packages` property for these lookups, and update the former usage of `all_pkgs()` accordingly. Similarly, signal `get_pkg_list()` should be private (in any sense that a python method can be) by renaming to `_get_pkg_list()` and update the single Plugin (`etcd`) referencing this method. Closes: #1817 Signed-off-by: Jake Hunsaker --- sos/policies/distros/redhat.py | 2 +- sos/policies/distros/suse.py | 4 +-- sos/policies/package_managers/__init__.py | 49 +++++++++++++------------------ sos/report/plugins/__init__.py | 4 ++- sos/report/plugins/etcd.py | 2 +- sos/report/plugins/networking.py | 2 +- tests/unittests/policy_tests.py | 2 +- 7 files changed, 28 insertions(+), 37 deletions(-) diff --git a/sos/policies/distros/redhat.py b/sos/policies/distros/redhat.py index a6ac5efc..b3cc675d 100644 --- a/sos/policies/distros/redhat.py +++ b/sos/policies/distros/redhat.py @@ -62,7 +62,7 @@ class RedHatPolicy(LinuxPolicy): self.valid_subclasses += [RedHatPlugin] - self.pkgs = self.package_manager.all_pkgs() + self.pkgs = self.package_manager.packages # If rpm query failed, exit if not self.pkgs: diff --git a/sos/policies/distros/suse.py b/sos/policies/distros/suse.py index d1b5ea77..7fa0b0c9 100644 --- a/sos/policies/distros/suse.py +++ b/sos/policies/distros/suse.py @@ -32,10 +32,8 @@ class SuSEPolicy(LinuxPolicy): self.usrmove = False self.package_manager = RpmPackageManager() - pkgs = self.package_manager.all_pkgs() - # If rpm query timed out after timeout duration exit - if not pkgs: + if not self.package_manager.packages: print("Could not obtain installed package list", file=sys.stderr) sys.exit(1) diff --git a/sos/policies/package_managers/__init__.py b/sos/policies/package_managers/__init__.py index 42eb9447..8a5c4ee5 100644 --- a/sos/policies/package_managers/__init__.py +++ b/sos/policies/package_managers/__init__.py @@ -22,7 +22,7 @@ class PackageManager(): package name|package.version - You may also subclass this class and provide a get_pkg_list method to + You may also subclass this class and provide a _generate_pkg_list method to build the list of packages and versions. :cvar query_command: The command to use for querying packages @@ -56,7 +56,7 @@ class PackageManager(): def __init__(self, chroot=None, query_command=None, verify_command=None, verify_filter=None, files_command=None, remote_exec=None): - self.packages = {} + self._packages = {} self.files = [] self.query_command = query_command or self.query_command @@ -75,6 +75,12 @@ class PackageManager(): if chroot: self.chroot = chroot + @property + def packages(self): + if not self._packages: + self._generate_pkg_list() + return self._packages + def all_pkgs_by_name(self, name): """ Get a list of packages that match name. @@ -85,7 +91,7 @@ class PackageManager(): :returns: List of all packages matching `name` :rtype: ``list`` """ - return fnmatch.filter(self.all_pkgs().keys(), name) + return fnmatch.filter(self.packages.keys(), name) def all_pkgs_by_name_regex(self, regex_name, flags=0): """ @@ -100,7 +106,7 @@ class PackageManager(): :rtype: ``list`` """ reg = re.compile(regex_name, flags) - return [pkg for pkg in self.all_pkgs().keys() if reg.match(pkg)] + return [pkg for pkg in self.packages.keys() if reg.match(pkg)] def pkg_by_name(self, name): """ @@ -112,15 +118,14 @@ class PackageManager(): :returns: The first package that matches `name` :rtype: ``str`` """ - pkgmatches = self.all_pkgs_by_name(name) - if (len(pkgmatches) != 0): - return self.all_pkgs_by_name(name)[-1] - else: + try: + return self.packages[name] + except Exception: return None - def get_pkg_list(self): - """Returns a dictionary of packages in the following - format:: + def _generate_pkg_list(self): + """Generates a dictionary of packages for internal use by the package + manager in the format:: {'package_name': {'name': 'package_name', 'version': 'major.minor.version'}} @@ -140,14 +145,12 @@ class PackageManager(): release = None elif pkg.count("|") == 2: name, version, release = pkg.split("|") - self.packages[name] = { + self._packages[name] = { 'name': name, 'version': version.split(".") } release = release if release else None - self.packages[name]['release'] = release - - return self.packages + self._packages[name]['release'] = release def pkg_version(self, pkg): """Returns the entry in self.packages for pkg if it exists @@ -158,22 +161,10 @@ class PackageManager(): :returns: Package name and version, if package exists :rtype: ``dict`` if found, else ``None`` """ - pkgs = self.all_pkgs() - if pkg in pkgs: - return pkgs[pkg] + if pkg in self.packages: + return self.packages[pkg] return None - def all_pkgs(self): - """ - Get a list of all packages. - - :returns: All packages, with name and version, installed on the system - :rtype: ``dict`` - """ - if not self.packages: - self.packages = self.get_pkg_list() - return self.packages - def pkg_nvra(self, pkg): """Get the name, version, release, and architecture for a package diff --git a/sos/report/plugins/__init__.py b/sos/report/plugins/__init__.py index ba1397a8..eed656ae 100644 --- a/sos/report/plugins/__init__.py +++ b/sos/report/plugins/__init__.py @@ -987,7 +987,9 @@ class Plugin(): :returns: ``True`` id the package is installed, else ``False`` :rtype: ``bool`` """ - return self.policy.pkg_by_name(package_name) is not None + return ( + len(self.policy.package_manager.all_pkgs_by_name(package_name)) > 0 + ) def is_service(self, name): """Does the service $name exist on the system? diff --git a/sos/report/plugins/etcd.py b/sos/report/plugins/etcd.py index fe017e9f..d1b7ff90 100644 --- a/sos/report/plugins/etcd.py +++ b/sos/report/plugins/etcd.py @@ -71,7 +71,7 @@ class etcd(Plugin, RedHatPlugin): # assume v3 is the default url = 'http://localhost:2379' try: - ver = self.policy.package_manager.get_pkg_list()['etcd'] + ver = self.policy.package_manager.packages['etcd'] ver = ver['version'][0] if ver == '2': url = 'http://localhost:4001' diff --git a/sos/report/plugins/networking.py b/sos/report/plugins/networking.py index 2ab5d840..cfe3d70f 100644 --- a/sos/report/plugins/networking.py +++ b/sos/report/plugins/networking.py @@ -261,7 +261,7 @@ class RedHatNetworking(Networking, RedHatPlugin): def setup(self): # Handle change from -T to -W in Red Hat netstat 2.0 and greater. try: - netstat_pkg = self.policy.package_manager.all_pkgs()['net-tools'] + netstat_pkg = self.policy.package_manager.packages['net-tools'] # major version if int(netstat_pkg['version'][0]) < 2: self.ns_wide = "-T" diff --git a/tests/unittests/policy_tests.py b/tests/unittests/policy_tests.py index 6d0c42b9..81210815 100644 --- a/tests/unittests/policy_tests.py +++ b/tests/unittests/policy_tests.py @@ -84,7 +84,7 @@ class PackageManagerTests(unittest.TestCase): self.pm = PackageManager() def test_default_all_pkgs(self): - self.assertEquals(self.pm.all_pkgs(), {}) + self.assertEquals(self.pm.packages, {}) def test_default_all_pkgs_by_name(self): self.assertEquals(self.pm.all_pkgs_by_name('doesntmatter'), []) -- cgit