Skip to content

Only do "booted BIOS with no update" check during actual update #1096

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

Merged
cgwalters merged 1 commit into from
May 21, 2026
+21 −7
Merged

Only do "booted BIOS with no update" check during actual update #1096

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
9 changes: 6 additions & 3 deletions src/bios.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,12 +244,15 @@ impl Component for Bios {
}

fn query_update(&self, sysroot: &openat::Dir) -> Result<Option<ContentMetadata>> {
let content_metadata = get_component_update(sysroot, self)?;
get_component_update(sysroot, self)
}

fn query_requires_update(&self, sysroot: &openat::Dir) -> Result<()> {
// Failed as expected if booted with BIOS and no update metadata
if content_metadata.is_none() && !sysroot.exists("sys/firmware/efi")? {
if !sysroot.exists("sys/firmware/efi")? {
anyhow::bail!("Failed to find BIOS update metadata");
}
Ok(content_metadata)
Ok(())
}

fn run_update(&self, rootcxt: &RootContext, _: &InstalledContent) -> Result<InstalledContent> {
Expand Down
5 changes: 4 additions & 1 deletion src/bootupd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,10 @@ pub(crate) fn update(name: &str, rootcxt: &RootContext) -> Result<ComponentUpdat
std::cmp::Ordering::Less => p, // current < available -> upgrade
_ => return Ok(ComponentUpdateResult::AtLatestVersion),
},
None => return Ok(ComponentUpdateResult::AtLatestVersion),
None => {
component.query_requires_update(sysroot)?;
return Ok(ComponentUpdateResult::AtLatestVersion);
}
};

ensure_writable_boot()?;
Expand Down
5 changes: 5 additions & 0 deletions src/component.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ pub(crate) trait Component {
/// Used on the client to query for an update cached in the current booted OS.
fn query_update(&self, sysroot: &openat::Dir) -> Result<Option<ContentMetadata>>;

/// This is called in the update code if query_update() returned no metadata.
/// It should return an error if the current booted system should expect some
/// metadata for this component.
fn query_requires_update(&self, sysroot: &openat::Dir) -> Result<()>;
Copy link
Copy Markdown
Member

@Johan-Liebert1 Johan-Liebert1 May 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

imo we should accept Option<ContentMetadata> here so that we only return an error if the metadata is None. Otherwise just looking at the function, it's hard to figure out why we just error our if sys/firmware/efi doesn't exist for BIOS, when it not existing would always be true for BIOS booted systems

Copy link
Copy Markdown
Contributor Author

@alexlarsson alexlarsson May 20, 2026

Choose a reason for hiding this comment

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

Then the caller is quite weird if it always calls query_update() and then query_requires_update(). Right now it only calls query_requires_update() if metadata is None. If you want to do it like that, then I think having query_requires_update() call query_update() and then do the check is better than having to call both and passing the metadata forward.

Copy link
Copy Markdown
Member

@Johan-Liebert1 Johan-Liebert1 May 20, 2026

Choose a reason for hiding this comment

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

Right now it only calls query_requires_update() if metadata is None

right, and this is a little weird to me as it's not documented. Maybe the best approach is to simply document this in the function comments? Something like This function assumes that metadata for the component was None. It then throws an error if the system was booted with "component"?

Copy link
Copy Markdown
Contributor Author

@alexlarsson alexlarsson May 21, 2026

Choose a reason for hiding this comment

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

I pushed a new version with the function comment having a bit more detail for when this is called and what it should do.


/// Used on the client to run an update.
fn run_update(
&self,
Expand Down
9 changes: 6 additions & 3 deletions src/efi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,12 +613,15 @@ impl Component for Efi {
}

fn query_update(&self, sysroot: &openat::Dir) -> Result<Option<ContentMetadata>> {
let content_metadata = get_component_update(sysroot, self)?;
get_component_update(sysroot, self)
}

fn query_requires_update(&self, _sysroot: &openat::Dir) -> Result<()> {
// Failed as expected if booted with EFI and no update metadata
if content_metadata.is_none() && is_efi_booted()? {
if is_efi_booted()? {
anyhow::bail!("Failed to find EFI update metadata");
}
Ok(content_metadata)
Ok(())
}

fn validate(&self, current: &InstalledContent, device: &Device) -> Result<ValidationResult> {
Expand Down
Loading