Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check for carriage return in {local|site|rescue}.conf files #1969

Merged
merged 2 commits into from Nov 20, 2018
Merged

check for carriage return in {local|site|rescue}.conf files #1969

merged 2 commits into from Nov 20, 2018

Conversation

gdha
Copy link
Member

@gdha gdha commented Nov 19, 2018

$ sudo usr/sbin/rear -v mkrescue
Relax-and-Recover 2.4 / Git
Running rear mkrescue (PID 15205)
Using log file: /projects/rear/myforks/rear/var/log/rear/rear-okido.log
ERROR: Carriage return character in /projects/rear/myforks/rear/etc/rear/local.conf (perhaps DOS or Mac format)
  • Brief description of the changes in this pull request: If configuration files contain hidden carriage returns this often runs into an error (which is sometimes not obvious to understand where the issue came from). With this patch we will bail out and tell the user to fix his config file

@gdha gdha added the enhancement Adaptions and new features label Nov 19, 2018
@gdha gdha added this to the ReaR v2.5 milestone Nov 19, 2018
@gdha gdha self-assigned this Nov 19, 2018
@gdha gdha requested a review from jsmeix November 19, 2018 11:45
Copy link
Member

@schlomo schlomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice trick with tr 😄

@jsmeix
Copy link
Member

jsmeix commented Nov 19, 2018

Currently it tests only site.conf local.conf and rescue.conf
but sbin/rear sources several other config files too (excerpts):

source $SHARE_DIR/conf/default.conf

perhaps we can assume default.conf is never modified by the user
but I think there have been already issues where users had
messed around with default.conf ?

test -r "$CONFIG_DIR/os.conf" && Source "$CONFIG_DIR/os.conf" || true
test -r "$CONFIG_DIR/$WORKFLOW.conf" && Source "$CONFIG_DIR/$WORKFLOW.conf" || true

I think all *.conf files in $CONFIG_DIR could have been broken by the user
so that all *.conf files in $CONFIG_DIR should be tested.

@jsmeix
Copy link
Member

jsmeix commented Nov 19, 2018

@schlomo
I think your Nice trick with 'tr' comment means
that you don't know a plain bash-only method and
#1965 (comment)
is answered.

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course I happily approve my own code ;-)

@jsmeix
Copy link
Member

jsmeix commented Nov 19, 2018

FYI:
With #1970
it would be possible to use the Source function return code
to actually check in commands like Source "$CONFIG_DIR/os.conf"
whether or not that was actually successful - but still note
#1965 (comment)
i.e. as long as it is allowed that the last command in config files could fail
we cannot use the Source function return code to check if config files
are valid as I did in
#1965 (comment)

@gdha
Copy link
Member Author

gdha commented Nov 20, 2018

@jsmeix Glad to use your code and as the issue was already closed I decided to add it to rear before we all forgot about it. Anyhow, a big thank you.
IMHO I think these 3 scripts are in 99% of the cases where end-users play with - if they touches other scripts or config files then we may consider them as experts, no?

@jsmeix
Copy link
Member

jsmeix commented Nov 20, 2018

@gdha
yes, as it is now it should detect 99.9% of such cases.

jsmeix added a commit that referenced this pull request Nov 20, 2018
…code_of_source_call_related_to_issue_1965

Now the Source function in lib/framework-functions.sh
returns the the return code of its actual work which is the
return code of its 'source $source_file' call so that the caller
of the Source function can now decide what to do and
there is a debug log message when 'source $source_file'
results a non-zero return code, see the related issues
#1965 and
#1969
Additionally the call of apply_bash_flags_and_options_commands
in the Source function (where actually only 'set +x' is done to switch off
debugscript mode) does no longer output long 'set -x' messages which
avoids many big but meaningless debugscript messages in the log.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants