[PING] RFR 6425769: jmx remote bind address

Tue Dec 1 11:33:38 UTC 2015

On 1.12.2015 11:17, Severin Gehwolf wrote:
> On Mon, 2015-11-09 at 10:32 +0100, Severin Gehwolf wrote:
>> On Wed, 2015-11-04 at 11:54 +0100, Severin Gehwolf wrote:
>>> 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!
>>
>> Ping? Friendly reminder that I still need reviewers and a sponsor for
>> this.
>
> Anyone?

I'm sorry for not spotting this earlier:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/03.no-rmi-ssl-factory-changes/jdk/src/java.management/share/classes/sun/management/jmxremote/ConnectorBootstrap.java.sdiff.html
* L442 - the log would contain 'com.sun.management.jmxremote.host = 
null' if host is not specified; might be better not to print this out at all

Other than that the change looks good to me. If no one else is 
volunteering I may sponsor this change.

Cheers,

-JB-

>
>> Thanks,
>> Severin
>>
>>> 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-dev at 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-
>>>>>>>> 64
>>>>>>>> 25
>>>>>>>> 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>