Support extending #35
Conversation
When primary partitions have some trailing free space, this PR allows to extend the partition to use up all trailing free space. Currently, only primary partitions are supported to be extended.
|
Ok I'm even more confused now because I don't think this answers to the original issue (yours) that was to resize the "disk" (I assumed the MBR partition table) and not a partition. Let's first go back to the original issue to clear out everything first. |
|
:) Yeah, I somehow expressed myself not clearly. I really meant to extend the partition. |
| /// .expect("could not read the partition table"); | ||
| /// ``` | ||
| pub fn read_from<R: ?Sized>(mut reader: &mut R, sector_size: u32) -> Result<MBR> | ||
| pub fn read_from<S>(mut reader: &mut S, sector_size: u32) -> Result<MBR> |
There was a problem hiding this comment.
I think we definitely need an extra function to make it easy for users to update the disk_size field of the MBR. So, we still need to add a function—similar to the one in gptman—that updates this information using the provided seekable object.
For reference, in gptman, this function updates various fields based on the seekable object:
/// Updates the header to match the specifications of the seeker given in argument.
/// `first_usable_lba`, `last_usable_lba`, `primary_lba`, `backup_lba`, `partition_entry_lba`
/// will be updated after this operation.
pub fn update_from<S: ?Sized>(&mut self, seeker: &mut S, sector_size: u64) -> Result<()>
where
S: Seek,
{In MBR, it's actually much simpler because there’s only one field to update: disk_size. However, it’s important to remember that this value is expressed in sectors! The function should look something like this:
self.disk_size = u32::try_from(reader.seek(SeekFrom::End(0))? / u64::from(sector_size))
.unwrap_or(u32::max_value());Why is this needed?
find_free_sectors() relies on disk_size to determine available space. It will never suggest free sectors beyond what the MBR partition table is supposed to handle.
A practical example:
Imagine swapping out your hard drive for a larger one and cloning (dd) your entire old disk onto the new one. The first thing you'd need to do is update the MBR header with the new disk size. Without this, the system wouldn’t recognize the newly available free space.
Let me know what you think! Happy to discuss further. 😊
| /// Currently, this function only supports primary partitions (index 1-4). | ||
| /// Logical partitions (index > 4) are not supported and will return an InvalidIndex error. |
There was a problem hiding this comment.
If we use find_free_sectors, this will work on all partitions. No need to implement anything special.
| /// assert_eq!(mbr[1].sectors, 40, "Partition should now be 40 sectors"); | ||
| /// assert_eq!(mbr[1].starting_lba + mbr[1].sectors, mbr[2].starting_lba); | ||
| /// ``` | ||
| pub fn extend_partition(&mut self, partition_index: usize) -> Result<u32> { |
There was a problem hiding this comment.
This function currently only extends a partition to its full possible size. I think we should split it into two separate functions for better clarity and usability:
-
A function to resize a partition to a specific size.[edit: does not sound very useful, it just does
This function should take a target size as an argument and update the partition’ssectorsfield accordingly. No need to updatefirst_chsandlast_chsas this is done for us already when writing the partition table on disk anyway.sectors = ...so I don't see the point of adding it at all actually] -
A function to determine the maximum possible size for a given partition.
This function should usefind_free_sectors()to identify the available space, as demonstrated in the test example below:let ss = 512_u32; let data = vec![0; 10 * ss as usize]; let mut cur = Cursor::new(data); let mut mbr = MBR::new_from(&mut cur, ss, [0xff; 4]).unwrap(); mbr.align = 1; mbr.header.partition_1.sys = 0x83; mbr.header.partition_1.starting_lba = 1; mbr.header.partition_1.sectors = 1; mbr.header.partition_3.sys = 0x0f; mbr.header.partition_3.starting_lba = 5; mbr.header.partition_3.sectors = 5; let spots = mbr.find_free_sectors(); let p = &mut mbr.header.partition_1; let the_spot = spots.into_iter().find(|(i, _)| *i == p.starting_lba + p.sectors); assert_eq!(the_spot, Some((2, 3))); let (_, free_sectors) = the_spot.unwrap();
By separating these concerns, we make the API clearer and more flexible, allowing users to either resize a partition to a specific size or extend it as much as possible based on available space. Let me know what you think!
| // Verify CHS addresses were updated | ||
| assert!(!mbr[1].last_chs.is_empty(), "Expected last_chs to be set"); | ||
| assert!( | ||
| mbr[1] | ||
| .last_chs | ||
| .is_valid(mbr.cylinders, mbr.heads, mbr.sectors), | ||
| "Expected last_chs to be valid for disk geometry" | ||
| ); |
There was a problem hiding this comment.
that is already tested elsewhere, no need to to this
| // Set disk geometry | ||
| mbr.cylinders = total_cylinders as u16; | ||
| mbr.heads = 16; | ||
| mbr.sectors = 63; | ||
|
|
||
| // Calculate cylinder size | ||
| let cylinder_size = (mbr.sectors as u32) * (mbr.heads as u32); |
There was a problem hiding this comment.
Don't bother with geometry at all. It's being taken care of and tested elsewhere. I prefer to keep the test as short as possible and really focus on what it is testing.
|
So you're sure you don't want to this? I'm going to create another PR now unless you want to do the contribution. |
|
Yes. I'm sure this is way to complicated and your proposal is sufficient for me. |
2 participants
Support extending primary partitions to the maximum available free trailing space + fix some clippy warnings.
I've to admit not to be the expert in the whole MBR area. Review and comments appreciated.
Closes #32