null' where it makes sense to get all info in the log¶

Labels: enhancement, cleanup, fixed / solved / done

jsmeix opened issue at 2017-06-27 10:04:¶

There are very many places in the ReaR code where
output is suppressed via redirection to /dev/null.

Herebey I ask for comments what to do when
stdout and/or stderr is redirected to /dev/null.

My personal opinion is that
in general ReaR should
suppress success output via stdout
but keep failure output via stderr in the log.

Details:

What to do when stderr is redirected to /dev/null:

In
https://github.com/rear/rear/pull/1359#pullrequestreview-37827146
@schlomo wrote

Why hide STDERR here? It might contain useful infos
that should appear in the log.

Accordingly
in general stderr should be in the log.

Exceptional cases are where tools blabber non-useful stuff
via stderr for example 'cp $verbose' in 400_copy_modules.sh
and in 420_copy_firmware_files.sh

What to do when stdout is redirected to /dev/null:

My personal opinion is that
in general stdout should not be in the log
so that in general stdout should be redirected to /dev/null.

A special case is properly fixing the old
deprecated usage of '>&8':

In https://github.com/rear/rear/issues/887#issue-161207988
@gdha wrote

We are not using the progress-bar code for years
anymore (the >&8 redirection), so the question was
asked, why not get rid of it and capture the code
in the logging (or not)?

Via
https://github.com/rear/rear/pull/1391/commits/4f4efb37db4c4ee7d1b05f247101c1680e1f8c31
I replaced all old deprecated usage of '>&8' with '>/dev/null'
to keep the current behaviour (i.e. suppress all the output)
both for stderr like

- ... 2>&8
+ ... 2>/dev/null

and in most cases also for stdout like

- ... >&8
+ ... >/dev/null

- ... 1>&8
+ ... 1>/dev/null

- ... >&8 2>&1
+ ... >/dev/null 2>&1

jsmeix commented at 2017-06-28 08:25:¶

After sleeping over it I think now that

in general stdout should not be in the log
so that in general stdout should be redirected to /dev/null

is not the right way because
it contradicts https://github.com/rear/rear/issues/885
where we intend to have stdout in the log.

Meanwhile I think regarding what to do with stdout:

In general stdout should not appear on the user's terminal
so that in general stdout should be redirected.

When usr/sbin/rear is run in verbose mode
stdout should be redirected to the log file.

When usr/sbin/rear is not run in verbose mode
stdout should be redirected to /dev/null.

I think this could be implemented in usr/sbin/rear like

# Redirect STDOUT and STDERR.
# Redirect STDERR to the log file because error messages should be always in the log.
# To be more on the safe side append to the log file '>>' instead of plain writing to it '>'
# because when a program (bash in this case) is plain writing to the log file it can overwrite
# output of a possibly simultaneously running process that likes to append to the log file
# (e.g. when a background process runs that also uses the ReaR log file for logging):
exec 2>>"$RUNTIME_LOGFILE"
# In verbose mode stdout is redirected to the log file
# otherwise stdout is redirected to /dev/null
# cf. https://github.com/rear/rear/issues/1395
if test "$VERBOSE" ; then
    # Make stdout the same what stderr already is (i.e. the log file)
    # This keeps strict ordering of stdout and stderr outputs in the log file
    # because now both stdout and stderr use one same file descriptor:
    exec 1>&2
else
    # In general stdout should not appear on the user's terminal
    # so that stdout is redirected to /dev/null when not in verbose mode:
    exec 1>/dev/null
fi

which seems to work o.k. for me during a very first test.

With that basically all those stdout redirections in the scipts
can be removed (which greatly cleans up the code in the scipts).

For completeness here what is already done with stderr:

In general stderr should not appear on the user's terminal
so that in general stderr should be redirected.

Error messages should be always in the log so that
stderr is always redirected to the log file.

Exceptions are useless stderr messages (like verbose warnings)
that can be redirected to /dev/null where needed in the scripts.

