Skip to content
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

image_partition: add ability to skip "start" and specify relative offsets #285

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 84 additions & 7 deletions actions/image_partition_action.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,8 @@ unique.

'none' fs type should be used for partition without filesystem.

- start -- offset from beginning of the disk there the partition starts.

- end -- offset from beginning of the disk there the partition ends.

For 'start' and 'end' properties offset can be written in human readable
form -- '32MB', '1GB' or as disk percentage -- '100%'.
- end -- offset from beginning of the disk there the partition ends or
offset from beginning of where the partition starts specified with prefix "+"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather then having end be either end point on the disk or the size of the disk it would make more sense to have a seperate size property and require either end or size to be provided.

size should be in similar to the units parted accepts (https://www.gnu.org/software/parted/manual/html_node/unit.html) ideally; The image size parsing is mostly there, only % and s really sould be handled. I don't think anyone still cares about cylinders :).

same parsing should probably be use for start and end rather then the current state were we just pass on whatever the user gives to parted and hope :)..

That way it's both more clear for end-user, cleanup one of the debos oddities and more flexible then this implementation where there seems to be very specific requirements between the start and end value "types"


Optional properties:

Expand All @@ -80,6 +76,11 @@ GPT sets the partition type to Linux Swap.
For msdos partition types hex codes see: https://en.wikipedia.org/wiki/Partition_type
For gpt partition type GUIDs see: https://systemd.io/DISCOVERABLE_PARTITIONS/

- start -- offset from beginning of the disk there the partition starts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to note why this is optional and what would happen if it isn't actually provided.


For 'start' and 'end' properties offset can be written in human readable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to duplicate this inline so that both start and end properties have this.

form -- '32MB', '1GB' or as disk percentage -- '100%'.

- features -- list of additional filesystem features which need to be enabled
for partition.

Expand Down Expand Up @@ -157,6 +158,7 @@ import (
"path"
"path/filepath"
"sort"
"strconv"
"strings"
"syscall"
"time"
Expand Down Expand Up @@ -584,6 +586,61 @@ func (i ImagePartitionAction) PostMachineCleanup(context *debos.DebosContext) er
return nil
}

func ParseOffset(offset string) (int, string, error) {
/* Extract value of offset (int) and type from string*/
var v, t []rune
for _, l := range offset {
switch {
case l >= '0' && l <= '9':
v = append(v, l)
case l >= 'A' && l <= 'Z', l >= 'a' && l <= 'z', l == '%':
t = append(t, l)
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop here allows for arbitrary alpha-numeric word soup. For example "A01Z99%" is considered sane by the above.
Let's use a regex akin to the one in #292.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thanhtunng Any update?


val, err := strconv.Atoi(string(v))
typ := string(t)

if err != nil {
return 0, "", fmt.Errorf("Can not parse %s to integer", string(v))
}

return val, typ, nil
}

func CalculateOffset(start, end string) (string, error) {
/* Do addition if end value uses a '+' prefix */
if strings.HasPrefix(end, "+") {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would move the "HasPrefix" check in the caller.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

end = strings.Split(end, "+")[1]
valEnd, typeEnd, errStart := ParseOffset(end)
valStart, typeStart, errEnd := ParseOffset(start)

if typeStart != typeEnd {
return "", fmt.Errorf("Relative offset types are not consistent")
}

if errStart != nil {
return "", errStart
} else if errEnd != nil {
return "", errEnd
}

end = strconv.Itoa(valStart+valEnd) + typeEnd
}
return end, nil
}

func ValidPercentage(offset string) error {
val, typ, err := ParseOffset(offset)
if err != nil {
return err
}
if typ == "%" && val > 100 {
return fmt.Errorf("Size can not exceed 100%%")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: two %%

}
return nil
}

func (i *ImagePartitionAction) Verify(context *debos.DebosContext) error {
if len(i.GptGap) > 0 {
log.Println("WARNING: special version of parted is needed for 'gpt_gap' option")
Expand All @@ -598,7 +655,9 @@ func (i *ImagePartitionAction) Verify(context *debos.DebosContext) error {
}

num := 1
prevEnd := "0%"
for idx, _ := range i.Partitions {
var err error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove this by having later on p.End, err := blah

p := &i.Partitions[idx]
p.number = num
num++
Expand Down Expand Up @@ -642,12 +701,30 @@ func (i *ImagePartitionAction) Verify(context *debos.DebosContext) error {
}

if p.Start == "" {
return fmt.Errorf("Partition %s missing start", p.Name)
p.Start = prevEnd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to move this logic into the Run stage rather than Verify stage potentially with some comment about what is going on.

}

err = ValidPercentage(p.Start)
if err != nil {
return err
}

if p.End == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really should have a test case as well.

return fmt.Errorf("Partition %s missing end", p.Name)
}

p.End, err = CalculateOffset(p.Start, p.End)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above, check here if prefix is +

if err != nil {
return err
}

err = ValidPercentage(p.End)
if err != nil {
return err
}

prevEnd = p.End

switch p.FS {
case "fat32":
p.FS = "vfat"
Expand Down