ibm-s390-linux / s390-tools

Tools for use with the s390 Linux kernel and device drivers
MIT License
63 stars 59 forks source link

zipl: fix the relative $BOOT support #76

Closed sharkcz closed 4 years ago

sharkcz commented 4 years ago

Likely a dirty solution, but it works. Comments are welcome.

sharkcz commented 4 years ago

IBM, any comments/opinions? I did hit the issue today again and took me a while to realize it is this one :-)

sharkcz commented 4 years ago

rebased to current master

stefan-haberland commented 4 years ago

Hi, thanks for your work... I will have a second look.

stefan-haberland commented 4 years ago

Just tried the code and beside that you addressed one issue regarding the error message, there is still an error that when there is no BLS file we still get a BLS error message:

./zipl Using config file '/etc/zipl.conf' Error: BLS parsing '/boot/loader/entries': File '/boot/image-head2' not accessible

which is misleading. Since there is not even a "/boot/loader/entries" directory only a /etc/zipl.conf file.

I have an additional idea in issue #84 to simply remove the scan_check_bls() since the original zipl code does the same checking. So I will likely remove this code. Which on the other hand will remove everything you have in this pull request. Could you have a look at this? And comment if this would be suitable for you as well?

sharkcz commented 4 years ago

Yeah, I have already seen #84 and the "BLS" error is confusing. Removing duplicate code/checks is also good. So if after your changes zipl will handle the optional load address for kernel/initrd/parameters when BLS snippets are used, then I have no objections.

@martinezjavier, what do you think?

martinezjavier commented 4 years ago

@martinezjavier, what do you think?

I've commented in #84. @stefan-haberland sorry for missing your previous mention.

stefan-haberland commented 4 years ago

yes, this should also correctly use the load address in a BLS config but to be sure I will give this a second try.

stefan-haberland commented 4 years ago

just double checked... with removed scan_check_bls() this works as expected. Snipped containing:

linux /boot/image-head,40000
initrd /boot/initrd.img,950000

gives:

  component address:
    internal loader.: 0x0000a000-0x0000dfff
    parameters......: 0x00009000-0x00009fff
    kernel image....: 0x00040000-0x0092efff
    parmline........: 0x00010000-0x00010fff
    initial ramdisk.: 0x00950000-0x01b39fff
stefan-haberland commented 4 years ago

Since I somehow missed that the scan_check_bls() code was added with commit d716283 I had a second look at the reason for the code. And so it might not be a good idea to just remove it since this would regress the original use case. It might be a better idea to keep the code to update the search path if needed but remove all error handling for missing files from it since this is already done in scan_check().

I just tested a quick patch based on this pull request which works quite well. Will give this some polish and post for internal review and get back here.

If you are interested I can provide an early version. Just give me a sign.

martinezjavier commented 4 years ago

It might be a better idea to keep the code to update the search path if needed but remove all error handling for missing files from it since this is already done in scan_check().

That's what I was thinking as well, thanks a lot for working on this.

sharkcz commented 4 years ago

I just tested a quick patch based on this pull request which works quite well. Will give this some polish and post for internal review and get back here.

If you are interested I can provide an early version. Just give me a sign.

I recall the workaround now, so it's not urgent. But if you would like get an external testing too, I can do it based on your patch.

stefan-haberland commented 4 years ago

just a quick update... I have pulled the patches from this branch and also applied my add on patch on internal devel branch. It will be released as soon as Jan pushes changes to master again.