Lookup blockdev D-Bus properties without raising not found exception #1250
Lookup blockdev D-Bus properties without raising not found exception #1250mulkieran wants to merge 2 commits intostratis-storage:masterfrom
Conversation
444eeb3 to
4320d2b
Compare
f9da5c0 to
7eb1526
Compare
|
We should do this for filesystem also, most likely. |
|
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. |
7eb1526 to
43dd2f9
Compare
3350c4a to
b3c9539
Compare
b3c9539 to
597bd22
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughIntroduce a defensive helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/stratis_cli/_actions/_list_filesystem.py (1)
210-250: Consider applying defensive wrappers to theDetailview as well.The
Table.display()method now gracefully handles missing properties viacatch_missing_property, butDetail.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 raiseDbusClientMissingPropertyError.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 toDefaultDetail._print_detail_viewas well.Similar to the filesystem listing, the
DefaultDetailclass 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
📒 Files selected for processing (4)
src/stratis_cli/_actions/_formatting.pysrc/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.pysrc/stratis_cli/_actions/_physical.py
8f836be to
9e13df6
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
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_triplemethods called on potential string fallback.If
catch_missing_propertycatches an exception,size_triplewill beTABLE_UNKNOWN_STRING(a string). Subsequent calls tosize_triple.total(),size_triple.used(), andsize_triple.free()on lines 269, 272, and 276 will raiseAttributeError: '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
SizeTripleat 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 forTABLE_UNKNOWN_STRINGinstead ofNone.If
catch_missing_propertycatches an exception fororigin, it returnsTABLE_UNKNOWN_STRING(notNone), so theif origin is not None:check will still enter the block. Thescheduledlookup 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
📒 Files selected for processing (5)
src/stratis_cli/_actions/_formatting.pysrc/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.pysrc/stratis_cli/_actions/_physical.pytests/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>
9e13df6 to
2ba7fbe
Compare
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>
2ba7fbe to
25a7fea
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/stratis_cli/_actions/_list_filesystem.py (1)
251-260:⚠️ Potential issue | 🔴 CriticalInvoke the
originwrapper before using it.Same wrapper-invocation issue, now on
origin. As written,originis the callable returned bycatch_missing_property(), not the resolved snapshot UUID, so detail output prints a function object and theif origin is not Nonebranch 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
📒 Files selected for processing (6)
src/stratis_cli/_actions/_formatting.pysrc/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.pysrc/stratis_cli/_actions/_physical.pysrc/stratis_cli/_actions/_utils.pytests/integration/pool/test_list.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stratis_cli/_actions/_physical.py
| 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 | ||
| ) |
There was a problem hiding this comment.
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) | ||
| ] |
There was a problem hiding this comment.
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.
| return ( | ||
| None | ||
| if self._used is None or self.total is None | ||
| else self._total - self._used |
There was a problem hiding this comment.
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.
| 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.
|
Superceded by a simpler approach. |
Related stratis-storage/stratisd#3947
Summary by CodeRabbit