#2014 PR merged: Improve rear dump output to clearly distinguish array elements

Labels: enhancement, fixed / solved / done

jsmeix opened issue at 2019-01-10 16:13:

Now rear dump shows array variable elements separated by ' characters as

    ARRAY_VARIABLE_NAME = 'first element' 'second element'

for example with BACKUP_RSYNC_OPTIONS=( this that 'something else' )
(excerpts):

# usr/sbin/rear dump
...
    BACKUP_RSYNC_OPTIONS = 'this' 'that' 'something else' 
...
    BACKUP_PROG_EXCLUDE = '/tmp/*' '/dev/shm/*' '/root/rear.github.master/var/lib/rear/output/*' '/nfs/*' '/tmp/rear.Prkdrs6sR9pybOv'

Before it would have been e.g.

    BACKUP_RSYNC_OPTIONS = this that something else

where it is not clear that actually something else is one array element.

I also cleaned up the indentation in lib/dump-workflow.sh to N*4 spaces.

schlomo commented at 2019-01-10 16:43:

Good idea, maybe we should also show which variable is an array and which isn't? Maybe produce output similar to that of declare -x? IMHO would be good if the output could be posted back into the configuration.

Idempotency between dump output and configuration input, in a way.

Great work!

jsmeix commented at 2019-01-11 12:30:

@schlomo
thank you for having a look.

I was also thinking about using declare
but I was thinking about using declare -p
to generate the output e.g. somehow as in

# STRING="string of words"

# export ARRAY=( '' this ' ' that 'something else' )

# declare -p STRING ARRAY
declare -- STRING="string of words"
declare -ax ARRAY=([0]="" [1]="this" [2]=" " [3]="that" [4]="something else")

This way rear dump could output

    STRING="string of words"
    ARRAY=([0]="" [1]="this" [2]=" " [3]="that" [4]="something else")

that can be sourced to set STRING and ARRAY
but it is less readable compared to the current output via this pull request

    STRING = 'string of words'
     ARRAY = '' 'this' ' ' 'that' 'something else'

If rear dump would output the declate -p output as is like

    declare -- STRING="string of words"
    declare -ax ARRAY=([0]="" [1]="this" [2]=" " [3]="that" [4]="something else")

the output becomes even harder to read for humans
but this could be sourced to set STRING and ARRAY
exactly as it was before (i.e. ARRAY gets even exported).

Hopefully I can continue next week (as time permits)...

schlomo commented at 2019-01-11 12:33:

Yes, I meant -p indeed. How about this:

# declare -p BASH_VERSINFO BASH_VERSION NASTY| sed -e 's/^declare -[[:alpha:]-]* //' -e 's/\([( ]\)\[[[:digit:]]\+\]=/\1/g'
BASH_VERSINFO=("4" "4" "19" "1" "release" "x86_64-pc-linux-gnu")
BASH_VERSION="4.4.19(1)-release"
NASTY=("/foo/bar*" "\\twith backslash and 'quotes'\"")

Personally I wouldn't mind the more bashy output format

jsmeix commented at 2019-01-11 12:34:

@schlomo
what do you mean with "copy-paste compatible with the configuration files"?

I guess first and foremost no artificual spaces around the =
and in case of arrays enclosing it within ( ... ) as in

    STRING='string of words'
     ARRAY=( '' 'this' ' ' 'that' 'something else' )

Something else?

I was always wondering why there are the artificual spaces around =
which make it impossible to copy&paste lines as variable assignent.

schlomo commented at 2019-01-11 12:55:

I mean that if we now change the output of rear dump to be more precise then we should also gain the ability to paste the output of rear dump into the configuration. That means that the output must look like the output of declare -p, more or less.

Previously, rear dump was meant to be easy to read at the cost of being less precise. For human consumption IMHO the precise details of the arrays is not so crucial, on most cases it is obvious.

If we start to be precise then we can also enjoy the ability to copy-paste the output.

Just my opinion, what do you think @rear/contributors ?

jsmeix commented at 2019-01-11 12:59:

The

