Request for Approval: 6425769: Allow specifying an address to bind JMX remote connector

[Severin Gehwolf]

Hi,

On Fri, 2016-02-19 at 13:38 -0500, Andrew Hughes wrote:
> 
> ----- Original Message -----
> > Hi,
> > 
> > Please approve and sponsor the following fix for JDK-6425769 to 7u-dev.
> 
> I'm not sure what you mean by "sponsor". Do you mean you don't have commit
> access and need someone else to push for you? I can do that if so.

Yes. I don't have push access. Thanks!

> > It's a one-to-one backport from the fix which went into JDK 8 and JDK
> > 9. What needed changing is the test to account for JDK 7 and
> > accompanying testlibrary. There were also two subsequent test fixes in
> > JDK 8/9 for which I'd like to ask for backport approval as well.
> > 
> > Main bug:
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-6425769
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/jdk7.00/
> > HG export:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/jdk7-exports/
> > 
> > The actual patch is the same as in 9 and 8. Only slight modifications
> > to the test have been done to make it work with JDK 7.
> 
> This looks fine. I assume the tests ran fine on OpenJDK 7?

Yes. Test runs fine on OpenJDK 7. Fails unpatched. Passes with the
patch. I've used:

$ JT_JAVA=/path/to/jdk jtreg \
    test/java/lang/management \
    test/com/sun/management \
    test/sun/management

No change in test results (modulo the newly added test as described
above).

> The one line I would change is:
> 
> (bindAddress == null ? "" : "\n\t" + PropertyNames.HOST + "=" + bindAddress)
> 
> I don't see a need for a check as the toString result of null is "null", but
> no point changing it if it's already in 8 & 9.

This has specifically been asked for during review in 9:
http://mail.openjdk.java.net/pipermail/jmx-dev/2015-December/000889.html

I'd rather keep it in sync with 8 and 9.

> Also, not sure why you need two URLs here? What is the difference between
> webrev (the one I review) and 'HG export'?

Convenience. If you don't use the hg-exports, that's fine by me. Some
people prefer that over webrev patches. I didn't mean to confuse
people.

> > 
> > 
> > Test fixes for the test introduced with JDK-6425769:
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8145982
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8145982/jdk7.00/
> > HG export:
> >  http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8145982/jdk7-exports/JDK-8145982.export.jdk.patch
> > 
> > It's the very same patch as in 8 and 9.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8146015
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8146015/jdk7.00/
> > HG export:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8146015/jdk7-exports/JDK-8146015.export.jdk.patch
> > 
> > It's the same patch as in 8 and 9. It only accounts for surrounding code
> > changes.
> > 
> > Please let me know if there are questions.
> > 
> > Thanks,
> > Severin
> > 
> 
> Looks ok to me.

Thanks for the review!

Cheers,
Severin