#741 Issue closed: Shouldn't "Source unsourceable_file" bail out with BugError?¶

Labels: enhancement, cleanup, won't fix / can't fix / obsolete

jsmeix opened issue at 2015-12-15 14:02:¶

I wonder if the Source function shouldn't bail out with BugError
when it is called with an argument that cannot be sourced.

Reason: I think any "Source unsourceable_file" in the code
is a programming error.

Or do I misunderstand it and there is a reason why
"Source unsourceable_file" is just ignored/skipped?

Here the story behind https://github.com/rear/rear/pull/738

I made a programming error by calling

Source "wrapup/default/99_copy_logfile.sh"

Instead of a direct error the Source function ignored/skipped
it and I had to find out later the hard way why I got no log file
copied into the target system.

Personally I prefer an immediate error exit directly at the place
where something is wrong that cannot be automatically corrected
over proceeding until a failure happens later - possibly in a very
unexpected way at a totally different place (see below).

See also https://github.com/rear/rear/issues/700 that has the same goal (immediate error exit when something is wrong).

Note the above "that cannot be automatically corrected":

I still like to implement that the Source function uses
"$SHARE_DIR/$source_file" if "$source_file" is not an
absolute path (i.e. when it has no leading '/') as described
in https://github.com/rear/rear/pull/738

But I would also like to implement that the Source function
does a BugError exit when it cannot source "$source_file"
or "$SHARE_DIR/$source_file".

FYI:
Such a late failure in an unexpected way at a totally different place
had happened in https://github.com/rear/rear/issues/544 see https://github.com/rear/rear/issues/544#issuecomment-151133875 how it proceeded with wrong data until it failed in the get_partition_number function with the strange "More than 128 partitions is not supported" error message.

schlomo commented at 2015-12-15 14:29:¶

This code in usr/sbin/rear relies on this feature:

SetOSVendorAndVersion
# Distribution configuration files:
for config in "$ARCH" "$OS" \
        "$OS_MASTER_VENDOR" "$OS_MASTER_VENDOR_ARCH" "$OS_MASTER_VENDOR_VERSION" "$OS_MASTER_VENDOR_VERSION_ARCH" \
        "$OS_VENDOR" "$OS_VENDOR_ARCH" "$OS_VENDOR_VERSION" "$OS_VENDOR_VERSION_ARCH" ; do
    test -r "$SHARE_DIR/conf/$config.conf" && Source "$SHARE_DIR/conf/$config.conf" || true
done
# User configuration files, last thing is to overwrite variables if we are in the rescue system:
for config in site local rescue ; do
    test -r "$CONFIG_DIR/$config.conf" && Source "$CONFIG_DIR/$config.conf" || true
done

All these config files are optional.

jsmeix commented at 2015-12-15 14:38:¶

I will have a closer look - but at first glance doesn't the

test -r "$file" && Source "$file"

ensure that the Source function is not called with unsourceable files?

Perhaps to better match the test in the Source function

    if ! test -s "$source_file" ; then
        Debug "Skipping Source() because source file '$source_file' not found or empty"
        return
    fi

it should be the same test used in usr/sbin/rear

test -s "$file" && Source "$file"

jsmeix commented at 2015-12-15 14:44:¶

I think - strictly speaking - the real correct test
to find out if $file is sourceable is

bash -n "$file" && Source "$file"

Of course "bash -n" is only limited useful

bash -n /etc/rear/local.conf && echo y || echo n
y
# bash -n /etc/fstab && echo y || echo n
y

but at least a little bit better than "test -s":

# bash -n /boot/vmlinuz && echo y || echo n
/boot/vmlinuz: /boot/vmlinuz: cannot execute binary file
n
# test -s /boot/vmlinuz && echo y || echo n
y

;-)

schlomo commented at 2015-12-15 14:53:¶

@jsmeix looking again at the code that I quoted makes me think that you are right. We should remember separation of concerns and decide what is the responsibility of the Source function.

Idea:
Add a --skipmissing command line parameter to the Source function and use that for reading the configs and not use it in the SourceStage code. Without this parameter Source will exit with BugError on missing scripts.

This should make the code in usr/sbin/rear simpler.

With regard to bash -n, maybe for reading configs this is a good idea. For code scripts I would prefer to do this during build and not during runtime.

In general, we should also make sure that we bail out with meaningful stack traces if users provide broken config.

jsmeix commented at 2015-12-15 15:15:¶

I fully agree to move the testing responsibility into the Source function
because then it is implemented once at one place plus a parameter
for the Source function to specify how it should do its tests.

I suggest to keep in the Source function

local source_file="$1"

so that all existsing "Source $file" works as before and
have an optional

local error_behaviour="$2"

to optionally specify what to do in case of errors.

For example things like:

Source "$file" "skip_if_missing"
Source "$file" "skip_if_syntax_error"
Source "$file" "skip_if_any_error"
Source "$file" "exit_if_missing"
Source "$file" "exit_if_syntax_error"
Source "$file" "exit_if_any_error""

where

Source "$file"

