install: Fail early if image sector size doesn't match destination #167
Conversation
|
Updated. |
|
CI is red due to a clippy warning. Other than that, PR looks fine and good to go. |
|
Updated for 📎. |
src/blockdev.rs
Outdated
| .iter() | ||
| .filter(|d| d.label.as_ref().unwrap_or(&"".to_string()) == "boot") | ||
| .collect::<Vec<&BlkDev>>(); | ||
| if boot_partitions.is_empty() { |
There was a problem hiding this comment.
Minor: since what we really care about here is getting the boot device, would make sense to compress as e.g.:
let dev = match boot_partitions.len() {
0 => bail!(...),
1 => boot_partitions[0],
_ => bail!(...),
};
src/blockdev.rs
Outdated
| source: BufReader<R>, | ||
| } | ||
|
|
||
| impl<R: Read> DiskSectorSizeReader<R> { |
There was a problem hiding this comment.
We're already reading and keeping the first 1M of the source image, right? Couldn't we just check that buffer? Doing it in a wrapping reader is fine, though it seems like unnecessary cognitive load/architecturing.
There was a problem hiding this comment.
Good call. That's much, much better.
We were complaining: Error: couldn't find boot device for /dev/sda but the problem isn't really the boot device, it's that the partition table isn't being read by the kernel.
Otherwise we'd copy the whole image, then complain that we couldn't find any partitions. Detect the image sector size by checking both possible offsets for a GPT, detect the destination sector size by ioctl, and fail if they don't match. If we don't detect any GPT, continue without error. In the future, detection could be expanded to cover additional partition table types.
Test fixtures are 10 MB disk images containing a single empty partition.
|
Updated. |
| // Were we asked to check sector size? | ||
| if let Some(expected) = expected_sector_size { | ||
| // Can we derive one from source data? | ||
| if let Some(actual) = detect_formatted_sector_size(&first_mb) { |
There was a problem hiding this comment.
If we're asked to expect a sector size, shouldn't we fail if we can't query the sector size from the image?
There was a problem hiding this comment.
It's a best-effort check to improve UX. We don't currently detect sector size on non-EFI CPU architectures.
|
Thanks all! |
Otherwise we'd copy the whole image, then complain that we couldn't find any partitions. Detect the image sector size by checking both possible offsets for a GPT, detect the destination sector size by
ioctl, and fail if they don't match.If we don't detect any GPT, continue without error. In the future, detection could be expanded to cover additional partition table types.
In addition, if no partitions are detected after writing the image, complain about it rather than saying
couldn't find boot device for /dev/sda. That should make it easier to track down non-sector-size issues with similar symptoms.Closes #161.