refactor(file): replace fdisk with gptman+mbrman by HarryWaschkeit · Pull Request #154 · omnect/omnect-cli

HarryWaschkeit · 2026-03-16T18:38:28Z

Summary

Replace the fragile fdisk-based partition detection in get_partition_info() with Rust-native gptman and mbrman crates.

Changes:

Add gptman (GPT with CRC32 validation) and mbrman (MBR including logical partitions) as dependencies
Add src/file/partition.rs with get_partition_data() and is_gpt()
Remove locale-sensitive fdisk shell invocation and regex parsing
Remove fdisk from package.metadata.deb depends
Change PartitionInfo fields from String to typed u32/u64

Reason

The previous implementation spawned fdisk -l and parsed its text output with regex — making it locale-sensitive, version-dependent, and requiring fdisk as a runtime system dependency.

Additionally, a pre-existing bug was fixed: PartitionInfo.end held the last sector (inclusive) from fdisk output and was passed directly as dd count=, but count expects the number of sectors. For a partition starting at sector 2048 and ending at 206847, this caused dd to copy 206847 sectors instead of the correct 204800.

Fixed semantics:

GPT: count = ending_lba - starting_lba + 1
MBR: count = p.sectors (already the sector count)

Verification

cargo check passes
cargo fmt -- --check passes
cargo clippy -- -D warnings passes
Manually tested against GPT and MBR image files

Replace fragile fdisk-based partition detection with Rust-native crates: - add gptman (GPT with CRC32 validation) and mbrman (MBR including logical partitions) as dependencies - add src/file/partition.rs: get_partition_data() and is_gpt() - remove locale-sensitive fdisk shell invocation and regex parsing - mbrman iter() correctly reaches logical partitions 5 and 6 - fix pre-existing bug: PartitionInfo.end (last sector) was used as dd count= instead of sector count; rename field to count with correct semantics (ending_lba - starting_lba + 1 for GPT, p.sectors for MBR) - remove fdisk from package.metadata.deb depends - change PartitionInfo fields from String to typed u32/u64 Signed-off-by: Harry Waschkeit <44188360+HarryWaschkeit@users.noreply.github.com>

HarryWaschkeit · 2026-03-17T15:21:18Z

@JoergZeidler @mlilien fyi

Copilot

Pull request overview

Refactors image partition detection away from shelling out to fdisk and instead uses Rust-native parsing via gptman (GPT) and mbrman (MBR), while also correcting partition sector-count semantics for the dd operations.

Changes:

Introduces src/file/partition.rs to read GPT/MBR partition start/count directly from the image.
Updates get_partition_info() and dd invocation to use typed u32/u64 fields and correct count semantics.
Adds gptman/mbrman dependencies and removes fdisk from Debian runtime dependencies.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/file/partition.rs`	New partition table parsing utilities using `gptman`/`mbrman`.
`src/file/mod.rs`	Wires in the new `partition` module.
`src/file/functions.rs`	Replaces `fdisk`+regex partition discovery with Rust-native parsing; fixes `dd count=` usage.
`Cargo.toml`	Adds `gptman`/`mbrman`; removes `fdisk` from `cargo-deb` depends.
`Cargo.lock`	Lockfile updates for the newly introduced crates and their transitive deps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/file/partition.rs

src/file/functions.rs

src/file/partition.rs

More a theoretical finding: if for some reason the GPT got corrupt in the WIC file, this change will at least bail out in case the upper sector number in the partition is less than the lower one. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Harry Waschkeit <44188360+HarryWaschkeit@users.noreply.github.com>

Copilot

Pull request overview

Refactors image partition detection to remove the locale/version-dependent fdisk shell parsing and replace it with Rust-native parsing via gptman (GPT) and mbrman (MBR), while also correcting partition sector count semantics for dd.

Changes:

Introduce src/file/partition.rs to detect GPT/MBR and compute (start, count) safely (including GPT end-start+1 semantics).
Update get_partition_info() to use the new Rust-based partition detection and change PartitionInfo fields to typed u32/u64.
Add gptman/mbrman dependencies and remove fdisk from the Debian runtime dependency list.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/file/partition.rs`	New module implementing GPT/MBR partition lookup and `dd`-compatible sector counts.
`src/file/functions.rs`	Replaces `fdisk`+regex parsing with calls into the new partition module; switches to typed partition fields and uses `count`.
`src/file/mod.rs`	Wires in the new `partition` module.
`Cargo.toml`	Adds `gptman`/`mbrman` crates; removes `fdisk` from `cargo-deb` depends.
`Cargo.lock`	Locks new transitive dependencies from `gptman`/`mbrman`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/file/partition.rs

src/file/functions.rs

- remove unused exec_cmd_with_output! macro left over after fdisk removal - partition.rs: capture GPT parse error and attach as context when MBR fallback also fails, avoiding silent misclassification of I/O errors - partition.rs: replace panicking gpt[n] index with iter().find() for safe bounds-checked GPT partition lookup Signed-off-by: Harry Waschkeit <44188360+HarryWaschkeit@users.noreply.github.com>