implements the current (default) behaviour.

gdha commented at 2015-12-15 15:35:¶

@jsmeix sounds good to me - back-worth compatibility is important

jsmeix commented at 2015-12-15 15:41:¶

Backward compatibility is mandatory!
Except for severe bugs and security issues where an
incompatible change is sometimes the only way to fix it.

Interestingly the Source function is only called in usr/sbin/rear
(as shown above in https://github.com/rear/rear/issues/741#issuecomment-164780547)
and
by the SourceStage function in
usr/share/rear/lib/framework-functions.sh

Therefore it is easy to change as we like and even
backward compatibility is actually no real issue here
because we could change that few places as we like.

Nevertheless I keep it backward compatible.

I will make a pull request...

schlomo commented at 2015-12-15 16:07:¶

Ah, I would really prefer to stick to the Unix command line standards.

What is wrong with something like this:

local skip_if_missing=""
if [[ "$1" == "--skipmissing" ]] ; then
    skip_if_missing=yes
    shift
fi
... previous code ...

And if we need to add more arguments then we can also employ getopts and friends.

I would initially also implement only the parts that we actually need for one change and not put there any features that we don't need ATM.

All that is actually backwards compatible because I don't think that we have any use case where we Source files that start from --.

jsmeix commented at 2015-12-15 16:48:¶

@schlomo
also o.k. for me.

I will change my current
https://github.com/jsmeix/rear/tree/add_optional_error_behaviour_for_Source_function
accordingly tomorrow (I also need to test it)
and then do a pull request.

Also I implemented only what is actually needed right now
(I like to avoid possible bugs with too much at once).

jsmeix commented at 2015-12-16 09:20:¶

I found out that I cannot keep the default error behaviour
backward compatible when I like to get it safer against
programming errors.

Assume there is /usr/share/rear/myscript.sh and

Source "myscirpt.sh"

then I like to have it directly bail out with BugError
"source file /usr/share/rear/myscirpt.sh not found or empty"
to be safer against such programming errors.

But I can implement the new default error behaviour
in a backward compatible way when I also change
all existing calls of the Source function to use explicitly
the old error behaviour:

Source --skipmissing "$file"

schlomo commented at 2015-12-16 09:29:¶

@jsmeix I am not sure I understand the problem you try to solve.

Do we really need to care so much about backwards compatibility if Source is just in just 2 files?

In how many places do you plan to Source myscript.sh? What is the specific use case you have in mind?

Again, let's try to keep the scope of this change as narrow as possible and fix potential problems when they actually show up.

jsmeix commented at 2015-12-18 10:58:¶

I have to postpone implementation until next year
because I will not have time for it during Christmas time
(cf. https://github.com/rear/rear/issues/744)

@schlomo
I was a bit wondering about your questions because
I assumed I had already answered them above.

Perhaps my "Backward compatibility is mandatory!"
had lead to misunderstanding?

With "backward compatibility" I do not mean that old behaviour
can never be changed. I only mean when bahaviour is
changed, then in a way that existing usage does not break.

This means that behaviour of internal functions can be changed
provided all exixting calls of that functions are adapted so that
they do not fail with the new behaviour.

Regarding "Do we really need to care so much about backwards compatibility if Source is just in just 2 files?":

See what I wrote in https://github.com/rear/rear/issues/741#issuecomment-164801983
"Interestingly the Source function is only called in ... and ...
Therefore it is easy to change as we like and even
backward compatibility is actually no real issue here
because we could change that few places as we like."

Regarding "keep the scope of this change as narrow as possible":

See my first commit https://github.com/jsmeix/rear/commit/87ce192904786f4bd24a346f03f52c952895036a where I only implemented "skip_if_missing".
I also implemented its logical counterpart "exit_if_missing"
only to keep its default behaviour the same as before.
But since https://github.com/rear/rear/issues/741#issuecomment-165043458 I found out that I cannot keep
its default behaviour the same as before.

Regarding "What is the specific use case you have in mind?":

See what I wrote in https://github.com/rear/rear/issues/741#issue-122279067

"I made a programming error by calling
Source "wrapup/default/99_copy_logfile.sh"
Instead of a direct error the Source function ignored/skipped
it and I had to find out later the hard way why I got no log file
copied into the target system.
Personally I prefer an immediate error exit directly at the place
where something is wrong..."

See also what I wrote in https://github.com/rear/rear/issues/741#issuecomment-165043458

"Assume there is /usr/share/rear/myscript.sh and
Source "myscirpt.sh"
then I like to have it directly bail out with BugError"

Note the programming error (typo) in
Source "myscirpt.sh"
(...scirpt... instead of ...script...).

And regarding what my final goal is, see also https://github.com/rear/rear/issues/741#issue-122279067

"immediate error exit when something is wrong"

In my relatively short personal experience with rear
I have had already too many issues where
it was relatively hard to find the root cause
because rear proceeded regardless of errors
until it later failed in an unexpected way
far off the place where the root cause happened.

Example:

From my point of view things like

# ( foo=$( cat qqq | grep something ) ; echo "foo='$foo'" ) ; echo $?
cat: qqq: No such file or directory
foo=''
0

are just bad.

From my point of view this behaves correctly:

# ( set -ue -o pipefail ; foo=$( cat qqq | grep something ) ; echo "foo='$foo'" ) ; echo $?
cat: qqq: No such file or directory
1

From my point of view if an error does not matter
there must be explicit code to ignore that specific error
or explicit code that deals with that specific error:

# ( set -ue -o pipefail ; foo=$( cat qqq | grep something || echo '' ) ; echo "foo='$foo'" ) ; echo $?
cat: qqq: No such file or directory
foo=''
0

Regardless that here the result is the same as
in my "just bad" example from my point of view there is
a major difference here because the explicit || echo ''
makes it explicit what the intended result is
if cat qqq | grep something did not succeed.

schlomo commented at 2015-12-19 16:11:¶

Hi Johannes,

I am all in favour to make error behaviour more explicit.

My comment was really mostly about using Unix-style command line options
also for the Source function and not to rely on positional arguments.

Kind Regards,
Schlomo

On 18 December 2015 at 11:58, Johannes Meixner notifications@github.com
wrote:

I have to postpone implementation until next year
because I will not have time for it during Christmas time
(cf. #744 https://github.com/rear/rear/issues/744)

@schlomo https://github.com/schlomo
I was a bit wondering about your questions because
I assumed I had already answered them above.

Perhaps my "Backward compatibility is mandatory!"
had lead to misunderstanding?

With "backward compatibility" I do not mean that old behaviour
can never be changed. I only mean when bahaviour is
changed, then in a way that existing usage does not break.

This means that behaviour of internal functions can be changed
provided all exixting calls of that functions are adapted so that
they do not fail with the new behaviour.

Regarding "Do we really need to care so much about backwards compatibility
if Source is just in just 2 files?":

See what I wrote in #741 (comment)
https://github.com/rear/rear/issues/741#issuecomment-164801983
"Interestingly the Source function is only called in ... and ...
Therefore it is easy to change as we like and even
backward compatibility is actually no real issue here
because we could change that few places as we like."

Regarding "keep the scope of this change as narrow as possible":

See my first commit jsmeix@87ce192
https://github.com/jsmeix/rear/commit/87ce192904786f4bd24a346f03f52c952895036a
where I only implemented "skip_if_missing".
I also implemented its logical counterpart "exit_if_missing"
only to keep its default behaviour the same as before.
But since #741 (comment)
https://github.com/rear/rear/issues/741#issuecomment-165043458 I found
out that I cannot keep
its default behaviour the same as before.

Regarding "What is the specific use case you have in mind?":

See what I wrote in #741 (comment)
https://github.com/rear/rear/issues/741#issue-122279067

"I made a programming error by calling
Source "wrapup/default/99_copy_logfile.sh"
Instead of a direct error the Source function ignored/skipped
it and I had to find out later the hard way why I got no log file
copied into the target system.
Personally I prefer an immediate error exit directly at the place
where something is wrong..."

And regarding what my final goal is, see also #741 (comment)
https://github.com/rear/rear/issues/741#issue-122279067

"immediate error exit when something is wrong"

In my relatively short personal experience with rear
I have had already too many issues where
it was relatively hard to find the root cause
because rear proceeded regardless of errors
until it later failed in an unexpected way
far off the place where the root cause happened.

Example:

From my point of view things like

( foo=$( cat qqq | grep something ) ; echo "foo='$foo'" ) ; echo $?

cat: qqq: No such file or directory
foo=''
0

are just bad.

From my point of view this behaves correctly:

( set -ue -o pipefail ; foo=$( cat qqq | grep something ) ; echo "foo='$foo'" ) ; echo $?

cat: qqq: No such file or directory
1

From my point of view if an error does not matter
there must be explicit code to ignore that specific error
or explicit code that deals with that specific error:

( set -ue -o pipefail ; foo=$( cat qqq | grep something || echo '' ) ; echo "foo='$foo'" ) ; echo $?¶

cat: qqq: No such file or directory
foo=''
0

Regardless that here the result is the same as
in my "just bad" example from my point of view there is
a major difference here because the explicit || echo ''
makes it explicit what the intended result is
if cat qqq | grep something did not succeed.

—
Reply to this email directly or view it on GitHub
https://github.com/rear/rear/issues/741#issuecomment-165748517.

jsmeix commented at 2015-12-19 20:18:¶

I fully agree that avoiding positional arguments
also helps to make it more robust against errors.

I wish you a Merry Christmas and a happy New Year!

Unfortunately @gdha had already closed https://github.com/rear/rear/issues/744
even before Christmas and New Year's Day... ;-)

gdha commented at 2016-09-13 14:30:¶

@jsmeix has this been completed? if not, move the milestone to 1.20, ok?

jsmeix commented at 2016-09-15 13:49:¶

That one came up while I was experimenting with
the future "rear install" workflow.
I will continue working on it in the future as time permits.

---

[Export of Github issue for rear/rear.]