Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleaned up how KERNEL_FILE is set (issues 1851 and 1983) #1985

Merged

Conversation

jsmeix
Copy link
Member

@jsmeix jsmeix commented Nov 28, 2018

In particular this pull request includes (and therefore obsoletes)
#1983
i.e. usr/share/rear/pack/GNU/Linux/400_guess_kernel.sh
is moved to usr/share/rear/prep/GNU/Linux/400_guess_kernel.sh

@gozora @gdha
I would much appreciate it if you could test whether or not the
changes in this pull request cause regressions on your systems.
Many thanks in advance!

@jsmeix jsmeix added cleanup minor bug An alternative or workaround exists labels Nov 28, 2018
@jsmeix jsmeix added this to the ReaR v2.5 milestone Nov 28, 2018
@jsmeix jsmeix self-assigned this Nov 28, 2018
@jsmeix jsmeix requested review from gdha and gozora November 28, 2018 15:08
@jsmeix
Copy link
Member Author

jsmeix commented Nov 28, 2018

Ouch!

It seems there is no verification when actually copying $KERNEL_FILE
into the recovery system because I added to the end of
the new prep/GNU/Linux/400_guess_kernel.sh

KERNEL_FILE="/qqqq/QQQQ"

# Show to the user what will actually be used as kernel in the recovery system:
LogPrint "Using '$KERNEL_FILE' as kernel in the recovery system"

and "rear -D mkrescue" blindly succeeds and in its log I get

+ source /root/rear.github.master/usr/share/rear/output/ISO/Linux-i386/800_create_isofs.sh
...
++ echo '2018-11-28 16:38:10.177417576 Copying kernel and initrd'
2018-11-28 16:38:10.177417576 Copying kernel and initrd
++ cp -pL -v /qqqq/QQQQ /tmp/rear.btMb4OpRSu51xUT/tmp/isofs/isolinux/kernel
cp: cannot stat '/qqqq/QQQQ': No such file or directory
++ cp -v /tmp/rear.btMb4OpRSu51xUT/tmp/initrd.cgz /tmp/rear.btMb4OpRSu51xUT/tmp/isofs/isolinux/initrd.cgz
'/tmp/rear.btMb4OpRSu51xUT/tmp/initrd.cgz' -> '/tmp/rear.btMb4OpRSu51xUT/tmp/isofs/isolinux/initrd.cgz'

I will fix that too...

…or messages in output/ISO/Linux-ia64/300_create_bootimg.sh
@jsmeix
Copy link
Member Author

jsmeix commented Nov 28, 2018

Now it fails as it should:

# usr/sbin/rear -D mkrescue
...
Using '/qqqq/QQQQ' as kernel in the recovery system
...
ERROR: Failed to copy KERNEL_FILE '/qqqq/QQQQ'
Some latest log messages since the last called script 800_create_isofs.sh:
  2018-11-28 16:56:25.174949019 Including output/ISO/Linux-i386/800_create_isofs.sh
  2018-11-28 16:56:25.175956312 Entering debugscripts mode via 'set -x'.
  2018-11-28 16:56:25.181098853 Copying kernel and initrd
  cp: cannot stat '/qqqq/QQQQ': No such file or directory
Aborting due to an error, check /root/rear.github.master/var/log/rear/rear-g243.log for details

@gozora
Copy link
Member

gozora commented Nov 28, 2018

Hello @jsmeix,

I've found some other files where kernel is copied without further checking of the operation success:

  • usr/share/rear/output/PXE/default/800_copy_to_tftp.sh
  • usr/share/rear/output/RAMDISK/Linux-i386/900_copy_ramdisk.sh
  • usr/share/rear/output/ISO/Linux-ppc64le/800_create_isofs.sh
  • usr/share/rear/output/default/940_grub_rescue.sh

Maybe we could add to usr/share/rear/prep/GNU/Linux/400_guess_kernel.sh check if KERNEL_FILE contains a real kernel.
Something like:

file $KERNEL_FILE | grep -q "Linux kernel" || Error "$KERNEL_FILE is not a valid Linux kernel"

