#2519 Issue `closed`: Misleading warnings about keyboard mappings (LogPrintError)¶

Labels: enhancement, fixed / solved / done

OliverO2 opened issue at 2020-11-16 18:00:¶

ReaR version ("/usr/sbin/rear -V"): 2.6 / Git
Description of the issue (ideally so that others can reproduce it):

Each rear mkrescue invocation (mkopalpba as well) prints:
```
Cannot include default keyboard mapping (no KEYMAPS_DEFAULT_DIRECTORY specified)
Cannot include keyboard mappings (neither KEYMAPS_DEFAULT_DIRECTORY nor KEYMAPS_DIRECTORIES specified)
```
This is misleading: Users may get the impression that something is wrong and the rescue system's keyboard will not operate correctly. However, everything is just fine, probably because dumpkeys/loadkeys is sufficient.

I'd suggest to delete these messages (or at least restrict them to the log file and not show them to the user).

jsmeix commented at 2020-11-17 07:19:¶

The reason for the code in rescue/GNU/Linux/500_clone_keyboard_mappings.sh
had been various really bad user experience when the keyboard in the recovery system
does not work as intended, cf.
https://github.com/rear/rear/pull/1781

Those messages are intentionally no warnings.
In particular there is no word "warning", cf.
https://schlomo.schapiro.org/2015/04/warning-is-waste-of-my-time.html

The messages are intentional plain info messages
because only dumpkeys/loadkeys is often not sufficient,
in particular not when you run the recovery system on another
machine where another keyboard mapping is needed, cf.
https://github.com/rear/rear/pull/1781#issuecomment-384232695

When then the recovery system does not contain the needed
keyboard mapping for your particular case you are out of the game
in practice - in particular in case of a real disaster recovery when
there is time pressure and then things do not just work and you need to
type a lot in the recovery system to do some workarounds or adaptions
(not so much fun when editing e.g. disklayout.conf or some ReaR scripts
but your keyboard mapping is wrong) - I know it because I was hit by it.

I do not get any of those LogPrintError messages on my systems
(with neither KEYMAPS_DEFAULT_DIRECTORY nor KEYMAPS_DIRECTORIES specified).

The code in rescue/GNU/Linux/500_clone_keyboard_mappings.sh
shows what keymaps should be by default copied into the recovery system
and if those default keymaps are not found the code tries to use
KEYMAPS_DEFAULT_DIRECTORY and KEYMAPS_DIRECTORIES
and if that also do not result something to copy it shows those messages
in any case to the user to tell him he may better have a closer look at his
keymap things.

I assume the code in rescue/GNU/Linux/500_clone_keyboard_mappings.sh
may need to be enhanced so that the intent behind
https://github.com/rear/rear/pull/1781
also works on your systems when neither KEYMAPS_DEFAULT_DIRECTORY
nor KEYMAPS_DIRECTORIES is specified?

OliverO2 commented at 2020-11-17 22:58:¶

@jsmeix
Thanks for the detailed explanation.

It is true that using a different keyboard during recovery can be annoying. It is also true that recovery should always be fully tested before use. So many things can go wrong otherwise.

As it seems, Debian-based systems (including Ubuntu) no longer contain any keymaps directory as part of the base system. Optionally installing one (under /usr/share/keymaps) is possible via the console-data package.

ReaR already does the best it can: It tries all known keymaps directories and makes these available on the recovery system. If these files don't exist, there is simply no support for alternative keyboards installed.

What could we do in that case?

We do not know whether different keyboard layouts might be an issue.
We do not know what is available.
We do not know how to install the required files (they might be packaged differently on different distributions).

The current messages sound alarming, yet do not provide any real help in this case. Just setting the KEYMAPS_* variables would not be sufficient. So my ideas are:

