#1171 PR `merged`: Removed needless login shell from chroot calls where possible¶

Labels: cleanup, fixed / solved / done, minor bug

jsmeix opened issue at 2017-01-18 15:46:¶

see https://github.com/rear/rear/issues/862

jsmeix commented at 2017-01-18 15:49:¶

The only "chroot ... bash --login" call that is left is in
finalize/Debian/i386/170_rebuild_initramfs.sh

  chroot $TARGET_FS_ROOT /bin/bash --login -c "/usr/share/mdadm/mkconf >/etc/mdadm/mdadm.conf"

because of the stdout redirection.

jsmeix commented at 2017-01-19 12:57:¶

Do not merge it!
It does not yet work.
This one is RFC 1925 (8): "It is more complicated than you think."

jsmeix commented at 2017-01-19 13:52:¶

Now it works for me on SLES12:

RESCUE e205:~ # rear -d -D recover
...
Restoring finished.
...
Running mkinitrd...
Recreated initrd (/sbin/mkinitrd).
Installing GRUB2 boot loader
Finished recovering your system. You can explore it under '/mnt/local'.

FYI:
I need in local.conf

OS_MASTER_VENDOR="SUSE_LINUX/i386"

(or something else like that) because otherwise
finalize/SUSE_LINUX/i386/170_rebuild_initramfs.sh
is not sourced.
Whether or not a rebuild_initramfs script should be sourced
in any case is a different issue - I never had issues with the
initrd on my test systems so that a rebuild_initramfs script
seems to be not really mandatory.

jsmeix commented at 2017-01-19 14:03:¶

@gdha
could you please review whether or not
it still works for you with my changes in
finalize/Fedora/i386/170_rebuild_initramfs.sh

FYI:
By the way I found a chroot call in
finalize/Linux-i386/220_install_grub2.sh
where I think that cannot work:

chroot $TARGET_FS_ROOT $grub_name-mkconfig -o /boot/$grub_name/grub.cfg

because $grub_name evaluates to "grub2" or "grub"
but chroot requires usually a full path to be executed like

RESCUE e205:~ # chroot /mnt/local ifconfig
chroot: failed to run command 'ifconfig': No such file or directory

RESCUE e205:~ # chroot /mnt/local /sbin/ifconfig
eth0      Link encap:Ethernet  HWaddr 52:54:00:25:A4:49
...

see also my new code regarding "mkinitrd_binary" in
finalize/Fedora/i386/170_rebuild_initramfs.sh

gdha commented at 2017-01-20 10:19:¶

@jsmeix Merged it - results will be posted in https://github.com/gdha/rear-automated-testing/issues/13

jsmeix commented at 2017-01-20 12:58:¶

@gdha
many thanks for your careful review!

FYI regarding who the 'I' actually is:
To find out who made what in a file I use usually
the git command "git log -p --follow file.sh".
For the 170_rebuild_initramfs.sh files
that command shows as the oldest entry always

commit 844d50b75ac4b7722f4fee7a5ee3350b93f3adb7
Author: Schlomo Schapiro 
Date:   Sun Jun 6 08:30:21 2010 +0000

    - Integrated P2V patch from Heinlein Support. We start with 1.9 now to

It is the same commit
https://github.com/rear/rear/commit/844d50b75ac4b7722f4fee7a5ee3350b93f3adb7
for
finalize/Fedora/i386/170_rebuild_initramfs.sh
finalize/Debian/i386/170_rebuild_initramfs.sh
finalize/SUSE_LINUX/i386/170_rebuild_initramfs.sh
so that I think the 'I' could be Peer Heinlein
and Schlomo only comitted his changes?

gdha commented at 2017-01-20 16:53:¶

@jsmeix I can confirm the modifications work well on CentOS7

jsmeix commented at 2017-01-23 09:50:¶

@schlomo
many thanks for the information!

I will do a separated pull request
where I let ReaR show same kind of warning
as near the end of the code about
"Cannot create initramfs"
if the udev requirement is not fulfilled.

Regarding LogPrint messages like

WARNING:
Cannot create initramfs
...
Check the recreated system (mounted at /mnt/local)
and decide yourself, whether the system will boot or not.

@schlomo
because I know what you think about warning messages:
Schould I remove the word "WARNING:" from those
messages and make it a plain information?

schlomo commented at 2017-01-23 10:21:¶

Thanks @jsmeix for your consideration.

One one hand the warning is justified as it really means "I don't know".

On the other hand we should try to give the user even more information here. For example, which systems don't have udev nowadays? Maybe we can say that ReaR 2 actually requires udev and fails without? Then you could get rid of this warning.

Another thought: Maybe we should expand this to be a generic feature and collect such warnings in a list of RECOVERY_NOTICE[@] which we display at the end. In an automated recovery having a non-empty list of recovery notices would be an error, in a manual recovery it would be printed as user info.

jsmeix commented at 2017-01-23 11:39:¶

I have to think a bit more about it.
I think the idea behind something like a RECOVERY_NOTICE array
is related to https://github.com/rear/rear/issues/1134
because also in an automated recovery proper exit codes
could be used to signal something to the user.
In other words:
If in an automated recovery a RECOVERY_NOTICE
would be an error, rear should exit with an appropriate
non-zero exit code in this case to singal the user:
Not a fatal error (e.g. after calling the Error function)
but also not a clean recovery.

jsmeix commented at 2017-05-08 07:58:¶

It seems using no login shell for chroot calls
causes more issues that it solves, see
https://github.com/rear/rear/pull/1345

[Export of Github issue for rear/rear.]

#1171 PR merged: Removed needless login shell from chroot calls where possible¶