schlomo commented at 2017-06-28 14:35:¶

IMHO ReaR should not willingly destroy information. So for me >/dev/null is not an option. If we don't want to show it to the user then it must go to the logfile. If we want to show it to the user then having it in the logfile too would be really nice.

jsmeix commented at 2017-06-29 09:39:¶

@schlomo
I agree with you.

Personally I don't mind if a log file gets big.
Personally I prefer a verbose log over a concise log.
Personally I prefer to always have full information in the log.
When something fails I want to have a complete log right now
without the need to reproduce the failure with some special
option settings to get a complete log.

I made my proposal in
https://github.com/rear/rear/issues/1395#issuecomment-311592122
to have STDOUT in the log only in case of verbose mode
because I (falsely) assumed that by default for ReaR
a concise log is preferred over a complete log.

With
https://github.com/rear/rear/issues/1395#issuecomment-311678896
things get simple and straightforward so that now
(if I understand it correctly) our intent is:

In general stdout should not appear on the user's terminal
so that in general stdout should be redirected to the log
(this is what we already have in current GitHub master code).

Furthermore because ReaR should not destroy information
basically all current '>/dev/null' in the code should be dropped
so that in general all stdout (and all stderr) appears in the log.

Exceptions are really useless stdout messages and
really useless or falsely scaring stderr messages
that can be redirected to /dev/null as needed in the scripts.

Regarding possibly useless stdout messages:

Perhaps things like 'pushd ... >/dev/null' and 'popd >/dev/null'
might be valid examples where stdout could be suppressed
(currently all 'pushd' and 'popd' suppress stdout except
in my restore/ZYPPER/default/940_generate_fstab.sh).

But on the other hand I do not see anything wrong
if 'pushd' and 'popd' stdout appears in the log
(cf. above "I prefer to have full information in the log").

I assume all 'pushd' and 'popd' suppress stdout
because before https://github.com/rear/rear/pull/1391
the 'pushd' and 'popd' stdout appeared on the users terminal
where it is meaningless so that it was simply suppressed
(instead of redirecting it to stderr to get it into the log
as in restore/ZYPPER/default/940_generate_fstab.sh).

jsmeix commented at 2017-06-29 09:48:¶

I will be investigating about a usable way how to call a command
that shows its stdout (and/or its stderr) to the user
and have both of them in the log file too.
But that will be something for the future.
For now I am trying to get ReaR running correctly
when both stdout and stderr are redirected into the log, cf.
https://github.com/rear/rear/issues/1398#issuecomment-311886920

jsmeix commented at 2017-08-25 11:17:¶

Regarding the previous
https://github.com/rear/rear/issues/1395#issuecomment-311918073
see also
https://github.com/rear/rear/pull/1449#issuecomment-324836426

jsmeix commented at 2019-10-11 14:11:¶

For an example where a particular script was fixed see
https://github.com/rear/rear/commit/af14e15db75bacd554d53dd1041d7852ceb8d9b9

jsmeix commented at 2019-10-11 14:16:¶

If time permits I would like to do
some general cleanup for ReaR 2.6. in all scripts
and remove '2>/dev/null' where it makes sense
and replace '&>/dev/null' by '1>/dev/null' where it makes sense
or also remove '&>/dev/null' where that seems to be better
BUT
see https://github.com/rear/rear/issues/799#issuecomment-531247109

jsmeix commented at 2019-10-18 12:00:¶

With https://github.com/rear/rear/pull/2252 merged
I hope this issue is sufficiently fixed.

But I cannot test all those affected code parts,
in particulart not the changes for TSM and NBU in
verify/TSM/default/390_request_point_in_time_restore_parameters.sh
and
verify/NBU/default/390_request_point_in_time_restore_parameters.sh

Please report if there are regressions so that I can fix them,
ideally each one as a new and separated GitHub issue.

[Export of Github issue for rear/rear.]

#1395 Issue closed: Remove '>/dev/null' where it makes sense to get all info in the log¶