On Mon, Jan 10, 2011 at 2:09 PM, Pawel Jakub Dawidek <p...@freebsd.org> wrote:
> On Mon, Jan 10, 2011 at 07:11:26PM +0000, Josh Paetzel wrote:
> [...]
>> while read line
>> do
>> # Check for data on this slice
>> - echo $line | grep "^${DISKTAG}-part=" >/dev/null 2>/dev/null
>> + echo $line | grep "^${_dTag}-part=" >/dev/null 2>/dev/null
>
> You can just use 'grep -q' instead of redirecting grep's output to
> /dev/null.
>
>> if [ "$?" = "0" ]
>
> This will work, but more elegant way is [ $? -eq 0 ] - there is no need
> to convert exit code to string.
I've seen people claim that the two items above are to make shell code
`more accessible' and `more portable' to developers, but I agree that
in this case you're not gaining much but instead just dumbing down
your audience.
>> + FOUNDROOT="1" ; export FOUNDROOT
>
> 'export FOUNDROOT="1"' should work too.
`export FOUNDROOT=1' will do.
>> + if [ "${FS}" != "UFS" -a "${FS}" != "UFS+S" -a "${FS}" != "UFS+J"
>> -a "${FS}" != "UFS+SUJ" ] ; then
>
> Something like this should work too:
>
> if [ "${FS%+*}" != "UFS" ]; then
Except they're catching less than that:
$ FS=UFS+FOO
$ echo ${FS%+*}
UFS
$
A case statement would be easier to digest:
case "$FS" in
UFS|UFS+J|UFS+S|UFS+SUJ)
# Do something here.
;;
esac
>> + dd if=/dev/zero of=${_pDisk}p${CURPART} count=2048 >/dev/null
>> 2>/dev/null
>
> If you specify 'of=' there is no need to redirect standard output to
> /dev/null, as it is already redirected somewhere else.
+1. Please keep the 2>/dev/null though because that's noise from the
summary output.
>> + if [ ! -z "${ENCPASS}" ] ; then
>
> '[ ! -z "{str}" ]' is equivalent of '[ -n "${str}" ]'.
>
>> + CURPART="`expr ${CURPART} + 1`"
>
> Simpler: CURPART=$((CURPART+1))
Or `: $(( CURPART += 1 ))'
>> + if [ "$?" != "0" ] ; then return ; fi
>
> [ $? -eq 0 ] || return
if [ $? -eq 0 ]; then
return
fi
is easier to follow for me because more people go buckwild with the
one-liners (and in some cases have introduced bugs that way because
they didn't properly think about precedence of the operations, etc).
The one-line above if ... fi above is ugly though.
>> - rc_halt "gpart add -b 34 -s 128 -t freebsd-boot ${_intDISK}"
>> + rc_halt "gpart add -b 34 -s 64 -t freebsd-boot ${_intDISK}"
>
> Gptzfsboot in HEAD is 27463 bytes. Gptzfsboot in ZFSv28 is 29659, so
> using 64 sectors leaves only 3109 bytes for it to grow.
> Note that, eg. RAIDZ3 support is not yet implemented and I expect it
> might be not be enough place left to implement it if you do that.
>
> PS. Only because those are shell scripts doesn't mean style is not
> important. They could really be easier to read if they follow style used
> in rcNG.
Thanks,
-Garrett
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
