----- Original Message ----- > 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! >
Ok, I'll push it later today, once I've done a test build. > > > 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 > Ok, I would have pointed out to them that it's not only inconsistent with the other lines in that debug message, but that "" and null are two different possible values. > I'd rather keep it in sync with 8 and 9. Yes, I agree this isn't the place to change it, as I said. > > > 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. Ok, I'm just not sure how they are different; the exported hg patch is in the webrev, isn't it? > > > > > > > > > > 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 > > -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: ed25519/35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
