#1475 PR merged: BACKUP_PROG_OPTIONS used to be a string variable, turn it into an array

Labels: enhancement, fixed / solved / done

gdha opened issue at 2017-09-06 16:41:

BACKUP_PROG_OPTIONS used to be a string variable, turn it into an array (GD, 06/SEP/2017 - issue #1175)

added new script prep/NETFS/GNU/Linux/205_inspect_tar_capabilities.sh which will automatically add capabilities to the BACKUP_PROG_OPTIONS array

jsmeix commented at 2017-09-07 10:58:

I wonder if that is really a "breaking change"
(i.e. a backward incompatible change) because
according to my current (limited) experience
in general bash behaves well backward compatible
for strings and arrays.
For example:
a string variable 'var' can also be evaluated
by using array syntax "${var[@]}" e.g.

# BACKUP_PROG_OPTIONS="--this --that"

# echo "${BACKUP_PROG_OPTIONS[@]}"
--this --that

and also for adding more array elements

# BACKUP_PROG_OPTIONS="--this --that"


# echo "${BACKUP_PROG_OPTIONS[@]}"
--this --that --xattrs

so that for users who specify BACKUP_PROG_OPTIONS
as a string of words in their local.conf things should still
work o.k. (unless I overlooked something special here).

jsmeix commented at 2017-09-07 11:02:

It looks as if this pull request already implements
or do I misunderstand something here?

gdha commented at 2017-09-07 11:18:

@jsmeix Good catch of #1411 (forgot about that request) - thanks.

schlomo commented at 2017-09-07 11:22:

@jsmeix please use declare -p to "view" variables and arrays. Consider this example:

$ v="hello world" ; declare -p v ; v=( "${v[@]}" yes we can) ; declare -p v
declare -- v="hello world"
declare -a v=([0]="hello world" [1]="yes" [2]="we" [3]="can")

We can see how the initial value that consists of two words is now a single value with two words. The root cause is that we now quote the array variable whereas the previous implementation on purpose did not quote it. This is actually the core of the breaking change:

jsmeix commented at 2017-09-07 11:58:

Good grief!
@schlomo you are right:

# set -x

# opts="-i -s"
+ opts='-i -s'

# echo 'Hello World' | grep $opts 'hello' | cat -n
+ grep -i -s hello
+ echo 'Hello World'
+ cat -n
     1  Hello World

# opts=( "${opts[@]}" "-o" )
+ opts=("${opts[@]}" "-o")

# echo 'Hello World' | grep "${opts[@]}" 'hello' | cat -n
+ grep '-i -s' -o hello
+ echo 'Hello World'
+ cat -n
grep: invalid option -- ' '
Usage: grep [OPTION]... PATTERN [FILE]...
Try `grep --help' for more information.

# echo 'Hello World' | grep ${opts[*]} 'hello' | cat -n
+ grep -i -s -o hello
+ echo 'Hello World'
+ cat -n
     1  Hello

schlomo commented at 2017-09-07 12:06:

Let's just try to keep unquoted arrays like ${opts[*]} to the absolute minimum :-)

jsmeix commented at 2017-09-07 12:11:

can you check if using explicitly unquoted


in the backup restore command pipe

dd if=$restore_input | $BACKUP_PROG_DECRYPT_OPTIONS $BACKUP_PROG_CRYPT_KEY | $BACKUP_PROG --block-number --totals --verbose ${BACKUP_PROG_OPTIONS[*]} "${BACKUP_PROG_COMPRESS_OPTIONS[@]}" -C $TARGET_FS_ROOT/ -x -f -

could make it behave still backward compatible
or - if that is not possible -
could you check if we could somehow import a user-defined
BACKUP_PROG_OPTIONS string of words like

BACKUP_PROG_OPTIONS="--this --that"

with each word as a separated array element
into the new BACKUP_PROG_OPTIONS() array?

schlomo commented at 2017-09-07 12:21:

I am against unquoted arrays for the sake of backwards compatibility. In general I want ReaR to look forward more than backward. That means that we should guide users in their upgrade process without compromising on the new functionality.

Specifically, I can imagine adding a test that occurs after reading the user configuration. This test would check for blanks in $BACKUP_PROG_OPTIONS (just the first value, therefore no array) and abort ReaR with a error message that the user has to update their configuration to use the array notation instead. The error message can even suggest how:

Error "The BACKUP_PROG_OPTIONS variable is now a Bash array and not a string. Please update your configuration to look like this:${IFS}BACKUP_PROG_OPTIONS+=( $BACKUP_PROG_OPTIONS )"


2017-09-07 14:19:53.284830783 ERROR: The BACKUP_PROG_OPTIONS variable is now a Bash array and not a string. Please update your configuration to look like this:     
BACKUP_PROG_OPTIONS+=( --anchored )

This will actually put the old - bad - options there and how how to write it 😄

jsmeix commented at 2017-09-07 12:22:

Only a quick unpolished proposal how we might import
user-defined BACKUP_PROG_OPTIONS string of words
as separated array elements into BACKUP_PROG_OPTIONS()

# BACKUP_PROG_OPTIONS="--this --that"

# old_backup_prog_options="$BACKUP_PROG_OPTIONS"


# for backup_prog_option in $old_backup_prog_options ; do BACKUP_PROG_OPTIONS=( "${BACKUP_PROG_OPTIONS[@]}" "$backup_prog_option" ) ; done

# for backup_prog_option in "${BACKUP_PROG_OPTIONS[@]}" ; do echo $backup_prog_option ; done

With some additional testing via "declare -p" whether or not
the initial BACKUP_PROG_OPTIONS is a string or
already an array we could keep backward compatibility.

jsmeix commented at 2017-09-07 12:24:

a test with a meaningful Error exit message is perfectly fine for me.
I only do not want to rely only on release notes
because nobody reads documentation until it is too late ;-)

schlomo commented at 2017-09-07 12:29:

I also initially thought about an automatic migration but favor the Error message to make the whole topic more explicit and to get our users to actually update their configuration.

jsmeix commented at 2017-09-07 12:37:

In general I would prefer a test using 'declare -p'
over a test of spaces in plain $VAR because it could be

VAR=( "first value" "second value" )

i.e. already an array where the first element intentionally
and rightfully contains a blank.
I mean "in general" for BACKUP_PROG_OPTIONS the
values might never contain blanks - hmmm wait:

BACKUP_PROG_OPTIONS=( '--suffix="my suffix"' )

or some other unexpected special things like that ;-)

schlomo commented at 2017-09-07 13:17:

Actually BACKUP_PROG_OPTIONS=( '--suffix="my suffix"' ) won't work as intended because the " would be part of the string. If at all then BACKUP_PROG_OPTIONS=( '--suffix=my suffix' ). However, in this specific case IMHO we can keep it simple and really just check for $IFS in the first value because the previous implementation for tar specifically did not really make it simple for users to put there complex multi word options (quoted quoting hell).

For the sake of simplicity I would therefore keep the test simple and trust that the actual backup will die complaining about strange options. So in any case, our test for the old form is just a courtesy to the user.

schlomo commented at 2017-09-07 13:17:

@gdha maybe you just merge and continue?

gdha commented at 2017-09-07 14:49:

@schlomo I like the Error bail-out : +1

$ BACKUP_PROG_OPTIONS="--anchored --selinux"
--anchored --selinux
$ echo "${BACKUP_PROG_OPTIONS}" | grep " "
--anchored --selinux
$ echo "${#BACKUP_PROG_OPTIONS[@]}"

If I understand it well we check if the variable (string) contains a blank and if the amount of values in the array is 1 we have a mismatch, right?

schlomo commented at 2017-09-07 14:54:

Yes, good point. If there are two array values and the first one has a blank then it might be intentional.

[[ ${#BACKUP_PROG_OPTIONS[@]} -eq 1 && "$BACKUP_PROG_OPTIONS" == *\ * ]] && Error "..."

should work just fine. No external program calls and positive logic 😄

gdha commented at 2017-09-08 07:08:

@schlomo @jsmeix Could you verify and give your blessing?

jsmeix commented at 2017-09-11 09:44:

now there is in prep/NETFS/GNU/Linux/205_inspect_tar_capabilities.sh

echo "BACKUP_PROG_OPTIONS=( ${BACKUP_PROG_OPTIONS[@]} )" >> $ROOTFS_DIR/etc/rear/rescue.conf

which fails if a BACKUP_PROG_OPTIONS array element
contains a string of words.
E.g. on commandline:

# arr=( 'first' 'second=foo.bar' 'the end' )

# for e in "${arr[@]}" ; do echo "'$e'" ; done
'the end'

# echo "arr=( ${arr[@]} )"
arr=( first second=foo.bar the end )

# arr=( first second=foo.bar the end )

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

What seems to work for me is:

# arr=( 'first' 'second=foo.bar' 'the end' )

# echo "arr=( $( for e in "${arr[@]}" ; do echo -n "'$e' " ; done ) )"
arr=( 'first' 'second=foo.bar' 'the end'  )

It looks rather complicated but currently I don't know
a better way.

schlomo commented at 2017-09-11 09:56:

declare -p is the official way to solve this problem, I indeed did not catch this one. Sorry.

jsmeix commented at 2017-09-11 10:18:

Somewhere in the ReaR code I noticed a comment
that in particular the "declare -p" output for arrays
cannot be used to re-create arrays
but I cannot find it right now.

At least on my SLES11 machine it seems to work
to use the "declare -p" output to re-create arrays:

# unset arr

# arr=( 'first' '--second=foo.bar' 'the end'  )

# for e in "${arr[@]}" ; do echo "'$e'" ; done
'the end'

# declare -p arr
declare -a arr='([0]="first" [1]="--second=foo.bar" [2]="the end")'

# declare -a arr='([0]="first" [1]="--second=foo.bar" [2]="the end")'

# for e in "${arr[@]}" ; do echo "'$e'" ; done
'the end'

jsmeix commented at 2017-09-11 10:26:

I found the comment about 'declare' in an older ReaR version
(but it is not about arrays - I confused that)
therein in rescue/NETFS/default/60_store_NETFS_variables.sh

# store all NETFS* variables
# I don't know why it does not work with the full declare -- var=value syntax
# found out by experiment that I need to remove the declare -- stuff.
declare -p ${!NETFS*} | sed -e 's/declare .. //' >>$ROOTFS_DIR/etc/rear/rescue.conf

and according to
that comment is from you @schlomo - could you
perhaps still explain what the reasoning behind was?

schlomo commented at 2017-09-11 10:30:

It might be related to Bash 2.x, not sure any more. If I wrote it then for sure I observed that behavior somewhere.

Maybe we should add some test code to our Makefile. If not to test ReaR scripts we could at least test for features that we use.

jsmeix commented at 2017-09-11 10:32:

For me on SLES11 'declare -p' also seems to
"just work" to recreate string variables:

# unset var

# var=" this and that "

# echo "'$var'"
' this and that '

# declare -p var
declare -- var=" this and that "

# declare -- var=" this and that "

# echo "'$var'"
' this and that '

gdha commented at 2017-09-11 11:44:

@jsmeix @schlomo there is not need to over-engineer this array declaration in rescue.conf as I could not find any option declaration with a space within tar.

jsmeix commented at 2017-09-11 12:05:

@gdha see

BACKUP_PROG_OPTIONS=( '--suffix="my suffix"' )

gdha commented at 2017-09-11 12:58:

@jsmeix I was referring to tar --usage option and not to hypothetical cases.

schlomo commented at 2017-09-11 13:00:

👍 for keeping it simple and sticking to problems that we know from reality.

That said, if we can use a Bash feature vs. self-coding then I would expect that to reduce the risk for bugs.

jsmeix commented at 2017-09-12 07:54:

I am referring to a real tar option and not to hypothetical cases. You could have done "man tar" or "tar --usage | grep STRING" to verify what I am talking about - didn't you? Here for your convenience a real example (on my SLES11 system):

# mkdir foo

# echo Hello >foo/hello

# tar -cvf foo.tar foo

# tar -xv --suffix="my suffix" -f foo.tar
Renaming `foo/hello' to `foo/hellomy suffix'

# find foo
foo/hellomy suffix

[Export of Github issue for rear/rear.]