.titleBar { margin-bottom: 5px!important; }

sh coding style

Dieses Thema im Forum "Freetz" wurde erstellt von johnbock, 27 März 2008.

  1. johnbock

    johnbock Mitglied

    Registriert seit:
    2 März 2008
    Beiträge:
    310
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    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;)
     
  2. johnbock

    johnbock Mitglied

    Registriert seit:
    2 März 2008
    Beiträge:
    310
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    #2 johnbock, 28 März 2008
    Zuletzt bearbeitet: 28 März 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 dyndns@dyndns.org" ;;
     	                        dyndns.org-statdns) OPTIONS="$OPTIONS statdns@dyndns.org" ;;
     	                        dyndns.org-custom) OPTIONS="$OPTIONS custom@dyndns.org" ;;
     	                        afraid.org) OPTIONS="$OPTIONS default@freedns.afraid.org" ;;
     	                        zoneedit.com) OPTIONS="$OPTIONS default@zoneedit.com" ;;
     	                        no-ip.com) OPTIONS="$OPTIONS default@no-ip.com" ;;
     	                        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 dyndns@dyndns.org'
                   ;;
                dyndns.org-statdns)
                   echo -e '\ndyndns_system statdns@dyndns.org'
                   ;;
                dyndns.org-custom)
                   echo -e '\ndyndns_system custom@dyndns.org'
                   ;;
                afraid.org)
                   echo -e '\ndyndns_system default@freedns.afraid.org'
                   ;;
                zoneedit.com)
                   echo -e '\ndyndns_system default@zoneedit.com'
                   ;;
                no-ip.com)
                   echo -e '\ndyndns_system default@no-ip.com'
                   ;;
                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;)
     
  3. McNetic

    McNetic Mitglied

    Registriert seit:
    7 Feb. 2007
    Beiträge:
    674
    Zustimmungen:
    0
    Punkte für Erfolge:
    16
    Ort:
    Aachen
    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.
     
  4. olistudent

    olistudent IPPF-Urgestein

    Registriert seit:
    19 Okt. 2004
    Beiträge:
    14,761
    Zustimmungen:
    5
    Punkte für Erfolge:
    38
    Beruf:
    Softwareentwickler
    Ort:
    Kaiserslautern
    #4 olistudent, 28 März 2008
    Zuletzt bearbeitet: 28 März 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.
     

    Anhänge:

  5. johnbock

    johnbock Mitglied

    Registriert seit:
    2 März 2008
    Beiträge:
    310
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    @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.
     
  6. olistudent

    olistudent IPPF-Urgestein

    Registriert seit:
    19 Okt. 2004
    Beiträge:
    14,761
    Zustimmungen:
    5
    Punkte für Erfolge:
    38
    Beruf:
    Softwareentwickler
    Ort:
    Kaiserslautern
    Who double checks the changeset? :mrgreen:
    r2054


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

    Greets Oliver
     
  7. buehmann

    buehmann Aktives Mitglied

    Registriert seit:
    11 Juni 2005
    Beiträge:
    1,810
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    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.
     
  8. buehmann

    buehmann Aktives Mitglied

    Registriert seit:
    11 Juni 2005
    Beiträge:
    1,810
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    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?
     
  9. olistudent

    olistudent IPPF-Urgestein

    Registriert seit:
    19 Okt. 2004
    Beiträge:
    14,761
    Zustimmungen:
    5
    Punkte für Erfolge:
    38
    Beruf:
    Softwareentwickler
    Ort:
    Kaiserslautern
    Thank you for reviewing the code though I'm not happy about this answer. ;-)

    Greets Oliver
     
  10. johnbock

    johnbock Mitglied

    Registriert seit:
    2 März 2008
    Beiträge:
    310
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    I am very happy that you guys are taking this subject seriously:eek: 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.
     
  11. johnbock

    johnbock Mitglied

    Registriert seit:
    2 März 2008
    Beiträge:
    310
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    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}`
     
  12. olistudent

    olistudent IPPF-Urgestein

    Registriert seit:
    19 Okt. 2004
    Beiträge:
    14,761
    Zustimmungen:
    5
    Punkte für Erfolge:
    38
    Beruf:
    Softwareentwickler
    Ort:
    Kaiserslautern
    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
     
  13. buehmann

    buehmann Aktives Mitglied

    Registriert seit:
    11 Juni 2005
    Beiträge:
    1,810
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    @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:
    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
     
  14. johnbock

    johnbock Mitglied

    Registriert seit:
    2 März 2008
    Beiträge:
    310
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    #14 johnbock, 30 März 2008
    Zuletzt bearbeitet: 30 März 2008
    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.


    not sure what I should make of that
     
  15. RalfFriedl

    RalfFriedl IPPF-Urgestein

    Registriert seit:
    22 Apr. 2007
    Beiträge:
    12,343
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    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.
     
  16. McNetic

    McNetic Mitglied

    Registriert seit:
    7 Feb. 2007
    Beiträge:
    674
    Zustimmungen:
    0
    Punkte für Erfolge:
    16
    Ort:
    Aachen
    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
     
  17. johnbock

    johnbock Mitglied

    Registriert seit:
    2 März 2008
    Beiträge:
    310
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    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?
     
  18. buehmann

    buehmann Aktives Mitglied

    Registriert seit:
    11 Juni 2005
    Beiträge:
    1,810
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    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
     
  19. johnbock

    johnbock Mitglied

    Registriert seit:
    2 März 2008
    Beiträge:
    310
    Zustimmungen:
    0
    Punkte für Erfolge:
    0
    o.k. bad example

    unless of course doublequotes are necessary
    Code:
    if [ -n "$(cat < /tmp/somefile)" ]; then
     
  20. cuma

    cuma Aktives Mitglied

    Registriert seit:
    16 Dez. 2006
    Beiträge:
    2,743
    Zustimmungen:
    0
    Punkte für Erfolge:
    36
    BUT, you don't need it here:
    Code:
    [  -z $MYVAR  ] &&  echo zero
    
    ^^