Skip to content
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.

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

WIP/DNM: Use 2024 edition #1130

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
9 changes: 5 additions & 4 deletions lib/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -976,7 +976,8 @@ pub fn global_init() -> Result<()> {
// Setting the environment is thread-unsafe, but we ask calling code
// to invoke this as early as possible. (In practice, that's just the cli's `main.rs`)
// xref https://internals.rust-lang.org/t/synchronized-ffi-access-to-posix-environment-variable-functions/15475
std::env::set_var("HOME", "/root");
// TODO: Audit that the environment access only happens in single-threaded code.
unsafe { std::env::set_var("HOME", "/root") };
}
Ok(())
}
Expand Down Expand Up @@ -1012,7 +1013,7 @@ impl Opt {
I::Item: Into<OsString> + Clone,
{
let mut args = args.into_iter();
let first = if let Some(first) = args.next() {
let first = match args.next() { Some(first) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and I think all the other changes this is cargo fix being conservative with the drop ordering change. IMO if let is easier to read than match and so we should not apply these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let first: OsString = first.into();
let argv0 = callname_from_argv0(&first);
tracing::debug!("argv0={argv0:?}");
Expand All @@ -1030,9 +1031,9 @@ impl Opt {
return Opt::parse_from(base_args.chain(args.map(|i| i.into())));
}
Some(first)
} else {
} _ => {
None
};
}};
Opt::parse_from(first.into_iter().chain(args.map(|i| i.into())))
}
}
Expand Down
6 changes: 3 additions & 3 deletions lib/src/deploy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -783,7 +783,7 @@ pub(crate) fn switch_origin_inplace(root: &Dir, imgref: &ImageReference) -> Resu

let mut ostree_deploys = root.open_dir("sysroot/ostree/deploy")?.entries()?;
let deploydir = loop {
if let Some(ent) = ostree_deploys.next() {
match ostree_deploys.next() { Some(ent) => {
let ent = ent?;
if !ent.file_type()?.is_dir() {
continue;
Expand All @@ -795,9 +795,9 @@ pub(crate) fn switch_origin_inplace(root: &Dir, imgref: &ImageReference) -> Resu
if let Some(d) = child_dir.open_dir_optional("deploy")? {
break d;
}
} else {
} _ => {
anyhow::bail!("Failed to find a deployment");
}
}}
};
let newest_deployment = find_newest_deployment_name(&deploydir)?;
let origin_path = format!("{newest_deployment}.origin");
Expand Down
18 changes: 9 additions & 9 deletions lib/src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -826,12 +826,12 @@ async fn install_container(
.with_context(|| format!("Recursive SELinux relabeling of {d}"))?;
}

if let Some(cfs_super) = root.open_optional(OSTREE_COMPOSEFS_SUPER)? {
match root.open_optional(OSTREE_COMPOSEFS_SUPER)? { Some(cfs_super) => {
let label = crate::lsm::require_label(policy, "/usr".into(), 0o644)?;
crate::lsm::set_security_selinux(cfs_super.as_fd(), label.as_bytes())?;
} else {
} _ => {
tracing::warn!("Missing {OSTREE_COMPOSEFS_SUPER}; composefs is not enabled?");
}
}}
}

// Write the entry for /boot to /etc/fstab. TODO: Encourage OSes to use the karg?
Expand Down Expand Up @@ -1531,12 +1531,12 @@ pub(crate) async fn install_to_disk(mut opts: InstallToDiskOpts) -> Result<()> {
}

// At this point, all other threads should be gone.
if let Some(state) = Arc::into_inner(state) {
match Arc::into_inner(state) { Some(state) => {
state.consume()?;
} else {
} _ => {
// This shouldn't happen...but we will make it not fatal right now
tracing::warn!("Failed to consume state Arc");
}
}}

installation_complete();

Expand Down Expand Up @@ -1579,12 +1579,12 @@ fn remove_all_in_dir_no_xdev(d: &Dir, mount_err: bool) -> Result<()> {
let name = entry.file_name();
let etype = entry.file_type()?;
if etype == FileType::dir() {
if let Some(subdir) = d.open_dir_noxdev(&name)? {
match d.open_dir_noxdev(&name)? { Some(subdir) => {
remove_all_in_dir_no_xdev(&subdir, mount_err)?;
d.remove_dir(&name)?;
} else if mount_err {
} _ => if mount_err {
anyhow::bail!("Found unexpected mount point {name:?}");
}
}}
} else {
d.remove_file_optional(&name)?;
}
Expand Down
3 changes: 2 additions & 1 deletion lib/src/kargs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,8 @@ match-architectures = ["x86_64", "aarch64"]
.to_string();
let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap();
assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]);
std::env::set_var("ARCH", "aarch64");
// TODO: Audit that the environment access only happens in single-threaded code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It definitely doesn't, rust runs unit tests in multiple threads by default. We need to stop setting this environment variable in the tests at all. At a quick look it's not clear we need this line of code, shouldn't we be just doing let sys_arch = "aarch64"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it doesn't even reference the environment variable anywhere. No idea what that was ever about. I'll go open a separate PR against main to fix just that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it doesn't even reference the environment variable anywhere. No idea what that was ever about. I'll go open a separate PR against main to fix just that.

