Hi, Updated webrev with jtreg test in Java: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/02/ bug: https://bugs.openjdk.java.net/browse/JDK-6425769
I believe this updated webrev addresses all concerns and incorporates suggestions brought up by Jaroslav and Daniel. I'm still looking for a sponsor and a hotspot/servicability-dev reviewer. Could somebody maintaining javax.rmi.ssl have a look at this as well? Thank you! Cheers, Severin On Tue, 2015-11-03 at 15:45 +0100, Jaroslav Bachorik wrote: > On 2.11.2015 19:06, Severin Gehwolf wrote: > > Hi, > > > > Thanks Jaroslav and Daniel for the reviews! Comments inline. > > > > On Mon, 2015-11-02 at 16:54 +0100, Jaroslav Bachorik wrote: > > > Hi, > > > > > > On 2.11.2015 16:19, Daniel Fuchs wrote: > > > > Hi Severin, > > > > > > > > Adding serviceability-...@openjdk.java.net into the loop - > > > > that's > > > > a better alias than hotspot-dev for this kind of changes - > > > > maybe > > > > someone from serviceability-dev will offer to sponsor :-) > > > > > > > > I will let serviceability team members comment on the hotspot > > > > changes. > > > > > > > > ConnectorBootstrap.java > > > > > > > > I have one suggestion and one concern: > > > > > > > > Suggestion: I would suggest to reuse 'csf' (Client Socket > > > > Factory) > > > > and > > > > ssf (Server Socket Factory) variables rather than introduce > > > > the > > > > two > > > > new variables rmiServerSocketFactory and > > > > rmiClientSocketFactory. > > > > You might want to create a new boolean 'useSocketFactory' > > > > variable, > > > > if that makes the code more readable. > > > > > > > > Concern: If I understand correctly how RMI socket factories > > > > work, > > > > the client socket factory will be serialized and sent to the > > > > client > > > > side. This is problematic for interoperability, as the class > > > > may > > > > not > > > > present in the remote client - if the remote client is e.g. jdk > > > > 8. > > > > > > > > As far as I can see, your new DefaultClientSocketFactory > > > > doesn't do > > > > anything useful - so I would suggest to simply get rid of it, > > > > and > > > > only > > > > set the Server Socket Factory when SSL is not involved. > > > > Thanks. Fixed in updated webrev. > > > > > > Tests: > > > > > > > > Concerning the tests - we're trying to get rid of shell scripts > > > > rather than introducing new ones :-) > > > > Could the test be rewritten in pure java using the Process API? > > > > > > > > I believe there's even a test library that will let you do that > > > > easily jdk/test/lib/testlibrary/jdk/testlibrary/ > > > > (see ProcessTools.java) > > > > It'll take me a bit to rewrite the test in pure Java, but should be > > fine. This is not yet fixed in the updated webrev. > > > > > > Other: > > > > > > > > Also - I believe the new option should be documented in: > > > > src/java.management/share/conf/management.properties > > > > Docs have been updated > > in src/java.management/share/conf/management.properties. > > > > > I share Daniel's concerns. Also, the part of the changeset is > > > related > > > to javax.rmi.ssl - someone maintaining this library should also > > > comment here. > > > > > > Also, the change is introducing new API (system property) and > > > changing the existing one (adding SslRmiServerSocketFactory > > > public > > > constructors) so compatibility review process will have to be > > > involved. > > > > OK. What exactly is there for me to do? I'm not familiar with this > > process. Note that the intent would be to get this backported to > > JDK 8. > Not much for you. But for the potential Oracle sponsor this means she > will have to remember to go through some extra hoops before > integrating your patch. I see. Thanks for clarifying it. > -JB- > > > > > New webrev at: > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/01/ > > > > Thanks, > > Severin > > > > > -JB- > > > > > > > > best regards, > > > > > > > > -- daniel > > > > > > > > On 02/11/15 11:38, Severin Gehwolf wrote: > > > > > Hi, > > > > > > > > > > Here is a patch addressing JDK-6425769. The issue is that the > > > > > JMX > > > > > agent > > > > > binds to the wildcard address by default, preventing users to > > > > > use > > > > > system properties for JMX agents on multi-homed hosts. Given > > > > > a > > > > > host > > > > > with local network interfaces, 192.168.0.1 and 192.168.0.2 > > > > > say, > > > > > it's > > > > > impossible to start two JMX agents binding to fixed ports but > > > > > to > > > > > different network interfaces, 192.168.0.1:{9111,9112} and > > > > > 192.168.0.2:{9111,9112} say. > > > > > > > > > > The JDK would bind to all local interfaces by default. In the > > > > > above > > > > > example to 192.168.0.1 *and* 192.168.0.2. The effect is that > > > > > the > > > > > second > > > > > Java process would get a "Connection refused" error because > > > > > another > > > > > process has already been bound to the specified JMX/RMI port > > > > > pairs. > > > > > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-6425769 > > > > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425 > > > > > 769/ > > > > > 00/ > > > > > > > > > > Testing done: > > > > > jdk_jmx and jdk_management tests all pass after this change > > > > > (no > > > > > regressions). There is also a new JTREG test which fails > > > > > prior > > > > > this > > > > > change and passes after. Updates to the diagnostic command > > > > > have > > > > > been > > > > > tested manually. > > > > > > > > > > I understand that, if approved, the JDK and hotspot changes > > > > > should get > > > > > pushed together? Hotspot changes are fairly trivial since > > > > > it's > > > > > only a > > > > > doc-update for the new JDK property in the relevant > > > > > diagnostic > > > > > command. > > > > > > > > > > Could someone please review and sponsor this change? Please > > > > > let > > > > > me know > > > > > if there are questions. > > > > > > > > > > Thanks, > > > > > Severin > > > > > > > > > > > > > > >
