Re: Code revire request: 6966259 (was Re: Avoid PrincipalName with realm == null)
3. Realm.java
63 } catch (KrbException ke) {
64 RealmException re = new RealmException(ke.getMessage());
65 re.initCause(ke);
66 throw re;
67 }
I think you make a lot cleanup to exception thrown with just one call,
like the one in KerberosPrincipal.java:
-IOException ioe = new IOException(e.getMessage());
-ioe.initCause(e);
-throw ioe;
+throw new IOException(e);
Would you like to use the same style for the update in Realm.java?
Unfortunately RealmException does not provide such a constructor. Are
you suggesting me to create one?
Why not? I think a Exception class should always have such constructor.
Good.
-Max
Xuelei
Re: Code revire request: 6966259 (was Re: Avoid PrincipalName with realm == null)
Webrev updated:
http://cr.openjdk.java.net/~weijun/6966259/webrev.01/
Changes made:
test/sun/security/krb5/name/Constructors.java
src/share/classes/sun/security/krb5/RealmException.java
src/share/classes/sun/security/krb5/Realm.java
src/share/classes/sun/security/krb5/PrincipalName.java
src/share/classes/sun/security/krb5/KrbException.java
1. New constructors for KrbException and RealmException that takes
Throwable as an argument.
2. In PrincipalName.java: More comments in PrincipalName, and
3. Consolidate all nameStrings check into a separate method
static void validateNameStrings(String[] ns)
and make sure it's called by all basic constructors. Also, remove all
other duplicate checks.
4. Small changes in parseName(String), say
-if (componentStart i) {
+if (componentStart = i) {
and
-if (componentStart i) {
to make sure that names like a//b and a/ are rejected.
5. Remove equals(PrincipalName other). I guess it was only used to
compare a PrincipalName and a ServiceName. Also, parseName() is now private.
6. In Realm.java: parseRealmAtSeparator(String). A small change to make
sure a name like a@ is treated as illegal but not silently bypassed.
7. Updating the Constructors regression test to check for illegal names.
Thanks
Max
On 06/15/2012 01:01 PM, Xuelei Fan wrote:
On 6/15/2012 12:19 PM, Weijun Wang wrote:
On 06/15/2012 10:28 AM, Xuelei Fan wrote:
Looks fine to me. Just some minor comments.
1. PrincipalName.java
need to make it more clear that PrincipalName is not only for the name
of a principal, but also include the realm.
- 48 * This class encapsulates a Kerberos principal.
+ 48 * This class encapsulates a Kerberos principal,
+ * including both of the realm and name of a principal.
Or some other words like that.
Sure.
2. KrbAppMessage.java
No copyright date in the header. Other files have the header like
Copyright (c) 2000, 2011, Oracle and/or its affiliates. All rights
reserved.
Quite some other files (see krb5/internal) look the same. I remember I
was told not to touch them.
OK.
3. Realm.java
63 } catch (KrbException ke) {
64 RealmException re = new RealmException(ke.getMessage());
65 re.initCause(ke);
66 throw re;
67 }
I think you make a lot cleanup to exception thrown with just one call,
like the one in KerberosPrincipal.java:
-IOException ioe = new IOException(e.getMessage());
-ioe.initCause(e);
-throw ioe;
+throw new IOException(e);
Would you like to use the same style for the update in Realm.java?
Unfortunately RealmException does not provide such a constructor. Are
you suggesting me to create one?
Why not? I think a Exception class should always have such constructor.
Xuelei
Otherwise, looks fine to me.
As there are too many changes, I would suggest you run a thoroughly
testing before integration in case of any missing.
Sure.
Thanks
Max
Regards,
Xuelei
On 6/11/2012 1:29 PM, Weijun Wang wrote:
Hi Valerie
Here is the webrev:
http://cr.openjdk.java.net/~weijun/6966259/webrev.00/
The patch is quite long, but most of the real changes are in a few
classes:
PrincipalName.java:
. All fields are final and non-null non-empty now
. All constructors have a realm argument, including those from DER
. Add a new static method tgsService(r1, r2) to get AS/TGS name
. Remove ServiceName class
. New reg test: Constructors.java
Realm.java:
. Field is now final
. New getDefault() method
KDCReqBody.java:
. cname and sname share the same realm field. This is also the only
message format that realm comes after name.
KrbKdcRep.java:
. Related to the class above. The check() method now has a new argument
isAsReq to deal with AS-REQ and TGS-REQ differently. The old code does
not check crealm equality, now both name and realm are checked, but only
for AS-REQ. For TGS-REQ, no more check for cname equality, this will be
useful for S4U2proxy where the cname of KDCRep comes from the additional
ticket in request. The old code checks if req.crealm is rep.srealm,
since KDCReqBody has only one realm field, this is the same as checking
req.srealm and rep.srealm.
CCacheInputStream.java:
. readPrincipal returns null when there is no realm field. This has the
benefit that a ccache is readable even if there is no valid krb5
setting. A normal ccache entry's principal should have its own realm
field. but when an entry is used to store non-ticket, returning null
won't trigger an exception. (See readCred about X-CACHECONF: style
entries)
Other trivial code changes:
. Methods with the realm/name argument pair now has only name
. In parsing DER, read realm and then merge the info into name
. In encoding DER, encode the realm from name.getRealm()
. No need to check realm == null for name
. No need to print realm in debug output
. No need to call setRealm()
Thanks
Max
On 06/09/2012 08:23 AM, Valerie (Yu-Ching) Peng wrote:
Max,
Re: Code revire request: 6966259 (was Re: Avoid PrincipalName with realm == null)
Looks fine to me.
Thanks,
Xuelei
On 6/15/2012 3:59 PM, Weijun Wang wrote:
Webrev updated:
http://cr.openjdk.java.net/~weijun/6966259/webrev.01/
Changes made:
test/sun/security/krb5/name/Constructors.java
src/share/classes/sun/security/krb5/RealmException.java
src/share/classes/sun/security/krb5/Realm.java
src/share/classes/sun/security/krb5/PrincipalName.java
src/share/classes/sun/security/krb5/KrbException.java
1. New constructors for KrbException and RealmException that takes
Throwable as an argument.
2. In PrincipalName.java: More comments in PrincipalName, and
3. Consolidate all nameStrings check into a separate method
static void validateNameStrings(String[] ns)
and make sure it's called by all basic constructors. Also, remove all
other duplicate checks.
4. Small changes in parseName(String), say
-if (componentStart i) {
+if (componentStart = i) {
and
-if (componentStart i) {
to make sure that names like a//b and a/ are rejected.
5. Remove equals(PrincipalName other). I guess it was only used to
compare a PrincipalName and a ServiceName. Also, parseName() is now
private.
6. In Realm.java: parseRealmAtSeparator(String). A small change to make
sure a name like a@ is treated as illegal but not silently bypassed.
7. Updating the Constructors regression test to check for illegal names.
Thanks
Max
On 06/15/2012 01:01 PM, Xuelei Fan wrote:
On 6/15/2012 12:19 PM, Weijun Wang wrote:
On 06/15/2012 10:28 AM, Xuelei Fan wrote:
Looks fine to me. Just some minor comments.
1. PrincipalName.java
need to make it more clear that PrincipalName is not only for the name
of a principal, but also include the realm.
- 48 * This class encapsulates a Kerberos principal.
+ 48 * This class encapsulates a Kerberos principal,
+ * including both of the realm and name of a principal.
Or some other words like that.
Sure.
2. KrbAppMessage.java
No copyright date in the header. Other files have the header like
Copyright (c) 2000, 2011, Oracle and/or its affiliates. All rights
reserved.
Quite some other files (see krb5/internal) look the same. I remember I
was told not to touch them.
OK.
3. Realm.java
63 } catch (KrbException ke) {
64 RealmException re = new RealmException(ke.getMessage());
65 re.initCause(ke);
66 throw re;
67 }
I think you make a lot cleanup to exception thrown with just one call,
like the one in KerberosPrincipal.java:
-IOException ioe = new IOException(e.getMessage());
-ioe.initCause(e);
-throw ioe;
+throw new IOException(e);
Would you like to use the same style for the update in Realm.java?
Unfortunately RealmException does not provide such a constructor. Are
you suggesting me to create one?
Why not? I think a Exception class should always have such constructor.
Xuelei
Otherwise, looks fine to me.
As there are too many changes, I would suggest you run a thoroughly
testing before integration in case of any missing.
Sure.
Thanks
Max
Regards,
Xuelei
On 6/11/2012 1:29 PM, Weijun Wang wrote:
Hi Valerie
Here is the webrev:
http://cr.openjdk.java.net/~weijun/6966259/webrev.00/
The patch is quite long, but most of the real changes are in a few
classes:
PrincipalName.java:
. All fields are final and non-null non-empty now
. All constructors have a realm argument, including those from DER
. Add a new static method tgsService(r1, r2) to get AS/TGS name
. Remove ServiceName class
. New reg test: Constructors.java
Realm.java:
. Field is now final
. New getDefault() method
KDCReqBody.java:
. cname and sname share the same realm field. This is also the only
message format that realm comes after name.
KrbKdcRep.java:
. Related to the class above. The check() method now has a new
argument
isAsReq to deal with AS-REQ and TGS-REQ differently. The old code does
not check crealm equality, now both name and realm are checked, but
only
for AS-REQ. For TGS-REQ, no more check for cname equality, this
will be
useful for S4U2proxy where the cname of KDCRep comes from the
additional
ticket in request. The old code checks if req.crealm is rep.srealm,
since KDCReqBody has only one realm field, this is the same as
checking
req.srealm and rep.srealm.
CCacheInputStream.java:
. readPrincipal returns null when there is no realm field. This has
the
benefit that a ccache is readable even if there is no valid krb5
setting. A normal ccache entry's principal should have its own realm
field. but when an entry is used to store non-ticket, returning null
won't trigger an exception. (See readCred about X-CACHECONF: style
entries)
Other trivial code changes:
. Methods with the realm/name argument pair now has only name
. In parsing DER, read realm and then merge the info into name
. In encoding DER, encode the realm from name.getRealm()
.
hg: jdk8/tl/jdk: 7156963: Incorrect copyright header in java/io/SerialCallbackContext
Changeset: 00c9d6cce3ec Author:coffeys Date: 2012-06-15 14:16 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/00c9d6cce3ec 7156963: Incorrect copyright header in java/io/SerialCallbackContext Reviewed-by: weijun, coffeys Contributed-by: edvard.wende...@oracle.com ! src/share/classes/java/io/SerialCallbackContext.java
hg: jdk8/tl/jdk: 3 new changesets
Changeset: 8deec0d1fc6f Author:mullan Date: 2012-06-15 08:43 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8deec0d1fc6f 7176326: CertPath/CertPathBuilderTest failures after webrev 6854712_6637288_7126011 Reviewed-by: xuelei ! src/share/classes/sun/security/provider/certpath/BasicChecker.java + test/java/security/cert/CertPathBuilder/zeroLengthPath/ZeroLengthPath.java Changeset: 0e382512610f Author:mullan Date: 2012-06-15 08:47 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0e382512610f Merge Changeset: e01b47409e37 Author:mullan Date: 2012-06-15 09:16 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e01b47409e37 Merge
hg: jdk8/tl/jdk: 7176485: (bf) Allow temporary buffer cache to grow to IOV_MAX
Changeset: 23394d686f74 Author:alanb Date: 2012-06-15 17:16 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/23394d686f74 7176485: (bf) Allow temporary buffer cache to grow to IOV_MAX Reviewed-by: chegar, coffeys ! make/java/nio/mapfile-linux ! make/java/nio/mapfile-solaris ! src/share/classes/sun/nio/ch/IOUtil.java ! src/share/classes/sun/nio/ch/Util.java ! src/solaris/native/sun/nio/ch/FileDispatcherImpl.c ! src/solaris/native/sun/nio/ch/IOUtil.c ! src/windows/native/sun/nio/ch/IOUtil.c ! src/windows/native/sun/nio/ch/SocketDispatcher.c ! src/windows/native/sun/nio/ch/nio_util.h
How to detect if a PKCS11 token presents in a system?
Hello, Right now I'm solving this problem by calling 'getMechanismsList' on the PKCS11 wrapper object. Considering that Windows x86-64 doesn't have PKCS11 support in Java, code becomes more tricky, because I have to instantiate all objects via reflection. Is it possible to avoid using such tricks and detect a token presence/removal in a more straightforward way? Sincerely, Andrey Atapin.
hg: jdk8/tl/jdk: 7175758: Improve unit test of Map iterators and Iterator.remove()
Changeset: e60cedd3a4aa Author:mduigou Date: 2012-06-15 13:01 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e60cedd3a4aa 7175758: Improve unit test of Map iterators and Iterator.remove() Summary: Adds additional tests of Map iterators and Iterator.remove() Reviewed-by: lancea ! test/java/util/Map/Collisions.java
Codereview for 7177556
This test has started failing in JDK nightly. We were working on it this week, but test approach may need alteration. Putting this test on ProblemList.txt for the time being to quiet the test noise. 7177556: Put TestProviderLeak.java on the ProblemList until test can be reworked http://cr.openjdk.java.net/~wetmore/7177556/ Brad
