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
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 say this because why log kdestroy output if the containing dir is
going to be removed right after, regardless of kdestroy's exist
status.
>> 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?
--
Will Fiveash
Sun Microsystems Inc.
http://opensolaris.org/os/project/kerberos/
