pengutronix / genimage

tool to generate multiple filesystem and flash images from a tree
GNU General Public License v2.0
309 stars 110 forks source link

image-hd: add forced-primary flag for higher MBR layout flexibility #248

Closed sairon closed 3 months ago

sairon commented 4 months ago

The current limitation of Genimage is that it is not able to create MBR images that have primary partitions that start after a logical partition. This can be useful for images that can be later resized based on the actual device size - for this operation the partition must be at the end of the device, and if it is present in a logical partition, it must be resized first, making it a two-step process.

This commit adds the "forced-primary" flag which can be used to indicate that the partition should be put into the disk's MBR instead of creating another logical partition. Validation ensures that this syntax allows to create such partitions only after an existing logical partition, and that the maximum number of MBR entries woudn't be exceeded by doing so.

Test cases for valid and invalid configuiration has been added. Also added few more details in the debug print to make it more obvious how the MBR/EBR layout looks like.

sairon commented 4 months ago

Having to set every partition to forced-primary after the first forced-primary is kinda over-constrained. genimage could do this implicitly. But maybe it is better for readability and validation. Either way works for me.

IMO "explicit is better than implicit" here - in the most extreme case you'll have to add it only to three partitions (if the first one would be the extended one) but for the resizing use case I mention above just one will be usually sufficient. And since it has this "I known what I'm doing" subtext, I'll prefer to do it this way. I think it wouldn't be harder to validate implicit forced-primary partitions though, since you can check if there's already been some forced-primary partition and know the number of MBR entries. But it's better for readability and less error-prone when moving things around.

michaelolbrich commented 4 months ago

I think this is getting a bit convoluted. Right now, we don't really track the actual extended partition and that makes things needlessly complex. So I think you should do the following:

It probably makes sense to keep a pointer to the extended partition in the hdimage.

sairon commented 4 months ago

Thanks Michael, I am not insisting on taking exactly the approach I propose in this PR, so anything that does the job works for me.

I think this is getting a bit convoluted. Right now, we don't really track the actual extended partition and that makes things needlessly complex. So I think you should do the following:

  • Insert an explicit extended partition at the right place in the list. It can be recognized by the partition_type 0x0f Then put extended_lba and extended_size as offset and size into that partition.

I am not sure how the resulting config you're suggesting should look like. Can you post an example? Currently you can specify a partition to be of whatever type but the rest of the code can't handle that you manually specified an extended partition. Also introducing this possibility shouldn't be a breaking change for old config syntax, so that might be also a limiting factor.

  • rename part->extended to logical, because that's what these partitions actually are.

Absolutely, it was initially slightly confusing for me too, so if we're doing any larger changes in this area, I agree renaming this field is a good idea.

sairon commented 4 months ago

Meh, sorry, I'm not really bright this morning so I misinterpreted that the comment was about the actual code and not the concept, thanks @agners for pointing that out in a private discussion.

Okay, then you can disregard basically the whole above comment :see_no_evil: Now I get it and agree with the rest as well, let's refactor it to make it less convoluted :+1:

sairon commented 4 months ago

Okay, I adjusted the extended partition handling as suggested, hope it's a bit more readable now. PTAL :pray:

michaelolbrich commented 3 months ago

Right, the check fails, because you didn't add the new files to EXTRA_DIST in Makefile.am. Can you fix that please? It's not noticeable for the tests that are expected to fail, but those files need to be added as well. I'll improve the test-suite to detect that.

michaelolbrich commented 3 months ago

With #252 "hdimage syntax" will fail as well. You can check for that locally by running make distcheck