Skip to content

Lookup blockdev D-Bus properties without raising not found exception #1250

Draft
mulkieran wants to merge 2 commits intostratis-storage:masterfrom
mulkieran:do-not-terminate-on-missing-pool
Draft

Lookup blockdev D-Bus properties without raising not found exception #1250
mulkieran wants to merge 2 commits intostratis-storage:masterfrom
mulkieran:do-not-terminate-on-missing-pool

Conversation

@mulkieran
Copy link
Copy Markdown
Member

@mulkieran mulkieran commented Jan 5, 2026

Related stratis-storage/stratisd#3947

Summary by CodeRabbit

  • Bug Fixes
    • Tables and detail views for filesystems, pools, and devices now handle missing or unavailable properties gracefully, showing placeholders or explicit "failure" indicators instead of crashing.
  • Tests
    • Added an integration test that simulates a missing size property to ensure listing and display remain robust under absent data.

@mulkieran mulkieran self-assigned this Jan 5, 2026
@mulkieran mulkieran force-pushed the do-not-terminate-on-missing-pool branch 2 times, most recently from 444eeb3 to 4320d2b Compare January 5, 2026 20:01
@mulkieran mulkieran moved this to In Progress in 2026January Jan 5, 2026
@mulkieran mulkieran changed the title Lookup pool name without raising not found exception Lookup blockdev D-Bus properties without raising not found exception Jan 5, 2026
@mulkieran mulkieran force-pushed the do-not-terminate-on-missing-pool branch 2 times, most recently from f9da5c0 to 7eb1526 Compare January 5, 2026 20:21
@mulkieran mulkieran marked this pull request as draft January 5, 2026 20:26
@mulkieran
Copy link
Copy Markdown
Member Author

We should do this for filesystem also, most likely.

@mulkieran mulkieran added this to the v3.9.0 milestone Jan 5, 2026
@mulkieran
Copy link
Copy Markdown
Member Author

We will have to have a plan to patch lookup methods like devs or pools or filesystems or fall back on just treating the GetManagedObjects result like a dict.

@mulkieran mulkieran force-pushed the do-not-terminate-on-missing-pool branch from 7eb1526 to 43dd2f9 Compare January 15, 2026 15:22
@mulkieran mulkieran removed this from 2026January Feb 17, 2026
@mulkieran mulkieran moved this to In Progress in 2026February Feb 17, 2026
@mulkieran mulkieran removed this from 2026February Mar 2, 2026
@mulkieran mulkieran moved this to In Progress in 2026March Mar 2, 2026
@mulkieran mulkieran force-pushed the do-not-terminate-on-missing-pool branch 10 times, most recently from 3350c4a to b3c9539 Compare March 13, 2026 01:41
@mulkieran mulkieran force-pushed the do-not-terminate-on-missing-pool branch from b3c9539 to 597bd22 Compare March 23, 2026 19:49
@mulkieran
Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 23, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 23, 2026

Walkthrough

Introduce a defensive helper catch_missing_property() and apply it across filesystem, pool, and physical-device listing code to convert property-access exceptions (DbusClientMissingPropertyError) into default values; update SizeTriple to permit a None total and propagate None in size computations; add an integration test for missing-size behavior.

Changes

