Page MenuHomeFreeBSD

mkimg: Add maximum capacity and percentage partition size
AbandonedPublic

Authored by manu on Apr 12 2017, 7:39 AM.

Details

Reviewers
marcel
bapt
gjb
bcr
Group Reviewers
manpages
Summary

mkimg: Add -m and -C argument

-c is still used to specify a minimum capacity for the disk.
Add -m which specify a maximum capacity, if the size of the given
partition exceed it, mkimg will fail.
Add -C for specifying a fixed capacity size, this is equivalent of
specifying -c and -m with the same value

Add support for percent in partition size
If a maximum capacity is given (either by -m or -C) a partition size
can be given in percentage of the whole disk capacity (minus the metadata
size).

Sponsored-By: Gandi.net

Test Plan

Creating disk images with maximum size and percentage value for sizes.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8875
Build 9258: arc lint + arc unit

Event Timeline

manu created this revision.Apr 12 2017, 7:39 AM
bapt accepted this revision.Apr 12 2017, 8:31 AM
bapt added a reviewer: gjb.

Add glen who is a heavy user of mkimg iirc for the releases

This revision is now accepted and ready to land.Apr 12 2017, 8:31 AM
bcr accepted this revision.Apr 12 2017, 9:29 AM
bcr added a subscriber: bcr.

OK from manpages.

marcel added inline comments.Apr 12 2017, 3:51 PM
usr.bin/mkimg/mkimg.c
394

Note that the current behaviour is deliberate. -c guarantees a minimum size, not a maximum size. I'm ok with changing the behaviour, but please update the manpage.

424–439

I think it's better if we have to specify something for the size. For one, it "feels" wrong to not specify something after '::', but secondly and more importantly I think we have to consider other use cases.

The use case you're addressing is to have the last partition take 100% of the available space. By stating it this way, it's easy to envision a use case where the last 2 partitions each get 50% of the available space. Thus, I think we should have the size of the last "flex" partition be specified as a percentage, as in:
-p freebsd-ufs::100%

Then later, we can enhance this to allow multiple "flex" partitions, each having a percentage (and the sum of all percentages not larger than 100%).

I think we also have a need for an indirect size. That is: the size of the partition is to be the same as the size of another partition. This is needed for dual root images, that ping-pong between the partitions as part of upgrades. This case can also be handled as "flex" partitions, but only if the images starts of with both root partitions empty. That's less likely.

In any case: the indirect size could be specified as "*X", where X is some reference to a partition.

Thus: I think it's better to have to specify something for the size and not keep it empty. That way future enhancements fall into place more naturally.

425

This logic is fine, given that you only handle 1 partition and that the partition is to be the last one. However, you don't actually make sure this is the last partition. Also, I would suggest renaming "endblock" to something like "metablocks" or anything that doesn't feel like it holds an logical block address.

manu added inline comments.Apr 12 2017, 4:08 PM
usr.bin/mkimg/mkimg.c
394

I did miss that in the manpage ... I will update it as I don't see any use case for having a minimum size but if you have one I'll add another argument for maximum size.

424–439

I'll update the code with size as percentage for the last partition, I like the idea.
I'll start to think at the other ideas as I like them too :)

425

If you provide two partition with no size it works because the first one takes all the space and the one after simply fail because no space is left.
I'll rename endblock to metablocks.

gjb edited edge metadata.Apr 12 2017, 6:33 PM
In D10367#214807, @bapt wrote:

Add glen who is a heavy user of mkimg iirc for the releases

The releases do not use the '-c' option, so this should have no impact either way.

manu updated this revision to Diff 27513.Apr 18 2017, 12:35 PM
manu edited edge metadata.

Restore original -c argument and add two new : -m (max capacity) and -C (fixed capacity)
-C is like specifying -c and -m with same value.
With -m, mkimg will fail is size of partition exceed the size of the disk

Change the dynamic partition size to use percent, actual size is calculated based on the max capacity of the disk.

