ipxe / wimboot

WIM bootloader
https://ipxe.org/wimboot
GNU General Public License v2.0
238 stars 42 forks source link

Wimboot specifying wim index not working in version 2.7.4 #43

Closed mcb30 closed 1 year ago

mcb30 commented 1 year ago

Discussed in https://github.com/ipxe/wimboot/discussions/40

Originally posted by **rushyn** January 20, 2023 Has any one else run in to an issue after updating to last wimboot(2.7.4) where index= (number) no longer works, and instead it boots the first index of the wim file? Updated from wimboot 2.5.2 going back to 2.5.2 resumes normal behavior.
rushyn commented 1 year ago

@mcb30 Sorry, its booting the last index of the wim file. I had originally said it was the first, but after checking it is the last index.

mcb30 commented 1 year ago

@mcb30 Sorry, its booting the last index of the wim file. I had originally said it was the first, but after checking it is the last index.

Thanks. I'm writing up some notes so I can be sure of what's going on internally. I'll post a write-up here when it's done, which will probably be tomorrow since it's already 1am local time.

mcb30 commented 1 year ago

A quick summary of what I believe is the root cause:

The original commit https://github.com/ipxe/wimboot/commit/350787d that added support for index=<N> worked by recording the specified index in patch->boot_index and the (extracted) boot image metadata corresponding to that index in patch->boot, then patching the WIM header at vdisk file read time to modify the index and boot image metadata fields within the WIM header.

Commit https://github.com/ipxe/wimboot/commit/7f42b27 added support for injecting arbitrary extra files into the WIM image, which involves a lot more patching of many more data structures. The code now maintains a permanent copy of the patched WIM header in patch->header, which gets copied in verbatim at vdisk file read time.

The extracted boot image metadata corresponding to the selected index still gets placed in patch->boot. It does not get written into the copy of the patched WIM header in patch->header. This is a bug that has existed ever since commit https://github.com/ipxe/wimboot/commit/7f42b27.

However, the bug has almost always been masked by the file injection feature that was introduced by that same commit. If any files are being injected, then the code needs to synthesize an additional boot image. This process includes updating the number of images and the selected boot image within patch->header, and also updating patch->header to point to a synthesized copy of the selected boot image metadata. So, when any files are injected, the boot image metadata in the patched WIM header in patch->header will be updated, and the bug will be masked.

And, in almost all cases under UEFI, there will always be at least one injected file, since wimboot itself will end up being injected, hence the bug was always masked under UEFI. At least until commit https://github.com/ipxe/wimboot/commit/ac4190a fixed that quirk. At which point it became possible to have no injected files under UEFI, and the original bug from four years earlier was finally exposed.

So: that's what has caused the problem. I'll defer attempting a fix until tomorrow!

mcb30 commented 1 year ago

@rushyn BTW, for when the time comes to commit a fix, could I please have your name and email address so you can get the appropriate credit in the commit log?

mcb30 commented 1 year ago

Oh, and there's unfortunately no way to test the "no injected files" code paths in the automated test suite, because the automated test mechanism relies on injecting files in order to be able to automatically detect when the test completes successfully. :sob:

rushyn commented 1 year ago

@rushyn BTW, for when the time comes to commit a fix, could I please have your name and email address so you can get the appropriate credit in the commit log?

@mcb30 Thank you, I have updated my GitHub profile you can now see my name and email.

mcb30 commented 1 year ago

@rushyn I've created PR #44 which should fix this. Could you please test and confirm whether or not this fixes the problem for you?