-
Notifications
You must be signed in to change notification settings - Fork 54
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
disk: tweak ensure{Btrfs,LVM}() to be more linear #1073
base: main
Are you sure you want to change the base?
Conversation
Tiny drive-by commit to make `ensure{Btrfs,LVM}()` more linear to read. I.e. avoid nested if/else when returning and do error checks early.
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.
🤏 tiny nitpick
} else { | ||
return fmt.Errorf("Unsupported parent for LVM") | ||
part.Type = "8e" |
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.
Let's make this a package constant alongside the others.
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 can push it here:
diff --git a/pkg/disk/disk.go b/pkg/disk/disk.go
index fdfd027cb..25da6d657 100644
--- a/pkg/disk/disk.go
+++ b/pkg/disk/disk.go
@@ -70,6 +70,9 @@ const (
// Partition type ID for ESP on dos
DosESPID = "ef00"
+
+ // Partition type ID for LVM partitions on dos
+ DosLvmID = "8e"
)
// pt type -> type -> ID mapping for convenience
diff --git a/pkg/disk/partition_table.go b/pkg/disk/partition_table.go
index d91df09bd..947631b37 100644
--- a/pkg/disk/partition_table.go
+++ b/pkg/disk/partition_table.go
@@ -733,7 +733,7 @@ func (pt *PartitionTable) ensureLVM() error {
if pt.Type == PT_GPT {
part.Type = LVMPartitionGUID
} else {
- part.Type = "8e"
+ part.Type = DosLvmID
}
return nil
but what is curious is that https://github.com/osbuild/images/blob/main/pkg/disk/disk.go#L82 uses a different DOS id for lvm it seems?
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 83
is still valid as it covers any generic Linux partition type but according to https://tldp.org/HOWTO/Partition-Mass-Storage-Definitions-Naming-HOWTO/x190.html you're right, 8e
is specific for LVM, so let's add 8e
and add it to the ID map.
Tiny drive-by commit to make
ensure{Btrfs,LVM}()
more linear to read. I.e. avoid nested if/else when returning and do error checks early.Best viewed side-by-side as the diff makes it hard to follow.
[edit: moved to draft as I have not inspected how much testing this function has, I'm pretty confident in the change but would really like to double check the test situation as well]
[edit2: played around a bit more with the tests and it seems it's well covered so moving out of draft again]