On 8/20/2013 9:37 AM, Weijun Wang wrote: > Looks fine now. > > Or maybe you can just remove KerberosClientKeyExchange()? I remember an > empty public constrictor is equivalent to no constructor. Of course you > made it protected so it's a little different. > > When I ask if you saw a failure, I had two different questions: > > 1. Is the code change necessary? i.e. if there is no impl at all, can a > program go as far as this way to this class? > It's a public internal class. It's a little better to restrict the usage of this constructor.
> 2. Is the code change sufficient? i.e. is there other places we need to > take care of? > No similar issue found on other krb5/ssl impl. Thanks, Xuelei > --Max > > On 8/20/13 9:24 AM, Xuelei Fan wrote: >> new webrev: http://cr.openjdk.java.net/~xuelei/8023230/webrev.01/ >> >> On 8/19/2013 9:53 PM, Weijun Wang wrote: >>> Only one change I don't understand: >>> >>> 73 public KerberosClientKeyExchange() { >>> 74 if (impl == null) { >>> 75 throw new IllegalStateException("Kerberos is >>> unavailable"); >>> 76 } >>> 77 } >>> >>> It seems this constructor will be automatically called when constructing >>> an instance of its child class -- KerberosClientKeyExchangeImpl. Isn't >>> that impl itself? There seems to be a chicken-or-egg puzzle here. >>> >> Good catch. It's a weird link between KerberosClientKeyExchangeImpl and >> KerberosClientKeyExchange. >> >> Then I would like to restrict the use of this constructor, and declare >> it as protected. See above webrev. >> >>> Also, did you really spot a failure when KerberosClientKeyExchangeImpl >>> does not exist? >>> >> Not really. But that's the purpose of existence of >> KerberosClientKeyExchangeImpl. Krb5 implementation should be able to be >> removed from jsse.jar. >> >> Thanks, >> Xuelei >> >>> Thanks >>> Max >>> >>> >>> On 8/19/13 8:49 PM, Xuelei Fan wrote: >>>> Hi Weijun, >>>> >>>> Please review this update when you are available. >>>> >>>> webrev: http://cr.openjdk.java.net/~xuelei/8023230/webrev.00/ >>>> >>>> If package sun.security.ssl.krb5 does not exist, the impl of >>>> KerberosClientKeyExchange (krb5.KerberosClientKeyExchangeImpl) will not >>>> present as well. Need to consider this case in the implementation of >>>> sun.security.ssl.KerberosClientKeyExchange. >>>> >>>> Thanks, >>>> Xuelei >>>> >>
