BACKUP_PROG_OPTIONS used to be a string variable, turn it into an array

[gdha]

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

[schlomo]

Very nice improvement, thanks a lot.

[schlomo]

IMHO tar --usage | grep -- --xattrs is easier to read (or quote the search term to make it even more visible)

[gdha]

@schlomo thanks for the tip.

[schlomo]

Does tar require the setfattr programs to restore the archive with xattrs? Or this this more for the convenience of the user?

[gdha]

@schlomo well it is always better to be as complete as possible. You never know.

[schlomo]

I think it is worth to extract this into a function, it appears several times.

[gdha]

@schlomo Indeed might be a good idea to do that

[schlomo]

This is a breaking change so that we should mention it in the release notes with an explanation of what users have to adjust.

[gdha]

@schlomo Yes that is the plan!

[jsmeix]

I blindly trust you @gdha and I even more blindly ;-) trust what @schlomo already approved :-)

[jsmeix]

@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]

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

[gdha]

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

[schlomo]

@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]

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]

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

[jsmeix]

@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]

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]

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]

@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]

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]

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]

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]

@gdha maybe you just merge and continue?

[gdha]

@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]

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]

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

[schlomo]

I think it looks good.

[schlomo]

I would suggest marking BACKUP_PROG_OPTIONS to be readonly so that nobody would accidentally modify it. Such a modification after this point would be in the rescue system.

Alternative approach would be storing these options together with the backup and not with the rescue system. One one hand they have a strong coupling to the backup archive and the stuff that it contains. On the other hand of course they depend on a certain minimum tar / rsync version and thus have a coupling with the rescue system.

[schlomo]

I guess @jsmeix would suggest here to check for tar or return out of this script instead.

[jsmeix]

@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]

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

[jsmeix]

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]

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
52b6c06
that comment is from you @schlomo - could you
perhaps still explain what the reasoning behind was?

[schlomo]

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]

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]

@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]

@gdha see
#1475 (comment)

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

[gdha]

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

[schlomo]

👍 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]

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