Thanks Roland. I'll merge it in in the AM.
Joe Roland Mainz wrote: > Joseph J VLcek wrote: >> Joseph J VLcek wrote: > [snip] >> The latest webrev is at >> >> http://cr.opensolaris.org/~joev/bug1462_latest > [snip] > > Quick 5min race through > http://cr.opensolaris.org/~joev/bug1462_latest/jjv_bug1462.patch (patch > code is quoted with "> "): > >> +[[ ! -d $tmpdir ]] && tmpdir_existed="YES" > > Please change "YES" to "true" (and use "false" as opposite boolean > value) and init the value before this point to "false" - then you can > replace stuff like > -- snip -- > if [[ "${tmpdir_existed}" == "NO" ]]; then > -- snip -- > ... with ... > -- snip -- > if ${tmpdir_existed} ; then > -- snip -- > ... since "true" and "false" are ksh93 builtin commands which return > either zero or non-zero return codes which are directly consumed by the > "if... then"-construct (this IMO simpler and faster than doing string > comparisations each time). > >> +cleanup () >> +{ >> + >> + trap "" ERR INT >> + set +o errexit >> + >> + # unmounting, and uninstalling the lofi'ed devices and >> + # cleanup temporary files. >> + >> + mount | grep "${usb_path}" > /dev/null 2>&1 && \ >> + umount "${usb_path}" > /dev/null 2>&1 >> + >> + mount | grep "${iso_path}" > /dev/null 2>&1 && \ >> + umount "${iso_path}" > /dev/null 2>&1 >> + >> + lofiadm "${usb_file}" > /dev/null 2>&1 && \ >> + lofiadm -d "${usb_file}" > /dev/null 2>&1 >> + >> + lofiadm "${iso_file}" > /dev/null 2>&1 && \ >> + lofiadm -d "${iso_file}" > /dev/null 2>&1 >> + >> + if [[ -d "${iso_path}" ]] ; then >> + rm -rf "${iso_path}" > /dev/null 2>&1 >> + fi >> + >> + if [[ -d "${usb_path}" ]] ; then >> + rm -rf "${usb_path}" > /dev/null 2>&1 >> + fi >> + >> + if [[ "${tmpdir_existed}" == "NO" ]]; then >> + rm -rf "${tmpdir}" > /dev/null 2>&1 >> + fi > > Erm... three items: > 1. AFAIK the whole "> /dev/null 2>&1" stuff can simply combined with a > block-level redict (see > http://www.opensolaris.org/os/project/shell/shellstyle/#group_identical_redirections_together) > 2. The use of "grep" (which should really be "fgrep") can be avoided by > using the builtin extended regualr expression pattern matching > facilities, e.g. in this case ~(F)pattern. For example $ x="yellow fish > world" ; [[ "$x" == ~(E).*fish.* ]] && print "fish in x" # prints "fish > in x" since the content of variable "x" matches the extended regular > pattern ".*fish.*". > Note the code below should use ~(F), however the ksh93 version in OS/Net > has a small bug which renders ~(F) defunct - that's why I am using ~(E) > instead (which is a bit wrong since neither "usb_path" nor "iso_path" > contain any extended regular patterns but it will work anyway (the > ksh93-integration update1 will fix ~(F) to work like "fgrep")) > 3. Please use "function cleanup"-style functions unless there is a > reason to use the Bourne-style "cleanup()"-syntax > > e.g. replace this with: > -- snip -- > function cleanup > { > { > > trap "" ERR INT > set +o errexit > > # unmounting, and uninstalling the lofi'ed devices > # and cleanup temporary files. > IFS='' > > typeset mount_output="$(mount)" > > [[ "${mount_output}" == ~(E)${usb_path} ]] && \ > umount "${usb_path}" > > [[ "${mount_output}" == ~(E)${iso_path} ]] && \ > umount "${iso_path}" > > lofiadm "${usb_file}" && \ > lofiadm -d "${usb_file}" > > lofiadm "${iso_file}" && \ > lofiadm -d "${iso_file}" > > if [[ -d "${iso_path}" ]] ; then > rm -rf "${iso_path}" > fi > > if [[ -d "${usb_path}" ]] ; then > rm -rf "${usb_path}" > fi > > if [[ "${tmpdir_existed}" == "NO" ]]; then > rm -rf "${tmpdir}" > fi > } >/dev/null 2>&1 > } > -- snip -- > > >> +error_handler () > > Please use "function error_handler" (see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_ksh_style_function_syntax). > >> +{ >> + trap "" ERR INT >> + set +o errexit >> + >> + print -u2 "\nError:" >> + print -u2 "\n${progname} \n $@" > > Please replace this with: > -- snip -- > print -u2 "\nError:\n" > print -u2 -r -- "${progname} \n $*" > -- snip -- > ... unless you want to interpret stuff like '\n' or '\r' in the > arguments (and the $@ vs. $* is something which I do sometimes wrong, > too... ;-( ... $@ is like $* except that if you put double-quotes > around $@ it will expand the string within the double-quotes for each > item in $@ while $* just expands all items in the array to one single > string) > > [snip] >> +if (( $usb_size == -1 )) ; then > > Erm... within (( expr ))-style expressions the '$' is redundant (see > http://www.opensolaris.org/os/project/shell/shellstyle/#avoid_unneccesary_string_number_conversions) > and sometimes dangerous since it forces the shell to expand the value as > string first (which leads to a base2--->base10--->base2 conversion cycle > which results in rounding errors when floating-point math is involved), > e.g. replace this with > -- snip -- > if (( usb_size == -1 )) ; then > -- snip -- > (yes, I know... the datatype for "usb_size" isn't floating-point... but > it may be usefull to treat integer and floating-point variables the same > way to avoid that this creeps in somehow in other parts). > > [snip] > >> +print "=== $0 completed at $(date)\n" > > Please replace this with: > -- snip -- > printf "=== %s completed at %s\n\n" "$0" "$(date)" > -- snip -- > or > -- snip -- > print -f "=== %s completed at %s\n\n" "$0" "$(date)" > -- snip -- > (to make sure "print"/"printf" don't try to expand any characters in > "$0" or "$(date)") > > The rest of the script looks good... :-) > > ---- > > Bye, > Roland >
