#1457 PR `merged`: Enhance MOUNTPOINTS_TO_RESTORE into DIRECTORIES_TO_CREATE¶

Labels: enhancement, cleanup, fixed / solved / done

jsmeix opened issue at 2017-08-25 14:56:¶

Now restore/default/900_create_missing_directories.sh
recreates by default some generic directories in any case
if they do not exist, namely:
mnt proc run sys dev/pts dev/shm tmp var/tmp
and
MOUNTPOINTS_TO_RESTORE was renamed
into DIRECTORIES_TO_CREATE
but all should be fully backward compatible - in particular
if exists MOUNTPOINTS_TO_RESTORE is still also used.

jsmeix commented at 2017-08-25 16:31:¶

@schlomo
regarding your
https://github.com/rear/rear/issues/1455#issuecomment-324953959
"I am against storing multi-word strings in a Bash array
(wrong tool for the job)"

I am puzzled.

For me a bash array is perfectly right to store
several strings (a string can have several words or be empty).
For me a bash array is exactly there to solve
the problem how to store several strings.
If I have to store only several words,
a bash string would be sufficient.
E.g.

things=( 'first thing' '' 'third thing' ' ' 'last thing' )
for thing in "${things[@]}" ; do
    test "$thing" && echo $thing || echo nothing
done

What is a better tool for such a job?

schlomo commented at 2017-08-26 20:00:¶

@jsmeix What we would want to have is a multi-dimensional dictionary like

directories = {
  "/tmp" = {
    "mode" = 1777,
    "owner" = "root",
    "group" = "root",
  }
}

But Bash of course doesn't have this. So we already have this file that has this data in table form which I think is a better (= less ambiguous and confusing) place than a bash array with multi word content. Reason is that people don't expect such complex data structures in a Bash array.

If you must use the Bash array to keep that kind of data than please make it gracefully fall back to sane defaults for mode, owner and group so that one can add just directories if the default mode of 755 and root:root are OK.

jsmeix commented at 2017-08-28 10:19:¶

@schlomo
I guess with "we already have this file that has this data in table form"
you mean the mountpoint_permissions file?

If yes I think there is a misunderstanding:
I do not want to replace the mountpoint_permissions file
with a bash array.
I want to use a bash array in default.conf to specify
the defaults and give the user a way to specify additional
durectories as he needs to get (re)created.
Therefore my question would be:
What else than a bash array should/could I use
in default.conf to specify such kind of defaults?

In general regarding a file that has data in table form:

Caution with that!

From my experience it does not "just solve"
the words versus strings issue because
the usual field separator for data in table form
is tab or space i.e. essentially the same as $IFS.

Accordingly data in table form where fields can be strings
also need special care and special handling - in particular
because 'read' separates fields by $IFS by default.

E.g. simple 'read' does not work for strings:

# echo "'first value' '' 'third value' ' ' 'fifth value'" >/tmp/data_file

# read first second third fourth fifth junk </tmp/data_file

# echo "first=$first" ; echo "second=$second" ; echo "third=$third" ; echo "fourth=$fourth" ; echo "fifth=$fifth" ; echo "junk=$junk"
first='first
second=value'
third=''
fourth='third
fifth=value'
junk=' ' 'fifth value'

We already had severe issues in ReaR because disklayout.conf
is such kind of file where read is used to input words.
These issues had a related but somewhat different cause:
The cause was that empty fields are not possible with simple 'read'.
Some error in code that created disklayout.conf had missed
to write a field and then the 'read' assigned wrong vaules
to wrong variables (i.e. field mismatch because of an empty field)
and as consequence subsequent code in ReaR run mad and
failed at a totally unexpected place with a totally unexpected
error message "More than 128 partitions is not supported".

schlomo commented at 2017-08-28 10:30:¶

Yes, I see your point. I guess that deep in my heart I think that this is a fine example of software complexity exceeding the powers of Bash. If we want to stay with Bash (we discussed this before and the bottom line was that ReaR should be easily understandable and extensible by admins who are supposedly most familiar with Bash) then I would ask: How can we reduce the complexity of this topic?

So in the meantime, I'd be very happy if our configuration variables can be used also in a simple mode. If you provide only the path then mode and owner/group should be "sane" defaults.

jsmeix commented at 2017-08-28 10:52:¶

Let me code something and then I think I can show
how bash arrays (regardless of their limitations)
are good enough for us to sufficiently fulfill
both simplicity for the default case and
if needed also support additional complexity.

Personally I think when there is real complexity
nothing can make that go away or even less complex
in particular no tool or "better programming language"
can reduce real complexity.

I even think that "powerful" programming languages could
make programmers too easily introduce needless complexity
by needless complex code (like pointers to pointers to pointers
to pointers ... or classes of classes of classes of classes ...)
which is in the end again my favourite ultimate root cause
of 90% of software problems:
"It is always possible to add another level of indirection."
(RFC 1925 item 6a), see also
https://github.com/rear/rear/pull/1449#issuecomment-324874100

jsmeix commented at 2017-08-28 11:47:¶

I will merge this on soon because it is a small step forward
which can be merged right now regardless that it is not yet
the final solution.

Then I will make a separated new pull request with a
"grand cleanup and enhancement"
of the whole "directories to be (re)-created" functionality.

jsmeix commented at 2017-08-29 14:13:¶

With https://github.com/rear/rear/pull/1459 merged
the whole "directories to be (re)-created" functionality
got overhauled and enhanced.

Of course if new issues appear now, I will fix them.

OliverO2 commented at 2017-09-05 20:43:¶

@jsmeix Have just tried that code (b213ae4ef488c60dc6bd435f93b6c04854b573a2). Some observations:

Running 900_create_missing_directories.sh without having a backup configured (BACKUP=REQUESTRESTORE) brings up lots of errors "Failed to chown ..." as /bin/bash is missing on the target file system.
Running IntelliJ IDEA's inspections on 900_create_missing_directories.sh shows several warnings, which make sense to me:
- 'local' must be used in a function (appears 4 times)
- variables are used without any quotes (appears 22 times, first example is $directory_permissions_owner_group which is quoted in line 18, but not in line 22)

jsmeix commented at 2017-09-06 10:20:¶

@OliverO2
what is your use-case to run "rear recover"
without restoring at least a minimal system
(i.e. even without /bin/bash on the target file system)?

Regarding IntelliJ IDEA's inspections:

The "'local' must be used in a function" is right
but it is plain wrong that something is wrong here
because the scripts are sourced by the
Source() function in lib/framework-functions.sh
so those 'local' variables are used in a function
(there are tons of 'local' variables in other ReaR scripts).

Regarding variables that are used without any quotes:
Please be helpful and do not let me search and guess
what that "22 times" might be.
At least the directory_permissions_owner_group looks
perfectly o.k. to me in particular because I have an
explanatory comment there why I don't use quotes
so IntelliJ IDEA might need to understand my comments
before it reports false stuff ;-)

Finally I have no idea what
https://github.com/rear/rear/commit/b213ae4ef488c60dc6bd435f93b6c04854b573a2
has to do with
restore/default/900_create_missing_directories.sh

OliverO2 commented at 2017-09-06 11:32:¶

@jsmeix
My use case was initial testing before setting up a full restore. More specifically, I was just verifying that partitions are set up as intended. So I chose the option BACKUP=REQUESTRESTORE in line with this comment in default.conf:

##
# BACKUP=REQUESTRESTORE stuff
##
# This mode stops the restore after formatting and mounting the filesystems and expects
# the backup data to appear by miracle (e.g. you doing something).
# I use this mode with DMZ servers that are saved with RBME (RSYNC BACKUP MADE EASY) and
# the magical restore is just me pushing the files back via rsync/ssh. That is the reason why
# Relax-and-Recover includes an SSH server for your convenience.

I would also use REQUESTRESTORE when restoring a system manually from a historic backup which might not be available in the standard backup repository. We use an internally developed backup/restore solution which usually communicates with a central repository server, but can use repositories on directly attached media, too. So the idea is to incorporate the standard (automatic) way of restoring into one rear image, and a non-standard manual restore option into a second rear image.

Thanks for explaining the use of local, I wasn't aware of the interdependencies. And blame me (not IntelliJ) for not having read your comment about the unquoted variable use. Regarding the other places, IntelliJ unfortunately has no easy way of exporting the information, so I'll try to paste this from IntelliJ-generated HTML. Hope that helps:

Simple variable usage (at line 21)
Simple variable usage (at line 25)
Simple variable usage (at line 32)
Simple variable usage (at line 44)
Simple variable usage (at line 45)
Simple variable usage (at line 48)
Simple variable usage (at line 48)
Simple variable usage (at line 53)
Simple variable usage (at line 53)
Simple variable usage (at line 53)
Simple variable usage (at line 57)
Simple variable usage (at line 57)
Simple variable usage (at line 58)
Simple variable usage (at line 58)
Simple variable usage (at line 62)
Simple variable usage (at line 63)
Simple variable usage (at line 63)
Simple variable usage (at line 63)
Simple variable usage (at line 66)
Simple variable usage (at line 69)
Simple variable usage (at line 69)
Simple variable usage (at line 78)

b213ae4ef488c60dc6bd435f93b6c04854b573a2 was just referring to the commit I was using.

jsmeix commented at 2017-09-06 14:00:¶

With BACKUP=REQUESTRESTORE I can perfectly
reproduce what happens during "rear recover" (excerpt)

RESCUE e205:~ # rear -d -D recover
.
.
.
Disk layout created.
Please start the restore process on your backup host.

Make sure that you restore the data into /mnt/local (by default '/mnt/local')
instead of '/' because the hard disks of the recovered system are mounted there.

Please restore your backup in the provided shell and, when finished, type exit
in the shell to continue recovery.

Welcome to Relax-and-Recover.

rear> exit
Did you restore the backup to /mnt/local ? Are you ready to continue recovery ? y
exit
Recreating directories (with permissions) from /var/lib/rear/recovery/directories_permissions_owner_group
Failed to 'chown root:root sys' 
Failed to 'chown root:root proc' 
Failed to 'chown root:root dev' 
Failed to 'chown root:root dev/shm' 
Failed to 'chown root:root dev/pts' 
Failed to 'chown root:root run' 
Failed to 'chown root:root dev/mqueue' 
Failed to 'chown root:root dev/hugepages' 
Failed to 'chown root:root run/user/0' 
Failed to 'chown root:root bin' 
Failed to 'chown root:root boot' 
Failed to 'chown root:root etc' 
Failed to 'chown root:root etc/opt' 
Failed to 'chown root:root etc/X11' 
Failed to 'chown root:root home' 
Failed to 'chown root:root lib' 
Failed to 'chown root:root lib64' 
Failed to 'chown root:root mnt' 
Failed to 'chown root:root opt' 
Failed to 'chown root:root root' 
Failed to 'chown root:root sbin' 
Failed to 'chown root:root srv' 
Failed to 'chown root:root tmp' 
Failed to 'chown root:root usr' 
Failed to 'chown root:root usr/bin' 
Failed to 'chown root:root usr/include' 
Failed to 'chown root:root usr/lib' 
Failed to 'chown root:root usr/lib64' 
Failed to 'chown root:root usr/local' 
Failed to 'chown root:root usr/sbin' 
Failed to 'chown root:root usr/share' 
Failed to 'chown root:root usr/src' 
Failed to 'chown root:root usr/X11R6' 
Failed to 'chown root:root var' 
Failed to 'chown root:root var/cache' 
Failed to 'chown root:root var/lib' 
Failed to 'chown root:root var/log' 
Failed to 'chown root:root var/opt' 
Failed to 'chown root:root var/spool' 
Failed to 'chown root:root var/spool/mail' 
Failed to 'chown root:root var/tmp' 
Failed to 'chown lp:sys path/to/mytmpdir' 
Updating udev configuration (70-persistent-net.rules)
ERROR: Could not copy '/etc/udev/rules.d/70-persistent-net.rules' -> '/mnt/local//etc/udev/rules.d/70-persistent-net.rules'
Aborting due to an error, check /var/log/rear/rear-e205.log for details
Terminated
RESCUE e205:~ #

Things like that are expected on an empty target system.

The various "Simple variable usage" are all false alarm
as far as I see because I used unquoted $variable when
its value cannot contain a space because the entries in
$VAR_DIR/recovery/directories_permissions_owner_group
must consist of words for directory mode owner group,
see also https://github.com/rear/rear/issues/1372

[Export of Github issue for rear/rear.]

#1457 PR merged: Enhance MOUNTPOINTS_TO_RESTORE into DIRECTORIES_TO_CREATE¶