-
Notifications
You must be signed in to change notification settings - Fork 92
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
rdcore: add bind-boot
command
#671
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collection of minor nits; superficially the high level looks sane.
.lines() | ||
.filter_map(|line| { | ||
let dev = split_lsblk_line(line); | ||
if dev.get("PARTTYPE").map(|t| t.as_str()) == Some(ESP_TYPE_GUID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another .as_deref()
candidate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I've also tried this at first, but
error[E0308]: mismatched types
--> src/blockdev.rs:895:55
|
895 | if dev.get("PARTTYPE").as_deref() == Some(ESP_TYPE_GUID) {
| ^^^^^^^^^^^^^ expected struct `std::string::String`, found `str`
|
= note: expected reference `&std::string::String`
found reference `&'static str`
I think it's because we're not dealing with an Option<String>
, but an Option<&String>
.
} | ||
vendor_dir.push(ent.path()); | ||
} | ||
if vendor_dir.len() != 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be cleaner to have let mut vendor_dir: Option<PathBuf>
and then move the error check inside like:
if vendor_dir.replace(ent.path()).is_some() {
bail!(...)
Then there'd be no need for an unwrap()
at the exit path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually did exactly that at first, but decided to match the logic in https://github.com/coreos/coreos-assembler/blob/d3c7ec094a02/src/cmd-buildextend-live#L492-L495 in that respect too since I like how it automatically handles the 0 case too.
src/blockdev.rs
Outdated
@@ -809,6 +809,56 @@ pub fn lsblk_single(dev: &Path) -> Result<HashMap<String, String>> { | |||
Ok(devinfos.remove(0)) | |||
} | |||
|
|||
fn lsblk_all() -> Result<Vec<HashMap<String, String>>> { | |||
let mut cmd = Command::new("lsblk"); | |||
cmd.arg("--noheadings") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: I find these slightly more readable as cmd.args(&["--noheadings", "--nodeps", "--list"])
etc.
(There's also my https://docs.rs/sh-inline/0.1.0/sh_inline/macro.bash_command.html macro which can make this just bash_command!("lsblk --noheadings --nodeps --list --paths --output NAME")
safely)
Requires: #658 (not conceptually, but just to reduce conflicts). |
Updated! Other comments will be addressed as part of #658. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM generally.
Updated for comments! |
This command does a few things: - adds a `boot=UUID=<uuid>` karg to ensure the rootfs mounts by UUID - "claims" the bootfs for the rootfs by adding a stamp file and erroring out if one already exists - adds a GRUB dropin containing the boot UUID A natural follow-up to this would be have `coreos-installer install` optimistically pre-randomize the bootfs UUID and adding the stamp file, though it's worth discussing separately. Part of coreos/fedora-coreos-tracker#976.
Rebased now that #658 is in and removed draft status! |
Opened #798 for this. |
This command does a few things:
boot=UUID=<uuid>
karg to ensure the rootfs mounts by UUIDout if one already exists
A natural follow-up to this would be have
coreos-installer install
optimistically pre-randomize the bootfs UUID and adding the stamp file,
though it's worth discussing separately.
Part of coreos/fedora-coreos-tracker#976.