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

Thu Aug 4 00:07:00 UTC 2011

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