On Jul 9, 2014, at 3:21, Sean Mullan <sean.mul...@oracle.com> wrote:
> Hi Max, > > Here are my comments: > > * KerberosKey > > 37: Did you mean to use @link instead of @see? Yes. > > - several methods are now defined to throw IllegalStateExc, but you (perhaps > accidentally) removed the code that does that (ex: getKeyType). That's because KeyImpl.getKeyType() already throws it. I mentioned it in that last paragraph of my previous mail. If you think it makes the code more difficult to read, I'll add them back. > > * KerberosTicket > > 352-3: put braces around the if (destroyed) statement; same comment applies > in rest of code. There are quite some single line block without braces in this file (see init()). My habit was if I don't touch the part I will not change it. Some people believe whenever a file is touched it's better to update all appearances of such codes. If you think this is good I'll update all of them. > > * Krb5Context > > 1446-9: can myName or peerName be null? No. They won't be null after a context is established. There exists something called Anonymous Kerberos (we don't support it) but in this case there is no KRB_CRED to send. > > * Context > > 451: I think you should also print the exception OK. > > * KerberosCredMessage > > 52-4: these fields should be private Absolutely correct. > > - is there a reason why you didn't override hashCode/equals? I don't think it's likely for someone to put several KerberosCredMessage into a set (which is where I think hashCode/equals is useful). Maybe it's a good habit to always verride hashCode/equals for these "data" classes? > > * KerberosSessionKey > > - methods don't need to be final since class is OK. > - some of the methods don't check if it is destroyed (ex: getKeyType) See above (KerberosKey). > > - I think it would actually be better to name this class EncryptionKey since > that is what it is and that way we would be able to reuse this type in the > future if necessary for other structures or APIs that reference this type > (EncryptionKey) This makes sense. I'll withdraw CCC and resubmit it. Thanks Max > > --Sean > > On 07/04/2014 02:12 AM, Wang Weijun wrote: >> Hi All >> >> Please review the change at >> >> http://cr.openjdk.java.net/~weijun/8043071/webrev.00/ >> >> Two new inquire type KRB5_GET_SESSION_KEY_EX and KRB5_GET_KRB_CRED are added >> to get the session key (in a new format) and the KRB_CRED message. Two new >> classes are created as the types of their return values. >> >> Spec for InquireType values are moved into the InquireType class itself. >> >> For the existing KerberosKey class, the "A call to any of its other methods >> after this will cause an IllegalStateException to be thrown" in the spec of >> destroy() is not precise since we know isDestroyed() and >> toString/hashCode/equals() do not throw it. The sentence is removed and a >> @throws clause is added to those methods that do throw the exception. The >> new KerberosSessionKey and KerberosCredMessage have the same style. >> >> Since the data class KeyImpl already throws IllegalStateException when it's >> destroyed, KerberosKey and KerberosSessionKey do not check again. If you >> think this makes the code difficult to read, I can add them back. >> >> Thanks >> Max >>