#1132

unsafe { std::env::set_var("ARCH", "aarch64") };
let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap();
assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]);
}
Expand Down
6 changes: 3 additions & 3 deletions lib/src/lsm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,12 +395,12 @@ where
// Apply the target mode bits
rustix::fs::fchmod(fd, mode).context("fchmod")?;
// If we have a label, apply it
if let Some(label) = label {
match label { Some(label) => {
tracing::debug!("Setting label for {destname} to {label}");
set_security_selinux(fd, label.as_bytes())?;
} else {
} _ => {
tracing::debug!("No label for {destname}");
}
}}
// Finally call the underlying writer function
f(w)
})
Expand Down
6 changes: 3 additions & 3 deletions lib/src/progress_jsonl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,11 +269,11 @@ impl ProgressWriter {
pub(crate) async fn into_inner(self) -> Result<Option<Sender>> {
// SAFETY: Propagating panics from the mutex here is intentional
let mut mutex = self.inner.lock().await;
if let Some(inner) = mutex.take() {
match mutex.take() { Some(inner) => {
Ok(Some(inner.fd.into_inner()))
} else {
} _ => {
Ok(None)
}
}}
}
}

Expand Down
12 changes: 6 additions & 6 deletions lib/src/status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,27 +131,27 @@ fn boot_entry_from_deployment(
cached_update,
},
incompatible,
) = if let Some(origin) = deployment.origin().as_ref() {
) = match deployment.origin().as_ref() { Some(origin) => {
let incompatible = crate::utils::origin_has_rpmostree_stuff(origin);
let (store, cached_imagestatus) = if incompatible {
// If there are local changes, we can't represent it as a bootc compatible image.
(None, CachedImageStatus::default())
} else if let Some(image) = get_image_origin(origin)? {
} else { match get_image_origin(origin)? { Some(image) => {
let store = deployment.store()?;
let store = store.as_ref().unwrap_or(&sysroot.store);
let spec = Some(store.spec());
let status = store.imagestatus(sysroot, deployment, image)?;

(spec, status)
} else {
} _ => {
// The deployment isn't using a container image
(None, CachedImageStatus::default())
};
}}};
(store, cached_imagestatus, incompatible)
} else {
} _ => {
// The deployment has no origin at all (this generally shouldn't happen)
(None, CachedImageStatus::default(), false)
};
}};