declare -p VARIABLE | sed -e 's/^declare -[[:alpha:]-]* //' -e 's/\([( ]\)\[[[:digit:]]\+\]=/\1/g'

is not fully fail safe.

Deliberately evil self-implicating array element content breaks it:

# ARRAY=( foo 'declare -ax ARRAY=([0]="foo" [1]="bar")' bar )

# for e in "${ARRAY[@]}" ; do echo "'$e'" ; done
'foo'
'declare -ax ARRAY=([0]="foo" [1]="bar")'
'bar'

# declare -p ARRAY
declare -ax ARRAY=([0]="foo" [1]="declare -ax ARRAY=([0]=\"foo\" [1]=\"bar\")" [2]="bar")

# declare -p ARRAY | sed -e 's/^declare -[[:alpha:]-]* //' -e 's/\([( ]\)\[[[:digit:]]\+\]=/\1/g'
ARRAY=("foo" "declare -ax ARRAY=(\"foo\" \"bar\")" "bar")

# ARRAY=("foo" "declare -ax ARRAY=(\"foo\" \"bar\")" "bar")

# for e in "${ARRAY[@]}" ; do echo "'$e'" ; done
'foo'
'declare -ax ARRAY=("foo" "bar")'
'bar'

# declare -ax ARRAY=([0]="foo" [1]="declare -ax ARRAY=([0]=\"foo\" [1]=\"bar\")" [2]="bar")

# for e in "${ARRAY[@]}" ; do echo "'$e'" ; done
'foo'
'declare -ax ARRAY=([0]="foo" [1]="bar")'
'bar'

So plain declare -p ARRAY output seems to work fail safe in any case
while readability improvements via sed fall apart in special cases.

jsmeix commented at 2019-01-11 13:03:

In my opinion be precise is more important for ReaR's use-case
than to make things look nicer (at the expense of subtle errors)
because ReaR is meant for experienced admins who should
be able to also understand the plain declare -p output.

jsmeix commented at 2019-01-11 13:12:

The current output via this pull request falls apart for

BACKUP_RSYNC_NASTY=( "/foo/bar*" "\\twith backslash and 'quotes'\"" )

that is shown in the rear dup output as

    BACKUP_RSYNC_NASTY = '/foo/bar*' '        with backslash and 'quotes'"'

in particular the \t got somehow evaluated...

schlomo commented at 2019-01-11 13:20:

Probably because of the echo -e in https://github.com/rear/rear/blob/bf3c3cc4adee1851846a9876f83e8c8ffb4f3df8/usr/share/rear/lib/_input-output-functions.sh#L249

Probably LogPrint is the wrong function for this sort of output. Maybe we need to add a new RawLogPrint function or support passing in command line options to the existing functions.

jsmeix commented at 2019-01-11 13:38:

The only places where \ are used in *Print functions
seem to be (just a quick find&grep, no deeper analysis):

# find usr/sbin/rear usr/share/rear | xargs grep -oh 'Print "[^"]*"' 2>/dev/null | grep '\\' | sort -u
Print "Creating $disk_image_size_MiB MiB raw disk image \"
Print "Decrypting backup archive with key defined in variable \$BACKUP_PROG_CRYPT_KEY"
Print "Encrypting backup archive with key defined in variable \$BACKUP_PROG_CRYPT_KEY"
Print "If the NSR_RETENTION_TIME=\"
Print "Using PBA image file \"
Print "Using local PBA image file \"

so that echo -e is perhaps not really needed in practice?

schlomo commented at 2019-01-11 13:51:

The cases you found seem to protect $ or " from being interpreted. I checked with git grep 'Print.*\\t' (and the same for \n) and also couldn't find anything.

It is worth trying to remove the -e from the Print function. Since the Log function does not have the -e option that might actually be a functional improvement as it makes sure that console and log output are the same.

schlomo commented at 2019-01-14 12:40:

Can you please post a rear dump example output here? Thanks

jsmeix commented at 2019-01-14 12:40:

To be able to return back to a less incompatible change
I implement copy-and-paste compatible output in steps
so that I could easily go back to a particular git commit.