?

V.

@gozora
Copy link
Member

gozora commented Nov 28, 2018

There is leftover symlink /usr/share/rear/pack/Linux-ia64, still pointing to deleted directory (Linux-i386) .
I guess it can be removed.

V.

Copy link
Member

@gozora gozora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've successfully tested this PR on following systems:

  • SLES12 SP2
  • Fedora release 26
  • Centos 6.9

Apart from minor details submitted earlier in this PR communication, I think this is OK to merge.

V.

@jsmeix
Copy link
Member Author

jsmeix commented Nov 29, 2018

@gozora
thank you for testing it!

I know about the missing things but yesterday it was too late
(after-work hours are for family and ReaR is at the rear ;-)

…issing and use aligned error messages in those cases
…ied KERNEL_FILE does not exist or is a broken symlink
…ns the word kernel (ingnore case) and show a WARNING in this case when KERNEL_FILE was specified by the user i.e. the user can enforce to use a file as kernel where the file command output does not contain the word kernel
@jsmeix
Copy link
Member Author

jsmeix commented Nov 29, 2018

@gdha
now things look good to me so far so that I would like
to merge it today unless you object soon because
I like to get that changes tested by your automated tests
and by users who use our GitHub master code.

@jsmeix jsmeix merged commit 66efc5d into rear:master Nov 29, 2018
@jsmeix jsmeix deleted the cleanup_how_KERNEL_FILE_is_set_issues_1851_and_1983 branch November 29, 2018 15:53
@jsmeix
Copy link
Member Author

jsmeix commented Nov 29, 2018

Hopefully it is "fixed / solved / done" - if not I will of course fix regressions.

@gozora
Copy link
Member

gozora commented Nov 29, 2018

Thanks @jsmeix!

V.

@jsmeix
Copy link
Member Author

jsmeix commented Nov 30, 2018

Using the file command to verify that it is actually a kernel as in
#1985 (comment)
will likely cause regressions because
on my openSUSE Leap 15.0 (SLES12-like) system
I have /boot/vmlinux-* files, cf.
https://en.wikipedia.org/wiki/Vmlinux
where neither file nor file -z show it is a 'kernel':

# file /boot/vmlinu*
/boot/vmlinux-4.12.14-lp150.12.22-default.gz: gzip compressed data, was "vmlinux-4.12.14-lp150.12.22-default", last modified: Sat Oct 13 14:55:30 2018, max compression, from Unix
/boot/vmlinux-4.12.14-lp150.12.25-default.gz: gzip compressed data, was "vmlinux-4.12.14-lp150.12.25-default", last modified: Fri Nov  2 07:20:36 2018, max compression, from Unix
/boot/vmlinuz:                                symbolic link to vmlinuz-4.12.14-lp150.12.25-default
/boot/vmlinuz-4.12.14-lp150.12.22-default:    Linux/x86 Kernel, Setup Version 0x20d, bzImage, Version 4.12.14-lp150.12.22-default (geeko@buildhost) #1 SMP Sat Oct 13 05:05:16 UTC 2018 (09415e8), RO-rootFS, swap_dev 0x6, Normal VGA
/boot/vmlinuz-4.12.14-lp150.12.25-default:    Linux/x86 Kernel, Setup Version 0x20d, bzImage, Version 4.12.14-lp150.12.25-default (geeko@buildhost) #1 SMP Thu Nov 1 06:14:23 UTC 2018 (3fcf457), RO-rootFS, swap_dev 0x6, Normal VGA

