Hi Alan Thanks for the review. All suggestions accepted.
Change for S4U2selfGSS is not related and can be reverted. In fact, I have a question. Is there a way to run a test with a module invisible? I can try my tests by removing the com.sun.security.jgss classes manually but that won't work for automatic regression tests. --Max On Aug 28, 2014, at 16:54, Alan Bateman <[email protected]> wrote: > On 27/08/2014 08:17, Wang Weijun wrote: >> Hi All >> >> Please review the code change at >> >> http://cr.openjdk.java.net/~weijun/8042900/webrev.00/ >> >> The purpose of this fix is to move com.sun.security.jgss (where the >> ExtendedGSSXXX classes reside) out of the java.security.jgss module, since >> it is openjdk-specific and not defined in Java SE. This is listed as the 1st >> open issue in http://openjdk.java.net/jeps/200. >> >> Because GSSManager::createContext() actually returns an ExtendedGSSContext >> object now, we will need to add a check there so that when the extended >> classes are available it still returns ExtendedGSSContext and otherwise a >> plain GSSContext. An internal OrgIetfJgssExtender class is defined and >> Extender in com.sun.security.jgss extends it. When GSSManagerImpl is >> creating a GSSContext (or GSSCredential) object, it detects whether an >> extension is available and if so returns a "wrapped" object that has >> extended functions. >> >> Please note that in this fix all extended functions are actually implemented >> in the basic (say, GSSContextImpl) class, but they are only exported through >> the extended interface (say, ExtendedGSSContext) to application developers. >> >> The detection of whether an extension is available is now performed by >> calling "Class.forName("com.sun.security.jgss.Extender")". This will be >> revisited later if we have other handy methods to detect whether a module is >> available or be converted to a service loader. >> >> A sub-task (JDK-8056141) will move com.sun.security.jgss and >> com.sun.security.sasl.gsskerb into a new jdk.security.jgss module. >> >> > I've looked through the changes, I don't claim to know this area very well > and so I think it needs a Reviewer from the security area to help review too. > > At a high-level then the approach seems reasonable and I see how this will > fits with the second part that will facilitate the refactoring of > com.sun.security.jgss to another module. The Class.forName to tickle the > Extender to register seems okay. > > A minor comment on Extender is that I think I'd prefer the methods to named > "wrap" rather than "wrapped", also I assume you'll add a copyright header to > the file before you push. > > A minor comment on OrgIetfJgssExtender is that perhaps JgssExtender would be > a simpler name. Can the setExtender method be protected rather than public, I > assume it will only ever be called by sub-types. Should theOthe be volatile? > I can't tell if there are any race conditions here. As per Extender I would > have a preference for wrap rather than wrapped. > > I don't think I understand the update to test S4U2selfGSS.java, is this > related to this change? > > Otherwise I don't see any issues and overall this seems good work. > > -Alan.
