Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/stratis_cli/_actions/_formatting.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,17 @@

# isort: STDLIB
import sys
from functools import wraps
from typing import Any, Callable, List, Optional
from uuid import UUID

# isort: THIRDPARTY
from dbus import Struct
from wcwidth import wcswidth

# isort: FIRSTPARTY
from dbus_client_gen import DbusClientMissingPropertyError

# placeholder for tables where a desired value was not obtained from stratisd
# when the value should be supported.
TABLE_FAILURE_STRING = "FAILURE"
Expand Down Expand Up @@ -160,3 +164,23 @@ def get_uuid_formatter(unhyphenated: bool) -> Callable:
return (
(lambda u: UUID(str(u)).hex) if unhyphenated else (lambda u: str(UUID(str(u))))
)


def catch_missing_property(
prop_to_str: Callable[[Any], Any], default: Any
) -> Callable[[Any], Any]:
"""
Return a function to just return a default if a property is missing.
"""

@wraps(prop_to_str)
def inner(mo: Any) -> str:
"""
Catch the exception and return a default
"""
try:
return prop_to_str(mo)
except DbusClientMissingPropertyError:
return default

return inner
91 changes: 70 additions & 21 deletions src/stratis_cli/_actions/_list_filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@
from ._constants import TOP_OBJECT
from ._formatting import (
TABLE_FAILURE_STRING,
TABLE_UNKNOWN_STRING,
TOTAL_USED_FREE,
catch_missing_property,
get_property,
print_table,
)
Expand Down Expand Up @@ -124,7 +126,12 @@ def size_triple(mofs: Any) -> SizeTriple:
"""
Calculate size triple
"""
return SizeTriple(Range(mofs.Size()), get_property(mofs.Used(), Range, None))
return SizeTriple(
catch_missing_property(lambda mo: Range(mo.Size()), None)(mofs),
catch_missing_property(
lambda mo: get_property(mo.Used(), Range, None), None
)(mofs),
)


class Table(ListFilesystem): # pylint: disable=too-few-public-methods
Expand Down Expand Up @@ -158,16 +165,31 @@ def filesystem_size_quartet(
)
return f"{triple_str} / {limit}"

def missing_property(func: Callable[[Any], str]) -> Callable[[Any], str]:
return catch_missing_property(func, TABLE_UNKNOWN_STRING)

pool_name_func = missing_property(
lambda mo: self.pool_object_path_to_pool_name.get(
mo.Pool(), TABLE_UNKNOWN_STRING
)
)
name_func = missing_property(lambda mofs: mofs.Name())
size_func = missing_property(
lambda mofs: filesystem_size_quartet(
ListFilesystem.size_triple(mofs),
get_property(mofs.SizeLimit(), Range, None),
)
)
devnode_func = missing_property(lambda mofs: mofs.Devnode())
uuid_func = missing_property(lambda mofs: self.uuid_formatter(mofs.Uuid()))

tables = [
(
self.pool_object_path_to_pool_name[mofilesystem.Pool()],
mofilesystem.Name(),
filesystem_size_quartet(
ListFilesystem.size_triple(mofilesystem),
get_property(mofilesystem.SizeLimit(), Range, None),
),
mofilesystem.Devnode(),
self.uuid_formatter(mofilesystem.Uuid()),
pool_name_func(mofilesystem),
name_func(mofilesystem),
size_func(mofilesystem),
devnode_func(mofilesystem),
uuid_func(mofilesystem),
)
for mofilesystem in self.filesystems_with_props
]
Expand Down Expand Up @@ -198,29 +220,52 @@ def display(self):

fs = self.filesystems_with_props[0]

size_triple = ListFilesystem.size_triple(fs)
limit = get_property(fs.SizeLimit(), Range, None)
created = (
date_parser.isoparse(fs.Created()).astimezone().strftime("%b %d %Y %H:%M")
)
def missing_property(func: Callable[[Any], str]) -> Callable[[Any], str]:
return catch_missing_property(func, TABLE_UNKNOWN_STRING)(fs)

