#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:¶
@schlomo
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"
# BACKUP_PROG_OPTIONS=( "${BACKUP_PROG_OPTIONS[@]}" "--xattrs" )
# 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
https://github.com/rear/rear/issues/1411
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:¶
@gdha
can you check if using explicitly unquoted
${BACKUP_PROG_OPTIONS[*]}
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 )"
Example:
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"
# unset 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
--this
--that
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:¶
@schlomo
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"
$ echo "${BACKUP_PROG_OPTIONS}"
--anchored --selinux
$ echo "${BACKUP_PROG_OPTIONS}" | grep " "
--anchored --selinux
$ echo "${#BACKUP_PROG_OPTIONS[@]}"
1
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:¶
@gdha
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
'first'
'second=foo.bar'
'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
'first'
'second=foo.bar'
'the'
'end'
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
'first'
'--second=foo.bar'
'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
'first'
'--second=foo.bar'
'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
https://github.com/rear/rear/commit/52b6c063ed4eb8874b2f8e9720565eefe71ab496
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
https://github.com/rear/rear/pull/1475#issuecomment-327786386
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 foo/ foo/hello # tar -xv --suffix="my suffix" -f foo.tar foo/ foo/hello Renaming `foo/hello' to `foo/hellomy suffix' # find foo foo foo/hello foo/hellomy suffix
[Export of Github issue for rear/rear.]