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
