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 [email protected] 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