origin = get_property(fs.Origin(), self.uuid_formatter, None)
uuid = missing_property(lambda mo: self.uuid_formatter(mo.Uuid()))
print(f"UUID: {uuid}")

name = missing_property(lambda mo: mo.Name())
print(f"Name: {name}")

pool = missing_property(
lambda mo: self.pool_object_path_to_pool_name.get(
mo.Pool(), TABLE_UNKNOWN_STRING
),
)
print(f"Pool: {pool}")

print(f"UUID: {self.uuid_formatter(fs.Uuid())}")
print(f"Name: {fs.Name()}")
print(f"Pool: {self.pool_object_path_to_pool_name[fs.Pool()]}")
devnode = missing_property(lambda mo: mo.Devnode())
print()
print(f"Device: {fs.Devnode()}")
print(f"Device: {devnode}")

created = missing_property(
lambda mo: date_parser.isoparse(mo.Created())
.astimezone()
.strftime("%b %d %Y %H:%M"),
)
print()
print(f"Created: {created}")

origin = catch_missing_property(
lambda mo: get_property(mo.Origin(), self.uuid_formatter, None), None
)
print()
print(f"Snapshot origin: {origin}")
if origin is not None:
scheduled = "Yes" if fs.MergeScheduled() else "No"
scheduled = missing_property(
lambda mo: "Yes" if mo.MergeScheduled() else "No"
)
print(f" Revert scheduled: {scheduled}")

size_triple = ListFilesystem.size_triple(fs)
print()
print("Sizes:")
print(f" Logical size of thin device: {size_triple.total()}")
print(
" Logical size of thin device: "
f"{TABLE_FAILURE_STRING if size_triple.total() is None else size_triple.total()}"
)
print(
" Total used (including XFS metadata): "
f"{TABLE_FAILURE_STRING if size_triple.used() is None else size_triple.used()}"
Expand All @@ -229,5 +274,9 @@ def display(self):
" Free: "
f"{TABLE_FAILURE_STRING if size_triple.free() is None else size_triple.free()}"
)