Cohort / File(s) Summary
Formatting utility
src/stratis_cli/_actions/_formatting.py
Add catch_missing_property(prop_to_str, default) to wrap property-access callables and return default when DbusClientMissingPropertyError is raised.
Filesystem listing
src/stratis_cli/_actions/_list_filesystem.py
Wrap filesystem property reads and size/limit computations with catch_missing_property(..., TABLE_UNKNOWN_STRING/None) so missing D‑Bus properties yield TABLE_UNKNOWN_STRING or None instead of raising.
Pool listing
src/stratis_cli/_actions/_list_pool.py
Guard pool property access and formatting (Name, size triple, properties string, Uuid, alerts) with catch_missing_property, substituting TABLE_UNKNOWN_STRING where appropriate; add pylint disable for DefaultTable.display.
Physical devices
src/stratis_cli/_actions/_physical.py
Introduce typed local formatter functions and route pool name, paths, size, tier, and UUID formatting through catch_missing_property wrappers returning TABLE_UNKNOWN_STRING on missing properties; rename sizesize_str.
Size utilities
src/stratis_cli/_actions/_utils.py
Change SizeTriple to accept total: Optional[Range] and make total() return Optional[Range]; update free() to return None if total or used is None.
Tests
tests/integration/pool/test_list.py
Add test_list_no_size which patches Default.size_triple to raise DbusClientMissingPropertyError for TotalPhysicalUsed and verifies pool list completes under missing-size conditions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: introducing error handling for D-Bus property lookups that gracefully handle missing properties instead of raising exceptions.
Docstring Coverage ✅ Passed Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/stratis_cli/_actions/_list_filesystem.py (1)

210-250: Consider applying defensive wrappers to the Detail view as well.

The Table.display() method now gracefully handles missing properties via catch_missing_property, but Detail.display() still uses direct property access (e.g., fs.Size(), fs.Name(), fs.Pool(), fs.Uuid() at lines 218, 226-228). If a property is missing in the detail view, it would still raise DbusClientMissingPropertyError.

This might be intentional since the detail view is for a single, specifically-requested filesystem, but for consistency with the PR's objective, consider whether the detail view should also handle missing properties gracefully.

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

In `@src/stratis_cli/_actions/_list_filesystem.py` around lines 210 - 250, The
Detail.display method directly calls D-Bus property accessors (e.g., fs.Size(),
fs.Used(), fs.SizeLimit(), fs.Created(), fs.Origin(), fs.Uuid(), fs.Name(),
fs.Pool(), fs.Devnode(), fs.MergeScheduled()) which can raise
DbusClientMissingPropertyError; update Detail.display to use the same defensive
helpers as Table.display — wrap each fs.<Property>() with get_property(...) or
catch_missing_property(...) (use self.uuid_formatter for Uuid and the existing
TABLE_FAILURE_STRING/defaults for missing sizes), parse Created only if not None
before date_parser.isoparse, and guard lookup of
self.pool_object_path_to_pool_name[fs.Pool()] with a safe get_property or
mapping fallback so missing properties render a sensible placeholder instead of
raising.
src/stratis_cli/_actions/_list_pool.py (1)

329-419: Consider applying defensive wrappers to DefaultDetail._print_detail_view as well.

Similar to the filesystem listing, the DefaultDetail class uses direct property access (e.g., mopool.Uuid(), mopool.Name(), mopool.Encrypted() at lines 340-343) without defensive wrappers. For consistency with the PR's objective of preventing stack traces from missing properties, the detail view could benefit from similar protection.

🤖 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 329 - 419, The
_print_detail_view method accesses mopool properties directly (mopool.Uuid(),
mopool.Name(), mopool.Encrypted(), mopool.KeyDescriptions(),
mopool.ClevisInfos(), etc.), which can raise when properties are missing; wrap
these accesses with the same defensive Default wrappers used elsewhere (e.g.,
replace direct calls with the existing Default.* helpers used in this file such
as Default.alert_codes(mopool), Default.metadata_version(mopool),
Default.size_triple(mopool) or add analogous
Default.uuid(mopool)/Default.name(mopool)/Default.encrypted(mopool)/Default.key_descriptions(mopool)/Default.clevis_infos(mopool)
wrappers) so _print_detail_view handles missing/inconsistent properties without
throwing; update uses of encryption_infos, free token slots, Alerts and printed
fields to consume the safe values returned by those wrappers and preserve
current formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 541-547: The function alert_func returns List[PoolAlertType] but
uses [TABLE_UNKNOWN_STRING] (a str) as a fallback; add a proper sentinel alert
type in src/stratis_cli/_alerts.py (e.g., class UnknownPoolAlert
implementing/union-compatible with PoolAlertType) and use an instance of that
sentinel instead of TABLE_UNKNOWN_STRING in the fallback passed to
catch_missing_property (or alternatively update alert_func's return annotation
to List[PoolAlertType | str] if you prefer the quick fix); update references to
Default.alert_codes and any code that consumes alert codes (e.g., the str(code)
use site) to accept the new sentinel type.

