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 -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;)
