Page MenuHomeFreeBSD
Differential D59377

mkimg: Restore offset support for the SIZE partition form
AcceptedPublic
Actions

Authored by olivier on Thu, Sep 3, 11:29 PM.
Tags
None
Referenced Files
F170904746: D59377.diff
Mon, Sep 7, 11:36 AM
Unknown Object (File)
Sat, Sep 5, 5:17 AM
Unknown Object (File)
Sat, Sep 5, 4:36 AM
Unknown Object (File)
Sat, Sep 5, 4:34 AM
Unknown Object (File)
Fri, Sep 4, 1:42 PM
Unknown Object (File)
Fri, Sep 4, 10:02 AM
Subscribers
jlduran
jrtc27

Details

Reviewers
imp
jlduran
Summary

Commit 50c1240ebfaf moved the offset parsing into the PART_KIND_FILE
case of the switch, leaving PART_KIND_SIZE with no offset handling.
The offset was then silently ignored, so "-p efi::$size:$start" as used
by release/${ARCH}/mkisoimages.sh packed the ESP immediately after the
preceding partition.

Parse the offset outside the switch so both forms honour it. Add tests
covering absolute and relative offsets in both forms.

Test Plan

Regression test added

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

olivier requested review of this revision.Thu, Sep 3, 11:29 PM
olivier created this revision.
jrtc27 added a subscriber: jrtc27.Thu, Sep 3, 11:45 PM
jrtc27 added inline comments.
mkimg/tests/mkimg_offset_test.sh
25

This test file feels like it came out an LLM, as does a lot of the revision's summary

jrtc27 added inline comments.Thu, Sep 3, 11:50 PM
mkimg/tests/mkimg_offset_test.sh
25

(The bug itself is quite boring and obvious when you read the commit that introduced it, and does not warrant more than a short paragraph written by a human for a human, rather than this spew of unnecessary detail that, in my view, makes it harder to understand what the actual issue is, not easier, besides just being obnoxiously verbose)

olivier added inline comments.Thu, Sep 3, 11:50 PM
mkimg/tests/mkimg_offset_test.sh
25

yes, LLM are great to generate tests.
Would you like I reduce the too verbose comments of this ?

imp accepted this revision.Fri, Sep 4, 3:01 AM

I'd remove the IMPACT "section" header, and just summarize more briefly.
And the last paragraph can be summarized with the standard 'Fixes: ' trailer.

mkimg/tests/mkimg_offset_test.sh
25

Yea, this feels more like a sentence that talks about needing to test all the different forms to ensure the offsets are right.

This revision is now accepted and ready to land.Fri, Sep 4, 3:01 AM
olivier updated this revision to Diff 185813.Fri, Sep 4, 12:00 PM
olivier retitled this revision from mkimg ignores the offset in the ::size form, producing unbootable amd64 installation ISOs on main to mkimg: Restore offset support for the SIZE partition form.
olivier edited the summary of this revision. (Show Details)

apply remarks

This revision now requires review to proceed.Fri, Sep 4, 12:00 PM
jlduran added a subscriber: jlduran.Fri, Sep 4, 1:23 PM
jlduran accepted this revision.Fri, Sep 4, 1:57 PM
jlduran added inline comments.
mkimg/tests/mkimg_offset_test.sh
30

If 4K sectors were to be tested:

off=$(( (2 * sector_size) + (index - 1) * 128 + 32 ))

where

sector_size=${3:-512}
This revision is now accepted and ready to land.Fri, Sep 4, 1:57 PM
olivier added inline comments.Fri, Sep 4, 2:16 PM
mkimg/tests/mkimg_offset_test.sh
30

good catch!
let’s add some 4K tests too.

olivier updated this revision to Diff 185822.Fri, Sep 4, 2:25 PM

Adding 4K sector size tests

This revision now requires review to proceed.Fri, Sep 4, 2:25 PM
jlduran accepted this revision.Fri, Sep 4, 2:41 PM
This revision is now accepted and ready to land.Fri, Sep 4, 2:41 PM

Revision Contents
Changeset List

PathSize
mkimg/
usr.bin/
mkimg/
20 lines
tests/
1 line
mkimg/
tests/
149 lines

Diff 185813

View Options

mkimg/mkimg.c

Loading...
View Options

mkimg/tests/Makefile

Loading...
View Options

mkimg/tests/mkimg_offset_test.sh

Loading...