dermotbradley / create-alpine-disk-image

Create cloud-init enabled Alpine disk images for physical machines (PCs & RPIs), VMs, and Cloud servers
GNU General Public License v2.0
93 stars 11 forks source link

Parameter expansion problem in evaluation of variables #28

Closed HugoFlorentino closed 2 years ago

HugoFlorentino commented 2 years ago

I had a doubt on the way you use parameter expansion for checking variables and settings. Unless I am missing something here, you should probably be using if [ -n "{setting:+x}" ] instead of if [ -n "${setting+x}" ] which always evaluates to true.

See the following small script to check both cases with a few examples:

#! /bin/sh

FOO1="FOO"
FOO2='FOO'
FOO3=$(expr 0 + 1)
FOO4=""
FOO5=''
FOO6=

CNT=0

testcase () {
  CNT=$(expr ${CNT} + 1)
  echo "Example ${CNT}"
  echo -n "Case 1: "
  if [ -n "${1:+x}" ]; then echo "true"; else echo "false"; fi
  echo -n "Case 2: "
  if [ -n "${1+x}" ]; then echo "true"; else echo "false"; fi
  echo
}

testcase "${FOO1}"
testcase "${FOO2}"
testcase "${FOO3}"
testcase "${FOO4}"
testcase "${FOO5}"
testcase "${FOO6}"
testcase "${FOO7}"
dermotbradley commented 2 years ago

Unless I am missing something here, you should probably be using if [ -n "{setting:+x}" ] instead of if [ -n "${setting+x}" ] which always evaluates to true.

[ -n "${setting+x}" ] does not always evaluate to true.

If you look at https://pubs.opengroup.org/onlinepubs/009604499/utilities/xcu_chap02.html#tag_02_06_02 at the table in section 2.6.2 Parameter Expansion, there are 3 situations to be considered:

The behaviour of the 2 types of test differs only in the 2nd situation above, when setting is set to "" (what the table describes as "Set But Null").

It is possible there may be some instances in my script where a variable being checked via "${variable+x}" is actually any empty string, rather than not set at all - I will check the code and fix any such occurances I find.

Also I don't think your test script is doing what you think it is doing. I changed the line:

echo "Example ${CNT}"

to also show the string it is working on:

echo "Example ${CNT}, for '$1'"

which gives the following output:

Example 1, for 'FOO' Case 1: true Case 2: true

Example 2, for 'FOO' Case 1: true Case 2: true

Example 3, for '1' Case 1: true Case 2: true

Example 4, for '' Case 1: false Case 2: true

Example 5, for '' Case 1: false Case 2: true

Example 6, for '' Case 1: false Case 2: true

Example 7, for '' Case 1: false Case 2: true

You can see that only examples 1-3 are given any actual (non empty) string to work with and both cases behave the same. The other examples are given empty strings which is exactly where the test behaviour (with and without a ":") is expected to differ.

HugoFlorentino commented 2 years ago

You can see that only examples 1-3 are given any actual (non empty) string to work with and both cases behave the same. The other examples are given empty strings which is exactly where the test behaviour (with and without a ":") is expected to differ.

That's exactly the point I was trying to make. If for example you test for an undeclared setting, wouldn't it evaluate to true if you don't use colon, thus making the script behave in unintended ways?

dermotbradley commented 2 years ago

If for example you test for an undeclared setting, wouldn't it evaluate to true if you don't use colon, thus making the script behave in unintended ways?

Simple example:

if [ -n "${option+x}" ]; then
  echo "set"
else
  echo "not set"
fi

and

if [ -n "${option:+x}" ]; then
  echo "set "
else
  echo "not set"
fi

Both behave the same if in the line before them you put unset option - they output not set. They also both behave the same if instead you put option="abcdef" - they output set

The only time they behave differently is if you put option="".

HugoFlorentino commented 2 years ago

I would prefer testing for declared and not null variable, but fair enough. Thanks for clarifying.

dermotbradley commented 2 years ago

For boolean settings (i.e. "--disable-this", "--enable-that") I'm only setting the related variable if the script option is actually specified.

For other options that have actual multiple values then I do set the related variable to at least a default value if the option is not specified.

HugoFlorentino commented 2 years ago

Makes sense.