This revision now requires review to proceed.Apr 18 2017, 12:35 PM
bapt accepted this revision.Apr 18 2017, 1:02 PM
This revision is now accepted and ready to land.Apr 18 2017, 1:02 PM
manu retitled this revision from mkimg: Respect disk capacity and allow dynamic size partition to mkimg: Add maximum capacity and percentage partition size.Apr 24 2017, 1:48 PM
manu edited the summary of this revision. (Show Details)
manu edited the test plan for this revision. (Show Details)
manu updated this revision to Diff 27679.Apr 24 2017, 1:52 PM
manu edited edge metadata.

Update usage on size format.

This revision now requires review to proceed.Apr 24 2017, 1:52 PM
marcel added inline comments.Apr 24 2017, 3:35 PM
usr.bin/mkimg/mkimg.1
40–42

Sorry to be such a nitpick, but I think the choice of letters is non-intuitive. If -c is to set the lower limit (=lower caps), then logically and intuitively -C is to be used for the upper limit (=upper caps). If it's documented that way (as in: minimum/lower capacity == lower capitalization and maximum/upper capacity == upper capitalization) then it's all very intuitive.

Using m for maximum is odd. It would be better to use -m for minimum and -M for maximum. But alas, we already have -c for minimum.

So, maybe we should take it 1 step at a time: Just add an option for the maximum/upper capacity, which means that if you want a fixed size image you need to specify both -c and -C and they both will have the same argument.

Then later we can add a third option to make the use-case a little friendlier. But I would actually argue for introducing a long option for that: --capacity. We're running out of single-letter options in that all the good ones are taken and we're forced to look for letters that are less intuitive and/or natural, without being entirely unrelated.

131

BTW: maximum is the word to use in this case.

360

For manpages, it helps to be a little pedantic (like me :-)

You use size multiple times in a single sentence and size refers to different entities. The sentence starts with "If a disk size is given", which explicitly states that size is the disk size. But later in the sentence it says "supports [a] percentage for the size", without explicitly stating that in this case size refers to the partition size. As such, people will read it as disk size, because that's the size the sentence begins with.
Then the sentence has "The actual size is calculated from", which is really causing confusion at that point because the disk size was given. After a few times reading that sentence people will probably realize that the 2nd and 3rd use of size relates to the partition, because that makes more sense.

It helps to prefix size with either disk or partition and do it unconditionally. Non-native english speakers/readers have an easier time reading the manpage in that case.

362

I like how this looks.

usr.bin/mkimg/mkimg.c
154

You actually implemented what I suggested, so I guess the manpage is wrong :-)

marcel added inline comments.Apr 24 2017, 3:54 PM
usr.bin/mkimg/mkimg.c
396

This is actually wrong. If the image size is smaller than the minimum capacity, then the size just needs to be increased to the minimum capacity. The MAX of min_capsz and max_capsz is always max_capsz (under the assumption that we error out if max_capsz is less than min_capsz and there's no reason to increase the image size to the maximum when it's smaller than the minimum.

426

Why not save the pointer that strchr(3) returns and just do: *percntptr = '\0'.

433

This is wrong. The fundamental problem is that we don't know the size of the other partitions yet. As such, while we know what the capacity of the image is, we don't know how much of that is taken by partitions that don't have a "flexible" size. The percentage given can only be a fraction of the unused space, not a fraction of the total space.

The code really only handles a percentage for the last partition, but doesn't actually enforce that. The manpage however shows that you can give a percentage for all partition sizes and that clearly doesn't work.

Either document that only the last partition can occupy a percentage of the available free space (as defined by the maximum capacity of the image) and enforce that in the code, or change the code to handle a percentage for any partition.

Note: in order to handle any partition, you first need to determine the size of the non-flexible partitions. This means looping over the partitions twice:
First the non-flex partitions are loaded with a flexible offset and with the flex partitions having a size of 0. Then secondly, you determine the size of the flex partitions, which has the automatic result of moving partitions to their final offsets (hence the need to support flexible offsets).
This is not trivial, so it's perfectly fine to only support the last partition for now, but please enforce that.

497

This doesn't match the usage. It matches the manpage though. Please only introduce -C for the maximum capacity.

616

The error is not that useful. Why not say that the minimum capacity cannot be larger than the maximum capacity?

manu abandoned this revision.Apr 26 2017, 9:24 AM

Yeah a few stuff got mixed up in the same review, I'll open new ones, one per change, this will be easier.