-
Notifications
You must be signed in to change notification settings - Fork 136
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
base: main
Are you sure you want to change the base?
Conversation
Hi, Just to make sure, this is not an essential feature for us. Thanks, |
@thanhtunng @sangorrin thanks for the neat feature - was looking to have The MR does 3 things (see below) yet it's a single commit - do you mind splitting it up? Mind you I don't have commit access to the repo, so this is just a humble request. Thanks o/
|
We can ignore start value unless required by using the end of previous partition. Signed-off-by: Vu Thanh Tung <tung1.vuthanh@toshiba.co.jp>
The size of partition can also be easier to see if using end value as relative offset (e.g: +10Mib, +5%). However, the type of end value must be the same as starts. Signed-off-by: Vu Thanh Tung <tung1.vuthanh@toshiba.co.jp>
Signed-off-by: Vu Thanh Tung <tung1.vuthanh@toshiba.co.jp>
597b571
to
9e4f3d8
Compare
@evelikov thanks for your suggestion, i changed follows yours. Please let me know if you need any improvements. |
Looks great thank you. In case I was ambiguous earlier - I cannot merge this MR. since I do not have commit access. |
Tested the MR and it seems kind of broken. So it needs extra work until it's in good state. Example:
Issues:
|
case l >= 'A' && l <= 'Z', l >= 'a' && l <= 'z', l == '%': | ||
t = append(t, l) | ||
} | ||
} |
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.
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.
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.
@thanhtunng Any update?
|
||
func CalculateOffset(start, end string) (string, error) { | ||
/* Do addition if end value uses a '+' prefix */ | ||
if strings.HasPrefix(end, "+") { |
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.
Nit: I would move the "HasPrefix" check in the caller.
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 agree.
@evelikov Yeah the mismatch issue I already mentioned in commit message, anw the case startval ==0 I didnt think about, thanks~
|
You might be conflating things: The code in #292 covers the imagesize, which is a completely orthogonal thing. |
@thanhtunng Do you have an update about this? |
@obbardc I'm keen on pushing this forward. Is there any particular changes you'd like to see (aka anything more than what I've raised already)? I'm thinking about adding some tests to the CI - the snippet above + another one with explicit start and other units. Any pointers how to properly fix the parted error mentioned before - #285 (comment). It seems like we're either hitting a parted bug or an off-by-one error in debos. |
Proper in-depth automated tests for both relative & non-relative cases would be great with various line endings etc. I was thinking something along the lines of manually creating a partition table and comparing that at runtime. Happy to start small though ;-). |
@thanhtunng @evelikov Any update here ? It'd be great to get this reworked. |
Nothing from me since debos maintainership has been MIA over the last 6 months or so. Will be quite busy in the upcoming 2-3 months so it's unlikely I'll have the time, sorry. |
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.
We are missing some tests & fixing some nits. I'd be happy to merge once the above is complete.
@@ -80,6 +75,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. |
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.
It'd be good to note why this is optional and what would happen if it isn't actually provided.
@@ -80,6 +75,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. | |||
|
|||
For 'start' and 'end' properties offset can be written in human readable |
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.
It'd be good to duplicate this inline so that both start
and end
properties have this.
@@ -642,12 +643,14 @@ func (i *ImagePartitionAction) Verify(context *debos.DebosContext) error { | |||
} | |||
|
|||
if p.Start == "" { | |||
return fmt.Errorf("Partition %s missing start", p.Name) | |||
p.Start = prevEnd |
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.
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.
@@ -642,12 +643,14 @@ func (i *ImagePartitionAction) Verify(context *debos.DebosContext) error { | |||
} | |||
|
|||
if p.Start == "" { | |||
return fmt.Errorf("Partition %s missing start", p.Name) | |||
p.Start = prevEnd | |||
} | |||
if p.End == "" { |
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.
This really should have a test case as well.
case l >= 'A' && l <= 'Z', l >= 'a' && l <= 'z', l == '%': | ||
t = append(t, l) | ||
} | ||
} |
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.
@thanhtunng Any update?
|
||
func CalculateOffset(start, end string) (string, error) { | ||
/* Do addition if end value uses a '+' prefix */ | ||
if strings.HasPrefix(end, "+") { |
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 agree.
@@ -600,6 +646,7 @@ func (i *ImagePartitionAction) Verify(context *debos.DebosContext) error { | |||
num := 1 | |||
prevEnd := "0%" | |||
for idx, _ := range i.Partitions { | |||
var err error |
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.
you can remove this by having later on p.End, err := blah
@@ -649,6 +696,11 @@ func (i *ImagePartitionAction) Verify(context *debos.DebosContext) error { | |||
return fmt.Errorf("Partition %s missing end", p.Name) | |||
} | |||
|
|||
p.End, err = CalculateOffset(p.Start, p.End) |
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.
as above, check here if prefix is +
return err | ||
} | ||
if typ == "%" && val > 100 { | ||
return fmt.Errorf("Size can not exceed 100%%") |
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.
nit: two %%
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 "+" |
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.
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"
Currently start and end value must be specified explicitly
We can ignore start value unless required by using the end of
previous partition.
The size of partition can also be easier to see if using
end value as relative offset (e.g: +10Mib, +5%). However,
the type of end value must be the same as start's
Example:
firmware partition - start: 0% (as default), end: 5%
root partition - start: 64MB. end: 320MB