blockdev: add support for saving mbr partitions #1047
Conversation
prestist
force-pushed
the
convert-mbr-to-gpt
branch
2 times, most recently
from
ed44938 to
7958259
Compare
prestist
force-pushed
the
convert-mbr-to-gpt
branch
2 times, most recently
from
44c102f to
ee7e164
Compare
prestist
force-pushed
the
convert-mbr-to-gpt
branch
from
ee7e164 to
2ee47d6
Compare
src/blockdev.rs
Outdated
|
|
||
| gpt_partition.partition_type_guid = *translate_mbr_types_to_gpt(mbr_partition.sys).as_bytes(); | ||
| gpt_partition.unique_partition_guid = *Uuid::new_v4().as_bytes(); | ||
| gpt_partition.starting_lba = u64::from(mbr_partition.starting_lba); |
There was a problem hiding this comment.
I think it's a bit more common to just use as u64 here. That said there's also the try_from() variant which might make sense to actually validate things.
src/blockdev.rs
Outdated
| for (i, p) in mbr.iter() { | ||
| if Self::matches_filters_mbr(i.try_into().unwrap(), p, filters) { | ||
| // if the partition is using any of the last 33 sectors, we need to bail. | ||
| if p.starting_lba + p.sectors < mbr.disk_size - 33 { |
There was a problem hiding this comment.
May be worth hosting 33 into a const FINAL_SECTORS: u32 = 33 above?
There was a problem hiding this comment.
We'd also need a comment explaining how we arrived at 33 sectors. But note also that MBR is supported on 4Kn-sector disks (they just aren't bootable). The backup GPT requires fewer sectors in that case.
GPTs are allowed to reserve more than the minimum number of sectors at either end of the disk. We don't really check for that in the non-MBR case either, and don't have a way to, since we haven't seen the install image's partition table. But I believe in that case we'd fail to restore partitions after installing and would then roll back to a fresh GPT with only the saved partitions, thus not losing data.
Long story short, I think we should probably skip this check. The dry run below should catch overlarge partitions if there are any. (But we should still have a test case for this.)
There was a problem hiding this comment.
In the commit message, the prefix should be blockdev:, not blockdev.rs:. Also I think "best effort" probably undersells the result. We do translate the important parts (offset/length/partition number); the boot flag isn't relevant and the type code is basically ignored on Linux. Let's explain which parts of the translation are fuzzy, but not introduce doubt about the validity of the result.
Cargo.toml
Outdated
| @@ -62,6 +62,7 @@ flate2 = "^1.0" | |||
| glob = "^0.3" | |||
| # disable default-enabled cli in gptman 0.x | |||
| gptman = { version = ">= 0.7, < 2", default-features = false } | |||
| mbrman = ">= 0.5, < 0.6" | |||
There was a problem hiding this comment.
Alphabetical order, please.
src/blockdev.rs
Outdated
| // if the partition is using any of the last 33 sectors, we need to bail. | ||
| if p.starting_lba + p.sectors < mbr.disk_size - 33 { | ||
| partitions.push(( | ||
| i.try_into().context("convert usize into u32")?, |
There was a problem hiding this comment.
If we're going to wrap with an error, let's make it clear what the problem was.
src/blockdev.rs
Outdated
| SavedPartitions::new(&mut disk, 512, &vec![label("*i*")]).unwrap(); | ||
| let mut mbr = mbrman::MBR::new_from(&mut disk, 512 as u32, [0xff; 4]) | ||
| .expect("could not create partition table"); | ||
|
|
There was a problem hiding this comment.
One logical test case spans multiple code blocks, which is confusing.
src/blockdev.rs
Outdated
| ); | ||
| // label and index | ||
| let saved = SavedPartitions::new(&mut disk, 512, &vec![Index(index(1), index(1))]).unwrap(); | ||
| assert!(saved.is_saved()); |
There was a problem hiding this comment.
We're only checking that we think we succeeded, but we're not actually verifying the results against expected values. Let's fix that. In particular, we should check the resulting type GUIDs.
We should be testing a non-trivial MBR layout, where we aren't asking to save all of the partitions. That layout should include logical partitions. And we should also test saving a wildcard label * (which we might decide should match either everything or nothing).
We should also test with a 4Kn disk.
prestist
force-pushed
the
convert-mbr-to-gpt
branch
from
2ee47d6 to
174f953
Compare
Fixes coreos#957. If a non extended MBR partition is marked to be saved, it will be translated to a gpt partition.
prestist
force-pushed
the
convert-mbr-to-gpt
branch
from
174f953 to
8889817
Compare
prestist
changed the title
Fixes #957. If a MBR partition is marked to be saved, it will be translated using best effort to GPT. To elaborate on "best effort"; there is metadata which is present in GPT but not MBR, so it might not translate 1:1.