My latest commit
https://github.com/rear/rear/pull/2014/commits/fa6b9e6c21d9e0a81a10aa3335feb55cc1a62b66
implements copy-and-paste compatible output
not yet by using the declare -p output but more as it was before
but now with arrays shown as arrays e.g. as in

Backup with NETFS:
              NETFS_KEEP_OLD_BACKUP_COPY=''
                            NETFS_PREFIX='g243'
              NETFS_RESTORE_CAPABILITIES=( 'No'  )
                   BACKUP_DUPLICITY_NAME='rear-backup'
                  BACKUP_INTEGRITY_CHECK=''
                         BACKUP_MOUNTCMD=''
                     BACKUP_ONLY_EXCLUDE='no'
                     BACKUP_ONLY_INCLUDE='no'
                          BACKUP_OPTIONS='nfsvers=3,nolock'
      BACKUP_RESTORE_MOVE_AWAY_DIRECTORY='/root/rear.github.master/var/lib/rear/moved_away_after_backup_restore/'
          BACKUP_RESTORE_MOVE_AWAY_FILES=( '/boot/grub/grubenv' '/boot/grub2/grubenv'  )
                      BACKUP_RSYNC_ARRAY=( 'foo' 'declare -ax ARRAY=([0]="foo" [1]="bar")' 'bar'  )
                      BACKUP_RSYNC_NASTY=( '/foo/bar*' '        with backslash and 'quotes'"'  )
                    BACKUP_RSYNC_OPTIONS=( '' 'this' ' ' 'that' 'something else'  )
                     BACKUP_RSYNC_STRING='string of words'
                  BACKUP_SELINUX_DISABLE='1'
                             BACKUP_TYPE=''
                        BACKUP_UMOUNTCMD=''
                              BACKUP_URL='nfs://10.160.67.243/nfs'

which is still not yet right for

BACKUP_RSYNC_NASTY=( "/foo/bar*" "\\twith backslash and 'quotes'\"" )


#### <img src="https://avatars.githubusercontent.com/u/101384?v=4" width="50">[schlomo](https://github.com/schlomo) commented at [2019-01-14 12:48](https://github.com/rear/rear/pull/2014#issuecomment-453993548):

@jsmeix this looks really nice! Please note that the output of `BACKUP_RSYNC_NASTY` is actually wrong, the nested quotes are not handles properly. I think that making the regex not greedy in my `sed` suggestion will solve the example you gave above. I'd caution against re-implementing the functionality of `declare -p` and hope that we can find a way to utilize it.

#### <img src="https://avatars.githubusercontent.com/u/1788608?u=925fc54e2ce01551392622446ece427f51e2f0ce&v=4" width="50">[jsmeix](https://github.com/jsmeix) commented at [2019-01-14 12:54](https://github.com/rear/rear/pull/2014#issuecomment-453995151):

My next step towards using the `declare -p` output as is
is commit
https://github.com/rear/rear/pull/2014/commits/a9e954a45c4aaae640569a3996d29d30ac63f50e
which removes the `-e` option from the `echo` commands
in lib/_input-output-functions.sh to output unchanged values
which is an any case needed to show variable values as is
(independent of what the `dump` workflow does), think about
user messages that show whatever variable values to the user
where the variable values should not be somehow interpreted.

#### <img src="https://avatars.githubusercontent.com/u/1788608?u=925fc54e2ce01551392622446ece427f51e2f0ce&v=4" width="50">[jsmeix](https://github.com/jsmeix) commented at [2019-01-14 12:56](https://github.com/rear/rear/pull/2014#issuecomment-453995628):

Now I will implement showing the `declare -p` output as is
in the `dump` workflow, the commit will happen probably tomorrow...

#### <img src="https://avatars.githubusercontent.com/u/101384?v=4" width="50">[schlomo](https://github.com/schlomo) commented at [2019-01-14 12:59](https://github.com/rear/rear/pull/2014#issuecomment-453996364):

I looked again at your example and at the `sed` line. It actually works "as designed" and I would consider the individual assignment of array elements an unnecessary feature.

We do not have any configuration feature that relies on specific array indices and I would veto any such feature as this would be way above our average user's experience.

