Date: Mon, 10 Jan 2011 15:52:12 -0800 From: Garrett Cooper <gcooper@FreeBSD.org> To: Pawel Jakub Dawidek <pjd@freebsd.org> Cc: Josh Paetzel <jpaetzel@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r217229 - head/usr.sbin/pc-sysinstall/backend Message-ID: <AANLkTimewVu2s5rCoJtD7E5d_tjFsoGHvuHb7rqNpsEO@mail.gmail.com> In-Reply-To: <20110110220957.GB1923@garage.freebsd.pl> References: <201101101911.p0AJBQKG090310@svn.freebsd.org> <20110110220957.GB1923@garage.freebsd.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jan 10, 2011 at 2:09 PM, Pawel Jakub Dawidek <pjd@freebsd.org> wrot=
e:
> On Mon, Jan 10, 2011 at 07:11:26PM +0000, Josh Paetzel wrote:
> [...]
>> =A0 =A0while read line
>> =A0 =A0do
>> =A0 =A0 =A0# Check for data on this slice
>> - =A0 =A0echo $line | grep "^${DISKTAG}-part=3D" >/dev/null 2>/dev/null
>> + =A0 =A0echo $line | grep "^${_dTag}-part=3D" >/dev/null 2>/dev/null
>
> You can just use 'grep -q' instead of redirecting grep's output to
> /dev/null.
>
>> =A0 =A0 =A0if [ "$?" =3D "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.
>> + =A0 =A0 =A0 =A0FOUNDROOT=3D"1" ; export FOUNDROOT
>
> 'export FOUNDROOT=3D"1"' should work too.
`export FOUNDROOT=3D1' will do.
>> + =A0 =A0 =A0 =A0if [ "${FS}" !=3D "UFS" -a "${FS}" !=3D "UFS+S" -a "${F=
S}" !=3D "UFS+J" -a "${FS}" !=3D "UFS+SUJ" ] ; then
>
> Something like this should work too:
>
> =A0 =A0 =A0 =A0if [ "${FS%+*}" !=3D "UFS" ]; then
Except they're catching less than that:
$ FS=3DUFS+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
>> + =A0 =A0 =A0 =A0dd if=3D/dev/zero of=3D${_pDisk}p${CURPART} count=3D204=
8 >/dev/null 2>/dev/null
>
> If you specify 'of=3D' 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.
>> + =A0 =A0 =A0 =A0if [ ! -z "${ENCPASS}" ] ; then
>
> '[ ! -z "{str}" ]' is equivalent of '[ -n "${str}" ]'.
>
>> + =A0 =A0 =A0 =A0CURPART=3D"`expr ${CURPART} + 1`"
>
> Simpler: CURPART=3D$((CURPART+1))
Or `: $(( CURPART +=3D 1 ))'
>> + =A0if [ "$?" !=3D "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.
>> - =A0rc_halt "gpart add -b 34 -s 128 -t freebsd-boot ${_intDISK}"
>> + =A0rc_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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTimewVu2s5rCoJtD7E5d_tjFsoGHvuHb7rqNpsEO>