---

Nitpick comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 210-250: The Detail.display method directly calls D-Bus property
accessors (e.g., fs.Size(), fs.Used(), fs.SizeLimit(), fs.Created(),
fs.Origin(), fs.Uuid(), fs.Name(), fs.Pool(), fs.Devnode(), fs.MergeScheduled())
which can raise DbusClientMissingPropertyError; update Detail.display to use the
same defensive helpers as Table.display — wrap each fs.<Property>() with
get_property(...) or catch_missing_property(...) (use self.uuid_formatter for
Uuid and the existing TABLE_FAILURE_STRING/defaults for missing sizes), parse
Created only if not None before date_parser.isoparse, and guard lookup of
self.pool_object_path_to_pool_name[fs.Pool()] with a safe get_property or
mapping fallback so missing properties render a sensible placeholder instead of
raising.

In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 329-419: The _print_detail_view method accesses mopool properties
directly (mopool.Uuid(), mopool.Name(), mopool.Encrypted(),
mopool.KeyDescriptions(), mopool.ClevisInfos(), etc.), which can raise when
properties are missing; wrap these accesses with the same defensive Default
wrappers used elsewhere (e.g., replace direct calls with the existing Default.*
helpers used in this file such as Default.alert_codes(mopool),
Default.metadata_version(mopool), Default.size_triple(mopool) or add analogous
Default.uuid(mopool)/Default.name(mopool)/Default.encrypted(mopool)/Default.key_descriptions(mopool)/Default.clevis_infos(mopool)
wrappers) so _print_detail_view handles missing/inconsistent properties without
throwing; update uses of encryption_infos, free token slots, Alerts and printed
fields to consume the safe values returned by those wrappers and preserve
current formatting.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b8cbc866-9ea0-4010-8cc6-95bb1a7ac630

📥 Commits

Reviewing files that changed from the base of the PR and between bf262b6 and 597bd22.

📒 Files selected for processing (4)
  • src/stratis_cli/_actions/_formatting.py
  • src/stratis_cli/_actions/_list_filesystem.py
  • src/stratis_cli/_actions/_list_pool.py
  • src/stratis_cli/_actions/_physical.py

@mulkieran mulkieran force-pushed the do-not-terminate-on-missing-pool branch 2 times, most recently from 8f836be to 9e13df6 Compare March 24, 2026 00:34
@mulkieran
Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 24, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/stratis_cli/_actions/_list_filesystem.py (1)

264-277: ⚠️ Potential issue | 🔴 Critical

size_triple methods called on potential string fallback.

If catch_missing_property catches an exception, size_triple will be TABLE_UNKNOWN_STRING (a string). Subsequent calls to size_triple.total(), size_triple.used(), and size_triple.free() on lines 269, 272, and 276 will raise AttributeError: 'str' object has no attribute 'total'.

🐛 Proposed fix: Check if size_triple is a SizeTriple before calling methods
         size_triple = catch_missing_property(
             ListFilesystem.size_triple, TABLE_UNKNOWN_STRING
         )(fs)
         print()
         print("Sizes:")
-        print(f"  Logical size of thin device: {size_triple.total()}")
-        print(
-            "  Total used (including XFS metadata): "
-            f"{TABLE_FAILURE_STRING if size_triple.used() is None else size_triple.used()}"
-        )
-        print(
-            "  Free: "
-            f"{TABLE_FAILURE_STRING if size_triple.free() is None else size_triple.free()}"
-        )
+        if isinstance(size_triple, SizeTriple):
+            print(f"  Logical size of thin device: {size_triple.total()}")
+            print(
+                "  Total used (including XFS metadata): "
+                f"{TABLE_FAILURE_STRING if size_triple.used() is None else size_triple.used()}"
+            )
+            print(
+                "  Free: "
+                f"{TABLE_FAILURE_STRING if size_triple.free() is None else size_triple.free()}"
+            )
+        else:
+            print(f"  Logical size of thin device: {TABLE_UNKNOWN_STRING}")
+            print(f"  Total used (including XFS metadata): {TABLE_UNKNOWN_STRING}")
+            print(f"  Free: {TABLE_UNKNOWN_STRING}")

