Will Fiveash wrote: > On Fri, Apr 03, 2009 at 12:10:16AM -0600, Shawn Emery wrote: > >> Will Fiveash wrote: >> >>> On Tue, Mar 17, 2009 at 02:41:36PM -0600, Shawn Emery wrote: >>> >>> >>>> Could someone please code review the following changes, as these changes >>>> are highly requested? >>>> >>>> Shawn. >>>> -- >>>> Shawn M Emery wrote: >>>> >>>> >>>>> Requesting a code review for the following fixes for: >>>>> >>>>> 6787343 kclient's site lookups fail in certain network environments >>>>> 6692646 kclient should output errors to stderr >>>>> >>>>> http://cr.opensolaris.org/~semery/6787343 >>>>> >>>>> >>> >>> >> Sorry for the slow response, I've been out of the office for several days. >> Comments in-line: >> >> >>> 33 function cleanup { >>> 34 integer ret=$1 >>> 35 36 [[ -z $TMP_FILE ]] && exit $ret >>> 37 kdestroy -q 1> $TMP_FILE 2>&1 >>> 38 39 [[ -z $TMPDIR ]] && exit $ret >>> 40 rm -r $TMPDIR > /dev/null 2>&1 >>> >>> - I don't get this logic. If TMP_FILE isn't set then TMPDIR won't get >>> removed? Is this intended? >>> >>> >> TMPDIR would not have been created since this is what mktemp uses in order >> to create TMP_FILE. >> > > - Here's what I see in kclient.sh: > > export TMPDIR="/var/run/kclient" > > mkdir $TMPDIR > /dev/null 2>&1 > > TMP_FILE=$(mktemp -q -t kclient-tmpfile.XXXXXX) > export KRB5_CONFIG=$(mktemp -q -t kclient-krb5conf.XXXXXX) > export KRB5CCNAME=$(mktemp -q -t kclient-krb5ccache.XXXXXX) > new_keytab=$(mktemp -q -t kclient-krb5keytab.XXXXXX) > > if [[ -z $TMP_FILE || -z $KRB5_CONFIG || -z $KRB5CCNAME || -z $new_keytab ]] > then > printf "\n$(gettext "Can not create temporary file, exiting").\n" >&2 > error_message > fi > > - looks to me like TMPDIR is being created regardless of mktemp. This > brings up several issues. Why doesn't kclient exit in the case that > the /var/run/kclient dir doesn't exist and mkdir fails? And if >
I've updated it to check mkdir fails and to remove the extra login in cleanup(). > only one instance of kclient is supposed run at a time, why bother > with removing TMP_FILE, why not just do: > > kdestroy -q > /dev/null > [[ -d $TMPDIR ]] && rm -r $TMPDIR > /dev/null 2>&1 > I will change the cases of initial error state to exit w/o using error_message(), which will remove the extra logic in cleanup() (i.e. by the time cleanup() is called TMPDIR must exist). > - I say this because why log kdestroy output if the containing dir is > going to be removed right after, regardless of kdestroy's exist > status. > I think this was originally made in the case that if you wanted to review the kclient log then you could just comment out the line to remove the file/directory. The log file is normally removed by default to avoid file clutter, etc. >>> 67 + typeset -ui10 a b c d >>> >>> - Why -u for i10? -u converts characters to uppercase but this >>> shouldn't be applicable to base 10. >>> >>> >> ksh93 has a new declaration for defining unsigned integers with "ui". "ui" >> is now required for a, b, c, and d as they are assigned the value of "num" >> which is also an unsigned integer. I will file a ksh* bug, in order to >> track the issue of -i representing 64b (instead of 32b) in the case of >> Solaris. >> > > - How about a comment there explaining what is going on? > Ok, please check the update. Thanks, Shawn. -- -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/kerberos-discuss/attachments/20090417/34a40e4a/attachment.html>
