Skip to content

store: Add accessors for the ostree repo #1509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+25 −14
Open

store: Add accessors for the ostree repo #1509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions crates/lib/src/deploy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ pub(crate) async fn cleanup(sysroot: &Storage) -> Result<()> {

// We create clones (just atomic reference bumps) here to move to the thread.
let repo = sysroot.repo();
let sysroot = sysroot.sysroot.clone();
let sysroot = sysroot.get_ostree_cloned()?;
let repo_prune =
ostree_ext::tokio_util::spawn_blocking_cancellable_flatten(move |cancellable| {
let locked_sysroot = &SysrootLock::from_assumed_locked(&sysroot);
Expand Down Expand Up @@ -543,7 +543,7 @@ async fn deploy(
None
};
// Clone all the things to move to worker thread
let sysroot_clone = sysroot.sysroot.clone();
let ostree = sysroot.get_ostree_cloned()?;
// ostree::Deployment is incorrectly !Send 😢 so convert it to an integer
let merge_deployment = merge_deployment.map(|d| d.index() as usize);
let stateroot = stateroot.to_string();
Expand All @@ -553,7 +553,7 @@ async fn deploy(
let r = async_task_with_spinner(
"Deploying",
spawn_blocking_cancellable_flatten(move |cancellable| -> Result<_> {
let sysroot = sysroot_clone;
let ostree = ostree;
let stateroot = Some(stateroot);
let mut opts = ostree::SysrootDeployTreeOpts::default();

Expand All @@ -565,11 +565,11 @@ async fn deploy(
if let Some(kargs) = override_kargs.as_deref() {
opts.override_kernel_argv = Some(&kargs);
}
let deployments = sysroot.deployments();
let deployments = ostree.deployments();
let merge_deployment = merge_deployment.map(|m| &deployments[m]);
let origin = glib::KeyFile::new();
origin.load_from_data(&origin_data, glib::KeyFileFlags::NONE)?;
let d = sysroot.stage_tree_with_options(
let d = ostree.stage_tree_with_options(
stateroot.as_deref(),
&ostree_commit,
Some(&origin),
Expand Down
29 changes: 20 additions & 9 deletions crates/lib/src/store/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub(crate) struct Storage {
pub physical_root: Dir,

/// The OSTree storage
pub sysroot: SysrootLock,
ostree: SysrootLock,
Copy link

Choose a reason for hiding this comment

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

Right, the change in scope here was indeed intentional because your helper functions here are now doing the cloning, rather than having them cloned in deploy.rs, I see.

/// The composefs storage
pub composefs: OnceCell<Arc<ComposefsRepository>>,
/// The containers-image storage used foR LBIs
Expand Down Expand Up @@ -81,7 +81,7 @@ impl Deref for Storage {
type Target = SysrootLock;

fn deref(&self) -> &Self::Target {
&self.sysroot
&self.ostree
}
}

Expand Down Expand Up @@ -116,22 +116,33 @@ impl Storage {

Ok(Self {
physical_root,
sysroot,
ostree: sysroot,
run,
composefs: Default::default(),
store,
imgstore: Default::default(),
})
}

/// Access the underlying ostree repository
pub(crate) fn get_ostree(&self) -> Result<&SysrootLock> {
Ok(&self.ostree)
}

/// Access the underlying ostree repository
pub(crate) fn get_ostree_cloned(&self) -> Result<ostree::Sysroot> {
let r = self.get_ostree()?;
Ok((*r).clone())
}
Comment on lines +127 to +136
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

These accessor functions can be simplified. Since neither get_ostree nor get_ostree_cloned can fail in their current implementation, they shouldn't return a Result.

Additionally, (*r).clone() in get_ostree_cloned is a bit confusing. It can be simplified to self.get_ostree().clone() after changing get_ostree's signature.

This change would make the API cleaner and more idiomatic, and simplify call sites by removing unnecessary ? operators.

    /// Access the underlying ostree repository
    pub(crate) fn get_ostree(&self) -> &SysrootLock {
        &self.ostree
    }

    /// Access the underlying ostree repository
    pub(crate) fn get_ostree_cloned(&self) -> ostree::Sysroot {
        self.get_ostree().clone()
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes the use of Result is intentional here because I want to change it to be a fallible operation in the future.


/// Access the image storage; will automatically initialize it if necessary.
pub(crate) fn get_ensure_imgstore(&self) -> Result<&crate::imgstorage::Storage> {
if let Some(imgstore) = self.imgstore.get() {
return Ok(imgstore);
}
let sysroot_dir = crate::utils::sysroot_dir(&self.sysroot)?;
let sysroot_dir = crate::utils::sysroot_dir(&self.ostree)?;

let sepolicy = if self.sysroot.booted_deployment().is_none() {
let sepolicy = if self.ostree.booted_deployment().is_none() {
// fallback to policy from container root
// this should only happen during cleanup of a broken install
tracing::trace!("falling back to container root's selinux policy");
Expand All @@ -141,8 +152,8 @@ impl Storage {
// load the sepolicy from the booted ostree deployment so the imgstorage can be
// properly labeled with /var/lib/container/storage labels
tracing::trace!("loading sepolicy from booted ostree deployment");
let dep = self.sysroot.booted_deployment().unwrap();
let dep_fs = deployment_fd(&self.sysroot, &dep)?;
let dep = self.ostree.booted_deployment().unwrap();
let dep_fs = deployment_fd(&self.ostree, &dep)?;
lsm::new_sepolicy_at(&dep_fs)?
};

Expand All @@ -167,7 +178,7 @@ impl Storage {

// Bootstrap verity off of the ostree state. In practice this means disabled by
// default right now.
let ostree_repo = &self.sysroot.repo();
let ostree_repo = &self.ostree.repo();
let ostree_verity = ostree_ext::fsverity::is_verity_enabled(ostree_repo)?;
if !ostree_verity.enabled {
tracing::debug!("Setting insecure mode for composefs repo");
Expand All @@ -182,7 +193,7 @@ impl Storage {
#[context("Updating storage root mtime")]
pub(crate) fn update_mtime(&self) -> Result<()> {
let sysroot_dir =
crate::utils::sysroot_dir(&self.sysroot).context("Reopen sysroot directory")?;
crate::utils::sysroot_dir(&self.ostree).context("Reopen sysroot directory")?;

sysroot_dir
.update_timestamps(std::path::Path::new(BOOTC_ROOT))
Expand Down
Loading