Note: You'll need to import SizeTriple at the top of the method or use a different type check approach.

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

In `@src/stratis_cli/_actions/_list_filesystem.py` around lines 264 - 277, The
code calls size_triple.total()/used()/free() after assigning size_triple =
catch_missing_property(ListFilesystem.size_triple, TABLE_UNKNOWN_STRING)(fs),
which can return TABLE_UNKNOWN_STRING (a str) on error; update the block to
first check whether size_triple is an instance of SizeTriple (import SizeTriple
or reference its class) and only call size_triple.total()/used()/free() when it
is; otherwise, print the TABLE_UNKNOWN_STRING or TABLE_FAILURE_STRING fallbacks
for each line (use TABLE_FAILURE_STRING when used()/free() are None) to avoid
AttributeError from calling methods on a string.
🧹 Nitpick comments (1)
src/stratis_cli/_actions/_list_filesystem.py (1)

252-262: Minor: Consider checking for TABLE_UNKNOWN_STRING instead of None.

If catch_missing_property catches an exception for origin, it returns TABLE_UNKNOWN_STRING (not None), so the if origin is not None: check will still enter the block. The scheduled lookup is safely wrapped, but printing "Revert scheduled" when the origin itself is unknown may be misleading to users.

💡 Optional: Skip scheduled output if origin is unknown
-        if origin is not None:
+        if origin is not None and origin != TABLE_UNKNOWN_STRING:
             scheduled = catch_missing_property(
                 lambda mo: "Yes" if fs.MergeScheduled() else "No", TABLE_UNKNOWN_STRING
             )(fs)
             print(f"    Revert scheduled: {scheduled}")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/stratis_cli/_actions/_list_filesystem.py` around lines 252 - 262, The
code uses catch_missing_property to set origin (via get_property and
mo.Origin()) which returns TABLE_UNKNOWN_STRING on error, but the subsequent
conditional checks "if origin is not None:" and thus still prints the scheduled
info; update the conditional to check for TABLE_UNKNOWN_STRING instead (e.g.,
skip printing "Revert scheduled" when origin == TABLE_UNKNOWN_STRING) so that if
origin is unknown you don't show the revert scheduled line that relies on
fs.MergeScheduled().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 231-241: The wrappers returned by catch_missing_property are never
invoked and will print the function objects; update the two assignments to call
the returned callables (i.e., invoke the result of catch_missing_property for
both pool and devnode), and fix the pool lambda to reference the lambda
parameter by using mo.Pool() instead of fs.Pool() so it looks up the pool from
the provided object; ensure you call the resulting callables (from
catch_missing_property) before printing to get actual values.

---

Outside diff comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 264-277: The code calls size_triple.total()/used()/free() after
assigning size_triple = catch_missing_property(ListFilesystem.size_triple,
TABLE_UNKNOWN_STRING)(fs), which can return TABLE_UNKNOWN_STRING (a str) on
error; update the block to first check whether size_triple is an instance of
SizeTriple (import SizeTriple or reference its class) and only call
size_triple.total()/used()/free() when it is; otherwise, print the
TABLE_UNKNOWN_STRING or TABLE_FAILURE_STRING fallbacks for each line (use
TABLE_FAILURE_STRING when used()/free() are None) to avoid AttributeError from
calling methods on a string.

---

Nitpick comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 252-262: The code uses catch_missing_property to set origin (via
get_property and mo.Origin()) which returns TABLE_UNKNOWN_STRING on error, but
the subsequent conditional checks "if origin is not None:" and thus still prints
the scheduled info; update the conditional to check for TABLE_UNKNOWN_STRING
instead (e.g., skip printing "Revert scheduled" when origin ==
TABLE_UNKNOWN_STRING) so that if origin is unknown you don't show the revert
scheduled line that relies on fs.MergeScheduled().

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cd3f0ca6-9a05-474c-9564-a30e9b3f54d3

📥 Commits

Reviewing files that changed from the base of the PR and between 597bd22 and 9e13df6.

📒 Files selected for processing (5)
  • src/stratis_cli/_actions/_formatting.py
  • src/stratis_cli/_actions/_list_filesystem.py
  • src/stratis_cli/_actions/_list_pool.py
  • src/stratis_cli/_actions/_physical.py
  • tests/integration/pool/test_list.py
✅ Files skipped from review due to trivial changes (1)
  • src/stratis_cli/_actions/_list_pool.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/stratis_cli/_actions/_formatting.py
  • src/stratis_cli/_actions/_physical.py

Signed-off-by: mulhern <amulhern@redhat.com>
@mulkieran mulkieran force-pushed the do-not-terminate-on-missing-pool branch from 9e13df6 to 2ba7fbe Compare March 24, 2026 01:35
zbus crate omits property values from the GetManagedObjects result if it
can not get them.

Instead of a stack trace, return a TABLE_UNKNOWN_STRING for printing.

Signed-off-by: mulhern <amulhern@redhat.com>
@mulkieran mulkieran force-pushed the do-not-terminate-on-missing-pool branch from 2ba7fbe to 25a7fea Compare March 24, 2026 01:40
@mulkieran
Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 24, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (1)
src/stratis_cli/_actions/_list_filesystem.py (1)

251-260: ⚠️ Potential issue | 🔴 Critical

Invoke the origin wrapper before using it.

Same wrapper-invocation issue, now on origin. As written, origin is the callable returned by catch_missing_property(), not the resolved snapshot UUID, so detail output prints a function object and the if origin is not None branch is always true.

🛠️ Suggested fix
-        origin = catch_missing_property(
-            lambda mo: get_property(mo.Origin(), self.uuid_formatter, None), None
-        )
+        origin = catch_missing_property(
+            lambda mo: get_property(mo.Origin(), self.uuid_formatter, None), None
+        )(fs)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/stratis_cli/_actions/_list_filesystem.py` around lines 251 - 260, The
variable origin is a wrapper callable returned by catch_missing_property, so
invoke it to get the actual snapshot identifier before printing and testing it;
e.g. call origin() and store the result (e.g. origin_value), use that in the if
check and print, and when computing scheduled invoke the missing_property
wrapper with the resolved origin (call the missing_property result with
origin_value) so scheduled reflects the snapshot's MergeScheduled() value rather
than a function object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 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.
- Around line 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.

In `@src/stratis_cli/_actions/_utils.py`:
- Around line 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.

---

Duplicate comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 251-260: The variable origin is a wrapper callable returned by
catch_missing_property, so invoke it to get the actual snapshot identifier
before printing and testing it; e.g. call origin() and store the result (e.g.
origin_value), use that in the if check and print, and when computing scheduled
invoke the missing_property wrapper with the resolved origin (call the
missing_property result with origin_value) so scheduled reflects the snapshot's
MergeScheduled() value rather than a function object.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 94e22c9e-51a0-4d18-8f06-500ddd9016a1

📥 Commits

Reviewing files that changed from the base of the PR and between 9e13df6 and 25a7fea.

📒 Files selected for processing (6)
  • src/stratis_cli/_actions/_formatting.py
  • src/stratis_cli/_actions/_list_filesystem.py
  • src/stratis_cli/_actions/_list_pool.py
  • src/stratis_cli/_actions/_physical.py
  • src/stratis_cli/_actions/_utils.py
  • tests/integration/pool/test_list.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/stratis_cli/_actions/_physical.py

Comment on lines +531 to +538
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
)
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.

Comment on lines +540 to +550
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)
]
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.

Comment on lines +302 to +305
return (
None
if self._used is None or self.total is None
else self._total - self._used
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.

@mulkieran
Copy link
Copy Markdown
Member Author

Superceded by a simpler approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

1 participant