Drop the two messages above.
Then either
- do not issue any message as ReaR has already done the best it can, or
- add a generic recommendation instead: TIP: To support different keyboard layouts, see 'KEYMAPS_DEFAULT_DIRECTORY' in `default.conf'.

jsmeix commented at 2020-11-18 07:36:¶

@OliverO2
thank you for your reply - very helpful - as always!

I don't like to drop the message because I made the code
with its messages intentionally to have the user at least informed.

I see now that because of the wording cannot
the message can be misunderstood as if something went actually wrong.
But it is only meant as user information message that is important
(so I like to show the message in any case on the user's terminal)
but its meaning is not that things went actually wrong.
In the end the wording cannot can trigger same reactions as warning does
and I agree with @schlomo that warning messages should be avoided,
cf. https://github.com/rear/rear/issues/564 starting at
https://github.com/rear/rear/issues/564#issuecomment-86188528
and the subsequent comments up to
https://github.com/rear/rear/issues/564#issuecomment-86462584

So I will change the wording to be more neutral.

jsmeix commented at 2020-11-18 08:32:¶

@OliverO2
please have a look at https://github.com/rear/rear/pull/2520

jsmeix commented at 2020-11-19 11:04:¶

With https://github.com/rear/rear/pull/2520 merged
this issue should be fixed.

jsmeix commented at 2020-11-19 11:08:¶

@OliverO2
again thank you for you further helpful explanations, in particular
https://github.com/rear/rear/pull/2520#issuecomment-729681053
that helped me to finally see and understand the actual reason behind
why the changes in https://github.com/rear/rear/pull/2520 are needed
because it seems newer Debian-based systems (including Ubuntu)
no longer contain any keymaps directory as part of the base system
so those distros no longer provide console-multi-keyboard support by default.
In such cases ReaR aligns with what there is without being needlessly verbose, cf.
https://github.com/rear/rear/pull/2520/commits/5559b6edbba5e02f62122fc731f87281c4430e87

OliverO2 commented at 2020-11-19 13:07:¶

@jsmeix
Thank you for taking care of this!

I have updated my local repository and ran tests with mkrescue and mkopalpba. If ReaR is owned by root, it runs cleanly on Ubuntu except for this one:

Symlink '/lib/modules/5.4.0-54-generic/build' -> '/usr/src/linux-headers-5.4.0-54-generic' refers to a non-existing directory on the recovery system.
It will not be copied by default. You can include '/usr/src/linux-headers-5.4.0-54-generic' via the 'COPY_AS_IS' configuration variable.

I had looked at the code copying the build symlink, and I do think getting rid of this is a bit tricky and in the end probably not worth the effort.

When running it from my repo directory with files owned by me I get the usual Skipped ldd test messages:

Skipped ldd test for '/bin/rear' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/bin/dhcpcd.sh' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/bin/ifup' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/bin/login' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/bin/dhclient-script' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/etc/scripts/do-shutdown' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/etc/scripts/run-serial' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/etc/scripts/run-syslog' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/etc/scripts/system-status.sh' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/etc/scripts/boot' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/etc/scripts/run-sshd' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/etc/scripts/system-setup' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/etc/scripts/unlock-opal-disks' (owner 'oliver' not in TRUSTED_FILE_OWNERS)

I managed to drop these by excluding ReaR from the symlink test:
https://github.com/rear/rear/blob/185d32a4de5acb222fa9567dd5d6d68ee8f00bf0/usr/share/rear/build/default/990_verify_rootfs.sh#L110

Changing this to

    egrep -q "/lib/modules/|/lib.*/firmware/|$SHARE_DIR|$SCRIPT_FILE" <<<"$binary" && continue

made the above messages go away, but I did not dare to create a PR messing with your code ;-).

OliverO2 commented at 2020-11-19 14:23:¶

I did not look closely enough. The line
https://github.com/rear/rear/blob/185d32a4de5acb222fa9567dd5d6d68ee8f00bf0/usr/share/rear/build/default/990_verify_rootfs.sh#L110

Needs to change to

    egrep -q "/lib/modules/|/lib.*/firmware/|$SHARE_DIR|/bin/rear$" <<<"$binary" && continue

to make the "Skipped ldd test" messages go away. The rear script seems to always end up in /bin.

jsmeix commented at 2020-11-19 14:37:¶

I will have a closer look tomorrow.

To use ReaR from a git clone
I do git clone https://github.com/rear/rear.git always as root
because I will run it only as root anyway.

Programs in the recovery system get all copied into /bin/, for example

# type -a parted
parted is /usr/sbin/parted

# find  /tmp/rear.ey5jCWA5XmCRvY5/rootfs/ -type f | grep parted
/tmp/rear.ey5jCWA5XmCRvY5/rootfs/bin/parted

jsmeix commented at 2020-11-19 14:45:¶

Did you add 'oliver' to TRUSTED_FILE_OWNERS?
I.e. in your local.conf via

TRUSTED_FILE_OWNERS+=( 'oliver' )

I mean if you did not add 'oliver' to TRUSTED_FILE_OWNERS
then the message is right - or do I misunderstand something?

OliverO2 commented at 2020-11-19 14:49:¶

No, I did not add anything to TRUSTED_FILE_OWNERS. Technically, the message is not completely wrong. It just makes no sense to scan shell scripts via ldd and ReaR cannot contain compiled code. So if a user downloads ReaR into a user directory (with user-owned files) and uses sudo only when necessary, these messages can be avoided. Better experience -> happier user.

jsmeix commented at 2020-11-20 12:38:¶

From my point of view that would go too far beyond what I think
that should be implemented in ReaR to happen automatically.

If a user downloads ReaR into a user directory for the first time he gets that

Skipped ldd test for '...' (owner '...' not in TRUSTED_FILE_OWNERS)

messages (the comment in build/default/990_verify_rootfs.sh explains
why those messages appear on his terminal in any case to have him informed)
and then he knows what he could do to get rid of those messages.

What I could think about is to add one more user config variable like

SKIP_LDD_TEST="/lib/modules/|/lib.*/firmware/"

to default.conf so that users can specify the 'egrep' pattern
as they need it for what is currently a hardcoded value in

egrep -q '/lib/modules/|/lib.*/firmware/' <<<"$binary" && continue

in build/default/990_verify_rootfs.sh
because in general I am against hardcoded values in ReaR.

But in your particular use case I think

TRUSTED_FILE_OWNERS+=( 'oliver' )

is the cleaner way to tell ReaR what is special in your particular case.

In general:

ReaR never was and for the foreseeable future
it will neither intend to be nor become some kind of

fully-automated-make-everyone-happy-out-of-the-box

tool.

ReaR always was and for the foreseeable future will stay to be a tool
that is made to a major extent by its users which are mainly system admins
so ReaR primarily works as they need it for their (business) environments.

In particular as SUSE employee I cannot spend too much of my
worktime that is paid exclusively by SUSE Linux Enterprise customers
on issues that are not primarily needed by SUSE Linux Enterprise customers.
And outside of my worktime I have two kids :-)

Of course things would change as soon as a SUSE Linux Enterprise customer
pays SUSE sufficiently to implement something what he specifically needs ;-)

OliverO2 commented at 2020-11-20 13:32:¶

@jsmeix
I use the user-owned repository for testing and development. There I always try not to change the configuration at all, as I want to test things close to production use. I also use safe practices on my development system. The principle of least privilege requires gaining root privileges only if absolutely necessary. My IDE (IntelliJ IDEA) never runs with root privileges.

I just wonder why there should be something configurable when ReaR already knows what is right: Scanning its own shell scripts with ldd makes no sense. ReaR knows exactly where they are so "hardcoding" its own paths is just the right way to do it. Limiting ldd to the minimum set of reasonable files is safer anyway: Maybe one time ldd will trip over such a shell script and crash on SLES...

I was just trying to pick this up for future users which might try a fresh ReaR version from the repository and would enjoy it more if it would not be unnecessarily verbose.

In the end, of course, it's your decision.

OliverO2 commented at 2020-11-20 13:43:¶

Just for reference – things have happened in the past: Cf. #2177, #2179

jsmeix commented at 2020-11-20 14:13:¶

Could you do a pull request or just a diff -u posted here
so I could have a look what you think how you could make
ReaR do the right thing in an automated way in this case?

It seems that is easy for you - probably I just don't see it.
If a simple enhancement makes things work well I am all for it.

I only don't want more and more complicated 'if then else' constructs
for more and more special case handling in the ReaR code because
all that has to be maintained for a long long time in the future.

I was hit too many times by "just one more nice to have thingy" here
and "yet another nice to have thingy" there in the ReaR code
that are used by ReaR users (in particular SUSE customers)
a long time after the one who implemented it had gone and
then I get bug reports about weird exotic use cases in ReaR
where I am each time shocked what the heck that stuff is at all.
But since it exists in ReaR it is me who has to fix that obscure stuff.

OliverO2 commented at 2020-11-20 14:18:¶

Sure, this is what I had tested to prevent ldd excursions into the rear main script and into ReaR's shared script directory:

Index: usr/share/rear/build/default/990_verify_rootfs.sh
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- usr/share/rear/build/default/990_verify_rootfs.sh   (revision 2376a6c1b89a1a0ddf5f5bd2f40d100e2aeb902d)
+++ usr/share/rear/build/default/990_verify_rootfs.sh   (date 1605795600021)
@@ -106,8 +106,8 @@
     # cf. https://github.com/rear/rear/issues/2177 which also shows that sometimes kernel modules could be
     # not only in the usual directory /lib/modules/ but also e.g. in /usr/lib/modules/
     # so we 'grep' for '/lib/modules/' anywhere in the full path of the binary.
-    # Also skip the ldd test for firmware files where it also does not make sense:
-    egrep -q '/lib/modules/|/lib.*/firmware/' <<<"$binary" && continue
+    # Also skip the ldd test for firmware and ReaR files where it also does not make sense:
+    egrep -q "/lib/modules/|/lib.*/firmware/|$SHARE_DIR|/bin/rear$" <<<"$binary" && continue
     # Skip the ldd test for files that are not owned by a trusted user to mitigate possible ldd security issues
     # because some versions of ldd may directly execute the file (see "man ldd") as user 'root' here
     # cf. the RequiredSharedObjects code in usr/share/rear/lib/linux-functions.sh

OliverO2 commented at 2020-11-20 14:20:¶

If that patch format does not work sufficiently for you, I could also create a PR.

jsmeix commented at 2020-11-20 14:22:¶

Regarding https://github.com/rear/rear/issues/2519#issuecomment-731177838
Do you really like to say when a program that is called by ReaR segfaults
it is a bug in ReaR that it had called the program?
Of course I avoided that ldd segfault in ReaR once I knew it
but running ldd on arbitrary executable files had never segfaulted before
so why should I have implemented sophisticated tests in ReaR
to filter out executables that are usually not meant to be run with ldd
when ldd itself had worked so well to skip what is not meant for it?

OliverO2 commented at 2020-11-20 14:32:¶

I absolutely agree that ldd should never segfault in the first place. It should always check magic numbers and other stuff even before going into details of a file.

On the other hand, we all build our code on the shoulders of giants (the vast existing code base out there). An in the end, whenever we use stuff, we are responsible as aggregators (just as a car maker takes responsibility for the car's components). We can try to make upstream fix things, but in the end, if our product fails because it uses defective components created by others, we have to find a way. Even the Linux kernel frequently includes fixes for broken hardware.

jsmeix commented at 2020-11-20 14:35:¶

@OliverO2
thank you for the diff.
That helped me a lot to understand what you actually mean.

Done via
https://github.com/rear/rear/commit/3e31b01fc074c9e81cb479cfbe146ef24132e7ef

I had thought it is about something much more complicated.
I.e. some generic filtering method for what 'ldd' gets
to avoid e.g. any bash scripts and things like that.

jsmeix commented at 2020-11-20 14:38:¶

Of couse I totally agree that if needed ReaR must work around issues,
cf. "Dirty hacks welcome" in https://github.com/rear/rear/wiki/Coding-Style

The reason behind is that ReaR is meant to work for what system admins need
cf. my above https://github.com/rear/rear/issues/2519#issuecomment-731146610

OliverO2 commented at 2020-11-20 14:50:¶

@jsmeix

I had thought it is about something much more complicated.
I.e. some generic filtering method for what 'ldd' gets
to avoid e.g. any bash scripts and things like that.

Oh no, beware!

Actually, I had posted the source line and suggested change in https://github.com/rear/rear/issues/2519#issuecomment-730363161, but unfortunately, GitHub does not seem to offer a real diff display in issues. I have tried to find one, but did not succeed.

And I also agree, one must weigh carefully what to include in a product. That's why I found this Ubuntu message not worth putting extra effort into it:

Symlink '/lib/modules/5.4.0-54-generic/build' -> '/usr/src/linux-headers-5.4.0-54-generic' refers to a non-existing directory on the recovery system.
It will not be copied by default. You can include '/usr/src/linux-headers-5.4.0-54-generic' via the 'COPY_AS_IS' configuration variable.

So we're actually pretty much on the same page here it seems. Thanks for picking this up and have a very relaxed weekend!

jsmeix commented at 2020-11-20 14:52:¶

Thank you too for again helping me to understand things.
I also wish you a relaxed and recovering weekend!

jsmeix commented at 2020-11-20 16:14:¶

Meanwhile I even understand what I did not understand:

I had focussed too much on the messages

When running it from my repo directory with files owned by me
...
Skipped ldd test for '/bin/rear' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/oliver/Repositories/open-source/rear/usr/share/rear/skel/default/bin/dhcpcd.sh' (owner 'oliver' not in TRUSTED_FILE_OWNERS)
...

in https://github.com/rear/rear/issues/2519#issuecomment-730363161
so I assumed you are asking for additional special case handling code in ReaR
when you run it from your repo directory with files owned by your normal user
regardless that TRUSTED_FILE_OWNERS+=( 'oliver' ) would have solved it.

What I didn't see was that the generically right thing to exclude ReaR's files from the ldd test
would also - as a positive side effect - make things work right for the special case
when running it from a repo directory with files owned by a non-root user.

The diff showed me all what is needed is a "generically right thing"
so I am really happy now because doing things generically right also leads
to the right behaviour in special cases (at least for this special case here).

OliverO2 commented at 2020-11-20 16:37:¶

Ah, I see. Yes, the code actually examines files below $ROOTFS_DIR but reports paths as they are on the original system. This makes sense for the user but can irritate when checking the code.

I have updated my repository and have tested the final change. Works as expected. I found that in addition to the new code, there is still one old line present which should be deleted:
https://github.com/rear/rear/blob/3e31b01fc074c9e81cb479cfbe146ef24132e7ef/usr/share/rear/build/default/990_verify_rootfs.sh#L114

Interesting side note: Now that there are fewer messages in total (just those about Ubuntu's kernel build source symlink), suddenly this one caught my attention during testing of rear mkrescue:

Ignoring non-existing btrfs subvolume listed as mounted: /home

It also occurred with an older rear version. This message is important as the recovery system will not work as expected.

After some more testing, the message just went away without me actually changing anything. So maybe sometimes Btrfs fails to report things as we expect them (maybe due to ongoing snapshot activity, but I do not know). For now, I just repeated rear mkrescue and all was fine.

Now that ReaR is pretty silent in normal operations, I am convinced that situations like this would not go unnoticed.

jsmeix commented at 2020-11-20 16:52:¶

Thank you for your careful review (seems I was already in weekend mode ;-)
Fixed via
https://github.com/rear/rear/commit/d220fa301d0b1dd9c7dba6460cdc045a8d0a1a61

Possible btrfs race condition issues investigations at the earliest next Monday ;-)

Now is really weekend!

OliverO2 commented at 2020-11-20 16:54:¶

Do not research Btrfs stuff, not even on Monday. For now, this is just a one-off event, so let's relax! :-)

EDIT: FYI – this might be really hard to reproduce as this was the sequence of events as I remember them:

ran rear mkrescue (current GitHub version) -> message Ignoring non-existing btrfs subvolume listed as mounted: /home appeared
about one minute later: ran the installed version (2.6 commit 44f85531) of rear mkrescue -> message appeared
did some local testing (a couple of minutes)
added extra debug output to ReaR
ran rear mkrescue (current GitHub version) -> message did not appear (and never came back)

[Export of Github issue for rear/rear.]

#2519 Issue closed: Misleading warnings about keyboard mappings (LogPrintError)¶