limit = missing_property(
lambda mo: get_property(mo.SizeLimit(), lambda x: str(Range(x)), str(None))
)
print()
print(f" Size Limit: {limit}")
43 changes: 29 additions & 14 deletions src/stratis_cli/_actions/_list_pool.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@
from ._constants import TOP_OBJECT
from ._formatting import (
TABLE_FAILURE_STRING,
TABLE_UNKNOWN_STRING,
TOTAL_USED_FREE,
catch_missing_property,
get_property,
print_table,
)
Expand Down Expand Up @@ -452,7 +454,7 @@ def __init__(self, uuid_formatter: Callable[[str | UUID], str]):
"""
self.uuid_formatter = uuid_formatter

def display(self):
def display(self): # pylint: disable=too-many-locals
"""
List pools in table view.
"""
Expand Down Expand Up @@ -526,21 +528,34 @@ def gen_string(has_property: bool, code: str) -> str:
(objpath, MOPool(info)) for objpath, info in pools().search(managed_objects)
]

name_func = catch_missing_property(lambda mo: mo.Name(), TABLE_UNKNOWN_STRING)
size_func = catch_missing_property(physical_size_triple, TABLE_UNKNOWN_STRING)
properties_func = catch_missing_property(
properties_string, TABLE_UNKNOWN_STRING
)
uuid_func = catch_missing_property(
lambda mo: self.uuid_formatter(mo.Uuid()), TABLE_UNKNOWN_STRING
)
Comment on lines +531 to +538
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Apply the same missing-property guard to DefaultDetail.

These wrappers only harden DefaultTable.display(). pool list --name/--uuid still goes through DefaultDetail._print_detail_view(), which directly reads Name(), AvailableActions(), size fields, and other D-Bus properties, so missing fields will still terminate the detail path.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/stratis_cli/_actions/_list_pool.py` around lines 531 - 538,
DefaultDetail._print_detail_view still accesses D-Bus properties directly (e.g.,
Name(), Uuid(), AvailableActions(), physical size fields) so missing properties
can crash the detail view; update DefaultDetail._print_detail_view to use the
same missing-property guards you added for DefaultTable.display() by wrapping
each direct property access with catch_missing_property (or reuse name_func,
size_func, properties_func, uuid_func) so every call like mo.Name(), mo.Uuid(),
mo.AvailableActions(), and any size field reads fall back to
TABLE_UNKNOWN_STRING instead of raising.


def alert_func(pool_object_path: str, mopool: Any) -> List[str]:
"""
Combined alert codes.
"""
return [
str(code)
for code in catch_missing_property(
Default.alert_codes, default=[TABLE_UNKNOWN_STRING]
)(mopool)
+ alerts.alert_codes(pool_object_path)
]
Comment on lines +540 to +550
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

This still fails on missing blockdev size properties.

Line 525 builds DeviceSizeChangedAlerts before any of these new wrappers run. That constructor still dereferences MODev.TotalPhysicalSize() / NewPhysicalSize() directly, so a missing blockdev property will abort pool list before alert_func() can fall back.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/stratis_cli/_actions/_list_pool.py` around lines 540 - 550,
DeviceSizeChangedAlerts is still directly calling MODev.TotalPhysicalSize() /
NewPhysicalSize(), causing crashes before alert_func() can handle missing
properties; update the DeviceSizeChangedAlerts constructor (or the factory that
builds it) to use the same safe getter pattern as alert_func — e.g., call
catch_missing_property or a small helper to fetch TotalPhysicalSize and
NewPhysicalSize with a default of TABLE_UNKNOWN_STRING (or None) and handle that
default in the constructor rather than dereferencing the raw MODev values;
ensure no direct calls to MODev.TotalPhysicalSize() or MODev.NewPhysicalSize()
happen without a fallback so building DeviceSizeChangedAlerts never raises on
missing blockdev properties.


tables = [
(
mopool.Name(),
physical_size_triple(mopool),
properties_string(mopool),
self.uuid_formatter(mopool.Uuid()),
", ".join(
sorted(
str(code)
for code in (
Default.alert_codes(mopool)
+ alerts.alert_codes(pool_object_path)
)
)
),
name_func(mopool),
size_func(mopool),
properties_func(mopool),
uuid_func(mopool),
", ".join(sorted(alert_func(pool_object_path, mopool))),
)
for (pool_object_path, mopool) in pools_with_props
]
Expand Down
31 changes: 22 additions & 9 deletions src/stratis_cli/_actions/_physical.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

# isort: STDLIB
from argparse import Namespace
from typing import Any, Callable

# isort: THIRDPARTY
from justbytes import Range
Expand All @@ -26,6 +27,7 @@
from ._constants import TOP_OBJECT
from ._formatting import (
TABLE_UNKNOWN_STRING,
catch_missing_property,
get_property,
get_uuid_formatter,
print_table,
Expand Down Expand Up @@ -79,7 +81,7 @@ def list_devices(namespace: Namespace): # pylint: disable=too-many-locals
).search(managed_objects)
)

def paths(modev):
def paths(modev: Any) -> str:
"""
Return <physical_path> (<metadata_path>) if they are different,
otherwise, just <metadata_path>.
Expand All @@ -100,7 +102,7 @@ def paths(modev):
else f"{physical_path} ({metadata_path})"
)

