sysroot: Remove now-spurious assertion change from soft reboot changes #3456
Conversation
We fixed the bug that caused soft reboot to trip the assertion here, so revert back to a hard check.
There was a problem hiding this comment.
Code Review
This pull request simplifies the code by removing a workaround for a bug related to soft reboots, which is a good improvement. I have one suggestion to enhance robustness: instead of using g_assert(), which can crash the application, I recommend returning a GError. This aligns better with the function's signature and provides a cleaner failure path.
| g_assert (self->booted_deployment); | ||
| } | ||
| } | ||
| g_assert (self->booted_deployment); |
There was a problem hiding this comment.
While the bug that required the previous complex check has been fixed, using g_assert() here could lead to a crash in production builds if this invariant is ever violated for any reason (e.g., a regression, or an unexpected system state).
Since this function can return a GError, it would be more robust to return an error instead of asserting. This provides a cleaner failure mode for callers and is more idiomatic for a library function.
if (G_UNLIKELY (!self->booted_deployment)) return glnx_throw (error, "Inconsistent state: system is booted but no deployment found");There was a problem hiding this comment.
We're not converting the runtime assertions in the codebase to errors right now.
We fixed the bug that caused soft reboot to trip the assertion here, so revert back to a hard check.