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).
>
Done, however it is initialized to "true" to avoid rm-ing if cleanup is
invoked in the small window prior to checking if it existed or not.
>> +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)
done
> 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"))
done
> 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 --
>
>
done, mostly.
I left cleanup and error_handler defined without the function key word.
I had experimented with this a bit. Using "function cleanup" and
"function error_handler" leads to an infinite loop though these
functions if a trap is raised do to an error being encountered in one of
them.
These 2 simple programming examples depict the issue.opensolaris > cat
trap_clear.ksh
#!/usr/bin/ksh93 -p
trap "error_handler Error or interupt encountered. Exiting" ERR INT
set -o errexit
cleanup () {
trap "" ERR
echo " cleanup invoked."
ls cleanup_this_file_does_not_exist # induce an error.
}
error_handler () {
trap "" INT
echo " error_handler invoked. $@"
cleanup
exit 1
}
echo start
ls this_file_does_not_exist # induce an error.
echo finish
# end of file trap_clear.ksh
opensolaris > cat trap_clear_function.ksh
#!/usr/bin/ksh93 -p
trap "error_handler Error or interupt encountered. Exiting" ERR INT
set -o errexit
function cleanup {
trap "" ERR
echo " cleanup invoked."
ls cleanup_this_file_does_not_exist # induce an error.
}
function error_handler {
trap "" INT
echo " error_handler invoked. $@"
cleanup
exit 1
}
echo start
ls this_file_does_not_exist # induce an error.
echo finish
# End of file trap_clear_function.ksh
I you have an idea let me know.
>> +error_handler ()
>
> Please use "function error_handler" (see
> http://www.opensolaris.org/os/project/shell/shellstyle/#use_ksh_style_function_syntax).
>
See above
>> +{
>> + 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)
done with 1 minor difference to avoid "\n" showing up in the message.
> print -u2 -r -- "${progname} \n $*"
changed to:
> print -u2 -r -- "${progname} $*"
>
> [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).
done
>
> [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)")
>
done
> The rest of the script looks good... :-)
>
> ----
>
> Bye,
> Roland
>
The webrev has been update to reflect these changes. I'll put this back
if I don't hear any reason why not to by tomorrow.
Huge thanks for your expertise and help!
Joe VLcek
