Hi, Michael D. Setzer II via users wrote: > Found that there is a package ShellCheck that has program shellcheck > that seems to find errors. It finds the one you mentioned, and also list a > few others??
ShellCheck can be handy, indeed.
The missing space after '[' is an error, while the others
are only warnings (or simply informational). You can limit
the severity of issues to report with the shellcheck
command, e.g.:
shellcheck -S error /usr/bin/firefox
shellcheck -S warning /usr/bin/firefox
etc. By default, shellcheck reports errors, warnings, info,
and style issues. If you have a copy of of the old firefox
script and use `-S error` it only reports the missing space
after '[' issue:
# This is a run in the git repository of the firefox
# package, with the commit prior to the syntax fix
# checked out.
$ shellcheck -S error firefox.sh.in
In firefox.sh.in line 186:
if [ -x $GETENFORCE_FILE ] && [`getenforce` != "Disabled" ]; then
^-- SC1035: You need a space after the [
and before the ].
For more information:
https://www.shellcheck.net/wiki/SC1035 -- You need a space after the [
and ...
When run with the firefox script from the most recent
update, no errors are reported.
It might be fun for someone to look through the warnings and
other issues reported and submit a patch series to the
firefox.sh.in script to harden and improve it.
While shellcheck suggests a rather minimal change, it can
often be improved further. (I think it's wise for
shellcheck to suggest a minimal change and link to their
wiki page for further discussion of the suggestion(s).)
Here's what shellcheck says about the current getenforce
conditional:
In /usr/bin/firefox line 186:
if [ -x $GETENFORCE_FILE ] && [ `getenforce` != "Disabled" ]; then
^----------^ SC2046: Quote this to
prevent word splitting.
^----------^ SC2006: Use $(...) notation
instead of legacy backticked `...`.
Did you mean:
if [ -x $GETENFORCE_FILE ] && [ $(getenforce) != "Disabled" ]; then
The "Did you mean" leaves out the quoting of $(getenforce)
so if one were to copy the suggestion directly, there would
still be a warning to quote it to prevent word splitting.
That would be easy to do, of course. But a better fix, IMO,
would be to use '[[' for the conditional expression rather
than '[' as '[[' does not perform word splitting. And since
the script is clearly a bash script rather than a generic sh
script, there's no reason not to use '[[' really.
If I were changing this chunk of code, I'd probably do
something like:
$ git diff
diff --git i/firefox.sh.in w/firefox.sh.in
index 78d908e..0d4d9e2 100644
--- i/firefox.sh.in
+++ w/firefox.sh.in
@@ -183,7 +183,7 @@ fi
# When Firefox is not running, restore SELinux labels for profile files
# (rhbz#1731371)
if [ $MOZILLA_DOWN -ne 0 ]; then
- if [ -x $GETENFORCE_FILE ] && [ `getenforce` != "Disabled" ]; then
+ if [[ -x $GETENFORCE_FILE ]] && [[ $($GETENFORCE_FILE) != Disabled ]];
then
(restorecon -vr ~/.mozilla/firefox/* &)
fi
fi
The changes and the reasoning for them are:
* Use '[[' rather than '[' to avoid issues with word
splitting.
* Replace `...` with $(...) for command substitution.
Only ancient, non-POSIX sh implementations don't
support the much nicer $(...) form.
* Use the $GETENFORCE_FILE variable consistently.
There's little point in defining that variable to
point to the location of the getenforce binary, using
it to test whether the binary exists and is
executable, and then _not_ using it to actually run
the getenforce command.
I wouldn't do something like that just for this chunk of
code though, since there are many similar uses of '[' and
`...` which would also benefit from being replaced by '[['
and $(...), respectively.
I'd end up submitting a short patch series that tackled each
of the general changes. That keeps the script consistent
and each change only needs to be justified once.
Wow, that was longer than I intended. I hope it didn't put
anyone to sleep (who weren't trying to get to there). :)
--
Todd
signature.asc
Description: PGP signature
_______________________________________________ users mailing list -- users@lists.fedoraproject.org To unsubscribe send an email to users-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/users@lists.fedoraproject.org
