#2675 PR merged: Use double quotes for remove_temporary_mountpoint "$BUILD_DIR/..."

Labels: fixed / solved / done, minor bug

jsmeix opened issue at 2021-09-03 10:52:

Use double quotes to get the variable evaluated in
remove_temporary_mountpoint "$BUILD_DIR/outputfs"
and
remove_temporary_mountpoint "$mountpoint"
to fix https://github.com/rear/rear/issues/2667

++ remove_temporary_mountpoint /var/tmp/rear.LEl5cQbfB1jzo68/outputfs
++ test -d /var/tmp/rear.LEl5cQbfB1jzo68/outputfs
++ rmdir -v /var/tmp/rear.LEl5cQbfB1jzo68/outputfs
rmdir: removing directory, '/var/tmp/rear.LEl5cQbfB1jzo68/outputfs'
  • Brief description of the changes in this pull request:
    Use double quotes to get the variable evaluated in
    remove_temporary_mountpoint "$BUILD_DIR/outputfs"

jsmeix commented at 2021-09-03 11:02:

@pcahyna
could you have a look here if my quoting is right now?

The remove_temporary_mountpoint calls are now in this pull request

usr/share/rear/lib/_input-output-functions.sh:
            remove_temporary_mountpoint "$BUILD_DIR/outputfs" || BugError "Directory $BUILD_DIR/outputfs not empty, cannot remove"

usr/share/rear/lib/global-functions.sh:
    AddExitTask "remove_temporary_mountpoint '$mountpoint'"
    remove_temporary_mountpoint "$mountpoint" && RemoveExitTask "remove_temporary_mountpoint '$mountpoint'"

I think the

AddExitTask "remove_temporary_mountpoint '$mountpoint'"
RemoveExitTask "remove_temporary_mountpoint '$mountpoint'"

already evaluate right.

pcahyna commented at 2021-09-03 13:35:

This looks correct, thanks, and sorry for having introduced this bug.

jsmeix commented at 2021-09-07 14:50:

Both "rear mkbackup" and "rear recover" work for me
so I would like to merge it tomorrow afternoon
if there are no objections.

jsmeix commented at 2021-09-07 15:35:

The correction here in this pull request causes
https://github.com/rear/rear/issues/2676

I still like to merge this pull request tomorrow afternoon
because it corrects code that did not work as intended.

The false ERROR ".../outputfs not empty, cannot remove" when OUTPUT_URL is unset
is (almost) certainly not caused by the corrected code in this pull request
but has a different root cause that needs to be fixed separatedly.

jsmeix commented at 2021-09-08 11:24:

@pcahyna
if you agree the change in
https://github.com/rear/rear/issues/2676#issuecomment-915133176
to make scheme_supports_filesystem() more fail-safe
which fixes https://github.com/rear/rear/issues/2676 for me
could be added here so that this pull request could fix
both https://github.com/rear/rear/issues/2667
and https://github.com/rear/rear/issues/2676

jsmeix commented at 2021-09-08 13:45:

Tomorrow I retest if things work for me with all the changes here
and if yes I would like to commit it as is to get
both https://github.com/rear/rear/issues/2667
and https://github.com/rear/rear/issues/2676
fixed to have a better working ReaR upstream master code.

Then we have time for more cleanup and improvements
to make things more fail-safe in general (without actual issues).

jsmeix commented at 2021-09-09 10:58:

I tested with all the changes here and things work well for me.

I tested "rear -v mkbackup" and "rear -v recover" with a "normal" local.conf

OUTPUT=ISO
BACKUP=NETFS
BACKUP_OPTIONS="nfsvers=3,nolock"
BACKUP_URL=nfs://192.168.122.1/nfs
...

I tested "rear -v mkrescue" with a minimal local.conf

OUTPUT=ISO

So I will merge it right now.

jsmeix commented at 2021-09-09 11:10:

@pcahyna
thank you so much for all your valuable help here!
You made me understand the root cause of https://github.com/rear/rear/issues/2676
and you made me understand how one could make functions
working more fail-safe even when set -eu is active.
It is much appreciated.

[Export of Github issue for rear/rear.]