You can decide. I'll go along with using the `declare -p` output as-is and with beautifying it via this `sed` snippet while knowing that it handles only "plain" arrays.

#### <img src="https://avatars.githubusercontent.com/u/1788608?u=925fc54e2ce01551392622446ece427f51e2f0ce&v=4" width="50">[jsmeix](https://github.com/jsmeix) commented at [2019-01-15 13:41](https://github.com/rear/rear/pull/2014#issuecomment-454395650):

With my commit
https://github.com/rear/rear/pull/2014/commits/d97c6f8ea2c05085e20555adfcea5a45e4dc1fc1
the dump workflow shows the `declare -p`
output which gets beautified/simplified unless in debug mode
so that it is possible (via debug mode) to get correct output
even in special cases where the beautified output gets wrong:

grep BACKUP_RSYNC_OPTIONS etc/rear/local.conf

BACKUP_RSYNC_OPTIONS=( '' this ' ' that 'something else' "don't use brackets '[...]' as in [123]='foo' (it fails)" 'end' )

usr/sbin/rear dump | grep BACKUP_RSYNC_OPTIONS

BACKUP_RSYNC_OPTIONS=("" "this" " " "that" "something else" "don't use brackets '[...]' as in 'foo' (it fails)" "end")

usr/sbin/rear -d dump | grep BACKUP_RSYNC_OPTIONS

declare -a BACKUP_RSYNC_OPTIONS=([0]="" [1]="this" [2]=" " [3]="that" [4]="something else" [5]="don't use brackets '[...]' as in [123]='foo' (it fails)" [6]="end")

Furthermore I made the output so that it can be directly sourced
(unless in verbose mode)

source <( usr/sbin/rear dump ) && echo OK || echo FAIL

OK

source <( usr/sbin/rear -v dump ) && echo OK || echo FAIL

