Re: Make rc scripts use [[ instead of [

[Jason McIntyre]

On Mon, Sep 05, 2016 at 04:39:49PM -0400, Anthony Coulter wrote:
> 
> While I vaguely remember reading long ago that `echo' and `[' were
> shell builtins, I'm much more strongly aware that /bin/echo and /bin/[
> are also available as separate executables. Their man pages don't
> mention that they have builtin ksh implementations, and I'm very aware
> that `[[' parses arguments differently from `[', e.g.
> 

hi.

any commands which have shell builtin equivalents do make note of this,
in the STANDARDS section of the relevant man page.

jmc

Re: Make rc scripts use [[ instead of [

[Alexander Hall]

Ok, so I'll throw in my view too.

On September 5, 2016 1:24:17 PM GMT+02:00, Anthony Coulter 
 wrote:
>Some of the system scripts make inconsistent use of ksh-specific
>features, specifically "print" and "[[" as efficient replacements for
>"echo" and "[". This change makes the /etc/ksh.kshrc and all the rc
>scripts use "print" and "[[" exclusively.

Why are they more efficient? They're all builtins.

>
>Switching from echo to print only brought up one interesting issue:
>/etc/rc.d/mountd uses "print -n >foo" instead of "touch foo".
>The latter is arguably more transparent but I didn't change to it.

Also it does not empty the file.

I'd do
  >foo
or possibly
  :>foo

>
>Switching from [ to [[ makes a lot of things more readable; most of
>the improvements result from removing unnecessary quotation marks.
>We also don't have to test for unset variables as aggressively; a
>number of awkward [ X"$foo" = XYes ] constructions were replaced
>with [[ $foo = Yes ]] .

A POSIX shell does not need the X'es. It needs quotes though.

>
>The test [[ ${_args} = "${_args# }" ]] appears in rcctl; I left the
>quotation marks on the right-hand side because I'm afraid of breaking
>something I don't understand.

Without quotes (on the right side) the expression is subject to pattern 
matching.

>
>The rcctl script also has a lot of lines like:
>   daemon_timeout="$(svc_getdef ${_svc} timeout)"
>I think the quotation marks are unnecessary

They are in our ksh, but I don't know what POSIX says.

> but the original author
>went to a lot of trouble to escape internal quotes, e.g.
>   daemon_flags="$(eval print \"\${${_svc}_flags}\")"

Since it uses $(...), those escaped quotes are unrelated to the outer quoting. 
This is by far the best reason to abandon backticks.

>so I'll give him the benefit of the doubt. I would encourage somone
>with greater confidence to take a closer look.
>
>I replaced `backticks` with $(subshell notation) in a few places, but
>only in places where I was already changing [ to [[. I wanted to remove
>them all but I can't really justify the change in terms of either
>readability or process efficiency.

As much as I hate them I love them, because oldschool. For simple cases I think 
there is no big reason to change them although I wouldn't introduce them in new 
code.

>
>Regarding numeric changes: in ksh, [[ 0 -eq 00 ]] is true (numeric
>comparison) but [[ 0 = 00 ]] is false (string comparison). The latter
>is more readable, so I made the switch in places where the left-hand
>side is a system output, e.g. [[ $(id -u ) = 0 ]] or [[ $# > 0 ]].
>I would not expect id(1) to print the string "00" for root, nor would
>I expect the shell to print "00" for $#. But I did *not* change
>comparison operators in more complicated situations, like the bottom
>of /etc/rc which has [[ ${daemon_rtable} -eq 0 ]]. It isn't immediately
>obvious what sort of string is in ${daemon_rtable} so the numeric
>comparison seems appropriate here.

I also tend to check $# = 0 but I never consider it a numeric comparison. If I 
understand you correctly, you want to change a numeric comparison to a string 
comparison. That is a style change, and with no defined code style for shell 
scripts, that could change back and forth forever. I see no point in that.

>
>I corrected a comment at the top of /etc/rc; it said that "set +o sh"
>turned Strict Bourne shell mode off. I'm pretty sure it's actually
>turning strict mode on.

As pointed out already, you were wrong.

-o sets, +o sunsets.

>
>My system still boots with these changes, and rcctl still appears to
>work. I can't easily test all of the code paths in each of the rc
>scripts. But the changes involved here are straightforward, and you
>can validate this diff line-by-line.

While I understand your reasoning, please beware that sending diffs that are 
untested requires other to do your job, and is not a good way to make someone 
willing to look into the subject.

And the diff is far too big. Find *one* (1) *problem*, fix that, test it, and 
send a diff."

I'll end this with a reply I sent off-list regarding parts of this email:

"AFAICT, this does not fix anything, so I don't see the reason for forcibly 
changing from working sh syntax to ksh'isms just "because we can". [] is as 
valid ksh as [[]], and I believe practicing writing portable (at least as far 
as posix) shell scripts is worthwhile, unless it becomes too messy."

Note that this is not meant to being you down. Small (where possible) diffs, 
tested and justified is the way to go.

/Alexander

>
>
>Index: etc/ksh.kshrc
>===
>RCS file: /open/anoncvs/cvs/src/etc/ksh.kshrc,v
>retrieving revision 1.20
>diff -u -p -u -r1.20 ksh.kshrc
>--- etc/ksh.kshrc  18 Feb 2015 08:39:32 -  1.20
>+++ etc/ksh.kshrc  5 Sep 2016 04:03:43 -
>@@ -62,7 +62,7 @@ case "$-" in
>   case "$TERM" in
>   sun*-s)
>   # sun 

Re: Make rc scripts use [[ instead of [

[Anthony Coulter]

> You don't explain why `print' is more efficient than `echo'.

Short answer: I'm an idiot who didn't realize that `echo' and `[' are
shell builtins. The only parts of my change that still seem worthwhile
to me now are the ones related to the awkward X-comparisons, but there
are few enough of those and they're handled in an idiomatic way, so I'm
not going to bother pushing it. Anybody who has real work to do can
stop reading the email now. The advantage of continuing to read is that
you get some insight into the psychology of a naive user.

Long answer (written partly in a pointless attempt to save face, and
partly to show appreciation to Jeremie's non-condescending response to
what turned out to be a stupid suggestion):

While I vaguely remember reading long ago that `echo' and `[' were
shell builtins, I'm much more strongly aware that /bin/echo and /bin/[
are also available as separate executables. Their man pages don't
mention that they have builtin ksh implementations, and I'm very aware
that `[[' parses arguments differently from `[', e.g.

$ foo="two words"
$ [[ $foo = "two words" ]] && echo True || echo False
True
$ [ $foo = "two words" ] && echo True || echo False
ksh: [: words: unexpected operator/operand
False

which I interpreted as further evidence that `[' and `[[' were
fundamentally different in implementation. Of course, this is wrong,
as "type [" and "type echo" prove. And I remember now that /bin/echo
and /bin/[ are available as separate executables for POSIX reasons,
and that their man pages don't cover the behavior of ksh because they
*aren't* ksh. The point of all of this is that I made an assumption
that seemed reasonable and while I checked some things in the manual,
there were some things I didn't even think to check because they seemed
so obvious. Oops. That happened in another way:

> Nope, ''set -o sh'' sets strict mode on, ''set +o sh'' disables it.
I checked to make sure that "sh" stood for strict mode and not
unstrict mode (since I couldn't understand how that comment could be
so flagrantly wrong), but it never occurred to me to check that `-'
enables something and `+' disables it. Oops again.

In any event, my main goal was to keep us from spawning needless
processes, and my first draft of the change was pretty much just
replacing `[' with `[[' and `echo' with `print.' But since I already
had to change things like [ "$foo" ] to [[ -n "$foo" ]] it seemed
wasteful not to take advantage of the improved argument-parsing of the
`[[' operator and remove the quotation marks, to get [[ -n $foo ]].
But if I'm doing it in some places, I should do it in all places. So
a lot of quotation marks went away to take advantage of the improved
parsing characteristics of `[['.

Some of my other "readability improvements" felt a little awkward and I
couldn't find any reference to shell scripts in style(9) (which I *did*
check before mailing in the proposed change), so I was careful to make
them only in places where I was already breaking stuff. Replacing ``
with $() was an example of that---I left backticks in place in yppasswdd
when I didn't already have a "legitimate" reason to touch the line, and
then I made a point to state explicitly what I had done to see if anyone
would comment on it. Which you did. (I won't try to rationalize my
change to numeric comparisons, since as you say it was a personal
preference, and it was also fairly dubious.)

There's one more specific point I want to make while I'm here about
the awkward X-comparisons: if you search for the phrase "A common
mistake" in the ksh manual, you'll find an example of a comparison
that fails when a variable is unset. Oddly, it only works when the
"if" keyword is used; the "[ foo ] && do_something" construct does not
have any problems.

So, this is the last you'll hear from me on this proposal---it was a
bad idea from the start, and if I had typed "whence -v [" from the
beginning I wouldn't have bothered. But overall I think my
misunderstanding was reasonable, and my "needless churn" would have
been better-received if the underlying reason for the change had been
valid.

And finally, I would again like to thank you (Jeremie) for a thorough
and non-condescending response to a stupid idea. You assumed that I was
sensible enough to read your reviewi and taught me something about the
quote behavior of the eval operator---I hope this (otherwise pointless)
email validates your faith that not all people with stupid ideas are
themselves stupid. And sometimes we read the man pages and we still
miss important things!

Regards,
Anthony

Re: Make rc scripts use [[ instead of [

[Jeremie Courreges-Anglas]

Hi,

Anthony Coulter  writes:

> Some of the system scripts make inconsistent use of ksh-specific
> features, specifically "print" and "[[" as efficient replacements for
> "echo" and "[". This change makes the /etc/ksh.kshrc and all the rc
> scripts use "print" and "[[" exclusively.

You don't explain why `print' is more efficient than `echo'.

> Switching from echo to print only brought up one interesting issue:
> /etc/rc.d/mountd uses "print -n >foo" instead of "touch foo".
> The latter is arguably more transparent but I didn't change to it.
>
> Switching from [ to [[ makes a lot of things more readable; most of
> the improvements result from removing unnecessary quotation marks.

I'm not sure I see how it makes things more readable; less bytes on the
screen?  Most contexts where variables are involved should use quotes
anyway.  So I am not sure this is a good thing in the end.

> We also don't have to test for unset variables as aggressively; a
> number of awkward [ X"$foo" = XYes ] constructions were replaced
> with [[ $foo = Yes ]] .

This is not related to quote removals.  AFAIK ksh handles [ "$foo" = Yes ]
properly, the "X"s aren't necessary.  They were probably added because
of horror stories about ancient shells.

> The test [[ ${_args} = "${_args# }" ]] appears in rcctl; I left the
> quotation marks on the right-hand side because I'm afraid of breaking
> something I don't understand.

FWIW I can't see why removing the quotes would break anything here.

> The rcctl script also has a lot of lines like:
>   daemon_timeout="$(svc_getdef ${_svc} timeout)"
> I think the quotation marks are unnecessary but the original author
> went to a lot of trouble to escape internal quotes, e.g.
>   daemon_flags="$(eval print \"\${${_svc}_flags}\")"

The "internal" quotes are escaped so that they are still present after
the `eval' pass, not because the whole command substitution itself is
surrounded by double quotes.

var='ab'; eval print "\$var"; eval print \"\$var\"

> so I'll give him the benefit of the doubt. I would encourage somone
> with greater confidence to take a closer look.
>
> I replaced `backticks` with $(subshell notation) in a few places, but
> only in places where I was already changing [ to [[. I wanted to remove
> them all but I can't really justify the change in terms of either
> readability or process efficiency.

While I prefer using $() in my scripts, this looks like needless
churn.  The diff doesn't simplify a single instance of nested ``-style
command substitutions.

> Regarding numeric changes: in ksh, [[ 0 -eq 00 ]] is true (numeric
> comparison) but [[ 0 = 00 ]] is false (string comparison). The latter
> is more readable,

This sounds like a personal preference.  I prefer to use numeric
operators when dealing with numbers.

> so I made the switch in places where the left-hand
> side is a system output, e.g. [[ $(id -u ) = 0 ]] or [[ $# > 0 ]].
> I would not expect id(1) to print the string "00" for root, nor would
> I expect the shell to print "00" for $#. But I did *not* change
> comparison operators in more complicated situations, like the bottom
> of /etc/rc which has [[ ${daemon_rtable} -eq 0 ]]. It isn't immediately
> obvious what sort of string is in ${daemon_rtable} so the numeric
> comparison seems appropriate here.
>
> I corrected a comment at the top of /etc/rc; it said that "set +o sh"
> turned Strict Bourne shell mode off. I'm pretty sure it's actually
> turning strict mode on.

Nope, ''set -o sh'' sets strict mode on, ''set +o sh'' disables it.

[...]

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: Make rc scripts use [[ instead of [

[ludovic coues]

2016-09-05 20:11 GMT+02:00  :
> The fact that one particular system still boots is not "good enough here".
>
> Kind regards,
> Anton
>

That's why he share his work. So other people can try on other system.

-- 

Cordialement, Coues Ludovic
+336 148 743 42

Make rc scripts use [[ instead of [

[Anthony Coulter]

Some of the system scripts make inconsistent use of ksh-specific
features, specifically "print" and "[[" as efficient replacements for
"echo" and "[". This change makes the /etc/ksh.kshrc and all the rc
scripts use "print" and "[[" exclusively.

Switching from echo to print only brought up one interesting issue:
/etc/rc.d/mountd uses "print -n >foo" instead of "touch foo".
The latter is arguably more transparent but I didn't change to it.

Switching from [ to [[ makes a lot of things more readable; most of
the improvements result from removing unnecessary quotation marks.
We also don't have to test for unset variables as aggressively; a
number of awkward [ X"$foo" = XYes ] constructions were replaced
with [[ $foo = Yes ]] .

The test [[ ${_args} = "${_args# }" ]] appears in rcctl; I left the
quotation marks on the right-hand side because I'm afraid of breaking
something I don't understand.

The rcctl script also has a lot of lines like:
daemon_timeout="$(svc_getdef ${_svc} timeout)"
I think the quotation marks are unnecessary but the original author
went to a lot of trouble to escape internal quotes, e.g.
daemon_flags="$(eval print \"\${${_svc}_flags}\")"
so I'll give him the benefit of the doubt. I would encourage somone
with greater confidence to take a closer look.

I replaced `backticks` with $(subshell notation) in a few places, but
only in places where I was already changing [ to [[. I wanted to remove
them all but I can't really justify the change in terms of either
readability or process efficiency.

Regarding numeric changes: in ksh, [[ 0 -eq 00 ]] is true (numeric
comparison) but [[ 0 = 00 ]] is false (string comparison). The latter
is more readable, so I made the switch in places where the left-hand
side is a system output, e.g. [[ $(id -u ) = 0 ]] or [[ $# > 0 ]].
I would not expect id(1) to print the string "00" for root, nor would
I expect the shell to print "00" for $#. But I did *not* change
comparison operators in more complicated situations, like the bottom
of /etc/rc which has [[ ${daemon_rtable} -eq 0 ]]. It isn't immediately
obvious what sort of string is in ${daemon_rtable} so the numeric
comparison seems appropriate here.

I corrected a comment at the top of /etc/rc; it said that "set +o sh"
turned Strict Bourne shell mode off. I'm pretty sure it's actually
turning strict mode on.

My system still boots with these changes, and rcctl still appears to
work. I can't easily test all of the code paths in each of the rc
scripts. But the changes involved here are straightforward, and you
can validate this diff line-by-line.


Index: etc/ksh.kshrc
===
RCS file: /open/anoncvs/cvs/src/etc/ksh.kshrc,v
retrieving revision 1.20
diff -u -p -u -r1.20 ksh.kshrc
--- etc/ksh.kshrc   18 Feb 2015 08:39:32 -  1.20
+++ etc/ksh.kshrc   5 Sep 2016 04:03:43 -
@@ -62,7 +62,7 @@ case "$-" in
case "$TERM" in
sun*-s)
# sun console with status line
-   if [ "$tty" != "$console" ]; then
+   if [[ $tty != $console ]]; then
# ilabel
ILS='\033]L'; ILE='\033\\'
# window title bar
@@ -81,7 +81,7 @@ case "$-" in
*)  ;;
esac
# do we want window decorations?
-   if [ "$ILS" ]; then
+   if [[ -n $ILS ]]; then
function ilabel { print -n "${ILS}$*${ILE}">/dev/tty; }
function label { print -n "${WLS}$*${WLE}">/dev/tty; }
 
@@ -136,14 +136,14 @@ function no_path {
 }
 # if $1 exists and is not in path, append it
 function add_path {
-  [ -d ${1:-.} ] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
+  [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
 }
 # if $1 exists and is not in path, prepend it
 function pre_path {
-  [ -d ${1:-.} ] && no_path $* && eval ${2:-PATH}="$1:\$${2:-PATH}"
+  [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="$1:\$${2:-PATH}"
 }
 # if $1 is in path, remove it
 function del_path {
-  no_path $* || eval ${2:-PATH}=`eval echo :'$'${2:-PATH}: |
+  no_path $* || eval ${2:-PATH}=`eval print :'$'${2:-PATH}: |
 sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;"`
 }
Index: etc/rc
===
RCS file: /open/anoncvs/cvs/src/etc/rc,v
retrieving revision 1.486
diff -u -p -u -r1.486 rc
--- etc/rc  10 Jul 2016 09:08:18 -  1.486
+++ etc/rc  4 Sep 2016 14:26:10 -
@@ -4,7 +4,7 @@
 # Output and error are redirected to console by init, and the console is the
 # controlling terminal.
 
-# Turn off Strict Bourne shell.
+# Turn on Strict Bourne shell.
 set +o sh
 
 # Subroutines (have to come first).
@@ -137,14 +137,14 @@ make_keys() {
local _iked_pub=/etc/iked/local.pub
 
if [[ ! -f $_isakmpd_key ]]; then
-   echo -n "openssl: generating isakmpd/iked RSA keys... "
+   print -n "openssl: