Skip to content

Commit 39ace6a

Browse files
committed
lib: Move sysroot_path into Storage struct
Refactor the unified storage code to get the sysroot path from the Storage struct instead of passing it as a parameter through multiple function calls. Why this change is needed: The previous implementation passed sysroot_path as an Option parameter to prepare_for_pull_unified() and pull_unified(). This was awkward because: - Callers had to know whether to pass None (for booted systems) or Some(path) (for install scenarios) - The path is inherently tied to the Storage instance's lifecycle - It required threading parameters through multiple layers This follows the existing pattern where Storage already encapsulates storage-related state, and addresses review feedback to "get this stuff from the Storage". Assisted-by: Claude Code (Opus 4.5) Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
1 parent b8ae4c2 commit 39ace6a

File tree

4 files changed

+24
-23
lines changed

4 files changed

+24
-23
lines changed

crates/lib/src/cli.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -965,7 +965,7 @@ async fn upgrade(
965965
let use_unified = crate::deploy::image_exists_in_unified_storage(storage, imgref).await?;
966966

967967
let fetched = if use_unified {
968-
crate::deploy::pull_unified(repo, imgref, None, opts.quiet, prog.clone(), storage, None)
968+
crate::deploy::pull_unified(repo, imgref, None, opts.quiet, prog.clone(), storage)
969969
.await?
970970
} else {
971971
crate::deploy::pull(repo, imgref, None, opts.quiet, prog.clone()).await?
@@ -1093,8 +1093,7 @@ async fn switch_ostree(
10931093
};
10941094

10951095
let fetched = if use_unified {
1096-
crate::deploy::pull_unified(repo, &target, None, opts.quiet, prog.clone(), storage, None)
1097-
.await?
1096+
crate::deploy::pull_unified(repo, &target, None, opts.quiet, prog.clone(), storage).await?
10981097
} else {
10991098
crate::deploy::pull(repo, &target, None, opts.quiet, prog.clone()).await?
11001099
};

crates/lib/src/deploy.rs

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -422,16 +422,11 @@ pub(crate) async fn image_exists_in_unified_storage(
422422

423423
/// Unified approach: Use bootc's CStorage to pull the image, then prepare from containers-storage.
424424
/// This reuses the same infrastructure as LBIs.
425-
///
426-
/// The `sysroot_path` parameter specifies the path to the sysroot where bootc storage is located.
427-
/// During install, this should be the path to the target disk's mount point.
428-
/// During upgrade/switch on a running system, pass `None` to use the default `/sysroot`.
429425
pub(crate) async fn prepare_for_pull_unified(
430426
repo: &ostree::Repo,
431427
imgref: &ImageReference,
432428
target_imgref: Option<&OstreeImageReference>,
433429
store: &Storage,
434-
sysroot_path: Option<&camino::Utf8Path>,
435430
) -> Result<PreparedPullResult> {
436431
// Get or initialize the bootc container storage (same as used for LBIs)
437432
let imgstore = store.get_ensure_imgstore()?;
@@ -467,14 +462,10 @@ pub(crate) async fn prepare_for_pull_unified(
467462
// Configure the importer to use bootc storage as an additional image store
468463
let mut config = ostree_ext::containers_image_proxy::ImageProxyConfig::default();
469464
let mut cmd = Command::new("skopeo");
470-
// Use the actual physical path to bootc storage
471-
// During install, this is the target disk's mount point; otherwise default to /sysroot
472-
let sysroot_base = sysroot_path
473-
.map(|p| p.to_string())
474-
.unwrap_or_else(|| "/sysroot".to_string());
465+
// Use the physical path to bootc storage from the Storage struct
475466
let storage_path = format!(
476467
"{}/{}",
477-
sysroot_base,
468+
store.physical_root_path,
478469
crate::podstorage::CStorage::subpath()
479470
);
480471
crate::podstorage::set_additional_image_store(&mut cmd, &storage_path);
@@ -525,19 +516,15 @@ pub(crate) async fn prepare_for_pull_unified(
525516
}
526517

527518
/// Unified pull: Use podman to pull to containers-storage, then read from there
528-
///
529-
/// The `sysroot_path` parameter specifies the path to the sysroot where bootc storage is located.
530-
/// For normal upgrade/switch operations, pass `None` to use the default `/sysroot`.
531519
pub(crate) async fn pull_unified(
532520
repo: &ostree::Repo,
533521
imgref: &ImageReference,
534522
target_imgref: Option<&OstreeImageReference>,
535523
quiet: bool,
536524
prog: ProgressWriter,
537525
store: &Storage,
538-
sysroot_path: Option<&camino::Utf8Path>,
539526
) -> Result<Box<ImageState>> {
540-
match prepare_for_pull_unified(repo, imgref, target_imgref, store, sysroot_path).await? {
527+
match prepare_for_pull_unified(repo, imgref, target_imgref, store).await? {
541528
PreparedPullResult::AlreadyPresent(existing) => {
542529
// Log that the image was already present (Debug level since it's not actionable)
543530
const IMAGE_ALREADY_PRESENT_ID: &str = "5c4d3e2f1a0b9c8d7e6f5a4b3c2d1e0f9";

crates/lib/src/install.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -935,7 +935,6 @@ async fn install_container(
935935
&spec_imgref,
936936
Some(&state.target_imgref),
937937
storage,
938-
Some(&root_setup.physical_root_path),
939938
)
940939
.await?
941940
} else {

crates/lib/src/store/mod.rs

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,14 @@ use std::sync::Arc;
2222

2323
use anyhow::{Context, Result};
2424
use bootc_mount::tempmount::TempMount;
25+
use camino::Utf8PathBuf;
2526
use cap_std_ext::cap_std;
2627
use cap_std_ext::cap_std::fs::{Dir, DirBuilder, DirBuilderExt as _};
2728
use cap_std_ext::dirext::CapStdExtDirExt;
2829
use fn_error_context::context;
2930

3031
use ostree_ext::container_utils::ostree_booted;
32+
use ostree_ext::prelude::FileExt;
3133
use ostree_ext::sysroot::SysrootLock;
3234
use ostree_ext::{gio, ostree};
3335
use rustix::fs::Mode;
@@ -185,6 +187,7 @@ impl BootedStorage {
185187

186188
let storage = Storage {
187189
physical_root,
190+
physical_root_path: Utf8PathBuf::from("/sysroot"),
188191
run,
189192
boot_dir: Some(boot_dir),
190193
esp: Some(esp_mount),
@@ -209,6 +212,7 @@ impl BootedStorage {
209212

210213
let storage = Storage {
211214
physical_root,
215+
physical_root_path: Utf8PathBuf::from("/sysroot"),
212216
run,
213217
boot_dir: None,
214218
esp: None,
@@ -254,6 +258,10 @@ pub(crate) struct Storage {
254258
/// Directory holding the physical root
255259
pub physical_root: Dir,
256260

261+
/// Absolute path to the physical root directory.
262+
/// This is `/sysroot` on a running system, or the target mount point during install.
263+
pub physical_root_path: Utf8PathBuf,
264+
257265
/// The 'boot' directory, useful and `Some` only for composefs systems
258266
/// For grub booted systems, this points to `/sysroot/boot`
259267
/// For systemd booted systems, this points to the ESP
@@ -301,17 +309,25 @@ impl Storage {
301309
// we need to explicitly distinguish the two and the storage
302310
// here hence holds a reference to the physical root.
303311
let ostree_sysroot_dir = crate::utils::sysroot_dir(&sysroot)?;
304-
let physical_root = if sysroot.is_booted() {
305-
ostree_sysroot_dir.open_dir(SYSROOT)?
312+
let (physical_root, physical_root_path) = if sysroot.is_booted() {
313+
(
314+
ostree_sysroot_dir.open_dir(SYSROOT)?,
315+
Utf8PathBuf::from("/sysroot"),
316+
)
306317
} else {
307-
ostree_sysroot_dir
318+
// For non-booted case (install), get the path from the sysroot
319+
let path = sysroot.path();
320+
let path_str = path.parse_name().to_string();
321+
let path = Utf8PathBuf::from(path_str);
322+
(ostree_sysroot_dir, path)
308323
};
309324

310325
let ostree_cell = OnceCell::new();
311326
let _ = ostree_cell.set(sysroot);
312327

313328
Ok(Self {
314329
physical_root,
330+
physical_root_path,
315331
run,
316332
boot_dir: None,
317333
esp: None,

0 commit comments

Comments
 (0)