Thanks lots of interesting information. Have run it on a few of my scripts, didn't find any errors, but did make some of the recommended changes to clear out all the results. Thanks for the time and information. Have a Great day. On 24 Oct 2020 at 20:18, Todd Zullinger wrote: Date sent: Sat, 24 Oct 2020 20:18:42 -0400 From: Todd Zullinger <tmz@xxxxxxxxx> To: users@xxxxxxxxxxxxxxxxxxxxxxx Subject: Re: Question on firefox script and zombie?? Send reply to: Community support for Fedora users <users@xxxxxxxxxxxxxxxxxxxxxxx> > 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 > +------------------------------------------------------------+ Michael D. Setzer II - Computer Science Instructor (Retired) mailto:mikes@xxxxxxxx mailto:msetzerii@xxxxxxxxx Guam - Where America's Day Begins G4L Disk Imaging Project maintainer http://sourceforge.net/projects/g4l/ +------------------------------------------------------------+ _______________________________________________ users mailing list -- users@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to users-leave@xxxxxxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxxxxxxxxxx
