svn commit: r217229 - head/usr.sbin/pc-sysinstall/backend

[Garrett Cooper]

On Mon, Jan 10, 2011 at 2:09 PM, Pawel Jakub Dawidek <pjd at 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