Conversation
Signed-off-by: Colin Walters <walters@verbum.org>
This is a nondestructive variant of `to-existing-root`. Signed-off-by: Colin Walters <walters@verbum.org>
There was a problem hiding this comment.
Code Review
The code changes introduce the install reset command, a valuable addition for managing bootc systems. The refactoring to use MergeState is a clean way to accommodate this new functionality alongside existing deployment logic. The new stateroot management functions in ostree-ext are also well-structured and come with thorough tests. I've found a few issues that should be addressed: a leftover debugging macro, a potential panic from an assertion on user input, and a logic issue in how new stateroot names are generated when the year changes. My detailed comments and suggestions below aim to resolve these points to improve the robustness and quality of the code.
| }; | ||
|
|
||
| let booted_stateroot = booted_deployment.osname(); | ||
| assert!(booted_stateroot.as_str() != target_stateroot); |
There was a problem hiding this comment.
Using assert! here is risky because target_stateroot can be user-provided. If a user specifies the same name as the booted stateroot, this will cause a panic in release builds. It's better to use anyhow::ensure! to return a graceful error message.
anyhow::ensure!(
booted_stateroot.as_str() != target_stateroot,
"Target stateroot cannot be the same as the booted one"
);| let (year, serial) = stateroots | ||
| .iter() | ||
| .filter_map(|v| { | ||
| if let StaterootKind::Auto(v) = v.kind { | ||
| Some(v) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .max() | ||
| .map(|(year, serial)| (year, serial + 1)) | ||
| .unwrap_or((current_year, 0)); |
There was a problem hiding this comment.
The current logic for allocating a new stateroot name can produce incorrect results when the year changes. For example, if the latest stateroot is from 2023 (e.g., state-2023-5) and the current year is 2024, this logic will generate state-2023-6 instead of state-2024-0.
The logic should reset the serial to 0 when the year advances. A more robust implementation would also handle cases where the system clock is set to a future time.
Here's a suggested improvement that handles year changes correctly:
| let (year, serial) = stateroots | |
| .iter() | |
| .filter_map(|v| { | |
| if let StaterootKind::Auto(v) = v.kind { | |
| Some(v) | |
| } else { | |
| None | |
| } | |
| }) | |
| .max() | |
| .map(|(year, serial)| (year, serial + 1)) | |
| .unwrap_or((current_year, 0)); | |
| let (year, serial) = stateroots | |
| .iter() | |
| .filter_map(|v| { | |
| if let StaterootKind::Auto(v) = v.kind { | |
| Some(v) | |
| } else { | |
| None | |
| } | |
| }) | |
| .max() | |
| .map(|(found_year, found_serial)| { | |
| if found_year < current_year { | |
| (current_year, 0) | |
| } else { | |
| (found_year, found_serial + 1) | |
| } | |
| }) | |
| .unwrap_or((current_year, 0)); |
| crate::status::get_status_require_booted(sysroot)?; | ||
|
|
||
| let stateroots = list_stateroots(sysroot)?; | ||
| dbg!(&stateroots); |
There was a problem hiding this comment.
This dbg! macro call appears to be leftover from debugging. It should be removed before merging to keep the command's output clean.
|
This continues in #1588 |
This is a nondestructive variant of
to-existing-root.