Hi Roland, Thanks for taking the time to look at this. I evidently missed some discussion over the weekend. In general, I share the viewpoint that self-consistency within a file usually outweighs style guidelines when making small changes to an existing file. That said, I've accepted some of your comments below since bfu.sh is inconsistent already its shell style in many respects. :-) I've re-generated the webrev to reflect the changes discussed below:
Roland Mainz wrote: > Some nitpicking based on > http://www.opensolaris.org/os/project/shell/shellstyle/ ("bfu.sh" is a > ksh script, follow the colored tags labelled with "ksh" in that > document): > - Plese use "if [[ ... ]]" instead of "if [ ... ]", e.g. > turn > -- snip -- > + if [ -d $rootprefix/var/sadm/pkg/SUNWmipr ]; then > -- snip -- > into > + if [[ -d $rootprefix/var/sadm/pkg/SUNWmipr ]]; then > -- snip -- I've made this change. > - Nitpicking: Please use "function foo" functions, not Bourne-style > "foo()", e.g. > turn > -- snip -- > +remove_eof_mobileip() { > + typeset -r mip_pkgs='SUNWmipr SUNWmipu' > + typeset pkg > -- snip -- > into > -- snip -- > +function remove_eof_mobileip { > + typeset -r mip_pkgs='SUNWmipr SUNWmipu' > + typeset pkg > -- snip -- > (see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_ksh_style_function_syntax > for an explanation of the background) Since there are no function definitions using this style in bfu.sh, I'll leave the code the way it is. If there's a need to transition to the preferred "function foo" ksh style, it will be simple enough to do the entire file at once instead of piece-meal. > - It may be usefull to put quotes around the "rm" calls to avoid that > "rm" may run amok when ${rootprefix} returns garbage (e.g. contains > spaces etc.), e.g. > turn > -- snip -- > + rm $rootprefix/etc/inet/mipagent.conf* > + rm $rootprefix/etc/init.d/mipagent > + rm $rootprefix/etc/rc0.d/K06mipagent > + rm $rootprefix/etc/rc1.d/K06mipagent > + rm $rootprefix/etc/rc2.d/K06mipagent > + rm $rootprefix/etc/rc3.d/S80mipagent > + rm $rootprefix/etc/rcS.d/K06mipagent > + rm $rootprefix/etc/snmp/conf/mipagent* > -- snip -- > into > -- snip -- > + rm "${rootprefix}/etc/inet/mipagent.conf"* > + rm "${rootprefix}/etc/init.d/mipagent" > + rm "${rootprefix}/etc/rc0.d/K06mipagent" > + rm "${rootprefix}/etc/rc1.d/K06mipagent" > + rm "${rootprefix}/etc/rc2.d/K06mipagent" > + rm "${rootprefix}/etc/rc3.d/S80mipagent" > + rm "${rootprefix}/etc/rcS.d/K06mipagent" > + rm "${rootprefix}/etc/snmp/conf/mipagent"* > -- snip -- I've added quotes as suggested; this makes sense. > - Just curious: What's the reason for making some variables like > "mip_pkgs" read-only ("typeset -r ..."), e.g. > -- snip -- > + typeset -r mip_pkgs='SUNWmipr SUNWmipu' > -- snip -- > ? There's no specific reason, but it's also not meant to be modified once set, so I don't see this as a problem. I was also following the convention used by other, similar, functions like remove_perl_500503, remove_eof_wildcat, remove_eof_aset, etc. Thanks, -Seb _______________________________________________ networking-discuss mailing list [email protected]
