Re: [PING-2] RFR 6425769: jmx remote bind address
On Wed, 2015-12-09 at 14:58 +0100, Jaroslav Bachorik wrote: > On 9.12.2015 14:55, Severin Gehwolf wrote: > > On Tue, 2015-12-01 at 14:19 +0100, Severin Gehwolf wrote: > > > Hi Jaroslav, > > > > > > On Tue, 2015-12-01 at 12:33 +0100, Jaroslav Bachorik wrote: > > > > 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/jm > > > > xrem > > > > ote/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 > > > > > > Updated webrev which does not print > > > 'com.sun.management.jmxremote.host = null' if unset: > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/05/ > > > > > > > Other than that the change looks good to me. If no one else is > > > > volunteering I may sponsor this change. > > > > > > Thank you! > > > > Jaroslav, are you still willing to sponsor this? There hasn't been > > much > > movement :-/ > > Sure. I need to start the compatibility review process before > integration and it might take some time. OK. Thanks for the heads-up! Cheers, Severin > -JB- > > > > > Thanks, > > Severin > > >
Re: [PING-2] RFR 6425769: jmx remote bind address
On 9.12.2015 14:55, Severin Gehwolf wrote: On Tue, 2015-12-01 at 14:19 +0100, Severin Gehwolf wrote: Hi Jaroslav, On Tue, 2015-12-01 at 12:33 +0100, Jaroslav Bachorik wrote: 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/jmxrem ote/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 Updated webrev which does not print 'com.sun.management.jmxremote.host = null' if unset: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/05/ Other than that the change looks good to me. If no one else is volunteering I may sponsor this change. Thank you! Jaroslav, are you still willing to sponsor this? There hasn't been much movement :-/ Sure. I need to start the compatibility review process before integration and it might take some time. -JB- Thanks, Severin
Re: [PING] RFR 6425769: jmx remote bind address
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? > 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-...@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, > > >
Re: [PING] RFR 6425769: jmx remote bind address
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-...@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
Re: [PING] RFR 6425769: jmx remote bind address
Hi Jaroslav, On Tue, 2015-12-01 at 12:33 +0100, Jaroslav Bachorik wrote: > 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 Updated webrev which does not print 'com.sun.management.jmxremote.host = null' if unset: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/05/ > Other than that the change looks good to me. If no one else is > volunteering I may sponsor this change. Thank you! Cheers, Severin > Cheers, > > -JB-
Re: jmx-dev RFR 6425769: jmx remote bind address
Hi, Re-posting this for review since I've done another version which duplicates some of the code in SslRMIServerSocketFactory.java but does not change API other than adding the new property. I personally don't like it, but maybe this version is preferred? Bug: https://bugs.openjdk.java.net/browse/JDK-6425769 webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/03.no-rmi-ssl-factory-changes/ Thanks, Severin 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. > > 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-...@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 > > >
Re: jmx-dev RFR 6425769: jmx remote bind address
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. 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-...@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, > > > > > >
Re: jmx-dev RFR 6425769: jmx remote bind address
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
Re: jmx-dev RFR 6425769: jmx remote bind address
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
>>>
>>
>
Re: RFR 6425769: jmx remote bind address
Adding core libs.
On 2.11.2015 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
Re: jmx-dev RFR 6425769: jmx remote bind address
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.
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)
Other:
Also - I believe the new option should be documented 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.
-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
>
Re: jmx-dev RFR 6425769: jmx remote bind address
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.
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
> > >
> >
>