def size(modev):
def size_str(modev: Any) -> str:
"""
Return in-use size (observed size) if they are different, otherwise
just in-use size.
Expand All @@ -113,24 +115,35 @@ def size(modev):
else f"{in_use_size} ({observed_size})"
)

def tier_str(value):
def tier_str(modev: Any) -> str:
"""
String representation of a tier.
"""
try:
return str(BlockDevTiers(value))
return str(BlockDevTiers(modev.Tier()))
except ValueError: # pragma: no cover
return TABLE_UNKNOWN_STRING

format_uuid = get_uuid_formatter(namespace.unhyphenated_uuids)

def missing_property(func: Callable[[Any], str]) -> Callable[[Any], str]:
return catch_missing_property(func, TABLE_UNKNOWN_STRING)

pool_name_func = missing_property(
lambda mo: path_to_name.get(mo.Pool(), TABLE_UNKNOWN_STRING)
)
paths_func = missing_property(paths)
size_func = missing_property(size_str)
tier_func = missing_property(tier_str)
uuid_func = missing_property(lambda modev: format_uuid(modev.Uuid()))

tables = [
[
path_to_name.get(modev.Pool(), TABLE_UNKNOWN_STRING),
paths(modev),
size(modev),
tier_str(modev.Tier()),
format_uuid(modev.Uuid()),
pool_name_func(modev),
paths_func(modev),
size_func(modev),
tier_func(modev),
uuid_func(modev),
]
for modev in modevs
]
Expand Down
10 changes: 7 additions & 3 deletions src/stratis_cli/_actions/_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,11 +279,11 @@ class SizeTriple:
Manage values in a size triple.
"""

def __init__(self, total: Range, used: Optional[Range]):
def __init__(self, total: Optional[Range], used: Optional[Range]):
self._total = total
self._used = used

def total(self) -> Range:
def total(self) -> Optional[Range]:
"""
Total.
"""
Expand All @@ -299,4 +299,8 @@ def free(self) -> Optional[Range]:
"""
Total - used.
"""
return None if self._used is None else self._total - self._used
return (
None
if self._used is None or self.total is None
else self._total - self._used
Comment on lines +302 to +305
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix the None check in SizeTriple.free().

self.total here is the bound method, not the stored value. If _total is missing but _used is present, this still falls through to self._total - self._used and raises TypeError, so filesystem size rendering can still crash on a missing total.

🛠️ Suggested fix
         return (
             None
-            if self._used is None or self.total is None
+            if self._used is None or self._total is None
             else self._total - self._used
         )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return (
None
if self._used is None or self.total is None
else self._total - self._used
return (
None
if self._used is None or self._total is None
else self._total - self._used
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/stratis_cli/_actions/_utils.py` around lines 302 - 305, The None check in
SizeTriple.free() incorrectly tests self.total (a bound method) instead of the
stored attribute, allowing _total to be None and causing a TypeError; change the
condition to check self._total and self._used (e.g., return None if self._used
is None or self._total is None) so the method returns None when either stored
value is missing before computing self._total - self._used.

)
24 changes: 23 additions & 1 deletion tests/integration/pool/test_list.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@
"""

# isort: STDLIB
from unittest.mock import patch
from uuid import uuid4

# isort: FIRSTPARTY
from dbus_client_gen import DbusClientUniqueResultError
from dbus_client_gen import DbusClientMissingPropertyError, DbusClientUniqueResultError

# isort: LOCAL
from stratis_cli import StratisCliErrorCodes
Expand Down Expand Up @@ -316,3 +317,24 @@ def test_list_detail(self):
Test detail view on running pool.
"""
TEST_RUNNER(self._MENU + [f"--name={self._POOLNAME}"])

def test_list_no_size(self):
"""
Test listing the pool when size information not included in
GetManagedObjects result.
"""
# isort: LOCAL
import stratis_cli # pylint: disable=import-outside-toplevel

with patch.object(
# pylint: disable=protected-access
stratis_cli._actions._list_pool.Default, # pyright: ignore
"size_triple",
autospec=True,
side_effect=DbusClientMissingPropertyError(
"oops",
stratis_cli._actions._constants.POOL_INTERFACE, # pyright: ignore
"TotalPhysicalUsed",
),
):
TEST_RUNNER(self._MENU)
Loading