# file -z /boot/vmlinu*
/boot/vmlinux-4.12.14-lp150.12.22-default.gz: ELF 64-bit LSB executable, x86-64, version 1 (SYSV) (gzip compressed data, was "vmlinux-4.12.14-lp150.12.22-default", last modified: Sat Oct 13 14:55:30 2018, max compression, from Unix)
/boot/vmlinux-4.12.14-lp150.12.25-default.gz: ELF 64-bit LSB executable, x86-64, version 1 (SYSV) (gzip compressed data, was "vmlinux-4.12.14-lp150.12.25-default", last modified: Fri Nov  2 07:20:36 2018, max compression, from Unix)
/boot/vmlinuz:                                symbolic link to vmlinuz-4.12.14-lp150.12.25-default
/boot/vmlinuz-4.12.14-lp150.12.22-default:    Linux/x86 Kernel, Setup Version 0x20d, bzImage, Version 4.12.14-lp150.12.22-default (geeko@buildhost) #1 SMP Sat Oct 13 05:05:16 UTC 2018 (09415e8), RO-rootFS, swap_dev 0x6, Normal VGA
/boot/vmlinuz-4.12.14-lp150.12.25-default:    Linux/x86 Kernel, Setup Version 0x20d, bzImage, Version 4.12.14-lp150.12.25-default (geeko@buildhost) #1 SMP Thu Nov 1 06:14:23 UTC 2018 (3fcf457), RO-rootFS, swap_dev 0x6, Normal VGA

I have the same on my old SLES11 system:

# file /boot/vmlinu*
/boot/vmlinux-3.0.101-108.21-pae.gz: gzip compressed data, from Unix, max compression
/boot/vmlinuz:                       symbolic link to `vmlinuz-3.0.101-108.21-pae'
/boot/vmlinuz-3.0.101-108.21-pae:    Linux/x86 Kernel, Setup Version 0x20b, bzImage, Version 3.0.101, RO-rootFS, swap_dev 0x3, Normal VGA

# file -z /boot/vmlinu*
/boot/vmlinux-3.0.101-108.21-pae.gz: ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV) (gzip compressed data, from Unix, max compression)
/boot/vmlinuz:                       symbolic link to `vmlinuz-3.0.101-108.21-pae'
/boot/vmlinuz-3.0.101-108.21-pae:    Linux/x86 Kernel, Setup Version 0x20b, bzImage, Version 3.0.101, RO-rootFS, swap_dev 0x3, Normal VGA

jsmeix added a commit that referenced this pull request Nov 30, 2018
…ess_kernel_issue_1985

Removed file command usage in 400_guess_kernel.sh because
it is not reliably working to test if a file is actually a kernel, see
#1985 (comment)
@jsmeix
Copy link
Member Author

jsmeix commented Nov 30, 2018

With #1988 merged
the possible regression in
#1985 (comment)
can no longer happen.

@gozora
Copy link
Member

gozora commented Nov 30, 2018

hello @jsmeix
Your vmlinuz files actually show Linux/x86 Kernel.
vmlinux contains kernel image which is normally not used for booting but for debugging...

V.

@jsmeix
Copy link
Member Author

jsmeix commented Nov 30, 2018

I don't know if a vmlinux file can never be used for booting
(I don't know about all the various possible boot methods - recently I even
learned about booting without a bootloader in between by using only the
plain firmware of the hardware to load a kernel ;-)

If vmlinux can never be used for booting
I can re-add the file test if it is actually helpful to avoid
that the user may have accidentally specified e.g.

KERNEL_FILE="/boot/vmlinux-4.12.14-lp150.12.25-default

instead of his actually bootable kernel

KERNEL_FILE="/boot/vmlinuz-4.12.14-lp150.12.25-default

and/or to avoid that somehow accidentally a vmlinux file
is autodetected and will be actually used as kernel.

@gozora
Copy link
Member

gozora commented Nov 30, 2018

I'd say, leave the code without file check ...
As far as I can remember no one was complaining about fact that he was not able to boot because ReaR wrongly detected kernel file. We should not poke into things that are actually not broken ...

V.

@jsmeix
Copy link
Member Author

jsmeix commented Nov 30, 2018

Oh, please, let us just shout a little nice WARNING at the user ;-)
https://blog.schlomo.schapiro.org/2015/04/warning-is-waste-of-my-time.html

I agree with your #1985 (comment)

Have a nice weekend!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup fixed / solved / done minor bug An alternative or workaround exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants