sh coding style

johnbock · 27 Mrz 2008

While rummaging through the sh code of freetz I've repeatedly seen the following constructs

Code:

cat /mod/etc/reg/cgi.reg | grep -v "^$2|" > $TMPFILE

and

Code:

[ -e "/mod/etc/reg/file.reg" ] || touch /mod/etc/reg/file.reg
[ -r "/mod/etc/default.$package/$package.save" ] && . /mod/etc/default.$package/$package.save

Both are obviously syntactically correct.
However, the former is overly complex and loads the cpu needlessly. I suggest using

Code:

grep -v "^$2|" /mod/etc/reg/cgi.reg > $TMPFILE

The latter is inconsistent and inconsequent: if the command using the same file as a prerequisite test requires no double quotes then too the test. And if the test requires double quotes then too the file in the subsequent command. And I suggest

Code:

[ -e /mod/etc/reg/file.reg ] || touch /mod/etc/reg/file.reg
[ -r "/mod/etc/default.$package/$package.save" ] && . "/mod/etc/default.$package/$package.save"

Concurrently, a rule-of-thumb for curly bracing environment variables should also be established. I suggest curly bracing env vars when they are adjacent to characters not in the default IFS definition.

Additionally, I would like to suggest using the following link as a basis for sh coding style. This is mostly inline with sh coding style in practice.

Thank you for reading this post and please do not take this critical insight personally

johnbock · 28 Mrz 2008

indexed environment variables

Freetz modules justly use 'indexed' environment variables to assign values to configuration sets. However, if not implemented properly these can easily become unwieldly and obtrusive. Consider the following

Code:

        if [ "$INADYN_ACTIVE0" = 'yes' ]; then
 	                configured=$configured+'.'
 	                OPTIONS="$OPTIONS --dyndns_system"
 	                case "$INADYN_SERVICE0" in
 	                        dyndns.org) OPTIONS="$OPTIONS [email protected]" ;;
 	                        dyndns.org-statdns) OPTIONS="$OPTIONS [email protected]" ;;
 	                        dyndns.org-custom) OPTIONS="$OPTIONS [email protected]" ;;
 	                        afraid.org) OPTIONS="$OPTIONS [email protected]" ;;
 	                        zoneedit.com) OPTIONS="$OPTIONS [email protected]" ;;
 	                        no-ip.com) OPTIONS="$OPTIONS [email protected]" ;;
 	                        benutzerdefiniert) OPTIONS="$OPTIONS custom@http_svr_basic_auth" ;;
 	                esac
 	                if [ ! -z "$INADYN_USER0" ]; then
 	                        OPTIONS="$OPTIONS --username $INADYN_USER0"
 	                fi
 	                if [ ! -z "$INADYN_PASS0" ]; then
 	                        OPTIONS="$OPTIONS --password $INADYN_PASS0"
 	                fi
 	                if [ ! -z "$INADYN_ALIAS0" ]; then
 	                        OPTIONS="$OPTIONS --alias $INADYN_ALIAS0"
 	                fi
 	                if [ ! -z "$INADYN_OPTIONS0" ]; then
 	                        OPTIONS="$OPTIONS $INADYN_OPTIONS0"
 	                fi
 	                echo "$OPTIONS" > /mod/etc/inadyn.conf
 	        fi

Repeating this schema a few times makes the resulting code unusually hard to maintain. Introducing a little sh function solving a variable for an index, one can easily simplify the overall code.

Code:

ivalueof () {
   eval "echo -n \$${1}${2}"
}

Now applying the sh function to the above snippet, one obtains

Code:

   ii=0
   # inadyn -> dyndns.h can maintain maximal 5 servers
   while [ 5 -gt $ii ]; do
      if [ yes = "`ivalueof INADYN_ACTIVE $ii`" ]; then

         case "`ivalueof INADYN_SERVICE $ii`" in
            dyndns.org)
               echo -e '\ndyndns_system [email protected]'
               ;;
            dyndns.org-statdns)
               echo -e '\ndyndns_system [email protected]'
               ;;
            dyndns.org-custom)
               echo -e '\ndyndns_system [email protected]'
               ;;
            afraid.org)
               echo -e '\ndyndns_system [email protected]'
               ;;
            zoneedit.com)
               echo -e '\ndyndns_system [email protected]'
               ;;
            no-ip.com)
               echo -e '\ndyndns_system [email protected]'
               ;;
            custom|benutzerdefiniert)
               value=`ivalueof INADYN_URL $ii | sed 's,\([^/]*\)\(.*\),\1 \2,'`
               [ -z "$value" ] && continue
               echo -e '\ndyndns_system custom@http_svr_basic_auth'
               echo "dyndns_server_name `echo $value | cut -d\  -f1`"
               echo "dyndns_server_url `echo $value | cut -d\  -f2`"
               ;;
         esac

         value=`ivalueof INADYN_USER $ii`
         [ -n "$value" ] && echo "username $value"
         value="`ivalueof INADYN_PASS $ii`"
         [ -n "$value" ] && echo "password `escvalue $value`"
         for value in `ivalueof INADYN_ALIAS $ii`; do
            echo "alias $value"
         done
         value=`ivalueof INADYN_OPTIONS $ii`
         [ -n "$value" ] && echo "$value"
      fi
      let ii=${ii}+1
   done

The length of the code albeit the format is just a little longer and is much, much easier to maintain over all configuration sets. Please note, the length of the new implementation is longer partially because of the fomatting style in the 'case-esac' block and the code in the while loop is essentially the same and should be taken as an example.

One could argue: the introduction of the function burdens the system more. While being true, the maintainability grossly outweighs the minimal additional cpu stress.

And as I have written in the first thread:
Thank you for reading this post and please do not take this critical insight personally

McNetic · 28 Mrz 2008

I can not speak for the Freetz developers, but I agree with you in every respect. In my opinion, yes, Freetz should have a general sh coding style (with the page referenced by you being a good starting point), and yes, in many places sh code is overly complex, bad to read, inefficient or all three

(as I do not have much sh coding experience, my own code may be the worst).

I think it would be very good if someone with more experience with sh coding would work on this.

olistudent · 28 Mrz 2008

This sounds good to me too. I'm not very experienced in writing shell scripts. I will try to respect your advices in future. Feel free to attach patches when you find code snippets like in inadyn...

Greets Oliver

edit: Please open a ticket with these issues. I have attached a first try. Please have a look at it.

johnbock · 29 Mrz 2008

@olistudent

Your patch looks good. There is only one more thing that's been bothering me though...

Code:

for pkg in $(cat /etc/static.pkg); do
( cat $TMPFILE; echo "$2|$3" ) | sort > /mod/etc/reg/cgi.reg

Both create subshells. The following constructs are executed in the current shell

Code:

for pkg in `cat /etc/static.pkg`; do
{ cat $TMPFILE; echo "$2|$3"; } | sort > /mod/etc/reg/cgi.reg

One should be aware though: the command in curly braces '{}' will add any environment variables defined to the current shell where as a subshell '$()' will not. For example:

Code:

{ this_var="something"; }
$( that_var="the other thing" )

The environment variable 'this_var' will be propagated to the current sh and 'that_var' not.

olistudent · 29 Mrz 2008

Who double checks the changeset? :mrgreen:
r2054

There are still 82 .cgi files with 'value="$(httpd ...)"' to replace.

Greets Oliver

buehmann · 29 Mrz 2008

johnbock schrieb:
Code:

for pkg in $(cat /etc/static.pkg); do

Both create subshells. The following constructs are executed in the current shell

Code:

for pkg in `cat /etc/static.pkg`; do

Excuse me, but if I'm not completely mistaken, there is no difference whatsoever regarding the subshell behaviour of `` and $(). Both execute the command in a subshell environment: See POSIX, for example.

The only difference lies in parsing: $() can be nested easily, whereas with the old-style `` you have to be careful to get the escaping right.

In my opinion, the new $() is more readable in almost any situation, and I would suggest using it exclusively in the Freetz code.

Regards,

Andreas

@Oliver: I am going to take a look at the changeset.

buehmann · 30 Mrz 2008

Ok, here's my review:

Code:

"$(pidof "$DAEMON")"
`pidof "$DAEMON"`

The double quotes enclosing the command substitution have been dropped; this will lead to errors whenever pidof returns more than one (unlikely with daemons but possible) or zero words (daemon not running!). (Of course, the double quotes surrounding $DAEMON are superfluous if $DAEMON can be guaranteed not to contain spaces; this is especially true in cases like these: )

Code:

 if [ -z "$(pidof "cifsoplockd")" -a -z "$(pidof "cifsdnotifyd")" ]; then

->

Code:

 if [ -z "$(pidof cifsoplockd)" -a -z "$(pidof cifsdnotifyd)" ]; then

Further unnecessary quotes:

Code:

 if [ ! -r "/mod/etc/conf/cifsmount.cfg" ]; then
if [ "$exitval" -eq 0 ]; then
if [ "$CIFSMOUNT_ENABLED" != "yes" ]; then

("yes" in the third case)

Code:

if [ $QUERY_STRING ]; then

in downlog.cgi is not robust and should be replaced with

Code:

if [ -n "$QUERY_STRING" ]; then

The same goes for

Code:

if [ "$_cgi_width" ]; then

Code:

httpd -e "$(cat $DOWNLOGFILE)"
httpd -e `cat "$DOWNLOGFILE"`

The outer double quotes are important (otherwise httpd will only process the first word in $DOWNLOGFILE). There are many instances of this mistake. (And if $DOWNLOGFILE might contain spaces: )

Code:

httpd -e "$(cat "$DOWNLOGFILE")"

Code:

if [ $(pidof smbd|wc -w) -gt 0 ]; then

An unnecessary pipe; this can be boiled down to one of

Code:

if [ -n "$(pidof smbd)" ]; then
if pidof smbd > /dev/null; then

Code:

[ -r /tmp/flash/tinyproxy_conf.def ] && deffile='/tmp/flash/tinyproxy_conf.def'

Single quotes could be removed

Code:

        let size="$(wc -c < $TMPFILE)"
        let size={ wc -c < $TMPFILE; }

The new version is a syntax error.

Code:

$(lang de:"Firmware" en:"Firmware"): $(get_env 'firmware_info')$(cat /etc/.subversion)<br>
$(lang de:"Firmware" en:"Firmware"): `get_env 'firmware_info'` `cat /etc/.subversion`<br>

A space character has been introduced (in front of the subversion); intention?

olistudent · 30 Mrz 2008

Thank you for reviewing the code though I'm not happy about this answer. ;-)

Greets Oliver

johnbock · 30 Mrz 2008

I am very happy that you guys are taking this subject seriously

And very glad that you have taken the initiative.

However, who is going to be brave enough to test the changes thoroughly? I would have preferred making the changes slowly, module for module. This would have been less of an ordeal. And much more easier to keep track of things.

@buehmann
I'm sorry your right... Both '$()' and '``' do generate subshells. The only difference is that the commands between the parenthesis will not be interpreted in the current sh whereas the commands between backquotes will be. I find the backquotes example easier to read. Although there are times when parenthesis are justified.

johnbock · 30 Mrz 2008

buehmann schrieb:
Ok, here's my review:

Code:

let size="$(wc -c < $TMPFILE)" let size={ wc -c < $TMPFILE; }

The second example is not inline with the codeing-style and I would prefer the following

Code:

size=`wc -c <$TMPFILE`

and if there's even a slight chance that $TMPFILE is empty then something like the following is best

Code:

size=`wc -c <${TMPFILE:-/dev/null}`

olistudent · 30 Mrz 2008

buehmann schrieb:
Code:

httpd -e "$(cat $DOWNLOGFILE)" httpd -e `cat "$DOWNLOGFILE"`

The outer double quotes are important (otherwise httpd will only process the first word in $DOWNLOGFILE). There are many instances of this mistake. (And if $DOWNLOGFILE might contain spaces: )

Code:

httpd -e "$(cat "$DOWNLOGFILE")"

Code:

@@ -17,5 +17,5 @@
             echo "<h1>$SYSLOGD_ALTERNATIVE_LOGFILE</h1>"
             echo -n '<textarea style="width: '$_width'px;" name="content" rows="30" cols="10" wrap="off" readonly>'
-            httpd -e "$(cat $SYSLOGD_ALTERNATIVE_LOGFILE)"
+            httpd -e `cat $SYSLOGD_ALTERNATIVE_LOGFILE`
             echo -n '</textarea>'

This was one of the few changes I tested out and it worked for me.

Greets Oliver

buehmann · 30 Mrz 2008

@Oliver: I tried something like the following to check the behaviour:

Code:

/var/mod/root # echo $(httpd -e "$(echo foo bar baz)")
[noparse]foo bar baz[/noparse]
/var/mod/root # echo $(httpd -e `echo foo bar baz`)   
foo
/var/mod/root # echo $(httpd -e "`echo foo bar baz`")
[noparse]foo bar baz[/noparse]

@johnbock:

johnbock schrieb:
Both '$()' and '``' do generate subshells. The only difference is that the commands between the parenthesis will not be interpreted in the current sh whereas the commands between backquotes will be.

Hmm, that sounds like a contradiction. (What exactly do you mean by "interpreted in the current shell"?) Could it be that you are confusing $() with ()?

`` and $() both create subshells, so the nested commands won't have any effect on the outside:

Code:

/var/mod/root # foo=1
/var/mod/root # echo `foo=2; echo $foo`
2
/var/mod/root # echo $foo
1
/var/mod/root # echo $(foo=2; echo $foo)
2
/var/mod/root # echo $foo
1

Of course, there is the difference of () vs. {}, but those do not perform command substitution but only grouping:

Code:

/var/mod/root # echo $foo
1
/var/mod/root # (foo=2; echo $foo) 
2
/var/mod/root # echo $foo
1
/var/mod/root # { foo=3; echo $foo; }
3
/var/mod/root # echo $foo
3

Regards,

Andreas

johnbock · 30 Mrz 2008

buehmann schrieb:
@Oliver: I tried something like the following to check
@johnbock:

Hmm, that sounds like a contradiction. (What exactly do you mean by "interpreted in the current shell"?) Could it be that you are confusing $() with ()?

Code:

`cat ${this_var}`
$(cat ${this_var})

The example in the first line AFAIK will first solve '${this_var}' and then execute the command and the second line will solve '${this_var}' in the subshell.

man bash schrieb:
When the old-style backquote form of substitution is used, backslash retains its
literal meaning except when followed by $, ‘, or \. The first backquote not
preceded by a backslash terminates the command substitution. When using the
$(command) form, all characters between the parentheses make up the command;
none are treated specially.

man dash schrieb:
The shell expands the command substitution by executing command in a subshell
environment and replacing the command substitution with the standard output of the
command, removing sequences of one or more 〈newline〉s at the end of the substitu‐
tion. (Embedded 〈newline〉s before the end of the output are not removed; however,
during field splitting, they may be translated into 〈space〉s, depending on the
value of IFS and quoting that is in effect.)

not sure what I should make of that

RalfFriedl · 30 Mrz 2008

Could you give an example where this really matters, assuming it is correct?

The bash manual doesn't mention a semantic difference between the two forms.

Also consider this example:

Code:

$ x=1; echo `x=2; echo $x` $x
2 1
$ x=1; echo $(x=2; echo $x) $x
2 1

Obviously the inner $x is interpreted in the subshell in both cases, otherwise it would not show the correct value.

McNetic · 30 Mrz 2008

RalfFriedl schrieb:
Code:

$ x=1; echo `x=2; echo $x` $x 2 1 $ x=1; echo $(x=2; echo $x) $x 2 1

I think this is the example johnbock meant, and it proves him wrong, as according to him, the first example should print '1 1'. Correct me if I'm wrong, but if in `-Style, variables would be expanded before execution of subshell, the subshell itself could not have any variables?

If there is really no difference between the two styles, I would definetely prefer the new style ($()), as it is more readable to me (and there has to be reason it is the new style).

Nico

johnbock · 30 Mrz 2008

McNetic schrieb:
I think this is the example johnbock meant, and it proves him wrong, as according to him, the first example should print '1 1'. Correct me if I'm wrong, but if in `-Style, variables would be expanded before execution of subshell, the subshell itself could not have any variables?

If there is really no difference between the two styles, I would definetely prefer the new style ($()), as it is more readable to me (and there has to be reason it is the new style).

Nico

accepted and accepted

That would mean

Code:

x=$(cat < /tmp/somefile)

is the preferred style unless of course doublequotes are required

Code:

x="$(cat < /tmp/somefile)"

Is that o.k. for everyone?

buehmann · 31 Mrz 2008

johnbock schrieb:
That would mean

Code:

x=$(cat < /tmp/somefile)

is the preferred style unless of course doublequotes are required

Code:

x="$(cat < /tmp/somefile)"

Is that o.k. for everyone?

BTW, the first variant can be used in any case, because word splitting and pathname expansion are not performed in variable assignments.

For the same reason, one can always write

Code:

x=$y

without having to worry about spaces or filename patterns in the expansion of $y.

Andreas

johnbock · 31 Mrz 2008

o.k. bad example

unless of course doublequotes are necessary

Code:

if [ -n "$(cat < /tmp/somefile)" ]; then

cuma · 31 Mrz 2008

BUT, you don't need it here:

Code:

[  -z $MYVAR  ] &&  echo zero

^^

sh coding style

Mitglied

Mitglied

Mitglied

IPPF-Urgestein

Anhänge

Mitglied

IPPF-Urgestein

Aktives Mitglied

Aktives Mitglied

IPPF-Urgestein

Mitglied

Mitglied

IPPF-Urgestein

Aktives Mitglied

Mitglied

IPPF-Urgestein

Mitglied

Mitglied

Aktives Mitglied

Mitglied

Aktives Mitglied

Statistik des Forums