On 01/11/2012 07:42 PM, Xuelei Fan wrote: > On 1/11/2012 7:12 PM, Weijun Wang wrote: >> >> >> On 01/11/2012 06:55 PM, Xuelei Fan wrote: >>> On 1/11/2012 6:42 PM, Weijun Wang wrote: >>>> >>>> >>>> On 01/11/2012 06:02 PM, Xuelei Fan wrote: >>>>> On 1/11/2012 5:50 PM, Weijun Wang wrote: >>>>>> Hi Andrew >>>>>> >>>>>> Take a brief look at the webrev. Looks like this Lengthable thing is the >>>>>> only change after your previous webrev. Please confirm. >>>>>> >>>>> Yes. >>>>> >>>>>> But I want something bigger. I would like to know if it is possible to >>>>>> add this keysize() method deep down into the very basic Key interface. >>>>>> If Key can have a method called getEncoded() I think this means it >>>>>> normally has a concrete form and surely has a publicly acceptable >>>>>> keysize() attribute. In JDK 8 we have default implementation for new >>>>>> interface methods. Is this issue a good candidate? >>>>>> >>>>> As Key is an java interface, we may not be able to add one more method >>>>> for compatibility reason. We may export the "Lengthable"/"Measurable" >>>>> interface in JDK 8. It's possible to implement Lengthable in all >>>>> sub-classes of Key in Oracle provider, but as would involve too many >>>>> changes. As we need to backport this fix into JDK 7, I think we'd better >>>>> consider the big picture in the future. >>>> >>>> Then I think the previous webrev is enough for JDK 7, and for JDK 8, we >>>> simply add a new keysize() method to Key. >>>> >>> If we add one new method to Key interfaces. The providers based on JDK 7 >>> and previous releases would have to update their codes so as to >>> implement this new method. As will result in serious compatibility issues. >> >> I am talking about the new default method language feature in JDK 8 ([1] >> Section 11, 12). Then the default impl of Key::keySize() returns -1, >> default impl of SecretKey::keySize() returns getEncoded().length()*8, etc. >> > A great feature! > >>> >>> It is possible that we export the "Lengthable" interface, and have >>> Oracle providers support this interface, and suggest other providers to >>> use this interfaces. >>> >>> The previous webrev hurt the performance a little because of reflections. >> >> Thanks for reminding me this. Yes, those P11 and MSCAPI keys. This >> webrev is still necessary, and the code changes are fine except for >> >> 1. SignatureAndHashAlgorithm.java:283, you left a System.out.println >> >> 2. KeyLength.java:58, more System.out.printlns >> > Should remove them. > >> 3. KeyLength.java:88, UnsupportedOperationException, necessary? >> > I want to reserve the flexibility that we may not be able to get key > size from a hardware device. Yes, the exception are not thrown in our > implementation. > > BTW, I will change the interface name from "Lengthable" to "Measurable".
Well, this is not much better. There are many things that can be measured, length, weight, duration. But at least it's a real word. > > Do you want to review more? If not, I will update the code locally and > push the changes. No more. Thanks Max > > Thanks, > Xuelei > >> Thanks >> Max >> >> [1] http://cr.openjdk.java.net/~briangoetz/lambda/lambda-state-4.html >> >>> >>> Xuelei >>> >>>> Max >>>> >>>>> >>>>>> At least, in KeyLength::getKeySize(), I would like to see "if (key >>>>>> instanceof Lengthable)" to be the first check, and, if possible, the >>>>>> only one needed, at least for keys from providers built in JDK. >>>>>> >>>>> It's OK to check it at first. But as we also need to support other >>>>> providers, I think we'd better also check other types of instance. >>>>> >>>>> Thanks, >>>>> Xuelei >>>>> >>>>>> Thanks >>>>>> Max >>>>>> >>>>>> >>>>>> On 01/11/2012 08:57 AM, Xuelei Fan wrote: >>>>>>> "Measurable" looks like a better name. I will update the name in the >>>>>>> next webrev after this round of code review: >>>>>>> >>>>>>> webrev: http://cr.openjdk.java.net/~xuelei/7106773/webrev.04/ >>>>>>> >>>>>>> Thanks, >>>>>>> Xuelei >>>>>>> >>>>>>> On 1/10/2012 11:47 PM, Vincent Ryan wrote: >>>>>>>> On 01/10/12 03:19 PM, Xuelei Fan wrote: >>>>>>>>> On 1/10/2012 11:09 PM, Weijun Wang wrote: >>>>>>>>>> It's late night and I'll read it tomorrow. But can you choose another >>>>>>>>>> word instead of Lengthable? Length is not a verb. >>>>>>>>>> >>>>>>>>> ;-) The name took me a lot of time, searching by google, dictionary, >>>>>>>>> and >>>>>>>>> any possible English translation. I have to agree that I failed to >>>>>>>>> find >>>>>>>>> a suitable name. I tried hardly to persuade myself that "lengthable" >>>>>>>>> is >>>>>>>>> also used by someother application code, so it might not too bad to >>>>>>>>> use >>>>>>>>> it here. >>>>>>>>> >>>>>>>>> With the word "lengthable", I want to express that the length is >>>>>>>>> measurable. Any suggestion for the better one? >>>>>>>>> >>>>>>>> >>>>>>>> Measurable ;-) >>>>>>>> >>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Xuelei >>>>>>>>> >>>>>>>>>> Max >>>>>>>>>> ------------------------------------------------------------------------ >>>>>>>>>> 发件人: Xuelei Fan >>>>>>>>>> 发送时间: 2012/1/10 22:51 >>>>>>>>>> 收件人: Weijun Wang >>>>>>>>>> 抄送: OpenJDK >>>>>>>>>> 主题: Re: Code review request, 7106773: 512 bits RSA key cannot work >>>>>>>>>> withSHA384 and SHA512 >>>>>>>>>> >>>>>>>>>> It has been around 50 days passed since the last day we talked about >>>>>>>>>> the >>>>>>>>>> issue. Hope you can recall it from the deep memory. ;-) >>>>>>>>>> >>>>>>>>>> webrev: >>>>>>>>>> http://javaweb.us.oracle.com/~xufan/bugbios/7106773/webrev.04/ >>>>>>>>>> >>>>>>>>>> In this update, as we agreed, a new Oracle private interface was >>>>>>>>>> introduced: sun.security.util.Lengthable, and Lengthable.length() is >>>>>>>>>> defined to get the length an object. sun.security.pkcs11.P11Key and >>>>>>>>>> sun.security.mscapi.Key will implements the interface. As will easy >>>>>>>>>> and >>>>>>>>>> speedup (comparing with reflection approach) the getting of key >>>>>>>>>> length >>>>>>>>>> of those unextractable keys in hardware device. >>>>>>>>>> >>>>>>>>>> In the webrev, I should also include another two signed jars, >>>>>>>>>> sunpkcs11.jar and sunmscapi.jar. I will include them when I get the >>>>>>>>>> official signed jars. >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Xuelei >>>>>>>>>> >>>>>>>>>> On 11/22/2011 8:41 AM, Weijun Wang wrote: >>>>>>>>>>> I really like this one. >>>>>>>>>>> >>>>>>>>>>> Thanks >>>>>>>>>>> Max >>>>>>>>>>> >>>>>>>>>>> On 11/21/2011 08:05 PM, Xuelei Fan wrote: >>>>>>>>>>>>>> How about this approach? This looks very safe. >>>>>>>>>>>>>> >>>>>>>>>>>> I also prefer this approach, although it need more updates in >>>>>>>>>>>> PKCS11 and >>>>>>>>>>>> MSCPI source code. If you vote for this approach, I will try to >>>>>>>>>>>> implement it. >>>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>> >>> >
