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.
-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-6425769/
>>> 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
>>>
>>
>