Copilot

Pull request overview

This PR removes the locale/version-sensitive fdisk shell parsing used for partition discovery and replaces it with Rust-native parsing via gptman (GPT w/ CRC validation) and mbrman (MBR incl. logical partitions), while also fixing the partition sector dd count= semantics.

Changes:

Introduce src/file/partition.rs implementing GPT/MBR partition lookup and GPT detection.
Refactor get_partition_info() to use the new Rust-native logic and switch PartitionInfo fields to typed numeric values (u32/u64).
Add gptman/mbrman dependencies and remove fdisk from Debian runtime dependencies.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/file/partition.rs	Adds GPT/MBR partition parsing utilities and sector-count calculation.
src/file/mod.rs	Registers the new `partition` module within `file`.
src/file/functions.rs	Switches partition lookup to the new module and uses `count` (not inclusive end) for `dd`.
Cargo.toml	Adds `gptman`/`mbrman` deps and removes `fdisk` from `cargo-deb` depends.
Cargo.lock	Updates lockfile for newly added crates and transitive deps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/file/partition.rs

src/file/functions.rs

src/file/partition.rs

Signed-off-by: Harry Waschkeit <44188360+HarryWaschkeit@users.noreply.github.com>

JanZachmann

Overall this is a solid improvement — replacing the locale-sensitive fdisk shell-out with native Rust crates eliminates a class of fragile parsing bugs and removes a runtime dependency. The dd count= bug fix is important and well-explained. Code is clean and prior review findings were addressed well.

A few minor items noted inline.

src/file/partition.rs

Replace silent truncating cast (partition_num as usize) with u32::try_from(i) for safe, self-documenting comparison in mbr.iter(). Signed-off-by: Harry Waschkeit <44188360+HarryWaschkeit@users.noreply.github.com>

HarryWaschkeit marked this pull request as draft March 16, 2026 18:39

HarryWaschkeit marked this pull request as ready for review March 17, 2026 15:19

HarryWaschkeit requested review from JanZachmann and Copilot March 17, 2026 15:20

Copilot started reviewing on behalf of HarryWaschkeit March 17, 2026 15:20 View session

Copilot AI reviewed Mar 17, 2026

View reviewed changes

src/file/partition.rs Outdated Show resolved Hide resolved

src/file/partition.rs Show resolved Hide resolved

src/file/functions.rs Show resolved Hide resolved

src/file/partition.rs Outdated Show resolved Hide resolved

src/file/partition.rs Outdated Show resolved Hide resolved

HarryWaschkeit requested a review from Copilot March 18, 2026 08:41

Copilot started reviewing on behalf of HarryWaschkeit March 18, 2026 08:41 View session

Copilot AI reviewed Mar 18, 2026

View reviewed changes

src/file/partition.rs Outdated Show resolved Hide resolved

src/file/partition.rs Show resolved Hide resolved

src/file/functions.rs Outdated Show resolved Hide resolved

HarryWaschkeit requested a review from Copilot March 18, 2026 12:52

Copilot started reviewing on behalf of HarryWaschkeit March 18, 2026 12:52 View session

Copilot AI reviewed Mar 18, 2026

View reviewed changes

src/file/partition.rs Show resolved Hide resolved

src/file/functions.rs Show resolved Hide resolved

src/file/partition.rs Show resolved Hide resolved

chore: bump version to 0.27.2

d951022

Signed-off-by: Harry Waschkeit <44188360+HarryWaschkeit@users.noreply.github.com>

JanZachmann reviewed Mar 18, 2026

View reviewed changes

src/file/partition.rs Show resolved Hide resolved

src/file/partition.rs Show resolved Hide resolved

src/file/partition.rs Show resolved Hide resolved

fix(file): use try_from for MBR partition index comparison

c9ec9b1

Replace silent truncating cast (partition_num as usize) with u32::try_from(i) for safe, self-documenting comparison in mbr.iter(). Signed-off-by: Harry Waschkeit <44188360+HarryWaschkeit@users.noreply.github.com>

HarryWaschkeit requested a review from JanZachmann March 19, 2026 13:40

JanZachmann approved these changes Mar 20, 2026

View reviewed changes

HarryWaschkeit merged commit 15a7d59 into omnect:main Mar 20, 2026
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(file): replace fdisk with gptman+mbrman#154

refactor(file): replace fdisk with gptman+mbrman#154
HarryWaschkeit merged 5 commits intoomnect:mainfrom
HarryWaschkeit:feature/gptman-mbrman-partition-detection

HarryWaschkeit commented Mar 16, 2026

Uh oh!

HarryWaschkeit commented Mar 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JanZachmann left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

HarryWaschkeit commented Mar 16, 2026

Summary

Reason

Verification

Uh oh!

HarryWaschkeit commented Mar 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JanZachmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants