Change in coreboot[master]: csb_patcher.sh: gets,checks,installs the coreboot and SeaBIOS patches
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/64873 )
Change subject: csb_patcher.sh: gets,checks,installs the coreboot and SeaBIOS patches ......................................................................
Patch Set 1:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/64873/comment/6e415318_fd0d370c PS1, Line 31: ./csb_patcher.sh Martin L Roth: "Maybe move it into util/scripts or make a new directory under util for it? It won't get merged into the coreboot root directory."
I need to address the other suggestions before doing this. + Ideally, get more patches merged to a coreboot master, although sometimes it is hard to do...
File csb_patcher.sh:
https://review.coreboot.org/c/coreboot/+/64873/comment/bf2124e1_aae54dc5 PS1, Line 280: floppy_mass_downloader Martin L Roth: "Move to a separate script? This isn't really related to doing patches, and would be useful by itself."
Me: "After my return I'm going to work more on SeaBIOS "multiple_floppies" patch, and - after it gets merged successfully - could integrate this floppy part into coreboot's configuration menu, similar to "Secondary payloads". Until this time I'm a bit hesitant about moving, although could copy it (with its' dependencies) into a separate script."
In short, this is blocked by that SeaBIOS master (without a "multiple_floppies" patch) does not support the multiple floppies. Need to work on that first.
https://review.coreboot.org/c/coreboot/+/64873/comment/4a7ac692_3f025266 PS1, Line 282: "./floppies/" Martin L Roth: "payloads/floppies? 3rdparty/floppies?"
https://review.coreboot.org/c/coreboot/+/64873/comment/6732b134_2bbf19dd PS1, Line 348: atombios_adder Paul Menzel: "The Video BIOS option ROM contains more than just ATOMBIOS, right? Update the function name?"
Me: "For the AMD cards, I think the whole ROM is occupied by AtomBIOS and its' function/data tables. Or is it wrong? I like a current name and hope its' not far from truth"
https://review.coreboot.org/c/coreboot/+/64873/comment/aef8eadc_7eeb2570 PS1, Line 649: unzipper "./patch?zip" : mover "./$4.diff" "./$1.diff" : csb_patcher_sha256sum_correct="$5 ./$1.diff" : csb_patcher_sha256sum_my=$( sha256sum "./$1.diff" ) Me: "Ideally the SHA256 checksums should also be verified BEFORE the unpacking."
https://review.coreboot.org/c/coreboot/+/64873/comment/7697d84f_cb0c5f50 PS1, Line 783: csb_mass_patcher Martin L Roth: "Instead of hardcoding these, it would be good to maybe pull them from a patch. It would be nice to be able to supply that patch on the command line, although defaulting something that points to your patchlist is fine, IMO.
That way this script would be useful other people's patch sets as well."
Me: "Although some things can't be pulled (i.e. SHA256), things like a diff filename - definitely could. Hope it can be done efficiently without adding many code lines (to keep this script smaller than 1k). I'll also try to create some interface for using this script with a different patches / patch sets, to make it more useful for the fellow coreboot'ers."
In addition, I need to think how to add more optional arguments without cluttering the whole thing.
-
Mike Banon (Code Review)