let r = BootEntry {
image,
Expand Down
12 changes: 6 additions & 6 deletions lib/src/store/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,17 +104,17 @@ impl Storage {

impl ContainerImageStore for ostree::Deployment {
fn store<'a>(&self) -> Result<Option<Box<dyn ContainerImageStoreImpl>>> {
if let Some(origin) = self.origin().as_ref() {
if let Some(store) = origin.optional_string("bootc", "backend")? {
match self.origin().as_ref() { Some(origin) => {
match origin.optional_string("bootc", "backend")? { Some(store) => {
let store =
crate::spec::Store::from_str(&store, true).map_err(anyhow::Error::msg)?;
Ok(Some(load(store)))
} else {
} _ => {
Ok(None)
}
} else {
}}
} _ => {
Ok(None)
}
}}
}
}

Expand Down
6 changes: 3 additions & 3 deletions ostree-ext/src/bootabletree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,11 @@ fn read_dir_optional(
/// The target directory will have a `vmlinuz` file representing the kernel binary.
pub fn find_kernel_dir_fs(root: &Dir) -> Result<Option<Utf8PathBuf>> {
let mut r = None;
let entries = if let Some(entries) = read_dir_optional(root, MODULES)? {
let entries = match read_dir_optional(root, MODULES)? { Some(entries) => {
entries
} else {
} _ => {
return Ok(None);
};
}};
for child in entries {
let child = &child?;
if !child.file_type()?.is_dir() {
Expand Down
40 changes: 20 additions & 20 deletions ostree-ext/src/chunking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,35 +136,35 @@ struct Generation {
dirmeta_found: BTreeSet<RcStr>,
}

fn push_dirmeta(repo: &ostree::Repo, gen: &mut Generation, checksum: &str) -> Result<()> {
if gen.dirtree_found.contains(checksum) {
fn push_dirmeta(repo: &ostree::Repo, r#gen: &mut Generation, checksum: &str) -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine as an automated fix, but I think it'd be nicer to rename this to generation instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100%, I did exactly that manually before I realized that cargo fix would do its own thing.

if r#gen.dirtree_found.contains(checksum) {
return Ok(());
}
let checksum = RcStr::from(checksum);
gen.dirmeta_found.insert(RcStr::clone(&checksum));
r#gen.dirmeta_found.insert(RcStr::clone(&checksum));
let child_v = repo.load_variant(ostree::ObjectType::DirMeta, checksum.borrow())?;
gen.metadata_size += child_v.data_as_bytes().as_ref().len() as u64;
r#gen.metadata_size += child_v.data_as_bytes().as_ref().len() as u64;
Ok(())
}

fn push_dirtree(
repo: &ostree::Repo,
gen: &mut Generation,
r#gen: &mut Generation,
checksum: &str,
) -> Result<glib::Variant> {
let child_v = repo.load_variant(ostree::ObjectType::DirTree, checksum)?;
if !gen.dirtree_found.contains(checksum) {
gen.metadata_size += child_v.data_as_bytes().as_ref().len() as u64;
if !r#gen.dirtree_found.contains(checksum) {
r#gen.metadata_size += child_v.data_as_bytes().as_ref().len() as u64;
} else {
let checksum = RcStr::from(checksum);
gen.dirtree_found.insert(checksum);
r#gen.dirtree_found.insert(checksum);
}
Ok(child_v)
}

fn generate_chunking_recurse(
repo: &ostree::Repo,
gen: &mut Generation,
r#gen: &mut Generation,
chunk: &mut Chunk,
dt: &glib::Variant,
) -> Result<()> {
Expand All @@ -176,7 +176,7 @@ fn generate_chunking_recurse(
let mut hexbuf = [0u8; 64];
for file in files {
let (name, csum) = file.to_tuple();
let fpath = gen.path.join(name.to_str());
let fpath = r#gen.path.join(name.to_str());
hex::encode_to_slice(csum, &mut hexbuf)?;
let checksum = std::str::from_utf8(&hexbuf)?;
let meta = repo.query_file(checksum, gio::Cancellable::NONE)?.0;
Expand All @@ -193,17 +193,17 @@ fn generate_chunking_recurse(
let (name, contents_csum, meta_csum) = item.to_tuple();
let name = name.to_str();
// Extend our current path
gen.path.push(name);
r#gen.path.push(name);
hex::encode_to_slice(contents_csum, &mut hexbuf)?;
let checksum_s = std::str::from_utf8(&hexbuf)?;
let dirtree_v = push_dirtree(repo, gen, checksum_s)?;
generate_chunking_recurse(repo, gen, chunk, &dirtree_v)?;
let dirtree_v = push_dirtree(repo, r#gen, checksum_s)?;
generate_chunking_recurse(repo, r#gen, chunk, &dirtree_v)?;
drop(dirtree_v);
hex::encode_to_slice(meta_csum, &mut hexbuf)?;
let checksum_s = std::str::from_utf8(&hexbuf)?;
push_dirmeta(repo, gen, checksum_s)?;
push_dirmeta(repo, r#gen, checksum_s)?;
// We did a push above, so pop must succeed.
assert!(gen.path.pop());
assert!(r#gen.path.pop());
}
Ok(())
}
Expand Down Expand Up @@ -246,7 +246,7 @@ impl Chunking {
let commit = commit.to_tuple();

// Load it all into a single chunk
let mut gen = Generation {
let mut r#gen = Generation {
path: Utf8PathBuf::from("/"),
..Default::default()
};
Expand All @@ -255,14 +255,14 @@ impl Chunking {
// Find the root directory tree
let contents_checksum = &hex::encode(commit.6);
let contents_v = repo.load_variant(ostree::ObjectType::DirTree, contents_checksum)?;
push_dirtree(repo, &mut gen, contents_checksum)?;
push_dirtree(repo, &mut r#gen, contents_checksum)?;
let meta_checksum = &hex::encode(commit.7);
push_dirmeta(repo, &mut gen, meta_checksum.as_str())?;
push_dirmeta(repo, &mut r#gen, meta_checksum.as_str())?;

generate_chunking_recurse(repo, &mut gen, &mut chunk, &contents_v)?;
generate_chunking_recurse(repo, &mut r#gen, &mut chunk, &contents_v)?;

let chunking = Chunking {
metadata_size: gen.metadata_size,
metadata_size: r#gen.metadata_size,
remainder: chunk,
..Default::default()
};
Expand Down
6 changes: 3 additions & 3 deletions ostree-ext/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -824,11 +824,11 @@ async fn container_export(

// Use enough layers so that each package ends in its own layer
// while respecting the layer ordering.
let max_layers = if let Some(contentmeta_data) = &contentmeta_data {
let max_layers = match &contentmeta_data { Some(contentmeta_data) => {
NonZeroU32::new((contentmeta_data.sizes.len() + 1).try_into().unwrap())
} else {
} _ => {
None
};
}};

let config = Config {
labels: Some(labels),
Expand Down
6 changes: 3 additions & 3 deletions ostree-ext/src/commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,11 @@ fn clean_subdir(root: &Dir, rootdev: u64) -> Result<()> {

fn clean_paths_in(root: &Dir, rootdev: u64) -> Result<()> {
for path in FORCE_CLEAN_PATHS {
let subdir = if let Some(subdir) = root.open_dir_optional(path)? {
let subdir = match root.open_dir_optional(path)? { Some(subdir) => {
subdir
} else {
} _ => {
continue;
};
}};
clean_subdir(&subdir, rootdev).with_context(|| format!("Cleaning {path}"))?;
}
Ok(())
Expand Down
Loading