If 'Relax-and-Recover' is not a typo you can use command-not-found to lookup the package that contains it, like this:
cnf Relax-and-Recover
-bash: /dev/fd/63: line 2: syntax error near unexpected token (' -bash: /dev/fd/63: line 2:Running rear dump (PID 18114)'
FAIL

Because debug mode implies verbose mode it is not possible to get
correct output even in special cases where the beautified output gets wrong
that can be directly sourced.

Here how full output looks like
in normal mode

usr/sbin/rear dump

Begin dumping out configuration and system information:

This is a 'Linux-x86_64' system, compatible with 'Linux-i386'.

Configuration tree:

Linux-i386.conf : OK

GNU/Linux.conf : OK

SUSE.conf : missing/empty

SUSE/i386.conf : missing/empty

SUSE/15.conf : missing/empty

SUSE/15/i386.conf : missing/empty

SUSE_LINUX.conf : OK

SUSE_LINUX/i386.conf : missing/empty

SUSE_LINUX/15.0.conf : missing/empty

SUSE_LINUX/15.0/i386.conf : missing/empty

site.conf : missing/empty

local.conf : OK

System definition:

ARCH="Linux-i386"
OS="GNU/Linux"
OS_MASTER_VENDOR="SUSE"
OS_MASTER_VERSION="15"
OS_MASTER_VENDOR_ARCH="SUSE/i386"
OS_MASTER_VENDOR_VERSION="SUSE/15"
OS_MASTER_VENDOR_VERSION_ARCH="SUSE/15/i386"
OS_VENDOR="SUSE_LINUX"
OS_VERSION="15.0"
OS_VENDOR_ARCH="SUSE_LINUX/i386"
OS_VENDOR_VERSION="SUSE_LINUX/15.0"
OS_VENDOR_VERSION_ARCH="SUSE_LINUX/15.0/i386"

Backup with NETFS:

NETFS_KEEP_OLD_BACKUP_COPY=""
NETFS_PREFIX="g243"
NETFS_RESTORE_CAPABILITIES=("No")
BACKUP_DUPLICITY_NAME="rear-backup"
BACKUP_INTEGRITY_CHECK=""
BACKUP_MOUNTCMD=""
BACKUP_ONLY_EXCLUDE="no"
BACKUP_ONLY_INCLUDE="no"
BACKUP_OPTIONS="nfsvers=3,nolock"
BACKUP_RESTORE_MOVE_AWAY_DIRECTORY="/root/rear.github.master/var/lib/rear/moved_away_after_backup_restore/"
BACKUP_RESTORE_MOVE_AWAY_FILES=("/boot/grub/grubenv" "/boot/grub2/grubenv")
BACKUP_RSYNC_ARRAY=("foo" "declare -ax ARRAY=("foo" "bar")" "bar")
BACKUP_RSYNC_NASTY=("/foo/bar*" "\twith backslash and 'quotes'"")
BACKUP_RSYNC_OPTIONS=("" "this" " " "that" "something else" "don't use brackets '[...]' as in 'foo' (it fails)" "end")
BACKUP_RSYNC_STRING="string of words"
BACKUP_SELINUX_DISABLE="1"
BACKUP_TYPE=""
BACKUP_UMOUNTCMD=""
BACKUP_URL="nfs://10.160.67.243/nfs"

Backup program is 'tar':

BACKUP_PROG_ARCHIVE="backup"
BACKUP_PROG_COMPRESS_OPTIONS=("--gzip")
BACKUP_PROG_COMPRESS_SUFFIX=".gz"
BACKUP_PROG_CRYPT_ENABLED="0"
BACKUP_PROG_CRYPT_KEY=""
BACKUP_PROG_CRYPT_OPTIONS="/usr/bin/openssl des3 -salt -k "
BACKUP_PROG_DECRYPT_OPTIONS="/usr/bin/openssl des3 -d -k "
BACKUP_PROG_EXCLUDE=("/tmp/" "/dev/shm/" "/root/rear.github.master/var/lib/rear/output/" "/nfs/" "/tmp/rear.f53sTaI6BEwwgWQ")
BACKUP_PROG_OPTIONS=("--anchored")
BACKUP_PROG_OPTIONS_CREATE_ARCHIVE=""
BACKUP_PROG_OPTIONS_RESTORE_ARCHIVE=""
BACKUP_PROG_SUFFIX=".tar"
BACKUP_PROG_WARN_PARTIAL_TRANSFER="1"

Output to ISO:

ISO_DEFAULT="boothd"
ISO_DIR="/root/rear.github.master/var/lib/rear/output"
ISO_ISOLINUX_BIN=""
ISO_MAX_SIZE=""
ISO_MKISOFS_BIN="/usr/bin/xorrisofs"
ISO_MKISOFS_OPTS=""
ISO_PREFIX="rear-g243"
ISO_RECOVER_MODE=""
ISO_VOLID="RELAXRECOVER"
OUTPUT_MOUNTCMD=""
OUTPUT_OPTIONS=""
OUTPUT_PREFIX="g243"
OUTPUT_PREFIX_PXE=""
OUTPUT_UMOUNTCMD=""
OUTPUT_URL=""

Validation status:

Your system is not yet validated. Please carefully check all functions

and create a validation record with 'rear validate'. This will help others

to know about the validation status of Relax-and-Recover on this system.

End of dump configuration and system information.

versus in verbose mode (excerpts - the actual dump part is same)

usr/sbin/rear -v dump

Relax-and-Recover 2.4 / Git
Running rear dump (PID 20481)
Using log file: /root/rear.github.master/var/log/rear/rear-g243.log.lockless

Begin dumping out configuration and system information:

...

End of dump configuration and system information.

Saving /root/rear.github.master/var/log/rear/rear-g243.log.lockless as /root/rear.github.master/var/log/rear/rear-g243.log
Exiting rear dump (PID 20481) and its descendant processes
Running exit tasks
You should also rm -Rf /tmp/rear.1bjD6VOVgttrN7O

versus in normal mode when it is validated (excerpts)

usr/sbin/rear dump

Begin dumping out configuration and system information:

...

Validation status:

Your system is validated with the following details:

Version: Relax-and-Recover 2.4 / Git

Validation: SUSE_LINUX/15.0/i386

Submitted: Johannes Meixner jsmeix@suse.de, SUSE, Germany

Date: 2019-01-15

Features: NETFS, ISO

Comment: Works out-of-the-box flawless with all features"

End of dump configuration and system information.

#### <img src="https://avatars.githubusercontent.com/u/1788608?u=925fc54e2ce01551392622446ece427f51e2f0ce&v=4" width="50">[jsmeix](https://github.com/jsmeix) commented at [2019-01-15 13:59](https://github.com/rear/rear/pull/2014#issuecomment-454401141):

With the fixed one remaining Print call that must be LogUserOutput
in normal mode when it is validated (excerpt)

Validation status:

/root/rear.github.master/usr/share/rear/lib/validated/SUSE_LINUX/15.0/i386.txt : OK

Your system is validated with the following details:

Version: Relax-and-Recover 2.4 / Git

Validation: SUSE_LINUX/15.0/i386

Submitted: Johannes Meixner jsmeix@suse.de, SUSE, Germany

Date: 2019-01-15

Features: NETFS, ISO

Comment: Works out-of-the-box flawless with all features"

versus in normal mode when it is not validated (excerpt)

Validation status:

/root/rear.github.master/usr/share/rear/lib/validated/SUSE_LINUX/15.0/i386.txt : missing/empty

Your system is not yet validated. Please carefully check all functions

and create a validation record with 'rear validate'. This will help others

to know about the validation status of Relax-and-Recover on this system.

#### <img src="https://avatars.githubusercontent.com/u/1788608?u=925fc54e2ce01551392622446ece427f51e2f0ce&v=4" width="50">[jsmeix](https://github.com/jsmeix) commented at [2019-01-15 14:33](https://github.com/rear/rear/pull/2014#issuecomment-454412179):

@schlomo 
does it now look o.k. for you or should I change something?
Your requested changes in
https://github.com/rear/rear/pull/2014#pullrequestreview-191635881
`improve the output to be copy-paste compatible`
should be now implemented - and even more because
now in normal mode the output can be directly sourced.

@gdha 
is the new and changed output format also o.k. for you?

#### <img src="https://avatars.githubusercontent.com/u/1788608?u=925fc54e2ce01551392622446ece427f51e2f0ce&v=4" width="50">[jsmeix](https://github.com/jsmeix) commented at [2019-01-16 10:04](https://github.com/rear/rear/pull/2014#issuecomment-454722111):

I found a simple way how to source `rear -d dump` output directly:

source <( usr/sbin/rear -d dump | grep '=' ) && echo OK || echo FAIL

OK

This way even problematic cases work, cf.
https://github.com/rear/rear/pull/2014#issuecomment-454395650

for e in "${BACKUP_RSYNC_OPTIONS[@]}" ; do echo "'$e'" ; done

''
'this'
' '
'that'
'something else'
'don't use brackets '[...]' as in [123]='foo' (it fails)'
'end'

Also readonly variables have the expected effect:

usr/sbin/rear -d dump | grep BACKUP_RESTORE_MOVE_AWAY_DIRECTORY

declare -r BACKUP_RESTORE_MOVE_AWAY_DIRECTORY="/root/rear.github.master/var/lib/rear/moved_away_after_backup_restore/"

source <( usr/sbin/rear -d dump | grep '=' ) && echo OK || echo FAIL

-bash: declare: BACKUP_RESTORE_MOVE_AWAY_DIRECTORY: readonly variable
OK

i.e. the second run to source `rear -d dump` output will not overwrite
the already declared readonly variable from the first run.

#### <img src="https://avatars.githubusercontent.com/u/1788608?u=925fc54e2ce01551392622446ece427f51e2f0ce&v=4" width="50">[jsmeix](https://github.com/jsmeix) commented at [2019-01-16 10:23](https://github.com/rear/rear/pull/2014#issuecomment-454727891):

@schlomo 
thank you for your helpful comments and in particular
for your `sed` command so that we can show by default
the `declare -p` output in a more user friendly form.

I think the current implementation is a good balance
between being precise and being user friendly.


-------------------------------------------------------------------------------



[Export of Github issue for [rear/rear](https://github.com/rear/rear).]