Code review request: 7064075 Security libraries don't build with javac -Xlint:all, -deprecation -Werror

Thu Aug 4 01:23:07 UTC 2011

My 2 cents:

@SuppressWarnings("serial") is never a good idea. There will still be a 
serialver automatically computed and you won't be able to control it.

Will the code changes go to jdk8 only? If so, I guess you should use the 
value calculated from jdk7 codes. If the values on jdk6 and jdk7 are 
already different, sorry, that means there is already a compatibility 
problem made and you won't be able to fix it. We can now only make sure 
jdk7 and jdk8 are compatible.

Ultimately you can write small test to see if serialized form from one 
version can be accepted by another version.

Thanks
Max

On 08/04/2011 08:10 AM, Alexandre Boulgakov wrote:
> Those were the ones I was talking about- the serialVersionUIDs I
> mentioned were the ones generated by serialver.exe. The webrev doesn't
> have any of the pkcs11 changes yet (including the added serialVersionUIDs).
>
> I'll wait for Brad's or Valerie's response.
>
> Thanks,
> Sasha
>
> On 8/3/2011 5:07 PM, Xuelei Fan wrote:
>> On 8/4/2011 7:52 AM, Alexandre Boulgakov wrote:
>>> There is currently no serialVersionUID defined for these classes. Do you
>>> mean that we cannot add one for backwards compatibility?
>> My answers only apply to you latest e-mail about the serialVersionUID
>> update in sun.security.pkcs11.P11Key.P11SecretKey and
>> sun.security.pkcs11.P11TlsPrfGenerator$1. I think you need to use the
>> current values unless you get an approval from PKCS11 owner.
>>
>> We may need to consider more for these classes without serialVersionUID,
>> I will look at your webrev again if I can get some time today. Normally,
>> I think if there is a new attribute in a class, it is OK to add a new
>> serialVersionUID value.
>>
>> Xuelei
>>
>>> If so, would
>>> the best solution be to add an @SuppressWarnings("serial") annotation to
>>> these classes?
>>>
>>> Thanks,
>>> Sasha
>>>
>>> On 8/3/2011 4:49 PM, Xuelei Fan wrote:
>>>> Oops, I missed this.
>>>>
>>>> I don't think we can modify serialVersionUID value for backward
>>>> compatibility.
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>> On 8/4/2011 7:39 AM, Alexandre Boulgakov wrote:
>>>>> Ping..?
>>>>>
>>>>> -Sasha
>>>>>
>>>>> On 7/27/2011 11:22 AM, Alexandre Boulgakov wrote:
>>>>>> Should I just use the newest serialVersionUID for both of them?
>>>>>>
>>>>>> -Sasha
>>>>>>
>>>>>> On 7/26/2011 10:31 AM, Alexandre Boulgakov wrote:
>>>>>>> I just noticed that pkcs11 is not built on my machine (64-bit
>>>>>>> Windows) so I missed all of the warnings there. There are two
>>>>>>> mission
>>>>>>> serialVersionUID warnings for classes that have had different
>>>>>>> generated serialVersionUID's in the past.
>>>>>>>
>>>>>>> sun.security.pkcs11.P11Key.P11SecretKey
>>>>>>> -currently: -7828241727014329084L;
>>>>>>> -JDK 1.5: -897881148551545872L;
>>>>>>>
>>>>>>> sun.security.pkcs11.P11TlsPrfGenerator$1
>>>>>>> -currently: -8090049519656411362L;
>>>>>>> -JDK 6: -3305145912345854189L;
>>>>>>>
>>>>>>> I'm not sure why the serialVersionUID changed for
>>>>>>> sun.security.pkcs11.P11TlsPrfGenerator$1; the code is the same, and
>>>>>>> the serialVersionUID for the base class javax.crypto.SecretKey
>>>>>>> hasn't
>>>>>>> changed.
>>>>>>>
>>>>>>> For sun.security.pkcs11.P11Key.P11SecretKey, the code is the same,
>>>>>>> but the base class sun.security.pkcs11.P11Key has changed.
>>>>>>>
>>>>>>> How should I go about resolving these issues?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sasha
>>>>>>>
>>>>>>> On 7/20/2011 3:37 PM, Xuelei.Fan at Oracle.Com wrote:
>>>>>>>> On Jul 21, 2011, at 1:25 AM, Alexandre
>>>>>>>> Boulgakov<alexandre.boulgakov at oracle.com> wrote:
>>>>>>>>
>>>>>>>>> This is a Netbeans warning, not generated by the compiler. The
>>>>>>>>> reason is that List.isEmpty() can be more efficient for some
>>>>>>>>> implementations. ArrayList.size() == 0 and ArrayList.isEmpty()
>>>>>>>>> should take the same time, so it doesn't matter for allResults,
>>>>>>>>> but
>>>>>>>>> keyTypeList is a List argument, so any implementation could be
>>>>>>>>> passed in. List.isEmpty() should never be slower than List.size()
>>>>>>>>> == 0 because AbstractCollection defines isEmpty() as size() == 0.
>>>>>>>>>
>>>>>>>>> Even if we don't get a performance improvement, it still improves
>>>>>>>>> readability.
>>>>>>>>>
>>>>>>>> Sounds reasonable.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Xuelei
>>>>>>>>
>>>>>>>>> -Sasha
>>>>>>>>>
>>>>>>>>> On 7/19/2011 7:32 PM, Xuelei Fan wrote:
>>>>>>>>>> I was looking at the updates in sun/security/ssl. Looks fine to
>>>>>>>>>> me.
>>>>>>>>>>
>>>>>>>>>> It's fine, but I just wonder why List.isEmpty() is preferred to
>>>>>>>>>> (List.size() == 0). What's the compiler warning for (List.size()
>>>>>>>>>> == 0)?
>>>>>>>>>>
>>>>>>>>>> src/share/classes/sun/security/ssl/X509KeyManagerImpl.java
>>>>>>>>>> - if (keyTypeList == null || keyTypeList.size() == 0) {
>>>>>>>>>> + if (keyTypeList == null || keyTypeList.isEmpty()) {
>>>>>>>>>>
>>>>>>>>>> - if (allResults == null || allResults.size() == 0) {
>>>>>>>>>> + if (allResults == null || allResults.isEmpty()) {
>>>>>>>>>>
>>>>>>>>>> Thanks for the cleanup.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Xuelei (Andrew) Fan
>>>>>>>>>>
>>>>>>>>>> On 7/20/2011 7:22 AM, Alexandre Boulgakov wrote:
>>>>>>>>>>> Hello Sean,
>>>>>>>>>>>
>>>>>>>>>>> Have you had a chance to look at this webrev?
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Sasha
>>>>>>>>>>>
>>>>>>>>>>> On 7/18/2011 6:21 PM, Alexandre Boulgakov wrote:
>>>>>>>>>>>> Hello,
>>>>>>>>>>>>
>>>>>>>>>>>> Here's an updated webrev:
>>>>>>>>>>>> http://cr.openjdk.java.net/~smarks/aboulgak/7064075.2/
>>>>>>>>>>>>
>>>>>>>>>>>> I've reexamined the @SuppressWarnings("unchecked")
>>>>>>>>>>>> annotations, and
>>>>>>>>>>>> added comments to all of the ones I've added. I was was also
>>>>>>>>>>>> able to
>>>>>>>>>>>> remove several of them by using covariant return types on
>>>>>>>>>>>> sun.security.x509.*Extension.get(String) inherited from Object
>>>>>>>>>>>> CertAttrSet<T>.get(String). I've also updated the consumers of
>>>>>>>>>>>> sun.security.x509.*Extension.get(String) to use the more
>>>>>>>>>>>> specific
>>>>>>>>>>>> return type, removing several casts and
>>>>>>>>>>>> @SuppressWarnings("unchecked")
>>>>>>>>>>>> annotations.
>>>>>>>>>>>>
>>>>>>>>>>>> Also, please take a closer look at my changes to
>>>>>>>>>>>> com.sun.security.auth.PolicyFile.getPrincipalInfo(PolicyParser.PrincipalEntry,
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> final CodeSource) in
>>>>>>>>>>>> src/share/classes/com/sun/security/auth/PolicyFile.java lines
>>>>>>>>>>>> 1088-1094. The preceding comment and the behavior of
>>>>>>>>>>>> Subject.getPrincipals(Class<T>) seem to be more consistent
>>>>>>>>>>>> with the
>>>>>>>>>>>> updated version, but I wanted to make sure.
>>>>>>>>>>>>
>>>>>>>>>>>> The classes where I added serialVersionUID's are either new or
>>>>>>>>>>>> have
>>>>>>>>>>>> the same serialVersionUID as the original implementation.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Sasha
>>>>>>>>>>>>
>>>>>>>>>>>> On 7/18/2011 5:33 PM, Brad Wetmore wrote:
>>>>>>>>>>>>> (Apologies to folks without access to the older sources.)
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 07/18/11 15:00, Sean Mullan wrote:
>>>>>>>>>>>>>> On 7/18/11 5:35 PM, Alexandre Boulgakov wrote:
>>>>>>>>>>>>>>> Is there an easy way to see when a class was added to the
>>>>>>>>>>>>>>> JDK?
>>>>>>>>>>>>>> For standard API classes, you can use the @since javadoc tag
>>>>>>>>>>>>>> which
>>>>>>>>>>>>>> will indicate
>>>>>>>>>>>>>> the release it was first introduced in.
>>>>>>>>>>>>> Standard, exported API classes. Some of the underlying support
>>>>>>>>>>>>> classes for API packages like java.*.* weren't always @since'd
>>>>>>>>>>>>> properly.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> For internal classes, there is no easy way, since most don't
>>>>>>>>>>>>>> have an
>>>>>>>>>>>>>> @since tag.
>>>>>>>>>>>>>> I would probably write a script that checks the rt.jar of
>>>>>>>>>>>>>> each of
>>>>>>>>>>>>>> the JREs that
>>>>>>>>>>>>>> are archived at /java/re/jdk. The pathnames should be fairly
>>>>>>>>>>>>>> consistent, just
>>>>>>>>>>>>>> the version number is different.
>>>>>>>>>>>>> Don't know which classes you're talking about here, but some
>>>>>>>>>>>>> classes
>>>>>>>>>>>>> started out in other jar files and eventually wound up in
>>>>>>>>>>>>> rt.jar.
>>>>>>>>>>>>> Also, some files live in files other than rt.jar. I usually
>>>>>>>>>>>>> go to
>>>>>>>>>>>>> the source when looking for something. If it's originally from
>>>>>>>>>>>>> JSSE/JGSS/JCE, you'll need to look on our restricted access
>>>>>>>>>>>>> machine.
>>>>>>>>>>>>>
>>>>>>>>>>>>> When I'm looking for something that is in the jdk/j2se
>>>>>>>>>>>>> workspaces, I
>>>>>>>>>>>>> go right to the old Codemgr data, specifically the nametable
>>>>>>>>>>>>> file,
>>>>>>>>>>>>> because many times the files you want may be in a
>>>>>>>>>>>>> src/<arch>/classes
>>>>>>>>>>>>> instead of src/share/classes.
>>>>>>>>>>>>>
>>>>>>>>>>>>> % grep -i SunMSCAPI.java
>>>>>>>>>>>>> <RE-repository>/5.0/latest/ws/j2se/Codemgr_wsdata/nametable
>>>>>>>>>>>>>
>>>>>>>>>>>>> % grep -i SunMSCAPI.java
>>>>>>>>>>>>> <RE-repository>/6.0/latest/ws/j2se/Codemgr_wsdata/nametable
>>>>>>>>>>>>> src/windows/classes/sun/security/mscapi/SunMSCAPI.java
>>>>>>>>>>>>> ada8dbe4
>>>>>>>>>>>>> a217f6b0 6c833bd3 d4ef32be
>>>>>>>>>>>>>
>>>>>>>>>>>>> That will usually give you a good starting point.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Brad
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Depending on rt.jar or not.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Chris, do you have any other ideas?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --Sean