Re: RFR: 8268286: ProblemList serviceability/sa/TestJmapCore.java on linux-aarch64 with ZGC

[Roger Riggs]

On Sat, 5 Jun 2021 15:12:50 GMT, Daniel D. Daugherty  wrote:

> A trivial fix to ProblemList serviceability/sa/TestJmapCore.java on 
> linux-aarch64 with ZGC.

Marked as reviewed by rriggs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4374

Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v11]

[Roger Riggs]

On Thu, 8 Jul 2021 03:12:24 GMT, Yi Yang  wrote:

>> After JDK-8265518(#3615), it's possible to replace all variants of 
>> checkIndex by 
>> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in 
>> the whole JDK codebase.
>
> Yi Yang has refreshed the contents of this pull request, and previous commits 
> have been removed. The incremental views will show differences compared to 
> the previous content of the PR. The pull request contains one new commit 
> since the last revision:
> 
>   restore JSR166; restore java.desktop

Looks good.

-

Marked as reviewed by rriggs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4507

Re: Integrated: 8274294: ProblemList sun/tools/jmap/BasicJMapTest.java

[Roger Riggs]

On Fri, 24 Sep 2021 15:34:18 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to ProblemList sun/tools/jmap/BasicJMapTest.java.

Marked as reviewed by rriggs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5685

Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal

[Roger Riggs]

On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian  wrote:

> Here are the code changes for the "Deprecate finalizers in the standard Java 
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code 
> review.
> 
> This change makes the indicated deprecations, and updates the API spec for 
> JEP 421. It also updates the relevant @SuppressWarning annotations.
> 
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).

LGTM

-

Marked as reviewed by rriggs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6465

Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed

[Roger Riggs]

On Wed, 22 Dec 2021 21:41:13 GMT, Mandy Chung  wrote:

>> Remove the use of Security Manager from jstatd.
>> Add use of an ObjectInputFilter to restrict RMI.
>> 
>> Also we can undo the property-setting Launcher.gmk change from: 8279007: 
>> jstatd fails to start because SecurityManager is disabled
>> ..as that is no longer needed.
>> 
>> Docs/man page update to follow (JDK-8278619).
>
> src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java line 51:
> 
>> 49: private static RemoteHost remoteHost;
>> 50: 
>> 51: private static final String rmiFilterPattern = 
>> "sun.jvmstat.monitor.remote.RemoteVm;com.sun.proxy.jdk.proxy1.$Proxy1;com.sun.proxy.jdk.proxy1.$Proxy2;java.lang.reflect.Proxy;java.rmi.server.RemoteObjectInvocationHandler;java.rmi.server.RemoteObject;!*";
> 
> The class name of the dynamic proxy is generated at runtime and can be 
> different.   As Bernd commented, the proxy classes cannot/should not be 
> listed in the filter pattern.

@mlchung The Proxy class passed to the filter has been created in this VM from 
the interfaces listed.
The interfaces have already been filtered prior to creating the proxy.
The Proxy classes can safely be allowed based on a wildcard of the name. (As 
Stuart said).

-

PR: https://git.openjdk.java.net/jdk/pull/6919

Re: RFR: 8278597: Remove outdated comments regarding RMISecurityManager in HotSpotAgent.java

[Roger Riggs]

On Wed, 12 Jan 2022 15:04:04 GMT, Kevin Walls  wrote:

> The HotSpotAgent.java setupDebugger method has a commmented out section 
> relating to possibly using RMISecurityManager.
> The comment is from pre-jdk7.  As RMISecurityManager has been deprecated for 
> a while the comments should be removed.
> 
> This is a comment-only change, no impact on what we build, so seems trivial.
> 
> This is slightly more relevant today as with JEP411 and plans to remove the 
> Security Mananger, it is natural to search the source for references to 
> Security Manager.  Removing a false-positive from those results is a good 
> thing.

LGTM

-

Marked as reviewed by rriggs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7050

Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v2]

[Roger Riggs]

On Mon, 10 Jan 2022 11:17:12 GMT, Kevin Walls  wrote:

>> Remove the use of Security Manager from jstatd.
>> Add use of an ObjectInputFilter to restrict RMI.
>> 
>> Also we can undo the property-setting Launcher.gmk change from: 8279007: 
>> jstatd fails to start because SecurityManager is disabled
>> ..as that is no longer needed.
>> 
>> Docs/man page update to follow (JDK-8278619).
>
> Kevin Walls has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Wildcard in object filter to permit proxies, in case other activity in this 
> JVM changes the nameing/numbering of proxy classes.

LGTM

test/jdk/sun/tools/jstatd/JstatdTest.java line 2:

> 1: /*
> 2:  * Copyright (c) 2013, 2021, Oracle and/or its affiliates. All rights 
> reserved.

Update copyrights.

-

Marked as reviewed by rriggs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6919

Re: RFR: 8272777: Clean up remaining AccessController warnings in test library

[Roger Riggs]

On Wed, 2 Feb 2022 21:35:59 GMT, Kevin Walls  wrote:

> Reduce noise in test output by adding the @SuppressWarnings("removal") 
> annotation (which has already been widely applied).

Marked as reviewed by rriggs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7328

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Roger Riggs]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

We usually request that these be be broken up by area to attract the 
appropriate reviewers and avoid eye-strain.  The client modules are usually 
separated out, as are hotspot.
And corresponding tests.
This kind of change is pretty low value for the code base and requires the 
involvement of quite a few reviewers.
If you had ask ahead of time, I would have suggested finding something with 
more value.

-

Changes requested by rriggs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7268

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Roger Riggs]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

Marked as reviewed by rriggs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7268

Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]

[Roger Riggs]

On Tue, 22 Mar 2022 11:50:13 GMT, Fei Yang  wrote:

>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the 
>> https://openjdk.java.net/projects/riscv-port branch.
>> 
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive 
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are 
>> also carried out regularly. So it should be good enough to run most Java 
>> programs.
>> 
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address review comments

The test/jdk files look ok. (I didn't look at the rest)

-

Marked as reviewed by rriggs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6294

Re: RFR: 8282819: Deprecate Locale class constructors

[Roger Riggs]

On Thu, 24 Mar 2022 22:01:30 GMT, Naoto Sato  wrote:

> Proposing to deprecate the constructors in the `java.util.Locale` class. 
> There is already a factory method and a builder to return singletons, so 
> there is no need to have constructors anymore unless one purposefully wants 
> to create `ill-formed` Locale objects, which is discouraged. We cannot 
> terminally deprecate those constructors for the compatibility to serialized 
> objects created with older JDKs. Please see the draft CSR for more detail.

src/java.base/share/classes/java/util/Locale.java line 245:

> 243:  * Factory Method
> 244:  *
> 245:  * The method {@link #forLanguageTag} obtains a {@code Locale}

The factory name `forLanguageTag` is a bit off-putting, it doesn't seem like 
the best name for the factory.
Yes, it already exists and does what's required but you might get better uptake 
with a more natural name.

Some alternatives:
 - `Locale.of("en_US")` - short and conventional
 - `Locale.ofLanguage("en_US")` - 'of' prefix is used in other factories
 - `Locale.forLanguage("en_US")` - natural but less conventional

-

PR: https://git.openjdk.java.net/jdk/pull/7947

Re: RFR: 8282819: Deprecate Locale class constructors

[Roger Riggs]

On Thu, 24 Mar 2022 22:01:30 GMT, Naoto Sato  wrote:

> Proposing to deprecate the constructors in the `java.util.Locale` class. 
> There is already a factory method and a builder to return singletons, so 
> there is no need to have constructors anymore unless one purposefully wants 
> to create `ill-formed` Locale objects, which is discouraged. We cannot 
> terminally deprecate those constructors for the compatibility to serialized 
> objects created with older JDKs. Please see the draft CSR for more detail.

Given the large number of cleanups, I would have suggested to keep them 
separate from the change to re-focus the API on factories.

-

PR: https://git.openjdk.java.net/jdk/pull/7947

Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v3]

[Roger Riggs]

On Wed, 11 May 2022 13:27:38 GMT, Adam Sotona  wrote:

>> That's good to know. I think the tricky part is mostly about keeping track 
>> of all these disabled warnings, so they are not kept around longer than 
>> necessary. And that needs coordination with all the subtasks of the umbrella 
>> issue.
>
> Thanks for quick reaction.
> I'll keep my eyes on this race of patches and update this pull request 
> accordingly or create a new PR.

I put out a PR for java.base, but thought I'd wait until the javac fixe were 
pushed before integrating and would re-enable the warnings at the same time.

-

PR: https://git.openjdk.java.net/jdk/pull/8599

Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]

[Roger Riggs]

On Thu, 31 Mar 2022 08:03:23 GMT, Andrey Turbanov  wrote:

>> Method `Class.isAssignableFrom` is often used in form of:
>> 
>> if (clazz.isAssignableFrom(obj.getClass())) {
>> Such condition could be simplified to more shorter and performarnt code
>> 
>> if (clazz.isInstance(obj)) {
>> 
>> Replacement is equivalent if it's known that `obj != null`.
>> In JDK codebase there are many such places.
>> 
>> I tried to measure performance via JMH
>> 
>> Class integerClass = Integer.class;
>> Class numberClass = Number.class;
>> 
>> Object integerObject = 45666;
>> Object doubleObject = 8777d;
>> 
>> @Benchmark
>> public boolean integerInteger_isInstance() {
>> return integerClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean integerInteger_isAssignableFrom() {
>> return integerClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isInstance() {
>> return integerClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isAssignableFrom() {
>> return integerClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isInstance() {
>> return numberClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isAssignableFrom() {
>> return numberClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isInstance() {
>> return numberClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isAssignableFrom() {
>> return numberClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public void numberIntegerDouble_isInstance(Blackhole bh) {
>> bh.consume(numberClass.isInstance(integerObject));
>> bh.consume(numberClass.isInstance(doubleObject));
>> }
>> 
>> @Benchmark
>> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
>> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
>> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
>> }
>> 
>> `isInstance` is a bit faster than `isAssignableFrom`
>> 
>> Benchmark  Mode  Cnt  Score   Error  Units
>> integerDouble_isAssignableFrom avgt5  1,173 ± 0,026  ns/op
>> integerDouble_isInstance   avgt5  0,939 ± 0,038  ns/op
>> integerIntegerDouble_isAssignableFrom  avgt5  2,106 ± 0,068  ns/op
>> numberIntegerDouble_isInstance avgt5  1,516 ± 0,046  ns/op
>> integerInteger_isAssignableFromavgt5  1,175 ± 0,029  ns/op
>> integerInteger_isInstance  avgt5  0,886 ± 0,017  ns/op
>> numberDouble_isAssignableFrom  avgt5  1,172 ± 0,007  ns/op
>> numberDouble_isInstanceavgt5  0,891 ± 0,030  ns/op
>> numberInteger_isAssignableFrom avgt5  1,169 ± 0,014  ns/op
>> numberInteger_isInstance   avgt5  0,887 ± 0,016  ns/op
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8280035: Use Class.isInstance instead of Class.isAssignableFrom where 
> applicable
>   apply suggestion to avoid second Method.invoke call

Marked as reviewed by rriggs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7061

hg: jdk8/tl/jdk: 2 new changesets

[roger . riggs]

Changeset: 53819fd0ab61
Author:rriggs
Date:  2013-08-16 11:28 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/53819fd0ab61

8022770: java/time/tck/java/time/chrono/TCKChronology.java start failing
8022766: java/time/test/java/time/chrono/TestUmmAlQuraChronology.java failed.
Summary: TCKChronology: corrected display name to match update from JDK-8015986
Reviewed-by: alanb

! test/java/time/tck/java/time/chrono/TCKChronology.java
! test/java/time/test/java/time/chrono/TestUmmAlQuraChronology.java

Changeset: d7fc4e149bb8
Author:rriggs
Date:  2013-08-16 11:11 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d7fc4e149bb8

8022193: java/time/test/java/util/TestFormatter.java failed in th locale.
Summary: Tests corrected to use fixed locale and not dependent on the 
environment
Reviewed-by: sherman

! src/share/classes/java/util/Formatter.java
! test/java/time/test/java/util/TestFormatter.java

hg: jdk8/tl/jdk: 8019185: Inconsistency between JapaneseEra start dates and java.util.JapaneseImperialDate

[roger . riggs]

Changeset: acaa256e3f7c
Author:rriggs
Date:  2013-08-16 13:58 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/acaa256e3f7c

8019185: Inconsistency between JapaneseEra start dates and 
java.util.JapaneseImperialDate
Summary: align Meiji start date with lib/calendar.properties to avoid any 
confusion
Reviewed-by: sherman

! src/share/classes/java/time/chrono/JapaneseEra.java

hg: jdk8/tl/jdk: 8011944: Sort fails with ArrayIndexOutOfBoundsException

[roger . riggs]

Changeset: 07585a2483fa
Author:rriggs
Date:  2013-08-26 11:46 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/07585a2483fa

8011944: Sort fails with ArrayIndexOutOfBoundsException
Summary: Increase the size of pending stack and add test cases
Reviewed-by: alanb

! src/share/classes/java/util/ComparableTimSort.java
! src/share/classes/java/util/TimSort.java
+ test/java/util/Arrays/TimSortStackSize.java

hg: jdk8/tl/jdk: 2 new changesets

[roger . riggs]

Changeset: 292d93f32aa1
Author:rriggs
Date:  2013-09-11 10:16 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/292d93f32aa1

8024164: JSR310 serialization should be described in details
Summary: The serialized-form.html should specify the stream format for 
interoperability
Reviewed-by: alanb

! src/share/classes/java/time/Duration.java
! src/share/classes/java/time/Instant.java
! src/share/classes/java/time/LocalDate.java
! src/share/classes/java/time/LocalDateTime.java
! src/share/classes/java/time/LocalTime.java
! src/share/classes/java/time/MonthDay.java
! src/share/classes/java/time/OffsetDateTime.java
! src/share/classes/java/time/OffsetTime.java
! src/share/classes/java/time/Period.java
! src/share/classes/java/time/Ser.java
! src/share/classes/java/time/Year.java
! src/share/classes/java/time/YearMonth.java
! src/share/classes/java/time/ZoneId.java
! src/share/classes/java/time/ZoneOffset.java
! src/share/classes/java/time/ZoneRegion.java
! src/share/classes/java/time/ZonedDateTime.java
! src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
! src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java
! src/share/classes/java/time/chrono/Chronology.java
! src/share/classes/java/time/chrono/HijrahChronology.java
! src/share/classes/java/time/chrono/HijrahDate.java
! src/share/classes/java/time/chrono/HijrahEra.java
! src/share/classes/java/time/chrono/IsoChronology.java
! src/share/classes/java/time/chrono/JapaneseChronology.java
! src/share/classes/java/time/chrono/JapaneseDate.java
! src/share/classes/java/time/chrono/JapaneseEra.java
! src/share/classes/java/time/chrono/MinguoChronology.java
! src/share/classes/java/time/chrono/MinguoDate.java
! src/share/classes/java/time/chrono/MinguoEra.java
! src/share/classes/java/time/chrono/Ser.java
! src/share/classes/java/time/chrono/ThaiBuddhistChronology.java
! src/share/classes/java/time/chrono/ThaiBuddhistDate.java
! src/share/classes/java/time/chrono/ThaiBuddhistEra.java
! src/share/classes/java/time/zone/Ser.java
! src/share/classes/java/time/zone/ZoneOffsetTransition.java
! src/share/classes/java/time/zone/ZoneOffsetTransitionRule.java
! src/share/classes/java/time/zone/ZoneRules.java
! test/java/time/tck/java/time/chrono/TCKChronologySerialization.java

Changeset: 8b4aef582087
Author:rriggs
Date:  2013-09-11 10:35 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8b4aef582087

Merge

hg: jdk8/tl/jdk: 8024618: Issues with French locale on compact1, 2: expected: but was:

[roger . riggs]

Changeset: 672f349fbad7
Author:rriggs
Date:  2013-09-12 10:58 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/672f349fbad7

8024618: Issues with French locale on compact1,2: expected: but 
was:
Summary: Tests against the data of the French locale are not valid as 
conformance tests and are redundant with testing of the US Locale above
Reviewed-by: alanb

! test/java/time/tck/java/time/format/TCKDateTimeTextPrinting.java

hg: jdk8/tl/jdk: 2 new changesets

[roger . riggs]

Changeset: 3255a4e348a1
Author:rriggs
Date:  2013-09-14 13:55 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3255a4e348a1

8023639: Difference between LocalTime.now(Clock.systemDefaultZone()) and 
LocalTime.now() executed successively is more than 100 000 000 nanoseconds for 
slow machines
Summary: Test timed out on a slow machine; it is not a conformance test and 
should be in the test subtree
Reviewed-by: darcy, sherman

! test/java/time/tck/java/time/TCKLocalTime.java
! test/java/time/test/java/time/TestLocalTime.java

Changeset: 35bb1c7f227c
Author:rriggs
Date:  2013-09-14 13:55 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/35bb1c7f227c

8023556: Update javadoc for start of Meiji era
Summary: correct the javadoc in JapaneseEra.MEIJI to match the implementation
Reviewed-by: darcy, sherman

! src/share/classes/java/time/chrono/JapaneseEra.java
! test/java/time/test/java/time/TestLocalTime.java

hg: jdk8/tl/jdk: 2 new changesets

[roger . riggs]

Changeset: 01b8604e8268
Author:rriggs
Date:  2013-08-22 10:01 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/01b8604e8268

8024896: Refactor java.time serialization tests into separate subpackage
Summary: Move serialization tests to .serial subpackage
Reviewed-by: sherman
Contributed-by: paul.r...@oracle.com

! test/java/time/tck/java/time/TCKDuration.java
! test/java/time/tck/java/time/TCKInstant.java
! test/java/time/tck/java/time/TCKLocalDate.java
! test/java/time/tck/java/time/TCKLocalDateTime.java
! test/java/time/tck/java/time/TCKLocalTime.java
! test/java/time/tck/java/time/TCKMonthDay.java
! test/java/time/tck/java/time/TCKOffsetDateTime.java
! test/java/time/tck/java/time/TCKOffsetTime.java
! test/java/time/tck/java/time/TCKPeriod.java
! test/java/time/tck/java/time/TCKYear.java
! test/java/time/tck/java/time/TCKYearMonth.java
! test/java/time/tck/java/time/TCKZoneId.java
! test/java/time/tck/java/time/TCKZoneOffset.java
! test/java/time/tck/java/time/TCKZonedDateTime.java
! test/java/time/tck/java/time/chrono/TCKChronoLocalDate.java
! test/java/time/tck/java/time/chrono/TCKChronoLocalDateTime.java
- test/java/time/tck/java/time/chrono/TCKChronologySerialization.java
+ test/java/time/tck/java/time/chrono/serial/TCKChronoLocalDate.java
+ test/java/time/tck/java/time/chrono/serial/TCKChronoLocalDateTime.java
+ test/java/time/tck/java/time/chrono/serial/TCKChronologySerialization.java
+ test/java/time/tck/java/time/serial/TCKDuration.java
+ test/java/time/tck/java/time/serial/TCKInstant.java
+ test/java/time/tck/java/time/serial/TCKLocalDate.java
+ test/java/time/tck/java/time/serial/TCKLocalDateTime.java
+ test/java/time/tck/java/time/serial/TCKLocalTime.java
+ test/java/time/tck/java/time/serial/TCKMonthDay.java
+ test/java/time/tck/java/time/serial/TCKOffsetDateTime.java
+ test/java/time/tck/java/time/serial/TCKOffsetTime.java
+ test/java/time/tck/java/time/serial/TCKPeriod.java
+ test/java/time/tck/java/time/serial/TCKYear.java
+ test/java/time/tck/java/time/serial/TCKYearMonth.java
+ test/java/time/tck/java/time/serial/TCKZoneId.java
+ test/java/time/tck/java/time/serial/TCKZoneOffset.java
+ test/java/time/tck/java/time/serial/TCKZonedDateTime.java
! test/java/time/tck/java/time/temporal/TCKWeekFields.java
+ test/java/time/tck/java/time/temporal/serial/TCKWeekFields.java
! test/java/time/tck/java/time/zone/TCKFixedZoneRules.java
! test/java/time/tck/java/time/zone/TCKZoneOffsetTransition.java
! test/java/time/tck/java/time/zone/TCKZoneOffsetTransitionRule.java
! test/java/time/tck/java/time/zone/TCKZoneRules.java
+ test/java/time/tck/java/time/zone/serial/TCKFixedZoneRules.java
+ test/java/time/tck/java/time/zone/serial/TCKZoneOffsetTransition.java
+ test/java/time/tck/java/time/zone/serial/TCKZoneOffsetTransitionRule.java
+ test/java/time/tck/java/time/zone/serial/TCKZoneRules.java

Changeset: e813b58bd6db
Author:rriggs
Date:  2013-10-03 15:16 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e813b58bd6db

8024427: Missing java.time.chrono serialization tests
Summary: Add tests and cleanup existing serialization tests
Reviewed-by: sherman

! src/share/classes/java/time/temporal/ValueRange.java
! test/java/time/tck/java/time/AbstractTCKTest.java
! test/java/time/tck/java/time/TCKClock_Fixed.java
! test/java/time/tck/java/time/TCKClock_Offset.java
! test/java/time/tck/java/time/TCKClock_System.java
! test/java/time/tck/java/time/TCKClock_Tick.java
! test/java/time/tck/java/time/chrono/CopticDate.java
! test/java/time/tck/java/time/chrono/TCKChronoZonedDateTime.java
! test/java/time/tck/java/time/chrono/TCKChronology.java
! test/java/time/tck/java/time/chrono/TCKTestServiceLoader.java
! 
test/java/time/tck/java/time/chrono/serial/TCKChronoLocalDateSerialization.java 
< test/java/time/tck/java/time/chrono/serial/TCKChronoLocalDate.java
! 
test/java/time/tck/java/time/chrono/serial/TCKChronoLocalDateTimeSerialization.java
 < test/java/time/tck/java/time/chrono/serial/TCKChronoLocalDateTime.java
+ 
test/java/time/tck/java/time/chrono/serial/TCKChronoZonedDateTimeSerialization.java
! test/java/time/tck/java/time/chrono/serial/TCKChronologySerialization.java
+ test/java/time/tck/java/time/chrono/serial/TCKCopticSerialization.java
+ test/java/time/tck/java/time/chrono/serial/TCKEraSerialization.java
+ test/java/time/tck/java/time/serial/TCKClockSerialization.java
! test/java/time/tck/java/time/serial/TCKDurationSerialization.java < 
test/java/time/tck/java/time/serial/TCKDuration.java
! test/java/time/tck/java/time/serial/TCKInstantSerialization.java < 
test/java/time/tck/java/time/serial/TCKInstant.java
! test/java/time/tck/java/time/serial/TCKLocalDateSerialization.java < 
test/java/time/tck/java/time/serial/TCKLocalDate.java
! test/java/time/tck/java/time/serial/TCKLocalDateTimeSerialization.java < 
test/java/time/tck/java/time/serial/TCKLocalDateTime.java
! test/java/time/tck/java/time/serial/TCKLocalTimeSerialization.java < 
test/java/time/tck/java/time/serial

hg: jdk8/tl/jdk: 7 new changesets

[roger . riggs]

Changeset: 1de0fac9b962
Author:rriggs
Date:  2013-08-29 20:38 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1de0fac9b962

8023764: Optimize Period addition
Summary: Optimise plus/minus for common cases
Reviewed-by: sherman
Contributed-by: scolebou...@joda.org

! src/share/classes/java/time/LocalDate.java
! src/share/classes/java/time/LocalDateTime.java
! src/share/classes/java/time/ZonedDateTime.java

Changeset: 356df1b99976
Author:rriggs
Date:  2013-08-30 11:43 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/356df1b99976

8023763: Rename ChronoDateImpl
Summary: Rename ChronoDateImpl to ChronoLocalDateImpl
Reviewed-by: sherman
Contributed-by: scolebou...@joda.org

- src/share/classes/java/time/chrono/ChronoDateImpl.java
! src/share/classes/java/time/chrono/ChronoLocalDate.java
+ src/share/classes/java/time/chrono/ChronoLocalDateImpl.java
! src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
! src/share/classes/java/time/chrono/HijrahDate.java
! src/share/classes/java/time/chrono/JapaneseDate.java
! src/share/classes/java/time/chrono/MinguoDate.java
! src/share/classes/java/time/chrono/ThaiBuddhistDate.java

Changeset: 5d73f7a2db51
Author:rriggs
Date:  2013-09-04 15:18 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/5d73f7a2db51

8023762: Add ChronoPeriod interface and bind period to Chronology
Summary: Make Period ISO-only, adding a Chronology-specific period concept
Reviewed-by: sherman
Contributed-by: scolebou...@joda.org

! src/share/classes/java/time/LocalDate.java
! src/share/classes/java/time/Period.java
! src/share/classes/java/time/chrono/ChronoLocalDate.java
+ src/share/classes/java/time/chrono/ChronoPeriod.java
+ src/share/classes/java/time/chrono/ChronoPeriodImpl.java
! src/share/classes/java/time/chrono/Chronology.java
! src/share/classes/java/time/chrono/HijrahDate.java
! src/share/classes/java/time/chrono/IsoChronology.java
! src/share/classes/java/time/chrono/JapaneseDate.java
! src/share/classes/java/time/chrono/MinguoDate.java
! src/share/classes/java/time/chrono/Ser.java
! src/share/classes/java/time/chrono/ThaiBuddhistDate.java
! src/share/classes/java/time/temporal/Temporal.java
! test/java/time/tck/java/time/TCKPeriod.java
+ test/java/time/tck/java/time/chrono/TCKChronoPeriod.java
! test/java/time/tck/java/time/chrono/TCKJapaneseChronology.java
! test/java/time/tck/java/time/chrono/TCKMinguoChronology.java
! test/java/time/tck/java/time/chrono/TCKThaiBuddhistChronology.java
! test/java/time/test/java/time/chrono/TestUmmAlQuraChronology.java

Changeset: 79077f1641cc
Author:rriggs
Date:  2013-09-14 22:46 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/79077f1641cc

8024835: Change until() to accept any compatible temporal
Summary: Method until(Temporal,TemporalUnit) now uses from() to convert; 
Enhance from() methods where necessary
Reviewed-by: sherman
Contributed-by: scolebou...@joda.org

! src/share/classes/java/time/Duration.java
! src/share/classes/java/time/Instant.java
! src/share/classes/java/time/LocalDate.java
! src/share/classes/java/time/LocalDateTime.java
! src/share/classes/java/time/LocalTime.java
! src/share/classes/java/time/MonthDay.java
! src/share/classes/java/time/OffsetDateTime.java
! src/share/classes/java/time/OffsetTime.java
! src/share/classes/java/time/Year.java
! src/share/classes/java/time/YearMonth.java
! src/share/classes/java/time/ZoneOffset.java
! src/share/classes/java/time/ZonedDateTime.java
! src/share/classes/java/time/chrono/ChronoLocalDate.java
! src/share/classes/java/time/chrono/ChronoLocalDateImpl.java
! src/share/classes/java/time/chrono/ChronoLocalDateTime.java
! src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
! src/share/classes/java/time/chrono/ChronoZonedDateTime.java
! src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java
! src/share/classes/java/time/temporal/ChronoUnit.java
! src/share/classes/java/time/temporal/IsoFields.java
! src/share/classes/java/time/temporal/Temporal.java
! src/share/classes/java/time/temporal/TemporalUnit.java
! test/java/time/tck/java/time/TCKDuration.java
! test/java/time/tck/java/time/TCKInstant.java
! test/java/time/tck/java/time/TCKLocalDate.java
! test/java/time/tck/java/time/TCKLocalDateTime.java
! test/java/time/tck/java/time/TCKLocalTime.java
! test/java/time/tck/java/time/TCKOffsetDateTime.java
! test/java/time/tck/java/time/TCKOffsetTime.java
! test/java/time/tck/java/time/TCKYear.java
! test/java/time/tck/java/time/TCKYearMonth.java
! test/java/time/tck/java/time/TCKZonedDateTime.java
! test/java/time/tck/java/time/chrono/CopticDate.java
! test/java/time/tck/java/time/temporal/TCKIsoFields.java

Changeset: 14a187dc4ffe
Author:rriggs
Date:  2013-10-04 12:01 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/14a187dc4ffe

8024999: Instant.Parse typo in example
Summary: javadoc only fix to correct example to use "." and "Z"
Reviewed-by: sherman

! src/share/classes/java/

hg: jdk8/tl/jdk: 2 new changesets

[roger . riggs]

Changeset: e3b70e601c1c
Author:rriggs
Date:  2013-10-09 11:02 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e3b70e601c1c

8024612: java/time/tck/java/time/format/TCKDateTimeFormatters.java failed
Summary: The test should be using the Locale.Category.FORMAT to verify the test 
data
Reviewed-by: lancea

! test/java/time/tck/java/time/format/TCKDateTimeFormatters.java

Changeset: 09e2a73aa1dc
Author:rriggs
Date:  2013-09-26 15:19 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/09e2a73aa1dc

8025718: Enhance error messages for parsing
Summary: Add values and types to exception messages
Reviewed-by: lancea
Contributed-by: scolebou...@joda.org

! src/share/classes/java/time/DayOfWeek.java
! src/share/classes/java/time/Instant.java
! src/share/classes/java/time/LocalDate.java
! src/share/classes/java/time/LocalDateTime.java
! src/share/classes/java/time/LocalTime.java
! src/share/classes/java/time/Month.java
! src/share/classes/java/time/MonthDay.java
! src/share/classes/java/time/OffsetDateTime.java
! src/share/classes/java/time/OffsetTime.java
! src/share/classes/java/time/Year.java
! src/share/classes/java/time/YearMonth.java
! src/share/classes/java/time/ZoneId.java
! src/share/classes/java/time/ZoneOffset.java
! src/share/classes/java/time/ZonedDateTime.java
! src/share/classes/java/time/format/Parsed.java
! test/java/time/test/java/time/format/TestDateTimeFormatter.java

hg: jdk8/tl/jdk: 8024076: Incorrect 2 -> 4 year parsing and resolution in DateTimeFormatter

[roger . riggs]

Changeset: d42fe440bda8
Author:rriggs
Date:  2013-10-09 13:34 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d42fe440bda8

8024076: Incorrect 2 -> 4 year parsing and resolution in DateTimeFormatter
Summary: Add appendValueReduced method based on a ChronoLocalDate to provide 
context for the value
Reviewed-by: sherman
Contributed-by: scolebou...@joda.org

! src/share/classes/java/time/format/DateTimeFormatterBuilder.java
! test/java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java
! test/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
! test/java/time/test/java/time/format/TestReducedParser.java
! test/java/time/test/java/time/format/TestReducedPrinter.java

hg: jdk8/tl/jdk: 3 new changesets

[roger . riggs]

Changeset: ea422834f880
Author:rriggs
Date:  2013-09-26 23:05 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ea422834f880

8025720: Separate temporal interface layer
Summary: Remove ZoneId and Chronology from TemporalField interface
Reviewed-by: sherman
Contributed-by: scolebou...@joda.org

! src/share/classes/java/time/format/Parsed.java
! src/share/classes/java/time/temporal/IsoFields.java
! src/share/classes/java/time/temporal/JulianFields.java
! src/share/classes/java/time/temporal/TemporalField.java
! src/share/classes/java/time/temporal/WeekFields.java
! test/java/time/tck/java/time/format/TCKDateTimeParseResolver.java

Changeset: 110107410393
Author:scolebourne
Date:  2013-07-09 21:35 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/110107410393

8025719: Change Chronology to an interface
Summary: Split Chronology and add AbstractChronology
Reviewed-by: darcy
Contributed-by: scolebou...@joda.org

+ src/share/classes/java/time/chrono/AbstractChronology.java
! src/share/classes/java/time/chrono/ChronoLocalDate.java
! src/share/classes/java/time/chrono/ChronoLocalDateTime.java
! src/share/classes/java/time/chrono/ChronoZonedDateTime.java
! src/share/classes/java/time/chrono/Chronology.java
! src/share/classes/java/time/chrono/HijrahChronology.java
! src/share/classes/java/time/chrono/IsoChronology.java
! src/share/classes/java/time/chrono/JapaneseChronology.java
! src/share/classes/java/time/chrono/MinguoChronology.java
! src/share/classes/java/time/chrono/Ser.java
! src/share/classes/java/time/chrono/ThaiBuddhistChronology.java
! test/java/time/tck/java/time/chrono/CopticChronology.java

Changeset: 087c8c1d2631
Author:rriggs
Date:  2013-10-15 13:14 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/087c8c1d2631

8025722: TemporalAdjusters and TemporalQueries
Summary: Move static from interfaces methods to supporting classes
Reviewed-by: sherman

! src/share/classes/java/time/DayOfWeek.java
! src/share/classes/java/time/Instant.java
! src/share/classes/java/time/LocalDate.java
! src/share/classes/java/time/LocalDateTime.java
! src/share/classes/java/time/LocalTime.java
! src/share/classes/java/time/Month.java
! src/share/classes/java/time/MonthDay.java
! src/share/classes/java/time/OffsetDateTime.java
! src/share/classes/java/time/OffsetTime.java
! src/share/classes/java/time/Period.java
! src/share/classes/java/time/Year.java
! src/share/classes/java/time/YearMonth.java
! src/share/classes/java/time/ZoneId.java
! src/share/classes/java/time/ZoneOffset.java
! src/share/classes/java/time/chrono/AbstractChronology.java
! src/share/classes/java/time/chrono/ChronoLocalDate.java
! src/share/classes/java/time/chrono/ChronoLocalDateTime.java
! src/share/classes/java/time/chrono/ChronoPeriodImpl.java
! src/share/classes/java/time/chrono/ChronoZonedDateTime.java
! src/share/classes/java/time/chrono/Chronology.java
! src/share/classes/java/time/chrono/Era.java
! src/share/classes/java/time/chrono/JapaneseChronology.java
! src/share/classes/java/time/format/DateTimeFormatterBuilder.java
! src/share/classes/java/time/format/DateTimePrintContext.java
! src/share/classes/java/time/format/Parsed.java
! src/share/classes/java/time/temporal/TemporalAccessor.java
! src/share/classes/java/time/temporal/TemporalAdjuster.java
! src/share/classes/java/time/temporal/TemporalAdjusters.java
! src/share/classes/java/time/temporal/TemporalAmount.java
! src/share/classes/java/time/temporal/TemporalQueries.java
! src/share/classes/java/time/temporal/TemporalQuery.java
! src/share/classes/java/time/temporal/package-info.java
! src/share/classes/java/time/zone/ZoneOffsetTransitionRule.java
! src/share/classes/java/util/Formatter.java
! test/java/time/tck/java/time/TCKDayOfWeek.java
! test/java/time/tck/java/time/TCKInstant.java
! test/java/time/tck/java/time/TCKLocalDate.java
! test/java/time/tck/java/time/TCKLocalDateTime.java
! test/java/time/tck/java/time/TCKLocalTime.java
! test/java/time/tck/java/time/TCKMonth.java
! test/java/time/tck/java/time/TCKMonthDay.java
! test/java/time/tck/java/time/TCKOffsetDateTime.java
! test/java/time/tck/java/time/TCKOffsetTime.java
! test/java/time/tck/java/time/TCKYear.java
! test/java/time/tck/java/time/TCKYearMonth.java
! test/java/time/tck/java/time/TCKZoneId.java
! test/java/time/tck/java/time/TCKZoneOffset.java
! test/java/time/tck/java/time/TCKZonedDateTime.java
! test/java/time/tck/java/time/TestIsoChronology.java
! test/java/time/tck/java/time/chrono/CopticDate.java
! test/java/time/tck/java/time/chrono/TCKChronoLocalDateTime.java
! test/java/time/tck/java/time/chrono/TCKChronoZonedDateTime.java
! test/java/time/tck/java/time/chrono/TCKIsoChronology.java
! test/java/time/tck/java/time/chrono/TCKJapaneseChronology.java
! test/java/time/tck/java/time/chrono/TCKMinguoChronology.java
! test/java/time/tck/java/time/chrono/TCKThaiBuddhistChronology.java
! test/java/time/tck/java/time/format/TCKChronoPrinterParser.java
! test/java/time/t

hg: jdk8/tl/jdk: 8026516: javadoc errors in java.time

[roger . riggs]

Changeset: 36fe6a9bd43e
Author:rriggs
Date:  2013-10-17 10:37 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/36fe6a9bd43e

8026516: javadoc errors in java.time
Summary: Corrected links to TemporalQuery and TemporalField.resolve
Reviewed-by: mduigou, darcy, lancea

! src/share/classes/java/time/Instant.java
! src/share/classes/java/time/LocalDateTime.java
! src/share/classes/java/time/LocalTime.java
! src/share/classes/java/time/OffsetDateTime.java
! src/share/classes/java/time/OffsetTime.java
! src/share/classes/java/time/ZoneId.java
! src/share/classes/java/time/ZoneOffset.java
! src/share/classes/java/time/ZonedDateTime.java
! src/share/classes/java/time/chrono/ChronoLocalDate.java
! src/share/classes/java/time/chrono/Chronology.java
! src/share/classes/java/time/chrono/Era.java
! src/share/classes/java/time/chrono/IsoChronology.java
! src/share/classes/java/time/chrono/IsoEra.java
! src/share/classes/java/time/chrono/MinguoChronology.java
! src/share/classes/java/time/chrono/MinguoEra.java
! src/share/classes/java/time/chrono/ThaiBuddhistChronology.java
! src/share/classes/java/time/chrono/ThaiBuddhistEra.java
! src/share/classes/java/time/format/DateTimeFormatter.java
! src/share/classes/java/time/format/DateTimeFormatterBuilder.java
! src/share/classes/java/time/temporal/IsoFields.java
! src/share/classes/java/time/temporal/JulianFields.java
! src/share/classes/java/time/temporal/Temporal.java
! src/share/classes/java/time/temporal/WeekFields.java
! src/share/classes/java/time/zone/ZoneOffsetTransitionRule.java
! src/share/classes/java/time/zone/ZoneRules.java

hg: jdk8/tl/jdk: 8026183: minor documentation problems in java.lang.invoke; ...

[roger . riggs]

Changeset: 456a9b199208
Author:rriggs
Date:  2013-10-17 13:43 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/456a9b199208

8026183: minor documentation problems in java.lang.invoke
8015808: Typo in MethodHandle javadoc
Summary: Fix typos and javadoc markup and extraneous paragraph tags
Reviewed-by: lancea

! src/share/classes/java/lang/invoke/CallSite.java
! src/share/classes/java/lang/invoke/ConstantCallSite.java
! src/share/classes/java/lang/invoke/MethodHandle.java
! src/share/classes/java/lang/invoke/MethodHandles.java
! src/share/classes/java/lang/invoke/MethodType.java

hg: jdk8/tl/jdk: 8025828: Late binding of Chronology to appendValueReduced

[roger . riggs]

Changeset: 7a947daa8f51
Author:rriggs
Date:  2013-10-18 16:37 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7a947daa8f51

8025828: Late binding of Chronology to appendValueReduced
Summary: Add a listener to the parseContext called when the Chronology changes
Reviewed-by: sherman

! src/share/classes/java/time/format/DateTimeFormatterBuilder.java
! src/share/classes/java/time/format/DateTimeParseContext.java
! test/java/time/test/java/time/format/TestReducedParser.java

hg: jdk8/tl/jdk: 3 new changesets

[roger . riggs]

Changeset: e2b814e68956
Author:rriggs
Date:  2013-10-22 15:03 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e2b814e68956

8024686: Cleanup of java.time serialization source
Summary: optimize serialized form of OffsetTime, OffsetDateTime; correct order 
of modifiers
Reviewed-by: sherman

! src/share/classes/java/time/Duration.java
! src/share/classes/java/time/OffsetDateTime.java
! src/share/classes/java/time/OffsetTime.java
! src/share/classes/java/time/Period.java
! src/share/classes/java/time/chrono/ChronoPeriodImpl.java
! src/share/classes/java/time/chrono/JapaneseDate.java
! src/share/classes/java/time/temporal/WeekFields.java
! src/share/classes/java/time/zone/Ser.java
! src/share/classes/java/time/zone/ZoneOffsetTransition.java
! test/java/time/tck/java/time/serial/TCKOffsetDateTimeSerialization.java
! test/java/time/tck/java/time/serial/TCKOffsetTimeSerialization.java

Changeset: d5c2b125ed0f
Author:rriggs
Date:  2013-10-22 15:06 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d5c2b125ed0f

Merge


Changeset: cd60848c87b2
Author:rriggs
Date:  2013-10-22 15:11 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/cd60848c87b2

Merge

hg: jdk8/tl/jdk: 8026982: javadoc errors in core libs

[roger . riggs]

Changeset: 4bb758a77fd7
Author:rriggs
Date:  2013-10-22 17:02 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4bb758a77fd7

8026982: javadoc errors in core libs
Summary: Cleanup of javadoc -Xlint errors
Reviewed-by: lancea, mduigou, darcy, mullan, mchung

! src/share/classes/java/io/ByteArrayInputStream.java
! src/share/classes/java/io/ByteArrayOutputStream.java
! src/share/classes/java/io/DataInput.java
! src/share/classes/java/io/DataOutput.java
! src/share/classes/java/io/FilePermission.java
! src/share/classes/java/io/InputStream.java
! src/share/classes/java/io/ObjectInputStream.java
! src/share/classes/java/io/PipedInputStream.java
! src/share/classes/java/io/PipedReader.java
! src/share/classes/java/io/RandomAccessFile.java
! src/share/classes/java/io/Serializable.java
! src/share/classes/java/io/SerializablePermission.java
! src/share/classes/java/lang/AbstractStringBuilder.java
! src/share/classes/java/lang/ArrayStoreException.java
! src/share/classes/java/lang/Byte.java
! src/share/classes/java/lang/Character.java
! src/share/classes/java/lang/Class.java
! src/share/classes/java/lang/ClassCastException.java
! src/share/classes/java/lang/Double.java
! src/share/classes/java/lang/Float.java
! src/share/classes/java/lang/Integer.java
! src/share/classes/java/lang/Iterable.java
! src/share/classes/java/lang/Long.java
! src/share/classes/java/lang/RuntimePermission.java
! src/share/classes/java/lang/Short.java
! src/share/classes/java/lang/String.java
! src/share/classes/java/lang/Thread.java
! src/share/classes/java/lang/management/ManagementFactory.java
! src/share/classes/java/lang/management/ManagementPermission.java
! src/share/classes/java/lang/management/MemoryUsage.java
! src/share/classes/java/lang/management/package.html
! src/share/classes/java/lang/reflect/ReflectPermission.java
! src/share/classes/java/net/DatagramSocket.java
! src/share/classes/java/net/Inet6Address.java
! src/share/classes/java/net/MulticastSocket.java
! src/share/classes/java/net/NetPermission.java
! src/share/classes/java/net/Proxy.java
! src/share/classes/java/net/Socket.java
! src/share/classes/java/net/SocketOptions.java
! src/share/classes/java/net/SocketPermission.java
! src/share/classes/java/net/URI.java
! src/share/classes/java/net/URLConnection.java
! src/share/classes/java/net/URLDecoder.java
! src/share/classes/java/net/URLEncoder.java
! src/share/classes/java/net/URLPermission.java
! src/share/classes/java/net/package-info.java
! src/share/classes/java/nio/X-Buffer.java.template
! src/share/classes/java/nio/file/FileSystem.java
! src/share/classes/java/rmi/activation/ActivationGroup.java
! src/share/classes/java/rmi/dgc/VMID.java
! src/share/classes/java/rmi/server/UnicastRemoteObject.java
! src/share/classes/java/security/AccessController.java
! src/share/classes/java/security/AlgorithmParameterGenerator.java
! src/share/classes/java/security/BasicPermission.java
! src/share/classes/java/security/CodeSource.java
! src/share/classes/java/security/Key.java
! src/share/classes/java/security/KeyPairGenerator.java
! src/share/classes/java/security/KeyStore.java
! src/share/classes/java/security/MessageDigest.java
! src/share/classes/java/security/Permission.java
! src/share/classes/java/security/PermissionCollection.java
! src/share/classes/java/security/SecurityPermission.java
! src/share/classes/java/security/Signature.java
! src/share/classes/java/security/SignedObject.java
! src/share/classes/java/security/acl/Acl.java
! src/share/classes/java/security/cert/CertificateFactory.java
! src/share/classes/java/security/cert/PKIXRevocationChecker.java
! src/share/classes/java/security/cert/PolicyQualifierInfo.java
! src/share/classes/java/security/cert/TrustAnchor.java
! src/share/classes/java/security/cert/X509CertSelector.java
! src/share/classes/java/security/interfaces/DSAKeyPairGenerator.java
! src/share/classes/java/text/MessageFormat.java
! src/share/classes/java/text/Normalizer.java
! src/share/classes/java/text/SimpleDateFormat.java
! src/share/classes/java/util/Base64.java
! src/share/classes/java/util/BitSet.java
! src/share/classes/java/util/Deque.java
! src/share/classes/java/util/Locale.java
! src/share/classes/java/util/Properties.java
! src/share/classes/java/util/PropertyPermission.java
! src/share/classes/java/util/Queue.java
! src/share/classes/java/util/ResourceBundle.java
! src/share/classes/java/util/Scanner.java
! src/share/classes/java/util/TimeZone.java
! src/share/classes/java/util/UUID.java
! src/share/classes/java/util/concurrent/BlockingDeque.java
! src/share/classes/java/util/concurrent/BlockingQueue.java
! src/share/classes/java/util/concurrent/Future.java
! src/share/classes/java/util/concurrent/locks/ReentrantReadWriteLock.java
! src/share/classes/java/util/jar/Pack200.java
! src/share/classes/java/util/logging/Logger.java
! src/share/classes/java/util/regex/Pattern.java
! src/share/classes/java/util/spi/LocaleServiceProvider.java

Re: JDK 14 RFR of JDK-8232442: Suppress warnings on non-serializable non-transient instance fields in java.management.*

[Roger Riggs]

Hi Joe,

These look ok.

A few of them might be handled by making them transient, but 
SuppressWarnings
calls more attention to them as fields that are being serialized, even 
indirectly.
The presence of a writeReplace method weakens the coupling of the fields 
to the serialized values.


Roger


On 10/16/19 10:31 PM, Joe Darcy wrote:

Hello,

Quick background, ahead of strengthening javac's -Xlint:serial checks 
to include more aspects of a Serializable or Externalizable type ( 
JDK-8160675 ), I've been working through the JDK libraries to review 
issue found by the upcoming analysis (JDK-8231641).


The latest segment of the libraries to get this treatment is the 
java.management and java.management.rmi modules:


    JDK-8232442: Suppress warnings on non-serializable non-transient 
instance fields in java.management.*

    http://cr.openjdk.java.net/~darcy/8232442.0/

The forthcoming warnings were generally suppressed using

    @SuppressWarnings("serial")

paired with an explanatory comment. Common comments are

    // Not statically typed as Serializable

which states the cause of the general warning and

    // Conditionally serializable

which is used for collections or collection-like objects that are 
designed to be serializable if and only if their contents are.


An Externalizable class is required by the spec to have a no-arg 
constructor. Two Externalizable classes in this review do *not* have 
such a constructor. The classes have both been in the platform for 
many years and I just suppressed the warning rather than adding the 
constructors.


Thanks,

-Joe

RFR 8232622: Technical debt in BadAttributeValueExpException

[Roger Riggs]

Please review a minor cleanup to remove code long since unnecessary.
The type of the BadAttributeValueExpException argument is String and
if it is not a string in the serialized stream, a suitable replacement 
is created.


Issue: https://bugs.openjdk.java.net/browse/JDK-8232622

Patch:

diff --git 
a/src/java.management/share/classes/javax/management/BadAttributeValueExpException.java 
b/src/java.management/share/classes/javax/management/BadAttributeValueExpException.java
--- 
a/src/java.management/share/classes/javax/management/BadAttributeValueExpException.java
+++ 
b/src/java.management/share/classes/javax/management/BadAttributeValueExpException.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1999, 2020, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -48,7 +48,7 @@ public class BadAttributeValueExpExcepti
  * for example, the string value can be the return of {@code 
attribute.toString()}

  */
 @SuppressWarnings("serial") // See handling in constructor and 
readObject

-    private Object val;
+    private String val;

 /**
  * Constructs a BadAttributeValueExpException using the specified 
Object to

@@ -72,19 +72,8 @@ public class BadAttributeValueExpExcepti
 ObjectInputStream.GetField gf = ois.readFields();
 Object valObj = gf.get("val", null);

-    if (valObj == null) {
-    val = null;
-    } else if (valObj instanceof String) {
-    val= valObj;
-    } else if (System.getSecurityManager() == null
-    || valObj instanceof Long
-    || valObj instanceof Integer
-    || valObj instanceof Float
-    || valObj instanceof Double
-    || valObj instanceof Byte
-    || valObj instanceof Short
-    || valObj instanceof Boolean) {
-    val = valObj.toString();
+    if (valObj instanceof String || valObj == null) {
+    val = (String)valObj;
 } else { // the serialized object is from a version without 
JDK-8019292 fix
 val = System.identityHashCode(valObj) + "@" + 
valObj.getClass().getName();

 }


Thanks, Roger

Re: RFR 8232622: Technical debt in BadAttributeValueExpException

[Roger Riggs]

Hi Daniel,

That's part of the technical debt that has to go.

The change to do the conversion in the constructor has been there since 
JDK8.

And the value of the argument is informative.

There isn't too much overhead in keeping them but the are just noise at 
this point.


Thanks, Roger


On 2/13/20 12:38 PM, Daniel Fuchs wrote:

Hi Roger,

I think you will need to preserve these cases:

On 13/02/2020 15:52, Roger Riggs wrote:

-    || valObj instanceof Long
-    || valObj instanceof Integer
-    || valObj instanceof Float
-    || valObj instanceof Double
-    || valObj instanceof Byte
-    || valObj instanceof Short
-    || valObj instanceof Boolean) {


They could legitimately be transmitted by an older unpatched JVM.
we don't want to use System.identityHashCode(valObj) + "@" + 
valObj.getClass().getName(); for these.


best regards,

-- daniel

Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools

[Roger Riggs]

Hi Daniil,

In the grand scheme of things, could servicability use the signature 
parsing support in HotSpot?


Thanks, Roger


On 5/9/20 12:29 PM, Daniil Titov wrote:

Please review a change[1] that centralizes the signature processing in 
serviceability tools to make it capable of being easily extensible in the 
future.

Testing: Mach5 tier1-tier3 tests successfully passed.

[1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01
[2] https://bugs.openjdk.java.net/browse/JDK-8241080

Thank you,
Daniil

Re: RFR 8197387: jcmd started by "root" must be allowed to access all VM processes

[Roger Riggs]

Hi,

- The incantations for identifying valid accesses occur enough times 
that it

   might be worth introducing a function to do the access check.

- With respect to "all processes" keep in mind that in containers like 
Ddocker, all may not really be all.

  Though I'm not sure that is worth a comment.

$.02, Roger


On 5/24/2018 12:54 AM, Thomas Stüfe wrote:

Hi Daniil, David,

I think this fix makes a lot of sense.

First off, contacting a VM with foreign jcmd should not cause the VM
to sputter out thread dumps, nor should jcmd hang and timeout after 10
seconds (which it does). So I'd consider that a bug in any case.

If the desired behavior is really that root shall not see and/or be
able to contact VMs started from a different UID, then this should be
handled gracefully and fast.

However, I think we want jcmd started by root to see all processes and
be able to contact all processes. It is not a security issue, we
agree, yes? Since we are root anyway and can su to be everyone, it
would be security-by-inconvenience :)

So the only reason one would want to prevent root from seeing other
user's processes is because one wants to see only root's processes.
Like in a scenario where tons of processes run on a machine, only some
of them root. But in my experience, this is not a common scenario. It
is way more common (and expected behavior) to want to see everything
as root.

We have a very similar tool in our port (which may slowly phase out in
favour of jcmd), and that tool behaves just like that: when root, you
see everything and can contact everyone. Our support people need that
too.

Just my 5 cent.

Thanks, Thomas



On Thu, May 24, 2018 at 4:53 AM, David Holmes  wrote:

Hi Daniil,

I'm not sure I can accept on face-value the proposition that root "must be
allowed to access all VM processes". I can see it may be convenient in some
cases. But is it really necessary? Is it always desirable? I'd like to know
what a sys admin might think of this. :)

Further root can always "su" to another user and run jcmd that way.

Cheers,
David


On 24/05/2018 11:11 AM, Daniil Titov wrote:

Please review the changes that fix JDK-8197387.

There are 2 problems here:
1. JVM ignores  .attach_pid file if it is owned by the user different
from the one that owns this JVM process
2. jcmd checks that .java_pid socket is owned by the same user that
runs jcmd and reports an error otherwise

The fix relaxes these checks to allow jcmd started by  "root"  (UID = 0)
access JVMs started by another users.

Bug: https://bugs.openjdk.java.net/browse/JDK-8197387
Webrev: http://cr.openjdk.java.net/~dtitov/8197387/webrev.01/

Best regards,
Daniil

Re: Ping!! Re: RFR: 8203357 Container Metrics

[Roger Riggs]

Hi,

Just a thought on the timeslice units,  would nanoseconds be more future 
proof?

In a variety of cases, milliseconds are starting to seem too coarse.

$.02, Roger


On 6/11/18 10:12 AM, Bob Vandette wrote:


On Jun 11, 2018, at 4:32 AM, David Holmes > wrote:


Sorry Bob I haven't had a chance to look at this detail.

For the Java code ... methods that return arrays should return 
zero-length arrays when something is not available rather than null.


All methods do return zero length arrays except I missed the 
getPerCpuUsage.  I’ll fix that one and

correct the javadoc.



For getCpuPeriod() the term "operating system time slice" can be 
misconstrued as being related to the scheduler timeslice that may, or 
may not, exist, depending on the scheduler and scheduling policy etc. 
This "timeslice" is something specific to cgroups - no?


The comments reads:

   * Returns the length of the operating system time slice, in
   * milliseconds, for processes within the Isolation Group.
The comment does infer that it’s process and cgroup (Isolation group) 
specific and not the generic os timeslice.

Isn’t this sufficient?

Thanks,
Bob.


David

On 8/06/2018 3:43 AM, Bob Vandette wrote:

Can I get one more reviewer for this RFE so I can integrate it?
http://cr.openjdk.java.net/~bobv/8203357/webrev.01 


Mandy Chung has reviewed this change.
I’ve run Mach5 hotspot and core lib tests.
I’ve reviewed the tests which were written by Harsha Wardhana
I filed a CSR for the command line change and it’s now approved and 
closed.

Thanks,
Bob.
On May 30, 2018, at 3:45 PM, Bob Vandette > wrote:


Please review the following RFE which adds an internal API, along 
with jtreg tests that provide
access to Docker container configuration data and metrics.  In 
addition to the API which we hope to
take advantage of in the future with Java Flight Recorder and a JMX 
Mbean, I’ve added an additional
option to -XshowSettings:system than dumps out the container or 
host cgroup confguration

information.  See the sample output below:

RFE: Container Metrics

https://bugs.openjdk.java.net/browse/JDK-8203357

WEBREV:

http://cr.openjdk.java.net/~bobv/8203357/webrev.01


This commit will also include a fix for the following bug.

BUG: [TESTBUG] Test /runtime/containers/cgroup/PlainRead.java fails

https://bugs.openjdk.java.net/browse/JDK-8203691

WEBREV:

http://cr.openjdk.java.net/~bobv/8203357/webrev.00/test/hotspot/jtreg/runtime/containers/cgroup/PlainRead.java.sdiff.html

SAMPLE USAGE and OUTPUT:

docker run —memory=256m --cpuset-cpus 4-7 -it ubuntu bash
./java -XshowSettings:system
Operating System Metrics:
   Provider: cgroupv1
   Effective CPU Count: 4
   CPU Period: 10
   CPU Quota: -1
   CPU Shares: -1
   List of Processors, 4 total:
   4 5 6 7
   List of Effective Processors, 4 total:
   4 5 6 7
   List of Memory Nodes, 2 total:
   0 1
   List of Available Memory Nodes, 2 total:
   0 1
   CPUSet Memory Pressure Enabled: false
   Memory Limit: 256.00M
   Memory Soft Limit: Unlimited
   Memory & Swap Limit: 512.00M
   Kernel Memory Limit: Unlimited
   TCP Memory Limit: Unlimited
   Out Of Memory Killer Enabled: true

TEST RESULTS:

testing runtime container APIs
Directory "JTwork" not found: creating
Passed: runtime/containers/cgroup/PlainRead.java
Passed: runtime/containers/docker/DockerBasicTest.java
Passed: runtime/containers/docker/TestCPUAwareness.java
Passed: runtime/containers/docker/TestCPUSets.java
Passed: runtime/containers/docker/TestMemoryAwareness.java
Passed: runtime/containers/docker/TestMisc.java
Test results: passed: 6
Results written to /export/users/bobv/jdk11/build/jtreg/JTwork

testing jdk.internal.platform APIs
Passed: jdk/internal/platform/cgroup/TestCgroupMetrics.java
Passed: jdk/internal/platform/docker/TestDockerCpuMetrics.java
Passed: jdk/internal/platform/docker/TestDockerMemoryMetrics.java
Passed: jdk/internal/platform/docker/TestSystemMetrics.java
Test results: passed: 4
Results written to /export/users/bobv/jdk11/build/jtreg/JTwork

testing -XshowSettings:system launcher option
Passed: tools/launcher/Settings.java
Test results: passed: 1


Bob.

Re: hprof format question

[Roger Riggs]

Hi Simon,

I can file a bug on BufferInputStream with a bit more information about 
how you are using it
or a failing test case. I was not able to reproduce it with a simple 
read of 1Mb buffers.


Thanks, Roger


On 11/01/2018 06:08 PM, Simon Roberts wrote:



Oh, while on the topic of what I learned from this exercise, I also 
believe I discovered a bug in BufferedInputStream (don't laugh!). It 
fails catastrophically after reading 2GB (I think I can guess why 
without even looking at the code :). Everything was reading properly 
from an unadorned FileInputStream, but then I needed to look at the 
byte sequence to work out what was happening with these broken parts, 
and everything that was previously working went crazy when I stuck the 
BIS on the front. I created a replacement so that I could actually 
look at the failing bytes in a debugger. Hmm, the point of that 
observation was to ask the off-topic question of one should report a 
bug in a core API?


Thanks again,
Cheers,
Simon

Re: RFR: JDK-8213916: no copyright in signature.html

[Roger Riggs]

Looks fine,  Roger

On 11/15/2018 09:41 AM, JC Beyler wrote:

Hi Gary,

Looks good to me (not a reviewer though),
Jc

On Thu, Nov 15, 2018 at 5:45 AM Gary Adams > wrote:


Here's a quick fix to add a missing copyright to signature.html.


diff --git
a/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
b/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
--- a/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
@@ -1,4 +1,29 @@

+
+






--

Thanks,
Jc

Re: Proposal: Always-on Statistical History

[Roger Riggs]

Hi,

This looks like it has significant overlap with JFR.
I don't think we want to start building in multiple mechanisms to keep 
tabs on a running VM.


$.02, Roger


On 11/14/2018 04:27 PM, Thomas Stüfe wrote:

Hi Bernd,

On Wed, Nov 14, 2018 at 10:07 PM Bernd Eckenfels  wrote:

Looks good Thomas,

thanks!


what would be the typical memory usage with the Default Settings?

~ 80 Kb. Its very small.


Does the downsampling support min/max style rollups?

Not sure what you mean. Do you mean does it preserve peaks? Not yet,
such a feature would have to be added.

Right now, downsampling is very primitive for performance reasons. For
snapshot values like heap size etc we just throw away the samples, so
you loose temporary peaks. For counter-like values-over-time (e.g.
number of pages swapped in etc), they just refer then to a larger time
span.

Best Regards, Thomas




--
http://bernd.eckenfels.net



Von: Thomas Stüfe
Gesendet: Mittwoch, 14. November 2018 16:29
An: serviceability-dev@openjdk.java.net serviceability-dev@openjdk.java.net
Betreff: Proposal: Always-on Statistical History



Hi all,



We have that feature in our port which we would like to contribute,

and I would like to gauge opinions.



First off, I am not sure which list is correct. This is more of a

serviceability issue, but implementation wise it fit hs-runtime

better. I'll start with serviceability, but feel free crosspost if

needed.



Second, I am aware that this may require a JEP. If necessary and the

feedback is positive, I will draft one.







In our port we have something called "Statistics History". Basically

this is a rolling history, spanning up to 10 days, of a number of key

values. Key values range from JVM specifics like heap size, metaspace

size, number of threads etc, to platform specifics like memory

footprint, cpu load, io- and swapping activity etc.



A periodic tasks collects those values, in - by default - 15 second

intervals. They are then fed into a FIFO. FIFO spans 10 days. To save

memory that FIFO is downsampled in two steps, so we have the last n

hours in high resolution and the last n days in low resolution (of

course all these parameters are configurable).



The history report can be triggered via jcmd, and also could get

printed in the hs.err file (open for debate).



---



Here some examples of how the whole thing looks like:



http://cr.openjdk.java.net/~stuefe/webrevs/stathist/examples/stathist-volker.txt



http://cr.openjdk.java.net/~stuefe/webrevs/stathist/examples/stathist-s390x.txt



---



This feature has been really popular with our support folk over the

years. Be it that the VM is starved for resources by the OS, that we

have some slow- or fast developing leak situation etc: these values

are a first and easy way to get a first stab at a situation, before we

start more expensive analysis.



The explicit design goal of this history was to be very cheap - cheap

enough to be *always on* and getting forgotten. It is, in our port,

enabled by default. That way, if a problem occurs at a customer site,

we immediately see developments spanning the last 10 days, without

having to reproduce the issue.



It is also robust enough to be usable during error reporting without

endangering the error reporting process or falsifying the picture.



I am aware that this crosses over into JFR territory. But this feature

does not attempt to replace JFR, it is intended instead a cheap always

on first stop historical overview.



--



I have a patch which can be applied atop of jdk12:



http://cr.openjdk.java.net/~stuefe/webrevs/stathist/stathist.patch



It works, passes our nightlies and no regressions are shown in dapapo

benchmarks.



Please tell me what you think. Given enough interest, I will attempt

to contribute (drafting a JEP if necessary.)



Thanks and Kind Regards,



Thomas

Re: RFR: 8220355: Improve assertion texts and exception messages in eventHandlerVMInit

[Roger Riggs]

Hi,

InvocationAdapter.c: 581, remove the space after "(".  As long as that 
line is being changed.


$.02, Roger



On 03/18/2019 06:00 AM, Baesken, Matthias wrote:


Hi Alan, thanks for the review .

>I think this looks okay except I think "emergency" should be dropped 
from the message (and the existing comment).


Are you referring  to  this “emergency”  :

  *  First make our emergency fallback InternalError throwable.

  */

 result = initializeFallbackError(jnienv);

- jplis_assert(result);

+ jplis_assert_msg(result, "emergency fallback init failed");

If you prefer I could  output  just  “fallback init failed"   - is 
this fine with you?


Best regards, Matthias

*From:*Alan Bateman 
*Sent:* Montag, 18. März 2019 10:09
*To:* Baesken, Matthias ; Jean Christophe 
Beyler 

*Cc:* serviceability-dev@openjdk.java.net
*Subject:* Re: RFR: 8220355: Improve assertion texts and exception 
messages in eventHandlerVMInit



On 18/03/2019 08:54, Baesken, Matthias wrote:

Hi JC, thanks for your comments .

Second webrev , may I have a second review please ?

http://cr.openjdk.java.net/~mbaesken/webrevs/8220355.1/



I think this looks okay except I think "emergency" should be dropped 
from the message (and the existing comment).


-Alan

Re: RFR: JDK-8203026: java.rmi.NoSuchObjectException: no such object in table

[Roger Riggs]

Hi Gary,

Holding a static reference to the implementation solves the problem.

But I noticed that the object that is bound in the registry is the 
RemoteHostImpl

and it should be the RemoteHost stub.

Line 145: should be:

bind(name.toString(), stub);

That is likely to solve the problem as well, because the distributed garbage
collector will correctly cause a hard reference to be retained to the 
implementation.


Roger


On 03/22/2019 04:05 AM, gary.ad...@oracle.com wrote:

Here's a proposed fix for the intermittent jstatd test failure.
By moving the exported object from a local method variable to a
static class variable a strong reference is held so the object
will not be garbage collected prematurely.

  Webrev: http://cr.openjdk.java.net/~gadams/8203026/webrev.00/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8203026

The test is currently filed as a core-libs/java.rmi, but it probably
should be core-svc/tools with label=jstatd. It is the callers
responsibility to ensure the strong reference to prevent
the premature garbage collection, so this is a test failure
rather than a need to change the underlying rmi behavior.

Re: [preview] Adding java.lang.Runtime.getVMArguments() method

[Roger Riggs]

Hi Jaroslav,

Is this just to accommodate the test library?  I found no other 
references in java.base.


If so, then perhaps it can be placed in another package that the tests 
routinely need access to.

Does this fit the scope of the WhiteBox?

Thanks, Roger



On 12/07/2015 06:23 AM, Jaroslav Bachorik wrote:
...
>> Adding j.l.Runtime.getVmArguments() method seems like the best 
thing we
>> can do - the VM arguments are related to the current runtime and 
doing

>> this it allows for decoupling from java.management.
>
> Any further thoughts on this? Any objections against moving this
> functionality to j.l.Runtime since implementing this in ProcessHandle
> would lead to inconsistency between the arguments reported for
> 'current()' and other processes?

I was left unclear as to the exact intent of the API - which "arguments"
does it provide? Commandline? launcher? VM?
The same as 
http://docs.oracle.com/javase/8/docs/api/java/lang/management/RuntimeMXBean.html#getInputArguments--
Basically, this is about moving the RuntimeMXBean.getInputArguments() 
to a place which would not require dependency on java.management in 
order to get the VM input arguments.



-JB-

Re: RFR: 8141211: Convert TraceExceptions to Unified Logging

[Roger Riggs]

Hi,

Are the aliases listed in the -XX usage help?
+TraceExceptions is so much easier to understand and use than the new flag.

Thanks, Roger


On 12/08/2015 10:42 AM, Rachel Protacio wrote:

Hello,

Please review my conversion of -XX:+TraceExceptions to 
-Xlog:exceptions=info. The existing (product) flag is aliased to the 
logging flag at the info level.


If you have any questions on the alias table (in the arguments.cpp and 
.hpp files), Max will chime in as he is the one who implemented that 
portion.


Open webrev: http://cr.openjdk.java.net/~rprotacio/8141211/
Bug: https://bugs.openjdk.java.net/browse/JDK-8141211

Testing: jtreg, JPRT, jck vm tests, refworkload performance tests, rbt 
quick & non-colo tests


Thank you!
Rachel

Re: RFR[9u-dev] 8130425: libjvm crash due to stack overflow in executables with 32k tbss/tdata

[Roger Riggs]

Hi,

If just doubling the size of the Reaper stack would suffice, I would 
start there.


If a mechanism (property) is needed to make an adjustment to the stack size,
then I'd prefer something that applied to all Threads, not just the reaper.
It can be done in the Java code in Thread and would be low overhead and not
so a specialized bit of code that needs to be maintained and documented
for a single code path.

We have seen other cases where the minimum stack size is not sufficient
when executing with various VM switch combinations.
Having an adjustment would address those to.

$.02, Roger



On 1/14/16 9:32 PM, David Holmes wrote:

On 15/01/2016 4:59 AM, Martin Buchholz wrote:

The process grim reaper ends up being the first point of failure since
it tries not to waste the user's memory and it's in a core library,
but in principle it's not special.  I think a more general workaround
would be to add a hotspot flag that would add a memory safety zone to
all threads.  If it's known that TLS is stealing 32k from every
thread's stack, then the flag should ensure that every thread stack is
32k larger.


I think we need a point fix for the process reaper case in the 
immediate term. We can then consider how to better address the general 
case in the medium to long term.



More generally, I was hoping that hotspot would ensure that the -Xss
size was available for actual java stack frames and OS overhead was
added on automatically, but that is hard to implement, so the best
alternative workaround is for users to be able to specify that
additional stack size padding.  Maybe -XX:StackSizeOverhead=32768 ?


Even this is still a band-aid - the user has to experience the 
problem, recognize it, and then figure out the right adjustment to 
add. Plus if any third-party library uses native TLS the requirement 
could change dynamically.


So I'd prefer a new RFE to look at this general issue.

Thanks,
David

Re: RFR[9u-dev] 8130425: libjvm crash due to stack overflow in executables with 32k tbss/tdata

[Roger Riggs]

Hi David,


On 1/18/16 2:48 AM, David Holmes wrote:

Hi Roger,

On 16/01/2016 3:43 AM, Roger Riggs wrote:

Hi,

If just doubling the size of the Reaper stack would suffice, I would
start there.


That's too specific. It might be fix this problem case today, but 
break again tomorrow.
I was suggesting a stopgap, though its not clear how long it would be 
needed.



If a mechanism (property) is needed to make an adjustment to the stack
size,
then I'd prefer something that applied to all Threads, not just the 
reaper.


We already have means of changing other threads (eg -Xss for Java 
threads) the problem here is that the process reaper thread stack size 
is hard-wired. So we need some way to say control that.

I was targeting the increased stack size due to libc/glibc thread locals,
so it would be in addition to -Xss.

Roger




David
-

It can be done in the Java code in Thread and would be low overhead 
and not

so a specialized bit of code that needs to be maintained and documented
for a single code path.

We have seen other cases where the minimum stack size is not sufficient
when executing with various VM switch combinations.
Having an adjustment would address those to.

$.02, Roger



On 1/14/16 9:32 PM, David Holmes wrote:

On 15/01/2016 4:59 AM, Martin Buchholz wrote:

The process grim reaper ends up being the first point of failure since
it tries not to waste the user's memory and it's in a core library,
but in principle it's not special.  I think a more general workaround
would be to add a hotspot flag that would add a memory safety zone to
all threads.  If it's known that TLS is stealing 32k from every
thread's stack, then the flag should ensure that every thread stack is
32k larger.


I think we need a point fix for the process reaper case in the
immediate term. We can then consider how to better address the general
case in the medium to long term.


More generally, I was hoping that hotspot would ensure that the -Xss
size was available for actual java stack frames and OS overhead was
added on automatically, but that is hard to implement, so the best
alternative workaround is for users to be able to specify that
additional stack size padding.  Maybe -XX:StackSizeOverhead=32768 ?


Even this is still a band-aid - the user has to experience the
problem, recognize it, and then figure out the right adjustment to
add. Plus if any third-party library uses native TLS the requirement
could change dynamically.

So I'd prefer a new RFE to look at this general issue.

Thanks,
David

Re: RFR(S, TESTONLY): JDK-8148315 Create a basic reproducer for JDI issues

[Roger Riggs]

As an alternative to a specific child application would it be possible / 
reasonable to use jshell

as the child?  Or use the jshell API to manage the child?

Then you can feed it any java expressions/functions that are interesting 
and get back any data needed.


$.02, Roger



On 1/27/2016 9:09 AM, Staffan Larsen wrote:

Have you looked at the com/sun/jdi test framework? The java framework is quite 
good and stable. The shell script framework should be removed.

/Staffan


On 27 jan. 2016, at 13:02, Dmitry Samersoff  wrote:

Staffan,

1. This is one more small step forward to remove wide variety of
Exit0.java (and similar) programs from jdk tests.

I will not happen today, but I hope, sometimes in a future, all tests
that launch a child process will do it the same way.

2. We have couple of old SA-JDI tests in jdk.hotspot.agent/test these
tests have to be cleaned up and ported to JTREG, we need a framework to
do it.

3. It's hard to debug JDI failures that comes from nightly without small
standalone reproducer. Especially if emulator or slow hardware is involved.

I use this class as a base for such reproducer, find it helpful, and
would like to have it in the test library.

-Dmitry


On 2016-01-27 10:23, Staffan Larsen wrote:

Can you explain more? There is very little information here or in the
bug about what problem you are trying to solve. Why aren’t the
current JDI tests (jdk/test/com/sun/jdi) sufficient? I have not read
your code, and I would like more background before I do so.


On 26 jan. 2016, at 22:53, Dmitry Samersoff
 wrote:

Everybody,

Please review an RFE

http://cr.openjdk.java.net/~dsamersoff/JDK-8148315/webrev.01/

This fix adds basic LingeredApp based reproducer (and template for
more sophisticated reproducers) to debug JDI and underlying JVMTI
issues.

-Dmitry

-- Dmitry Samersoff Oracle Java development team, Saint Petersburg,
Russia * I would love to change the world, but they won't give me
the sources.


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(S, TESTONLY): JDK-8148315 Create a basic reproducer for JDI issues

[Roger Riggs]

Hi Dmitry,

I have no particular problem with LingeredApp but it is one more special 
purpose test function.


I had a similar problem with the process tests and created a JavaChild that
accepts commands and produces useful stream output used to synchronize.

It was just a thought that now with jshell there is a controllable, 
flexible command driven
java runtime.It is quite a bit more complex and that might be an 
issue for test reliability.


Roger


On 1/27/2016 10:36 AM, Dmitry Samersoff wrote:

Roger,

If a test need multiple processes, parent process have to control its
child. I.e. parent process have to know when child is ready to do some
work, parent process should have a way to terminate child etc.

Current tests uses different methods to control child process and not
all of them works reliably on all platforms. This problem used to create
lots of noise in test results.

About a year ago, I introduced LingeredApp.java[1] as a solution for
this problem.

It works well, and now I try to extend it to all cases where the test
need to launch a child process.

1.
  
http://hg.openjdk.java.net/jdk9/hs-rt/file/tip/test/lib/share/classes/jdk/test/lib/apps/LingeredApp.java

-Dmitry

On 2016-01-27 17:29, Roger Riggs wrote:

As an alternative to a specific child application would it be possible /
reasonable to use jshell
as the child?  Or use the jshell API to manage the child?

Then you can feed it any java expressions/functions that are interesting
and get back any data needed.

$.02, Roger



On 1/27/2016 9:09 AM, Staffan Larsen wrote:

Have you looked at the com/sun/jdi test framework? The java framework
is quite good and stable. The shell script framework should be removed.

/Staffan


On 27 jan. 2016, at 13:02, Dmitry Samersoff
 wrote:

Staffan,

1. This is one more small step forward to remove wide variety of
Exit0.java (and similar) programs from jdk tests.

I will not happen today, but I hope, sometimes in a future, all tests
that launch a child process will do it the same way.

2. We have couple of old SA-JDI tests in jdk.hotspot.agent/test these
tests have to be cleaned up and ported to JTREG, we need a framework to
do it.

3. It's hard to debug JDI failures that comes from nightly without small
standalone reproducer. Especially if emulator or slow hardware is
involved.

I use this class as a base for such reproducer, find it helpful, and
would like to have it in the test library.

-Dmitry


On 2016-01-27 10:23, Staffan Larsen wrote:

Can you explain more? There is very little information here or in the
bug about what problem you are trying to solve. Why aren’t the
current JDI tests (jdk/test/com/sun/jdi) sufficient? I have not read
your code, and I would like more background before I do so.


On 26 jan. 2016, at 22:53, Dmitry Samersoff
 wrote:

Everybody,

Please review an RFE

http://cr.openjdk.java.net/~dsamersoff/JDK-8148315/webrev.01/

This fix adds basic LingeredApp based reproducer (and template for
more sophisticated reproducers) to debug JDI and underlying JVMTI
issues.

-Dmitry

-- Dmitry Samersoff Oracle Java development team, Saint Petersburg,
Russia * I would love to change the world, but they won't give me
the sources.

--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: JDK 9 RFR of JDK-8149616: Problem list RmiSslBootstrapTest.sh

[Roger Riggs]

Hi Joe,

Seems correct to put it on the problem list.

Roger


On 2/11/2016 12:38 AM, joe darcy wrote:

Hello,

The test

sun/management/jmxremote/bootstrap/RmiSslBootstrapTest.sh

has recently been seen to fail frequently on various platforms. Until 
the root cause is addressed (JDK-8145919), the test should be placed 
on the problem list.


Please review the patch below which does this,

Thanks,

-Joe

diff -r 854a1100be00 test/ProblemList.txt
--- a/test/ProblemList.txtTue Feb 09 11:58:36 2016 -0800
+++ b/test/ProblemList.txtWed Feb 10 21:36:30 2016 -0800
@@ -165,6 +165,9 @@
 # 8147985
 sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java 
generic-all


+# 8145919
+sun/management/jmxremote/bootstrap/RmiSslBootstrapTest.sh generic-all
+
  



 # jdk_net

Re: RFR[9u-dev] 8130425: libjvm crash due to stack overflow in executables with 32k tbss/tdata

[Roger Riggs]

Hi,

ok.

A release note will be needed to document the new property.
Please add the label 'release-note' and a separate comment with the 
proposed release note text.


Thanks, Roger

On 2/17/2016 10:17 AM, cheleswer sahu wrote:

Hi,
I have made changes in the property name 
(jdk.lang.processReaperUseDefaultStackSize) and code as suggested in 
the earlier reviews.


 --- old/src/java.base/share/classes/java/lang/ProcessHandleImpl.java 
2016-02-17 18:48:10.54563 +0530
+++ new/src/java.base/share/classes/java/lang/ProcessHandleImpl.java 
2016-02-17 18:48:10.08163 +0530

@@ -81,9 +81,8 @@
 ThreadGroup systemThreadGroup = tg;

 ThreadFactory threadFactory = grimReaper -> {
-// Our thread stack requirement is quite modest.
-Thread t = new Thread(systemThreadGroup, grimReaper,
-"process reaper", 32768);
+long stackSize = 
Boolean.getBoolean("jdk.lang.processReaperUseDefaultStackSize") ? 0 : 
32768;
+Thread t = new Thread(systemThreadGroup, 
grimReaper, "process reaper", stackSize);

 t.setDaemon(true);
 // A small attempt (probably futile) to avoid 
priority inversion

 t.setPriority(Thread.MAX_PRIORITY);

I would really like to thanks "Martin" for the patch 
(http://cr.openjdk.java.net/~martin/webrevs/openjdk9/tls-size-guarantee/) 
which he has provided. Since we support a wider range of glibc 
versions and platform using patch will
require more testing and work. I think the use of system property for 
this issue will be safer at this point of time.


Regards,
Cheleswer


On 1/19/2016 5:40 PM, David Holmes wrote:

On 19/01/2016 9:53 PM, Kevin Walls wrote:

|
Hi Cheleswer, I think Martin is suggesting something like:
|

// Use a modest stack size, unless requested otherwise:
long stackSize = 
Boolean.getBoolean("processReaperUseDefaultStackSize") ? 0 : 32768;


Someone from core-libs needs to chime on what the appropriate 
namespace for such a property would be.


David
-

Thread t = new Thread(systemThreadGroup, grimReaper, "process 
reaper", stackSize);


|||
If that tests OK for you, it may be the way we can go ahead with the
point fix for this.

Regards
Kevin
|
On 18/01/2016 16:52, Martin Buchholz wrote:

Historical note - I never liked having a reaper thread for each
subprocess, but no other reliable implementation is known. Given the
potential for many reaper threads, I at least wanted to keep the
memory waste low.

On Wed, Jan 13, 2016 at 2:25 AM, cheleswer sahu
 wrote:


+   Thread t = null;
+   if
(Boolean.getBoolean("processReaperUseDefaultStackSize")) {
+   t = new Thread(systemThreadGroup, grimReaper,
"process reaper");
+} else {
+   // Our thread stack requirement is quite 
modest.

+   t = new Thread(systemThreadGroup, grimReaper,
"process reaper", 32768);
+}

If we do end up using this strategy, cleaner would be to use
https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#Thread-java.lang.ThreadGroup-java.lang.Runnable-java.lang.String-long- 


stackSize - the desired stack size for the new thread, or zero to
indicate that this parameter is to be ignored.

Re: JDK 9 RFR of JDK-8169949: Remove java/lang/instrument/BootClassPath/BootClassPathTest.sh from ProblemList.txt

[Roger Riggs]

Looks fine.

Roger


On 11/18/2016 1:57 AM, Amy Lu wrote:

Please review the patch to bring back test:
java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Mac OSX

As mentioned in JDK-8072130, failure is not reproducible.
I also tested it with different version of macOS, test all pass. JPRT 
results for all platforms are good, test pass.


bug: https://bugs.openjdk.java.net/browse/JDK-8169949

Thanks,
Amy

--- old/test/ProblemList.txt2016-11-18 14:51:43.0 +0800
+++ new/test/ProblemList.txt2016-11-18 14:51:43.0 +0800
@@ -132,8 +132,6 @@
 java/lang/instrument/RedefineBigClass.sh 8065756 generic-all
 java/lang/instrument/RetransformBigClass.sh 8065756 generic-all

-java/lang/instrument/BootClassPath/BootClassPathTest.sh 8072130 
macosx-all

-
 java/lang/management/MemoryMXBean/Pending.java 8158837 generic-all
 java/lang/management/MemoryMXBean/PendingAllGC.sh 8158837 generic-all

Re: RFR - 8169575: com/sun/management/DiagnosticCommandMBean/DcmdMBeanPermissionsTest.java failing with jtreg tip

[Roger Riggs]

Hi Harsha,

The addition of the permission for 
"getProperty.jdk.jar.disabledAlgorithms" is fine.


But the addition of a permission for exitVM.95 to satisfy the need of 
the test harness to exit cleanly
should instead be fixed by changing the "exitVM.97" to "exitVM.*" to 
avoid future maintenance

in the case where the test harness changes its exit status.

Roger


On 11/18/2016 4:47 AM, Harsha Wardhana B wrote:

Hello All,

Please review below fix for issue

https://bugs.openjdk.java.net/browse/JDK-8169575

having webrev at,

http://cr.openjdk.java.net/~hb/8169575/webrev.00/

Regards

Harsha

Re: RFR - 8169575: com/sun/management/DiagnosticCommandMBean/DcmdMBeanPermissionsTest.java failing with jtreg tip

[Roger Riggs]

+1

On 11/18/2016 10:43 AM, Harsha Wardhana B wrote:

Hello,

Please review below webrev incorporating Roger's comment.

http://cr.openjdk.java.net/~hb/8169575/webrev.01/

-Harsha


On Friday 18 November 2016 09:08 PM, Frederic Parain wrote:

"exitVM.*" should be fine to avoid future maintenance burden.
Thank you for pointing this out Roger.

Fred

On 11/18/2016 10:32 AM, Harsha Wardhana B wrote:

Hi Roger,

I was not sure about adding exitVM.* as that would give permissions for
all exit codes.

Maybe Fred can comment since he is the test case author.

-Harsha


On Friday 18 November 2016 08:58 PM, Roger Riggs wrote:

Hi Harsha,

The addition of the permission for
"getProperty.jdk.jar.disabledAlgorithms" is fine.

But the addition of a permission for exitVM.95 to satisfy the need of
the test harness to exit cleanly
should instead be fixed by changing the "exitVM.97" to "exitVM.*" to
avoid future maintenance
in the case where the test harness changes its exit status.

Roger


On 11/18/2016 4:47 AM, Harsha Wardhana B wrote:

Hello All,

Please review below fix for issue

https://bugs.openjdk.java.net/browse/JDK-8169575

having webrev at,

http://cr.openjdk.java.net/~hb/8169575/webrev.00/

Regards

Harsha

Re: RFR: JDK-8165765: javax/management/remote/mandatory/connection/RMIConnectionIdTest.java: failed when looking at RMI connection IDs:

[Roger Riggs]

Hi,

RMIConnectionIdTest.java:

 - The method NetworkInterface.getByInetAddress(addr) may be a better 
choice

   without all the looping and repetitive checks of localAddr == null.

Roger

p.s.

This would have been a good use of streams with 
NetworkInterface.networkInterfaces

   and NetworkInterface.inetAddresses.


On 11/23/2016 4:08 AM, Ujwal Vangapally wrote:

Please review this small change for the bug below

https://bugs.openjdk.java.net/browse/JDK-8165765

Webrev: http://cr.openjdk.java.net/~hb/sponsorship/8165765/webrev.00/

Thanks,

Ujwal.

Re: RFR: JDK-8165765: javax/management/remote/mandatory/connection/RMIConnectionIdTest.java: failed when looking at RMI connection IDs:

[Roger Riggs]

Hi Ujwal,

looks fine.

Have you been able to verify the test on a system similar to where it 
failed originally?


Thanks, Roger



On 11/27/2016 11:47 PM, Ujwal Vangapally wrote:


gentle reminder

Thanks,

Ujwal


On 11/24/2016 1:50 PM, Ujwal Vangapally wrote:


Thanks for the review Roger, please find the new webrev incorporating 
the review comments.


webrev :http://cr.openjdk.java.net/~hb/sponsorship/8165765/webrev.01/

-Ujwal

On 11/23/2016 10:10 PM, Roger Riggs wrote:

Hi,

RMIConnectionIdTest.java:

 - The method NetworkInterface.getByInetAddress(addr) may be a 
better choice

   without all the looping and repetitive checks of localAddr == null.

Roger

p.s.

This would have been a good use of streams with 
NetworkInterface.networkInterfaces

   and NetworkInterface.inetAddresses.


On 11/23/2016 4:08 AM, Ujwal Vangapally wrote:

Please review this small change for the bug below

https://bugs.openjdk.java.net/browse/JDK-8165765

Webrev: http://cr.openjdk.java.net/~hb/sponsorship/8165765/webrev.00/

Thanks,

Ujwal.

Re: RFR: JDK-8165765: javax/management/remote/mandatory/connection/RMIConnectionIdTest.java: failed when looking at RMI connection IDs:

[Roger Riggs]

Hi Juwal,

Ok, thanks for the followup.

If you need a sponsor for this patch, let me know.

Roger


On 11/29/2016 4:24 AM, Ujwal Vangapally wrote:


Hi Roger,

I didn't verify it on a similar system as it is difficult to find one 
, instead  verified by just hard coding a  InetAddress different from 
which it is picking by default to  String clientAddr and executed the 
test .


Thanks,
Ujwal



On 11/29/2016 3:12 AM, Roger Riggs wrote:

Hi Ujwal,

looks fine.

Have you been able to verify the test on a system similar to where it 
failed originally?


Thanks, Roger



On 11/27/2016 11:47 PM, Ujwal Vangapally wrote:


gentle reminder

Thanks,

Ujwal


On 11/24/2016 1:50 PM, Ujwal Vangapally wrote:


Thanks for the review Roger, please find the new webrev 
incorporating the review comments.


webrev :http://cr.openjdk.java.net/~hb/sponsorship/8165765/webrev.01/

-Ujwal

On 11/23/2016 10:10 PM, Roger Riggs wrote:

Hi,

RMIConnectionIdTest.java:

 - The method NetworkInterface.getByInetAddress(addr) may be a 
better choice
   without all the looping and repetitive checks of localAddr == 
null.


Roger

p.s.

This would have been a good use of streams with 
NetworkInterface.networkInterfaces

   and NetworkInterface.inetAddresses.


On 11/23/2016 4:08 AM, Ujwal Vangapally wrote:

Please review this small change for the bug below

https://bugs.openjdk.java.net/browse/JDK-8165765

Webrev: 
http://cr.openjdk.java.net/~hb/sponsorship/8165765/webrev.00/


Thanks,

Ujwal.

Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts "0" instead of assigned port

[Roger Riggs]

Hi Harsha,

On 1/16/2017 1:21 AM, Harsha Wardhana B wrote:

Hi Amit,

In JdpJmxRemoteDynamicPortTestCase:48 needs null/empty check for jmx url.

JdpJmxRemoteDynamicPortTestCase:49, array length needs to checked 
before accessing index at token[6].


It is possible that port number need not always be present at given 
index and hence we may have to follow different approach to extract 
port number. Please check if approach below works.




int idx = jmxurl.indexOf(':');
while (idx != -1) {
jmxurl = jmxurl.substring(idx+1);
idx = jmxurl.indexOf(':');
}
This loop would very eagerly find the last ":" in the string even it was 
well past the host/port field.

String.lastIndex would be equivalent.


if(jmxurl.indexOf('/') == -1) {
throw new RuntimeException("Test failed : Invalid 
JMXServiceURL");

}
It would be more efficient to compare the index of the '/' after the 
last ":" than to re-create new substrings.

int colon = jmxurl.lastIndexOf(':');
int slash = jmxurl.indexOf('/', colon);
int port = Integer.parseInt(jmxurl, colon + 1, slash, 10);



String portStr = jmxurl.substring(0,jmxurl.indexOf('/'));
int port = Integer.parseInt(portStr);
if( port == 0 ) {
throw new RuntimeException("Test failed : Zero port for 
jmxremote");

}

Or It might be just as effective to just to check if ":0/" is present.
if (jmxurl.contains(":0/")) {...}

$.02, Roger






Regards

Harsha


On Monday 16 January 2017 11:16 AM, Amit Sapre wrote:

Thanks Dmitry for the review.

Can I have one more reviewer for this fix ?

Thanks,
Amit


-Original Message-
From: Dmitry Samersoff
Sent: Sunday, January 15, 2017 4:49 PM
To: Amit Sapre; serviceability-dev
Subject: Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts
"0" instead of assigned port

Amit,

Changes looks good to me.

-Dmitry


On 2017-01-13 09:17, Amit Sapre wrote:

Hello,



Please review the fix for JDK-8167337



Bug Id : https://bugs.openjdk.java.net/browse/JDK-8167337

Webrev :
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8167337/webrev.00/



Thanks,

Amit



--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts "0" instead of assigned port

[Roger Riggs]

Hi,

yes, but the pattern looks for the ":" before and the "/" after the zero.
It would not match the port ":00/" ; in this test code the URL is 
assumed/known to be relatively well formed.


Roger


On 1/17/2017 11:26 AM, Harsha Wardhana B wrote:

Hi Roger,

Your approach is more elegant. However checking for ":0/" may not work 
as we can have non-zero port number that can end in 0.


Regards

Harsha


On Tuesday 17 January 2017 09:39 PM, Roger Riggs wrote:

Hi Harsha,

On 1/16/2017 1:21 AM, Harsha Wardhana B wrote:

Hi Amit,

In JdpJmxRemoteDynamicPortTestCase:48 needs null/empty check for jmx 
url.


JdpJmxRemoteDynamicPortTestCase:49, array length needs to checked 
before accessing index at token[6].


It is possible that port number need not always be present at given 
index and hence we may have to follow different approach to extract 
port number. Please check if approach below works.




int idx = jmxurl.indexOf(':');
while (idx != -1) {
jmxurl = jmxurl.substring(idx+1);
idx = jmxurl.indexOf(':');
}
This loop would very eagerly find the last ":" in the string even it 
was well past the host/port field.

String.lastIndex would be equivalent.


if(jmxurl.indexOf('/') == -1) {
throw new RuntimeException("Test failed : Invalid 
JMXServiceURL");

}
It would be more efficient to compare the index of the '/' after the 
last ":" than to re-create new substrings.

int colon = jmxurl.lastIndexOf(':');
int slash = jmxurl.indexOf('/', colon);
int port = Integer.parseInt(jmxurl, colon + 1, slash, 10);



String portStr = jmxurl.substring(0,jmxurl.indexOf('/'));
int port = Integer.parseInt(portStr);
if( port == 0 ) {
throw new RuntimeException("Test failed : Zero port for 
jmxremote");

}

Or It might be just as effective to just to check if ":0/" is present.
if (jmxurl.contains(":0/")) {...}

$.02, Roger






Regards

Harsha


On Monday 16 January 2017 11:16 AM, Amit Sapre wrote:

Thanks Dmitry for the review.

Can I have one more reviewer for this fix ?

Thanks,
Amit


-Original Message-
From: Dmitry Samersoff
Sent: Sunday, January 15, 2017 4:49 PM
To: Amit Sapre; serviceability-dev
Subject: Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP 
broadcasts

"0" instead of assigned port

Amit,

Changes looks good to me.

-Dmitry


On 2017-01-13 09:17, Amit Sapre wrote:

Hello,



Please review the fix for JDK-8167337



Bug Id : https://bugs.openjdk.java.net/browse/JDK-8167337

Webrev :
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8167337/webrev.00/ 





Thanks,

Amit



--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the 
sources.

Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts "0" instead of assigned port

[Roger Riggs]

Hi Amit,

Looks fine to me.

Thanks, Roger


On 1/19/2017 6:54 AM, Amit Sapre wrote:

Hi,
I updated the parsing logic for extracting port number in test case. Here is 
the updated webrev :

http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8167337/webrev.01/

Thanks,
Amit


-Original Message-
From: Amit Sapre
Sent: Wednesday, January 18, 2017 2:09 PM
To: Roger Riggs; serviceability-dev@openjdk.java.net; Harsha Wardhana B
Subject: RE: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts
"0" instead of assigned port

Hello,

Looks like basic check on Jdp packet includes a check for
JMX_SERVICE_URL
https://java.se.oracle.com/source/xref/jdk9-
dev/jdk/test/sun/management/jdp/JdpTestCase.java#184

I feel we don't need any further check on jmx service url.



-Original Message-----
From: Roger Riggs
Sent: Tuesday, January 17, 2017 10:05 PM
To: serviceability-dev@openjdk.java.net
Subject: Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP

broadcasts

"0" instead of assigned port

Hi,

yes, but the pattern looks for the ":" before and the "/" after the
zero.
It would not match the port ":00/" ; in this test code the URL is
assumed/known to be relatively well formed.

Roger

I kept the focus on what needs to be tested. This however doesn't mean
we shouldn't validate the url format.
I would prefer to do that in a separate test case all together.



On 1/17/2017 11:26 AM, Harsha Wardhana B wrote:

Hi Roger,

Your approach is more elegant. However checking for ":0/" may not

work

as we can have non-zero port number that can end in 0.

Regards

Harsha


On Tuesday 17 January 2017 09:39 PM, Roger Riggs wrote:

Hi Harsha,

On 1/16/2017 1:21 AM, Harsha Wardhana B wrote:

Hi Amit,

In JdpJmxRemoteDynamicPortTestCase:48 needs null/empty check for

jmx

url.

JdpJmxRemoteDynamicPortTestCase:49, array length needs to checked
before accessing index at token[6].

It is possible that port number need not always be present at
given index and hence we may have to follow different approach to
extract port number. Please check if approach below works.



 int idx = jmxurl.indexOf(':');
 while (idx != -1) {
 jmxurl = jmxurl.substring(idx+1);
 idx = jmxurl.indexOf(':');
 }

This loop would very eagerly find the last ":" in the string even
it was well past the host/port field.
String.lastIndex would be equivalent.

 if(jmxurl.indexOf('/') == -1) {
 throw new RuntimeException("Test failed : Invalid
JMXServiceURL");
 }

It would be more efficient to compare the index of the '/' after
the last ":" than to re-create new substrings.
int colon = jmxurl.lastIndexOf(':'); int slash =
jmxurl.indexOf('/', colon); int port = Integer.parseInt(jmxurl,
colon + 1, slash, 10);


 String portStr = jmxurl.substring(0,jmxurl.indexOf('/'));
 int port = Integer.parseInt(portStr);
 if( port == 0 ) {
 throw new RuntimeException("Test failed : Zero port
for jmxremote");
 }

Or It might be just as effective to just to check if ":0/" is

present.

if (jmxurl.contains(":0/")) {...}

$.02, Roger





Regards

Harsha


On Monday 16 January 2017 11:16 AM, Amit Sapre wrote:

Thanks Dmitry for the review.

Can I have one more reviewer for this fix ?

Thanks,
Amit


-Original Message-
From: Dmitry Samersoff
Sent: Sunday, January 15, 2017 4:49 PM
To: Amit Sapre; serviceability-dev
Subject: Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP
broadcasts "0" instead of assigned port

Amit,

Changes looks good to me.

-Dmitry


On 2017-01-13 09:17, Amit Sapre wrote:

Hello,



Please review the fix for JDK-8167337



Bug Id : https://bugs.openjdk.java.net/browse/JDK-8167337

Webrev :
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-

8167337/webrev

.00/




Thanks,

Amit


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the
sources.

Re: RFE Review : JDK-5016517 - Replace plaintext passwords by hashed passwords for out-of-the-box JMX Agent

[Roger Riggs]

Hi Harsha,

Thanks for this important improvement. Comments:


* jmxremote.password.template:
  "Passwords will be hashed by server if they are in clear." Perhaps 
should be more explicit:


   "The jmxremote.passwords file will be re-written by the server to 
replace all plain text passwords with hashed passwords when the file is 
read by the server."


line 35: "Base64 encoded hash"  -> drop the "Base64" in this line isn't 
needed and

make it seems like it should appear as 1 field instead of 2 or 3.

37+: The syntax of the file may be clearer if it includes the complete 
syntax in (line 39) not

just the password/hash fragment.

Line 41:  "W = spaces"; above "tabs" are allowed as a delimiter; it 
would be good to be consistent
and include the usualy white-space characters in the set, be as specific 
as possible.

Is this the same set of whitespace used by Regex '\\s'.

45: "java platform""  ->   "MD5, SDA-1, SHA-256 are supported algorithms."

49: be more specific about 'hashing is requested'  how?  Refer to the 
management.properties

  com.sun.management.jmxremote.password.hash value.



51:  "replace hashed" -> "replace *the *hashed"
52: "with clear text or new" -> "with the clear text or the new"
52: "If new password" -> "If the new password"
53: "when new login" -> "when a new login"

60: "User generated" -> "A User generated"

67: Will the file be ignored if it has the wrong permissions. (With a 
logged message)


* management.properties

306: "(Case for true/false ignored)"  - what does this mean; I think it 
can be removed.


307: missing period at the end of the sentence.
309: "in password file" -> "in the password file"


* FileLoginModule.java

102: can this match better the similar name in the management.properties 
if it has the same function:

com.sun.management.jmxremote.password.hash
103: "replaces clear text passwords" -> "replaces each clear text password"
104: indent to match previous  enteries.

* JMXPluggableAuthenticator.java

119: There is no need to copy the password to a new local

128: add a space after ","

256 private static final String HASH_PASSWORDS =
257 "jmx.remote.x.password.file.hash";

The name ".hash" part does not clearly communicate that passwords are to 
be hashed.

"hashPasswords" might be more self explanatory.
Also, can this be NOT duplicated here and in ConnectorBootStrap.java?


* ConnectorBootStrap.java:
 482: Add space after ","s; no spaces before.

770: use the same name for the option/property if possible to avoid 
confusion.


770:  if the HASH_PASSWORDS static is appropriate use it instead of 
literal "true".


* HashedPasswordManager

80-83: The fields can be final and use the constructor to initialize in 
all cases and make the class final

to avoid unintentional subclassing.


113: canWriteToFile:   It should be made clear in the template that 
*both* the Security policy
   and the file access value are used to check that the file can be 
updated.


200: loadPasswords() - should this confirm the access to the file is 
allowed and it has

the correct file access before reading?

Is the re-writing of the passwords intended to be done by a 'priveleged' 
system.

Does this need doPrivileged?

* HashedPasswordFileTest:

88: should use the TestLibrary Utils.getRandomInstance so it logs the 
seed and can be replayed if necessary.



Thanks, Roger


On 4/23/2017 6:20 AM, Harsha Wardhana B wrote:


Hi All,

Please review this enhancement to replace plain-text password for JMX 
agent with SHA-256 hash.


Issue: https://bugs.openjdk.java.net/browse/JDK-5016517


webrev: http://cr.openjdk.java.net/~hb/5016517/webrev.00/

Overview of implementation:

Currently, the JMX agent password file used to authenticate user, 
stores user name and password as clear text. Though system level 
restrictions are recommended for jmx password file, passwords are 
vulnerable since they are stored in clear. The current RFE proposes to 
store passwords as SHA256 hash instead of clear text.


In current implementation, if password file is writable, and if 
passwords are in clear, they will be replaced by SHA256 hash upon 
agent boot-up or when login attempt is made.


The file, 
src/jdk.management.agent/share/conf/jmxremote.password.template 
contains more details about the implementation.


- Harsha

Re: RFR: JDK-8173180 VirtualMachine.startLocalManagementAgent() returns URI with unreliable IP address

[Roger Riggs]

Hi,

I would expect to see a mix of versions in many operational cases. For 
example, in a large
deployment there will be a mix of versions active and the folks 
monitoring it should not

have to change their management tools unnecessarily.

The usual rule for interoperability between versions is that it should 
work between the

previous and next versions.  (+/- 1).

If there is any issue with compatibility between versions, you'll need 
to file a CCC

to make sure it gets adequate review.

Roger


On 6/20/17 7:42 AM, Ujwal Vangapally wrote:

Thanks for the Review Daniel, Harsha.

Yes with this fix JConsole running on JDK 8 won't be able to connect 
to a process running on higher version of Java containing this change.


I think even Harsha is agreeing this, his point is that the use case 
where a client running on JDK 8 trying to connect to a process running 
on latest releases is rare. As mostly for Local Management both client 
and server will be running on same machine using same JDK for starting 
both client and server .


please correct me if this is not the case.

can we have this fix with interoperability issue between versions ?

Thanks,

Ujwal.



On 6/20/2017 2:40 PM, Daniel Fuchs wrote:

Hi Harsha,

Maybe I'm missing something.
How is the local agent started?

If it's started when you connect jconsole to a process
by specifying the process ID - then I suspect this will
prevent e.g. jconsole or jvisualvm running on JDK 8 to
connect to your process.

Can you verify that it's not the case?

best regards,

-- daniel

On 20/06/2017 09:11, Harsha Wardhana B wrote:

Hi Daniel,

The fix is applicable only to local JMX agent and the most common 
use case would be client and server running from same JVM.
It is highly unlikely that local JMX agent will be started to cater 
for out-of-jvm clients.


I don't see how introducing this fix can cause new interoperability 
problems. Can you please elaborate?


-Harsha

On Monday 19 June 2017 10:23 PM, Daniel Fuchs wrote:

Hi,

If I'm not mistaken then this will make it impossible
for earlier release to interoperate with newer releases
as the LocalRMIClientSocketFactory class will not be
present the client tries to deserialize the stub.

best regards,

-- daniel

On 19/06/2017 11:52, Ujwal Vangapally wrote:

Hi,

Kindly review the fix for bug below

https://bugs.openjdk.java.net/browse/JDK-8173180

webrev: 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8173180/webrev.00/ 



Thanks,

Ujwal

Re: RFR : 8182485 - JMX connections should have configurable ObjectInputFilter

[Roger Riggs]

+1

Roger


On 6/20/2017 1:24 PM, Daniel Fuchs wrote:

The fix looks good to me Harsha.

best regards,

-- daniel

On 20/06/2017 07:10, Harsha Wardhana B wrote:

Hi,

Please review the below RFE,

JDK-8182485 : JMX connections should have configurable ObjectInputFilter

having webrev at

http://cr.openjdk.java.net/~hb/8182485/webrev.00/

The enhancement adds ObjectInputFilter to JMX connections to 
configure filters during deserialization.


-Harsha

Re: Proposal:JdpController.getProcessId() VM compatibility improvement

[Roger Riggs]

Hi Andrew,

Redirecting back to serviceability-dev@openjdk.java.net, it  is the 
better list for this topic.


I don't know these tests but it seems odd to stick that assert into every
callback to packetFromThisVMReceived.  Perhaps someone more familiar can 
review and sponsor.


Thanks, Roger




On 6/26/2017 10:37 AM, Andrew Leonard wrote:

Hi Roger,
I have now updated to the latest openjdk9 and examined the existing 
jdp tests. I have added to those tests to assert the JDP packet 
processId, which was in fact "null", so it was failing to set it 
before... Hence, with this testcase update they fail without my patch, 
and succeed with the new patch.

Being new to this contribution process, where do I go from here please?
Thanks
Andrew

diff -r 2425838cfb5e 
src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
--- 
a/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java 
 Fri Jun 23 14:32:59 2017 -0400
+++ 
b/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java 
 Mon Jun 26 15:31:15 2017 +0100

@@ -133,20 +133,8 @@

 // Get the process id of the current running Java process
 private static Integer getProcessId() {
-try {
-  // Get the current process id using a reflection hack
-  RuntimeMXBean runtime = ManagementFactory.getRuntimeMXBean();
-  Field jvm = runtime.getClass().getDeclaredField("jvm");
-  jvm.setAccessible(true);
-
-  VMManagement mgmt = (sun.management.VMManagement) jvm.get(runtime);
-  Method pid_method = mgmt.getClass().getDeclaredMethod("getProcessId");
-  pid_method.setAccessible(true);
-  Integer pid = (Integer) pid_method.invoke(mgmt);
-  return pid;
-} catch(Exception ex) {
-  return null;
-}
+// Get the current process id
+return (int)ProcessHandle.current().pid();
 }

diff -r 2425838cfb5e test/sun/management/jdp/JdpOnTestCase.java
--- a/test/sun/management/jdp/JdpOnTestCase.javaFri Jun 23 
14:32:59 2017 -0400
+++ b/test/sun/management/jdp/JdpOnTestCase.javaMon Jun 26 
15:30:51 2017 +0100

@@ -31,6 +31,7 @@

 import java.net.SocketTimeoutException;
 import java.util.Map;
+import static jdk.testlibrary.Asserts.assertNotEquals;

 public class JdpOnTestCase extends JdpTestCase {

@@ -58,6 +59,7 @@
 final String jdpName = payload.get("INSTANCE_NAME");
 log.fine("Received correct JDP packet #" + 
String.valueOf(receivedJDPpackets) +

 ", jdp.name=" + jdpName);
+assertNotEquals(null, payload.get("PROCESS_ID"), "Expected 
payload.get(\"PROCESS_ID\") to be not null.");

 }




Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com




From: Roger Riggs 
To: Andrew Leonard 
Cc: core-libs-...@openjdk.java.net
Date: 23/06/2017 13:57
Subject: Re: Proposal:JdpController.getProcessId() VM compatibility 
improvement





Hi Leonard,

No need to stray from your intended focus.
The other is just another cleanup.

Thanks, Roger


On 6/23/17 6:57 AM, Andrew Leonard wrote:
Thanks Roger,
So yes that issue does look similar, although I was only looking in 
the management interfaces. I'll have a peek at those references to see 
if they are similar, it would make sense to clean those up too, to be 
consistent. Your thoughts?
I'll also see if there's a junit for this method, if not i'll see if I 
can add one.

Cheers
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: _andrew_m_leon...@uk.ibm.com_ 
<mailto:andrew_m_leon...@uk.ibm.com>





From: Roger Riggs __ 
<mailto:roger.ri...@oracle.com>
To: _core-libs-...@openjdk.java.net_ 
<mailto:core-libs-...@openjdk.java.net>

Date: 22/06/2017 18:03
Subject: Re: Proposal:JdpController.getProcessId() VM compatibility 
improvement
Sent by: "core-libs-dev" __ 
<mailto:core-libs-dev-boun...@openjdk.java.net>





HI,

This looks like:

JDK-8074569 <_https://bugs.openjdk.java.net/browse/JDK-8074569_> Update
implementation to use ProcessHandle.current() to get the PID

I'm happy to sponsor.

Roger

On 6/22/2017 12:27 PM, Andrew Leonard wrote:
> Thanks Alan,
> Yes, you're right the exception swallowing can be removed too.
> I'll re-post to serviceability-dev...
> Cheers
> Andrew
>
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> Phone internal: 245913, external: 01962 815913
> internet email: _andrew_m_leon...@uk.ibm.com_ 
<mailto:andrew_m_leon...@uk.ibm.com>

>
>
>
>
> From:   Alan Bateman 

Re: RFR: JDK-8181895 javax management docs contain links to technotes

[Roger Riggs]

Hi Ujwal,

As Alan noted earlier, it would be helpful in the package javadoc of 
javax.management.remote.rmi
if there was some mention of the class or package within the javadoc 
that supported the mentioned dynamic classloading. As is, the feature is 
mentioned but with no suggestion about where to start to find the details.


The other references to the specification are fine.

Roger



On 7/18/2017 7:20 AM, Ujwal Vangapally wrote:

Hi,

kindly review the changes made.

previously technotes were present in pubs repo at 
/pubs/docs/technotes/guides/


pubs repo  has been removed by JDK-8175825

now we can't include content from that repo in our generated docs.

currently there is no other way to access the content available in 
technotes .


JMX specification is available from the jcp 
(https://jcp.org/aboutJava/communityprocess/mrel/jsr160/index2.html) 
which i used to replace previous link to spec in tech notes.


Sufficient description is already present as part of existing java doc.

Purpose of the link to technotes was to provide more information.

Replaced with alternate links where ever possible.


https://bugs.openjdk.java.net/browse/JDK-8181895

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8181895/webrev.00/



Thanks,

ujwal.

Re: RFR: JDK-8181895 javax management docs contain links to technotes

[Roger Riggs]

Hi Ujwal,

The updated links look fine.

Thanks, Roger


On 7/24/2017 1:48 PM, Ujwal Vangapally wrote:

Thanks for the review Roger.

please see the webrev incorporating review comments.

http://cr.openjdk.java.net/~uvangapally/webrev/2017/8181895/webrev.01/

-Ujwal.


On 7/20/2017 2:16 AM, Roger Riggs wrote:

Hi Ujwal,

As Alan noted earlier, it would be helpful in the package javadoc of 
javax.management.remote.rmi
if there was some mention of the class or package within the javadoc 
that supported the mentioned dynamic classloading. As is, the feature 
is mentioned but with no suggestion about where to start to find the 
details.


The other references to the specification are fine.

Roger



On 7/18/2017 7:20 AM, Ujwal Vangapally wrote:

Hi,

kindly review the changes made.

previously technotes were present in pubs repo at 
/pubs/docs/technotes/guides/


pubs repo  has been removed by JDK-8175825

now we can't include content from that repo in our generated docs.

currently there is no other way to access the content available in 
technotes .


JMX specification is available from the jcp 
(https://jcp.org/aboutJava/communityprocess/mrel/jsr160/index2.html) 
which i used to replace previous link to spec in tech notes.


Sufficient description is already present as part of existing java doc.

Purpose of the link to technotes was to provide more information.

Replaced with alternate links where ever possible.


https://bugs.openjdk.java.net/browse/JDK-8181895

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8181895/webrev.00/



Thanks,

ujwal.

Re: RFR: JDK-8185003 JMX: Add a version of ThreadMXBean.dumpAllThreads with a maxDepth argument

[Roger Riggs]

Hi Ujwal,

(Reviewer, but not specifically servicability).

Comments,

java/lang/management/ThreadMXBean.java:

809: It may be useful to state that the behavior is the same as {@link 
#dumpAllThreads} except that the depth is limited.


828:  Do not duplicate specification of the meaning of maxDepth = 
MAX_VALUE, either put the entire spec
in the @param or in the description.  Duplication creates an opportunity 
for disagreement or a maintenance issue when updating.


833: terminate the {@code maxDepth} before "is negative".

As Erik comments, there should be a space after "," everywhere; both to 
be consistent with existing style and the style guide.


sun/management/ThreadImpl.java:

484: Since the native code is going to check maxDepth, why is it checked 
here also?

-or- remove the check in native
490:  This could be simpler to not translate maxDepth to -1,
 Here and in the native code, passing MAX_INTEGER would dump the 
entire stack, there is no need to special case it


services/threadService.cpp:
565:  checking count >= maxDepth is a sufficient test if to dump all 
frames, passing MAX_INTEGER is used

  and it will dump zero should a negative number get this far.

Thanks, Roger

On 8/3/2017 10:18 AM, Ujwal Vangapally wrote:

kindly use the below link for accessing webrev

http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.01/

previously shared link is no longer accessible hence providing this 
new link.


Thanks,

Ujwal.


On 8/3/2017 3:08 PM, Ujwal Vangapally wrote:

Hi,

kindly review the changes made.

https://bugs.openjdk.java.net/browse/JDK-8185003

webrev: 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.00/


CSR: https://bugs.openjdk.java.net/browse/JDK-8185705

Thanks,

Ujwal.

Re: RFR: JDK-8185003 JMX: Add a version of ThreadMXBean.dumpAllThreads with a maxDepth argument

[Roger Riggs]

Hi Ujwal,


On 8/3/2017 12:44 PM, Ujwal Vangapally wrote:

Thanks for the review Roger, please see my comments inline.


On 8/3/2017 8:23 PM, Roger Riggs wrote:

Hi Ujwal,

(Reviewer, but not specifically servicability).

Comments,

java/lang/management/ThreadMXBean.java:

809: It may be useful to state that the behavior is the same as 
{@link #dumpAllThreads} except that the depth is limited.


828:  Do not duplicate specification of the meaning of maxDepth = 
MAX_VALUE, either put the entire spec
in the @param or in the description.  Duplication creates an 
opportunity for disagreement or a maintenance issue when updating.


833: terminate the {@code maxDepth} before "is negative".

As Erik comments, there should be a space after "," everywhere; both 
to be consistent with existing style and the style guide.

will make changes to doc as suggested.


sun/management/ThreadImpl.java:

484: Since the native code is going to check maxDepth, why is it 
checked here also?

-or- remove the check in native
As  maxDepth value should not be negative for dumpAllThreads 
sun/management/ThreadImpl.java: 484: throws IllegalArgumentException.


Check on native side can be removed but having it might help in 
enforcing the convention of using only -1 for dumping all the stack 
frames

otherwise any negative number can be passed to dump all the frames.
There is no need for a separate indication of dumping all stack frames; 
a count of MAX_INTEGER

will have the same effect.

Even the specification in ThreadMXBean does not need to call out the 
special value for maxDepth
 as MAX_INTEGER as a special value.  It just makes the spec more 
complicated.

490:  This could be simpler to not translate maxDepth to -1,
 Here and in the native code, passing MAX_INTEGER would dump the 
entire stack, there is no need to special case it


services/threadService.cpp:
565:  checking count >= maxDepth is a sufficient test if to dump all 
frames, passing MAX_INTEGER is used

  and it will dump zero should a negative number get this far.

currently getThreadInfo method accepts maxDepth argument and it uses 
the translation of maxDepth to "-1"  as it's implementation detail for 
printing all stack frames,
Still, with a sufficiently large maxDepth it is indistinguishable from 
the 'all frames' sentinel value.

The code is just more complicated than necessary as a result.

As getThreadInfo also uses services/threadService.cpp: 565: 
(dump_stack_at_safepoint) expecting it to print all stack trace 
elements when maxDepth is given as "-1".

Yes, it has the same effect as a very large maxDepth.
I followed same convention so that it will not affect getThreadInfo 
method with maxDepth argument.
fine, no need to change the native code, but the same result is achieved 
at the java level without

the adding the complexity in the implementation of ThreadImpl:489.

Roger


Thanks, Roger



Thanks,
Ujwal

On 8/3/2017 10:18 AM, Ujwal Vangapally wrote:

kindly use the below link for accessing webrev

http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.01/

previously shared link is no longer accessible hence providing this 
new link.


Thanks,

Ujwal.


On 8/3/2017 3:08 PM, Ujwal Vangapally wrote:

Hi,

kindly review the changes made.

https://bugs.openjdk.java.net/browse/JDK-8185003

webrev: 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.00/


CSR: https://bugs.openjdk.java.net/browse/JDK-8185705

Thanks,

Ujwal.

Re: RFE Review : JDK-5016517 - Replace plaintext passwords by hashed passwords for out-of-the-box JMX Agent

[Roger Riggs]

Hi Harsha,

FileLoginModule.java:  104:  Add a period at the end of the the sentence.

JMXPluggableAuthenticator.java: line 306:  Is the difference between 
singular and plural significant?

  It would be less confusing if both were plural (hashPasswords).

ConnectorBootstrap:
134: ...password.file.hash" and HashedPasswordManager disagree on the 
exact string.

I would propose 'hashpasswords' as the suffix in all places to be consistent
in ConnectorBootstrap.java, HashedPasswordManager (except for 
capitalization),

jmxremote.password.template, and management.properties

As is you have a mix of "...password.hash", "...password.file.hash", 
"...hashpassword";

that's not good for knowing there is only one semantic.

line 482:  " ," -> ", "  space after comma, not before

line: 771: is it intentional to discard the reference to the new 
HashedPasswordManager?
If the intention is only to use the side effect of loadPasswords, then 
please

create a static method in HashedPasswordManager for that purpose.
(Even if just does the same code; it would be clear that's the purpose).
(It probably also implies that the password file will be read a second 
time somewhere else in the initialization).


line:770:  the string constant would be nicer as a final static string 
somewhere.

  "jmx.remote.x.password.file.hashpassword"

Roger



On 10/3/2017 3:47 PM, Harsha Wardhana B wrote:


Hi Roger,

Thanks for the detailed review. Below is the webrev addressing all the 
review comments.


http://cr.openjdk.java.net/~hb/5016517/webrev.01/

-Harsha


On Tuesday 25 April 2017 10:56 PM, Roger Riggs wrote:

Hi Harsha,

Thanks for this important improvement. Comments:


* jmxremote.password.template:
  "Passwords will be hashed by server if they are in clear." Perhaps 
should be more explicit:


   "The jmxremote.passwords file will be re-written by the server to 
replace all plain text passwords with hashed passwords when the file 
is read by the server."


line 35: "Base64 encoded hash"  -> drop the "Base64" in this line 
isn't needed and

make it seems like it should appear as 1 field instead of 2 or 3.

37+: The syntax of the file may be clearer if it includes the 
complete syntax in (line 39) not

just the password/hash fragment.

Line 41:  "W = spaces"; above "tabs" are allowed as a delimiter; it 
would be good to be consistent
and include the usualy white-space characters in the set, be as 
specific as possible.

Is this the same set of whitespace used by Regex '\\s'.
Only spaces and tabs are allowed. '\s' matches newline as well hence 
not allowed.


45: "java platform""  ->   "MD5, SDA-1, SHA-256 are supported 
algorithms."


49: be more specific about 'hashing is requested'  how?  Refer to the 
management.properties

  com.sun.management.jmxremote.password.hash value.



51:  "replace hashed" -> "replace *the *hashed"
52: "with clear text or new" -> "with the clear text or the new"
52: "If new password" -> "If the new password"
53: "when new login" -> "when a new login"

60: "User generated" -> "A User generated"

67: Will the file be ignored if it has the wrong permissions. (With a 
logged message)

Addressed all the above review comments.


* management.properties

306: "(Case for true/false ignored)"  - what does this mean; I think 
it can be removed.


307: missing period at the end of the sentence.
309: "in password file" -> "in the password file"


Done.


* FileLoginModule.java

102: can this match better the similar name in the 
management.properties if it has the same function:

    com.sun.management.jmxremote.password.hash
Are you suggesting that 'hashPassword' be renamed to something similar 
to com.sun.management.jmxremote.password.hash? Variable names cannot 
be similar to property names since property names are long and provide 
complete context which local variables need not have to do.

the suffix should be the same in all places since it is a single semantic.
103: "replaces clear text passwords" -> "replaces each clear text 
password"

104: indent to match previous  enteries.

* JMXPluggableAuthenticator.java

119: There is no need to copy the password to a new local
It is required since variables accessed from inner class must be final 
or effectively final.

right


128: add a space after ","

256 private static final String HASH_PASSWORDS =
257 "jmx.remote.x.password.file.hash";

The name ".hash" part does not clearly communicate that passwords are 
to be hashed.

"hashPasswords" might be more self explanatory.

Changed it to "jmx.remote.x.password.file.ha

Re: RFR : JDK-8044122 MBean access to the PID

[Roger Riggs]

Hi Ujwal,

In the implementation RuntimeMXBean.java: 72:  Include a message 
"getProcessId" in the throw new Unsupported...

In the text and @return change "PID" to "process ID" as Alan suggested.
66: the @implSpec should be on its own line so the text starts on a new 
line to make the source more readable.


Adding a test for getProcessId() should fit into one of the existing 
tests that spawns and then checks

the attributes of a vm.  Perhaps MXBeanInteropTest1.java

Roger



On 10/10/2017 1:20 PM, mandy chung wrote:



On 10/10/17 4:47 AM, Ujwal Vangapally wrote:

Kindly review the changes made.

https://bugs.openjdk.java.net/browse/JDK-8044122

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/




RuntimeMXBean.java
   @since is missing

   Process::pid is long rather than int. The javadoc for this method 
should be consistent with Process::pid, as Alan points out.


VMManagementImpl.java
    I think getProcessId should probably be replaced to implement with 
ProcessHandle.current().pid();


Please include an unit test for it.

Mandy

Re: RFE Review : JDK-5016517 - Replace plaintext passwords by hashed passwords for out-of-the-box JMX Agent

[Roger Riggs]

Hi Harsha,

conf/management.properties: - typo line 307: pa*sss*words


HashedPasswordManager.java:
 - line 46: "classes" -> "class"

- line 84-87 "private" and 'static" come before "final" in declarations.

 - 158 and everywhere: add space after "if"  before "("

 - line 202: add "the" before password file.

 - line 287:  why a separate canWriteToFile(); it does the same check 
as newFileWriter(passwordFile);

   instead catch the exception and log then ignore.

Looking good

Regards, Roger

On 10/11/2017 5:00 AM, Daniel Fuchs wrote:

Hi Harsha,

Your changes look good. However I have still a nagging doubt:

What happens if two Java process share the same password file,
and it needs hashing? Are there any protection in place
to prevent the two processes from writing to the same
file concurrently?

best regards,

-- daniel

On 09/10/2017 06:34, Harsha Wardhana B wrote:

Hi Daniel,

Below is the webrev addressing the review comments.

http://cr.openjdk.java.net/~hb/5016517/webrev.04/

On Friday 06 October 2017 03:38 PM, Daniel Fuchs wrote:

Hi Harsha,

Good work!

> http://cr.openjdk.java.net/~hb/5016517/webrev.03/

long standing typo in management.properties at line 90:

  measureRole => monitorRole

Done.


HashedPasswordManager.java: loadPasswords()

It seems this function will add the header to the file even
if it already contains the header.

So every time a user/administrator wants to change/add a password,
the header will be inserted again.


Yes. It is fixed in the new webrev.


HashedPasswordFileTest should probably have a test for this
scenario as well:

generate password file with clear text password
load it, then verify passwords have been hashed (properly)
add some new user/name password to the same file
load it again, verify all passwords are hashed
(do this a number of times - to make sure it doesn't
 break the second or third time)
and finally verify the header is only present once ;-)


Done. Added a testcase for the same.

I'm surprised no other tests had to be modified.
Is password hash disabled by default in the default agent?

Password hashing is enabled by default. But it is only the 
implementation that is changed. The pluggable JAAS mechanism isolates 
interfaces from implementation. So in theory, all tests should pass.

If not then you should try (locally) running jtreg
more than once over the default agent tests.
Just make sure running the same test twice doesn't
make the legacy tests that use password files failing the
second time when they discover that passwords have been
hashed under their feet (the client part of the test
might be reading the password file too to see which
password it should send to the agent).

Otherwise I think it looks good to me - provided all
tests are passing!


Done. Had a few test failures but nothing related to this enhancement.

best regards,

-- daniel

Thanks
Harsha


On 06/10/2017 06:25, Harsha Wardhana B wrote:

Hi All,

Previously, for default agent, hashing of the passwords was done 
during the agent boot-up (ConnectorBootstrap.java). That was an 
error since login configuration could be different and is 
determined only when a login attempt is made. It would be then 
pointless to hash the password file. The fix for above and some 
off-list comments are incorporated in webrev below.


http://cr.openjdk.java.net/~hb/5016517/webrev.03/

-Harsha


On Wednesday 04 October 2017 01:53 PM, Harsha Wardhana B wrote:

Hi Roger,

Below is the webrev incorporating changes suggested by you.

http://cr.openjdk.java.net/~hb/5016517/webrev.02/

-Harsha

On Wednesday 04 October 2017 12:54 AM, Roger Riggs wrote:

Hi Harsha,

FileLoginModule.java:  104:  Add a period at the end of the the 
sentence.


JMXPluggableAuthenticator.java: line 306:  Is the difference 
between singular and plural significant?

  It would be less confusing if both were plural (hashPasswords).

Ok.

ConnectorBootstrap:
134: ...password.file.hash" and HashedPasswordManager disagree on 
the exact string.
I would propose 'hashpasswords' as the suffix in all places to be 
consistent
in ConnectorBootstrap.java, HashedPasswordManager (except for 
capitalization),

jmxremote.password.template, and management.properties

Do you want to rename HashedPasswordManager class?


As is you have a mix of "...password.hash", 
"...password.file.hash", "...hashpassword";

that's not good for knowing there is only one semantic.

line 482:  " ," -> ", "  space after comma, not before


Will incorporate above comments.
line: 771: is it intentional to discard the reference to the new 
HashedPasswordManager?
If the intention is only to use the side effect of loadPasswords, 
then please

create a static method in HashedPasswordManager for that purpose.
(Even if just does the same code; it would be clear that's the 

Re: RFR : JDK-8044122 MBean access to the PID

[Roger Riggs]

Hi Ujwal,

Looks fine.

Please correct the copyright date in ProcessIdTest.  A new file usually 
has only the current year.


Thanks, Roger


On 10/20/17 2:07 AM, Ujwal Vangapally wrote:

kindly see the new webrev incorporating review comments.

webrev: 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/


Thanks,

Ujwal.


On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:

Thanks for the review and suggestions Mandy, Roger.

kindly see my comments inline.


On 10/10/2017 11:25 PM, Roger Riggs wrote:

Hi Ujwal,

In the implementation RuntimeMXBean.java: 72:  Include a message 
"getProcessId" in the throw new Unsupported...

In the text and @return change "PID" to "process ID" as Alan suggested.
66: the @implSpec should be on its own line so the text starts on a 
new line to make the source more readable.


Adding a test for getProcessId() should fit into one of the existing 
tests that spawns and then checks

the attributes of a vm.  Perhaps MXBeanInteropTest1.java

I will make changes as suggested.


Roger



On 10/10/2017 1:20 PM, mandy chung wrote:



On 10/10/17 4:47 AM, Ujwal Vangapally wrote:

Kindly review the changes made.

https://bugs.openjdk.java.net/browse/JDK-8044122

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/ 





RuntimeMXBean.java
   @since is missing


I will add it.
   Process::pid is long rather than int. The javadoc for this 
method should be consistent with Process::pid, as Alan points out.

will do it.


VMManagementImpl.java
    I think getProcessId should probably be replaced to implement 
with ProcessHandle.current().pid();


you mean it would be better to use ProcessHandle.current().pid(); in 
RuntimeImpl.java instead of jvm.getVmPid();


kindly clarify.

Please include an unit test for it.


will it be sufficient to add it to existing test MXBeanInteropTest1.java

 System.out.println("getName\t\t"
 + runtime.getName());
+    System.out.println("getPid\t\t"
+    + runtime.getPid());
 System.out.println("getSpecName\t\t"
 + runtime.getSpecName());


Mandy




Ujwal

Re: RFR : JDK-8044122 MBean access to the PID

[Roger Riggs]

Hi Ujwal,

In RuntimeMXBean:

Please add @throws UnsupportedOperationException if the process id is 
not available


Otherwise, looks fine.

Thanks, Roger


On 10/26/2017 4:29 AM, Ujwal Vangapally wrote:


Thanks for the review Mandy, Roger, Harsha, Christoph.

kindly see the new webrev incorporating review comments.

webrev: 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.02/


csr : https://bugs.openjdk.java.net/browse/JDK-8189091

Ujwal.


On 10/24/2017 10:50 PM, mandy chung wrote:
The permission check should be done separately, if needed. Otherwise, 
the frames in the stack when this method is called must have the 
security permission granted.


Mandy

On 10/23/17 1:00 PM, Bernd Eckenfels wrote:

Hello,

When running this privileged it means one can bypass the permission 
by using the MBean, is that intentional? (Besides it is already 
available as the JMVID)


Gruss
Bernd
--
http://bernd.eckenfels.net

*From:* serviceability-dev 
 on behalf of mandy 
chung 

*Sent:* Friday, October 20, 2017 4:42:00 PM
*To:* Ujwal Vangapally; Roger Riggs
*Cc:* serviceability-dev
*Subject:* Re: RFR : JDK-8044122 MBean access to the PID
Process::pid may throw SecurityException.  You have to wrap the call
with doPrivileged.   Process::pid can throw UOE on platform that 
doesn't

support this operation.  RuntimeMXBean::getPid should specify when the
platform does not support this operation.

ProcessIdTest - can you also check if getName contains the pid matching
the value of getPid()?

Mandy

On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
> kindly see the new webrev incorporating review comments.
>
> webrev:
> 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/ 
<http://cr.openjdk.java.net/%7Euvangapally/webrev/2017/8044122/webrev.01/>

>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan 
suggested.

>>> 66: the @implSpec should be on its own line so the text starts on a
>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the 
existing

>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/ 
<http://cr.openjdk.java.net/%7Euvangapally/webrev/2017/8044122/webrev.00/> 


>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan points out.
>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to implement
>>>> with ProcessHandle.current().pid();
>>>>
>> you mean it would be better to use ProcessHandle.current().pid(); in
>> RuntimeImpl.java instead of jvm.getVmPid();
>>
>> kindly clarify.
>>>> Please include an unit test for it.
>>>>
>> will it be sufficient to add it to existing test 
MXBeanInteropTest1.java

>>
>>  System.out.println("getName\t\t"
>>  + runtime.getName());
>> +    System.out.println("getPid\t\t"
>> +    + runtime.getPid());
>> System.out.println("getSpecName\t\t"
>>  + runtime.getSpecName());
>>
>>>> Mandy
>>>
>>
>> Ujwal
>

Re: RFR : JDK-8044122 MBean access to the PID

[Roger Riggs]

Looks good to me.

Thanks, Roger

On 10/30/2017 12:50 PM, Ujwal Vangapally wrote:


Hi Mandy,

yes, this makes it more clear.

kindly take a look at new webrev.

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.04/


Thanks,

Ujwal.

On 10/28/2017 4:35 AM, mandy chung wrote:
This interface method provides a default implementation to throw UOE 
to ease migration but RuntimeMXBean::getPid is not intended to be 
optional.  In other words, the platform mbean (i.e. JDK 
implementation of RuntimeMXBean) must implement this method.  This 
deserves further clarification.


What about:

@throws UOE if this default implementation is not overridden.
RuntimeMXBean for the Java platform obtained from {@link 
ManagementFactory} supports this operation.


Mandy

On 10/26/17 9:36 AM, Ujwal Vangapally wrote:


Hi Roger,

made changes as suggested.

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.03/


Thanks,
Ujwal.

On 10/26/2017 6:54 PM, Roger Riggs wrote:

Hi Ujwal,

In RuntimeMXBean:

Please add @throws UnsupportedOperationException if the process id 
is not available


Otherwise, looks fine.

Thanks, Roger


On 10/26/2017 4:29 AM, Ujwal Vangapally wrote:


Thanks for the review Mandy, Roger, Harsha, Christoph.

kindly see the new webrev incorporating review comments.

webrev: 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.02/


csr : https://bugs.openjdk.java.net/browse/JDK-8189091

Ujwal.


On 10/24/2017 10:50 PM, mandy chung wrote:
The permission check should be done separately, if needed.  
Otherwise, the frames in the stack when this method is called 
must have the security permission granted.


Mandy

On 10/23/17 1:00 PM, Bernd Eckenfels wrote:

Hello,

When running this privileged it means one can bypass the 
permission by using the MBean, is that intentional? (Besides it 
is already available as the JMVID)


Gruss
Bernd
--
http://bernd.eckenfels.net

*From:* serviceability-dev 
 on behalf of mandy 
chung 

*Sent:* Friday, October 20, 2017 4:42:00 PM
*To:* Ujwal Vangapally; Roger Riggs
*Cc:* serviceability-dev
*Subject:* Re: RFR : JDK-8044122 MBean access to the PID
Process::pid may throw SecurityException.  You have to wrap the 
call
with doPrivileged.   Process::pid can throw UOE on platform that 
doesn't
support this operation.  RuntimeMXBean::getPid should specify 
when the

platform does not support this operation.

ProcessIdTest - can you also check if getName contains the pid 
matching

the value of getPid()?

Mandy

On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
> kindly see the new webrev incorporating review comments.
>
> webrev:
> 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/ 
<http://cr.openjdk.java.net/%7Euvangapally/webrev/2017/8044122/webrev.01/>

>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a 
message

>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan 
suggested.
>>> 66: the @implSpec should be on its own line so the text 
starts on a

>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the 
existing

>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/ 
<http://cr.openjdk.java.net/%7Euvangapally/webrev/2017/8044122/webrev.00/> 


>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan 
points out.

>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to 
implement

>>>> with ProcessHan

Re: RFE Review : JDK-5016517 - Replace plaintext passwords by hashed passwords for out-of-the-box JMX Agent

[Roger Riggs]

Hi Harsha,

Sorry for the late editorial recommendations:

In jmxremote.password.template:

41: "Clear text" -> "A clear text"
43: 'below format" -> "format below"
53: "in clear" -> "in the clear"
63: "in clear" -> "in the clear"
77: "by ONLY the owner" -> "ONLY by the owner"
80-81: Is not consistent with the 77-78;  80-81 can be removed
82: "should" -> "must" to be consistent with 77:
82: "except for owner" ->  "ONLY by the owner"
92: should end with "." or ':"
97: "passwords will" -> "the passwords will"
98: "below entries with clear" -> "the entries below with the clear"
99: "should end with "." or ":"

management.properties:
311: sentence should end with "."

Thanks, Roger

On 10/31/2017 1:07 PM, mandy chung wrote:



On 10/31/17 8:55 AM, Harsha Wardhana B wrote:


Hi Mandy,

Below is the new webrev incorporating below review comments.

http://cr.openjdk.java.net/~hb/5016517/webrev.06/


Looks okay in general except this:
  286 // Check if header needs to be inserted
  287 if (sbuf.indexOf("# The passwords in this file are hashed") != 0) 
{
  288 String lastUpdated = "# file last updated on - "
  289 + new SimpleDateFormat("MM/dd/ HH:mm:ss").format(new 
Date()) + "\n\n";
  290 sbuf.insert(0, header + lastUpdated);
  291 }

Relying on matching the partial header string is fragile.
Also the timestamp is not updated if the file contains such
heading but the file is re-written again.

You should probably drop the header (auto-inserted), not add
it to sbuf, and always add the header when updating the
password file.

Mandy

Re: RFR : JDK-8024352 - MBeanOperationInfo accepts any int value as "impact"

[Roger Riggs]

Hi Ujwal,

MBeanOperationInfo:163:
Since the values are fixed, you could more concisely just compare impact 
>=0 and impact <= UNKNOWN.


257/263:  I don't see a reason to change the toString in the default 
case for getImpact().



A suggestion would be to introduce an Enum for the action values and the 
corresponding new

method; perhaps deprecating the current method (or not).
The enum would use the same values as currently and so internally the 
implementation does not

change significantly.

$.02, Roger


On 11/7/2017 6:05 AM, Ujwal Vangapally wrote:

Kindly review the fix for bug below.

https://bugs.openjdk.java.net/browse/JDK-8024352

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.00/


Thanks,

Ujwal.

Re: RFR : JDK-8024352 - MBeanOperationInfo accepts any int value as "impact"

[Roger Riggs]

+1

Looks good,

Thanks, Roger


On 11/9/17 6:34 AM, Daniel Fuchs wrote:

On 09/11/2017 10:40, Ujwal Vangapally wrote:

Thanks for the Review Daniel, made changes as suggested.

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.03/


Looks good to me.

-- daniel



Ujwal.

Re: RFR : JDK-8024352 - MBeanOperationInfo accepts any int value as "impact"

[Roger Riggs]

+1

On 11/15/2017 10:09 AM, Daniel Fuchs wrote:

Hi Ujwal,

Still looks good to me.

best regards,

-- daniel

On 15/11/2017 13:18, Ujwal Vangapally wrote:
kindly review the updated webrev including changes to 
MBeanInfoHashCodeNPETest.java


webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.05/


Thanks,

Ujwal.


On 11/9/2017 10:33 PM, Ujwal Vangapally wrote:

Thanks for the review Mandy,

kindly check if this version is better.

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.04/


Ujwal


On 11/9/2017 9:10 PM, mandy chung wrote:



On 11/9/17 2:40 AM, Ujwal Vangapally wrote:

Thanks for the Review Daniel, made changes as suggested.

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.03/ 





Looks good.

Minor comment: in the new test, it can fold some of the println 
together e.g. line 81 can be merged with line 39 to include the 
value being passed.  Similarly for the println in the main method.


Mandy

Re: RFR : JDK-8191313 - deprecate RMIConnectorServer.CREDENTIAL_TYPES

[Roger Riggs]

Hi Ujwal,

Looks fine.

(There may be a bit of confusion because in the webrev it looks like the 
value is being added and deprecated.
The field is present in JDK 9 and this is formalizing the intention to 
remove it after JDK 1).


(I cleaned up the csr a bit to remove extraneous information in the 
changset).


Thanks, Roger


On 11/15/2017 8:09 AM, Ujwal Vangapally wrote:

kindly review the changes for bug below.

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8191313

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8191313/webrev.00/


CSR : https://bugs.openjdk.java.net/browse/JDK-8191314

Thanks,

Ujwal.

Re: RFR(XXS): 8182307 - Error during JRMP connection establishment

[Roger Riggs]

+1

On 12/7/2017 12:00 PM, Gerald Thornbrugh wrote:

Hi Dan,

Your fix looks good.

Jerry


Adding core-libs-dev@... since this is RMI code. Thanks Alan!

Folks, this review spans three OpenJDK aliases so Thunderbird's
reply-to-list feature won't get it right. This is one of the
few times that reply-to-all is the right thing to do...

Dan

On 12/7/17 11:38 AM, Daniel D. Daugherty wrote:

Greetings,

I have a small fix for a very intermittent ServerSocket related test
failure:

    JDK-8182307: Error during JRMP connection establishment
    https://bugs.openjdk.java.net/browse/JDK-8182307

The fix is copied from Jerry's work on the following bugs:

    JDK-8182757 JDWP: Socket Transport handshake hangs on Solaris
    https://bugs.openjdk.java.net/browse/JDK-8182757

    JDK-8178676 nsk/jvmti/AttachOnDemand/attach045 fails with Exception
    https://bugs.openjdk.java.net/browse/JDK-8178676

For the gory details of the reasons for this fix please see
Jerry's bugs.

Webrev URL: http://cr.openjdk.java.net/~dcubed/8182307-webrev/jdk10-0/


I observed this test failure one time in my Thread-SMR work and have
been including this fix in months of Thread-SMR stress testing. It was
also included in both JPRT and Mach5 tier[1-5] testing of the 
Thread-SMR

changes.

Thanks, in advance, for any comments, suggestions or questions.

Dan

Re: RFR: JDK-8167253: com.sun.jdi invokeMethod has duplicated @throws for InvalidTypeException

[Roger Riggs]

Hi Gary,

Looks fine,

Roger


On 1/22/2018 12:43 PM, Gary Adams wrote:

Here's a small fix to remove a duplicate InvalidTypeException.
The other reported locations appear to have already been fixed.

   Issue: https://bugs.openjdk.java.net/browse/JDK-8167253
   Webrev: http://cr.openjdk.java.net/~gadams/8167253/

Re: RFR: 8041626: [Event Request] Shutdown reason

[Roger Riggs]

Hi Robin,

It looks like the status argument to BeforeHalt is discarded in 
JVM_BeforeHalt

and is not inserted into the event.
That suggests it should be removed all the way back to Shutdown.beforeHalt.

Roger



On 2/13/2018 9:59 AM, Robin Westberg wrote:

Hi Alan,


On 12 Feb 2018, at 09:02, Alan Bateman  wrote:



On 12/02/2018 07:07, David Holmes wrote:

Okay, I’ve removed the code related to the status field, certainly makes the 
change a bit less intrusive.

Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/
Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/

This looks much cleaner/neater to me - thanks.


The updates to Runtime/Shutdown seems okay.

Thanks for reviewing!

Best regards,
Robin


-Alan

Re: RFR: 8041626: [Event Request] Shutdown reason

[Roger Riggs]

Hi Robin,

Looks fine to me.

(How is this tested?, Normal, exceptional, etc.)

Thanks, Roger


On 2/15/2018 8:35 AM, Robin Westberg wrote:

Hi again,

Here’s the (hopefully) final version:

Full: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.02/ 
<http://cr.openjdk.java.net/%7Erwestberg/8041626/webrev.02/>
Incremental: 
http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01-02/ 
<http://cr.openjdk.java.net/%7Erwestberg/8041626/webrev.01-02/>


Best regards,
Robin

On 14 Feb 2018, at 16:15, Robin Westberg <mailto:robin.westb...@oracle.com>> wrote:


Hi Roger,

On 13 Feb 2018, at 16:17, Roger Riggs <mailto:roger.ri...@oracle.com>> wrote:


Hi Robin,

It looks like the status argument to BeforeHalt is discarded in 
JVM_BeforeHalt

and is not inserted into the event.
That suggests it should be removed all the way back to 
Shutdown.beforeHalt.


You are right, my thinking was that the interface wouldn’t need to be 
changed if we decided to revisit the event in the future and add the 
status code. But that is perhaps unlikely, so can certainly remove 
the argument for now.


Best regards,
Robin



Roger



On 2/13/2018 9:59 AM, Robin Westberg wrote:

Hi Alan,

On 12 Feb 2018, at 09:02, Alan Bateman <mailto:alan.bate...@oracle.com>> wrote:




On 12/02/2018 07:07, David Holmes wrote:
Okay, I’ve removed the code related to the status field, 
certainly makes the change a bit less intrusive.


Updated webrev: 
http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/ 
<http://cr.openjdk.java.net/%7Erwestberg/8041626/webrev.01/>
Incremental: 
http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/ 
<http://cr.openjdk.java.net/%7Erwestberg/8041626/webrev.00-01/>

This looks much cleaner/neater to me - thanks.


The updates to Runtime/Shutdown seems okay.

Thanks for reviewing!

Best regards,
Robin


-Alan

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

[Roger Riggs]

Hi,

I'm a bit leary of command line arguments being special cased and the 
corresponding custom
mappings to system properties.   The convenience is fine but we need to 
keep the handling
out of native code so it is easier to maintain.  We don't have a Java 
API for processing

(VM) command line arguments so they are being shoehorned into properties.

$.02, Roger

On 2/21/2018 12:55 AM, Harsha Wardhana B wrote:



On Wednesday 21 February 2018 01:51 AM, mandy chung wrote:

The code review and CSR review can be in parallel.  For this case,
I agree with Kumar to have CSR written that would help the code
review. Please specify the behavior and its relationship with
jcmd and other relevant diagnosability tools.

ok.


On 2/20/18 6:41 AM, Kumar Srinivasan wrote:


What is the behavior when 
-Dcom.sun.management.jmxremote.port=1234 --start-management-agent 
port=2345 -Dcom.sun.management.jmxremote.port=3456?


What is the value of the system property 
com.sun.management.jmxremote.port at runtime?  What port number 
does the management server start with?
As said earlier, values set via new flags override values set by 
-D flags. So 2345 will be the value of 
com.sun.management.jmxremote.port. Added a test case to validate 
that.


VM options are the last one wins if same option specified multiple 
times.  In this case, it could cause confusion (the last -D option 
sets the value to 3456 but it's set to a different value).


Why not taking the simplest approach - when --start-management-agent 
is set, it does not accept mixing the old way (i.e. does not accept 
the management properties to be set via -D)?   This RFE is to make 
the command-line simpler and ease-of-use.  I don't see any downside 
to migrate entirely to the new form.
We cannot get rid of specifying options via -D. We have plenty of -D 
flags but very few have short-hand alternative via 
--start-management-agent. If management properties are specified by 
--start-management-agent, the options specified by -D are anyway 
overwritten if specified.


Mandy

Harsha

RFR 8199467 Compilation Errors in libinstrument Reentrancy.c with VS2017

[Roger Riggs]

Please review a small change to avoid sign extension in libinstrument/ 
Reentrancy.c

to correct an compilation warning with vs2017.

diff --git a/src/java.instrument/share/native/libinstrument/Reentrancy.c 
b/src/java.instrument/share/native/libinstrument/Reentrancy.c

--- a/src/java.instrument/share/native/libinstrument/Reentrancy.c
+++ b/src/java.instrument/share/native/libinstrument/Reentrancy.c
@@ -90,7 +90,7 @@ assertTLSValue( jvmtiEnv *  jvmtienv
 jthread thread,
 const void *    expected) {
 jvmtiError  error;
-    void *  test = (void *) 0x;
+    void *  test = (void *) 0xul;

 /* now check if we do a fetch we get what we wrote */
 error = (*jvmtienv)->GetThreadLocalStorage(

Issue:
  https://bugs.openjdk.java.net/browse/JDK-8199467

Thanks, Roger

Re: RFR 8199467 Compilation Errors in libinstrument Reentrancy.c with VS2017

[Roger Riggs]

Hi Martin,

Good recommendation; pushed.

Thanks, Roger



On 3/21/2018 3:31 PM, Martin Buchholz wrote:



On Wed, Mar 21, 2018 at 12:13 PM, Roger Riggs <mailto:roger.ri...@oracle.com>> wrote:



-    void *  test = (void *) 0x;
+    void *  test = (void *) 0xul;


Martin's 15th law: Never use "l" in a numeric constant unless the 
constant is 0xCafeBabel,


so 0xUL

hg: jdk8/tl/jdk: 8024458: DataInput.readDouble refers to "readlong" instead of "readLong"

[roger . riggs]

Changeset: 04f071a95c29
Author:rriggs
Date:  2013-11-07 20:56 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/04f071a95c29

8024458: DataInput.readDouble refers to "readlong" instead of "readLong"
Summary: fix the typo
Reviewed-by: lancea, chegar, dxu

! src/share/classes/java/io/DataInput.java

hg: jdk8/tl/jdk: 8028041: Serialized Form description of j.l.String is not consistent with the implementation

[roger . riggs]

Changeset: df2f7f288353
Author:rriggs
Date:  2013-11-08 17:50 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/df2f7f288353

8028041: Serialized Form description of j.l.String is not consistent with the 
implementation
Summary: Replaced incorrect description with reference to the serialization 
specification
Reviewed-by: alanb, smarks

! src/share/classes/java/lang/String.java

hg: jdk8/tl/jdk: 8028092: Lint cleanup of java.time.format

[roger . riggs]

Changeset: 3add16c86970
Author:rriggs
Date:  2013-11-09 14:30 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3add16c86970

8028092: Lint cleanup of java.time.format
Summary: correct declarations and add @SuppressWarnings
Reviewed-by: darcy, lancea

! src/share/classes/java/time/format/DateTimeParseContext.java
! src/share/classes/java/time/format/Parsed.java

hg: jdk8/tl/jdk: 8028014: Doclint warning/error cleanup in javax.management

[roger . riggs]

Changeset: ebe27e1a2e2d
Author:rriggs
Date:  2013-11-12 14:03 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ebe27e1a2e2d

8028014: Doclint warning/error cleanup in javax.management
Summary: Improve generated html by fixing doclint warnings
Reviewed-by: sla, jbachorik

! src/share/classes/javax/management/AttributeList.java
! src/share/classes/javax/management/BooleanValueExp.java
! src/share/classes/javax/management/Descriptor.java
! src/share/classes/javax/management/DescriptorKey.java
! src/share/classes/javax/management/ImmutableDescriptor.java
! src/share/classes/javax/management/JMX.java
! src/share/classes/javax/management/MBeanFeatureInfo.java
! src/share/classes/javax/management/MBeanInfo.java
! src/share/classes/javax/management/MBeanServer.java
! src/share/classes/javax/management/MBeanServerConnection.java
! src/share/classes/javax/management/MBeanServerNotification.java
! src/share/classes/javax/management/MXBean.java
! src/share/classes/javax/management/NumericValueExp.java
! src/share/classes/javax/management/ObjectName.java
! src/share/classes/javax/management/PersistentMBean.java
! src/share/classes/javax/management/Query.java
! src/share/classes/javax/management/loading/MLet.java
! src/share/classes/javax/management/loading/MLetParser.java
! src/share/classes/javax/management/modelmbean/DescriptorSupport.java
! src/share/classes/javax/management/modelmbean/ModelMBeanAttributeInfo.java
! src/share/classes/javax/management/modelmbean/ModelMBeanConstructorInfo.java
! src/share/classes/javax/management/modelmbean/ModelMBeanInfo.java
! 
src/share/classes/javax/management/modelmbean/ModelMBeanNotificationBroadcaster.java
! src/share/classes/javax/management/modelmbean/ModelMBeanNotificationInfo.java
! src/share/classes/javax/management/modelmbean/ModelMBeanOperationInfo.java
! src/share/classes/javax/management/modelmbean/RequiredModelMBean.java
! src/share/classes/javax/management/monitor/Monitor.java
! src/share/classes/javax/management/openmbean/ArrayType.java
! 
src/share/classes/javax/management/openmbean/CompositeDataInvocationHandler.java
! src/share/classes/javax/management/openmbean/CompositeType.java
! 
src/share/classes/javax/management/openmbean/OpenMBeanAttributeInfoSupport.java
! 
src/share/classes/javax/management/openmbean/OpenMBeanParameterInfoSupport.java
! src/share/classes/javax/management/openmbean/SimpleType.java
! src/share/classes/javax/management/openmbean/TabularType.java
! src/share/classes/javax/management/relation/Relation.java
! src/share/classes/javax/management/relation/RelationService.java
! src/share/classes/javax/management/relation/RelationServiceMBean.java
! src/share/classes/javax/management/relation/RelationSupport.java
! src/share/classes/javax/management/remote/JMXConnectionNotification.java
! src/share/classes/javax/management/remote/JMXConnector.java
! src/share/classes/javax/management/remote/JMXConnectorProvider.java
! src/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
! src/share/classes/javax/management/remote/rmi/RMIConnector.java
! src/share/classes/javax/management/remote/rmi/RMIConnectorServer.java
! src/share/classes/javax/management/remote/rmi/RMIServerImpl.java

RFR 8028141 intermittent test failure of LocalManagementTest

[roger riggs]

Please review this fix to improve the reliability of the 
jmxRemote/LocalManagementTest and CustomLauncherTest. The solution may 
apply to other tests that fail with ClassNotFound.


The tests did not include the jdk.testlibrary in the classpath when it 
spawns a new process. The failure mode is dependent on the order that 
tests had been run.


Also note that if a test depends on classes in the jdk.testlibrary it 
should include @build  to ensure they are built in the 
@library directory


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-localmanagement-8028141/

Thanks, Roger

Re: RFR 8028141 intermittent test failure of LocalManagementTest

[roger riggs]

Hi Chris,

Here's an example from a failing test run:

 
-J-Dtest.classes=/export/jdk8-tl-test/JTwork/classes/0/sun/management/jmxremote/bootstrap
 \\
 
-J-Dtest.class.path=/export/jdk8-tl-test/JTwork/classes/0/sun/management/jmxremote/bootstrap:/export/jdk8-tl-test/JTwork/classes/0/lib/testlibrary

Note that test.classes does not include the @library path lib/testlibrary.

So if lib/testlibrary is supposed to be available to the spawned process 
then using test.class.path is needed.


Including @build will compile the library classes into /lib/testlibrary 
but that's not enough.


It happens that if the classes are not already compiled in 
lib/testlibrary (and there is no @build)
then the files will be compiled on demand by javac into the test.classes 
directory.
And in that case they are 'accidentally' available to the spawned 
process but by using @library

that does not seem to be the intention.

The test fails if the lib/testlibrary classes have been compiled by a 
previous test.


Yes, there are other tests using jdk.testlibrary that do not have the 
@build;

there maybe some tests that fail to pass the full test.class.path.
I'll recheck , but that's out of scope for the particular issue.

Thanks, Roger





On 11/18/2013 5:25 PM, Chris Hegarty wrote:

On 18 Nov 2013, at 21:59, roger riggs  wrote:

Please review this fix to improve the reliability of the 
jmxRemote/LocalManagementTest and CustomLauncherTest. The solution may apply to 
other tests that fail with ClassNotFound.

The tests did not include the jdk.testlibrary in the classpath when it spawns a 
new process. The failure mode is dependent on the order that tests had been run.

Also note that if a test depends on classes in the jdk.testlibrary it should include 
@build  to ensure they are built in the @library directory

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-localmanagement-8028141/

I've come across some of these before. The changes to the tags make sense.

About the change to the property, test.class.path, I thought test.classes was 
right. Is this different from what other tests are doing?

-Chris.


Thanks, Roger

Re: RFR 8028141 intermittent test failure of LocalManagementTest

[roger riggs]

Hi,

The new utilities are useful though not directly for this fix because 
the code

needs to append to the classpath, not just the command arguments.

Minor comments on the new utilities, for future consideration.
 - In the getCommandLine impl; java.util.StringJoiner may be useful 
though not much shorter.
 - A bit more esoteric but using the latest lambda/streams, the method 
could be implemented as:


public static String getCommandLine(ProcessBuilder pb) {

return pb.command().stream().collect(Collectors.joining(" 
")).toString();

}

 - getTestOpts:93, 105 could supply a String[opts.size()] so the string 
array does not needs to be reallocated.


 - The mapping between lists and arrays seems awkward here and in 
ProcessTools.
   Some cleanup might be able to treat that more consistently and be 
easier to read.


Thanks, Roger


On 11/19/2013 3:05 AM, taras ledkov wrote:
Dis you see the review: 
http://cr.openjdk.java.net/~ykantser/8023138/webrev.00/test/lib/testlibrary/jdk/testlibrary/Utils.java.sdiff.html

(e.g. please take a look at the Utils.addTestJavaOpts)

What do you think about common solution for test properties inheritance
(for ProcessTools utils) because it is the frequent pattern.

On 19.11.2013 1:59, roger riggs wrote:

Please review this fix to improve the reliability of the
jmxRemote/LocalManagementTest and CustomLauncherTest. The solution may
apply to other tests that fail with ClassNotFound.

The tests did not include the jdk.testlibrary in the classpath when it
spawns a new process. The failure mode is dependent on the order that
tests had been run.

Also note that if a test depends on classes in the jdk.testlibrary it
should include @build  to ensure they are built in the
@library directory

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-localmanagement-8028141/

Thanks, Roger

hg: jdk8/tl/jdk: 8028141: test/sun/management/jmxremote/bootstrap/LocalManagementTest|CustomLauncherTest.java failing again

[roger . riggs]

Changeset: 3f47e393e1dd
Author:rriggs
Date:  2013-11-19 13:20 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3f47e393e1dd

8028141: 
test/sun/management/jmxremote/bootstrap/LocalManagementTest|CustomLauncherTest.java
 failing again
Summary: Correct to use the test.class.path instead of test.classes
Reviewed-by: alanb, chegar

! test/sun/management/jmxremote/bootstrap/CustomLauncherTest.java
! test/sun/management/jmxremote/bootstrap/LocalManagementTest.java

Re: Review request for 7195249: Some jtreg tests use hard coded ports

[roger riggs]

Hi,

fyi,  The jdk.testlibrary.Utils.getFreePort() method will Open an free 
Socket, close it and return

the port number.

And as Alan recommended, use (0) when possible to have the system assign 
the port #.


Roger

On 11/20/2013 8:04 AM, Dmitry Samersoff wrote:

Taras,

*The only* correct way to take really free port is:

1. Chose random number between 49152 and 65535
2. Open socket

if socket fails - repeat step 1
if socket OK - return *socket*


If you can't keep the socket open (e.g. you have to pass port number as
property value) you shouldn't do any pre-check as it has no value - as
as soon as you close socket someone can take the port.

So just choose a random number within the range above and let networking
code opening socket to handle port conflict.

-Dmitry



On 2013-11-20 15:54, taras ledkov wrote:

Hi Everyone,

I am working on bug https://bugs.openjdk.java.net/browse/JDK-7195249.

There are two webrevs:
Webrev for jdk part:
http://cr.openjdk.java.net/~anazarov/7195249/jdk/webrev.00/

Webrev for hs part:
http://cr.openjdk.java.net/~anazarov/7195249/hs/webrev.00/

Please take a look at some notes:
- After discussing with Yekaterina Kantserova & Jaroslav Bachorik some
shell tests have been converted to java based tests

- PasswordFilePermissionTest & SSLConfigFilePermissionTest tests looked
very similar, so a common parent class was created for them:
AbstractFilePermissionTest

- What was called RmiRegistrySslTest.java I've renamed to
RmiRegistrySslTestApp.java. The java code to replace old shell script
RmiRegistrySslTest.sh is called RmiRegistrySslTest.java, hence the huge
diff.

- The new RmiRegistrySslTest.java has some lines similar to the
AbstractFilePermissionTest.java, I nevertheless decided to not
complicate the code further and leave it as is. Please let me know if
this is somehow not acceptable

- com/oracle/java/testlibrary/Utils.java that is added to hotspot
repository is taken from this patch:
http://cr.openjdk.java.net/~ykantser/8023138/webrev.00/test/lib/testlibrary/jdk/testlibrary/Utils.java.sdiff.html


- These tests will need additional changes when test library process
tools will support command line options inheritance
(http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-November/013235.html)

hg: jdk8/tl/jdk: 8028019: AWT Doclint warning/error cleanup

[roger . riggs]

Changeset: 3e95aadb479f
Author:rriggs
Date:  2013-12-03 16:20 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3e95aadb479f

8028019: AWT Doclint warning/error cleanup
Summary: Fix numerious javadoc and html errors and warnings
Reviewed-by: yan

! src/share/classes/java/applet/Applet.java
! src/share/classes/java/applet/AppletContext.java
! src/share/classes/java/awt/AWTPermission.java
! src/share/classes/java/awt/AlphaComposite.java
! src/share/classes/java/awt/BasicStroke.java
! src/share/classes/java/awt/BorderLayout.java
! src/share/classes/java/awt/Button.java
! src/share/classes/java/awt/Checkbox.java
! src/share/classes/java/awt/CheckboxGroup.java
! src/share/classes/java/awt/CheckboxMenuItem.java
! src/share/classes/java/awt/Choice.java
! src/share/classes/java/awt/Component.java
! src/share/classes/java/awt/Container.java
! src/share/classes/java/awt/EventFilter.java
! src/share/classes/java/awt/EventQueue.java
! src/share/classes/java/awt/FileDialog.java
! src/share/classes/java/awt/FlowLayout.java
! src/share/classes/java/awt/Font.java
! src/share/classes/java/awt/Graphics.java
! src/share/classes/java/awt/GridBagConstraints.java
! src/share/classes/java/awt/GridBagLayout.java
! src/share/classes/java/awt/GridLayout.java
! src/share/classes/java/awt/Label.java
! src/share/classes/java/awt/LinearGradientPaint.java
! src/share/classes/java/awt/LinearGradientPaintContext.java
! src/share/classes/java/awt/List.java
! src/share/classes/java/awt/MenuItem.java
! src/share/classes/java/awt/RadialGradientPaint.java
! src/share/classes/java/awt/Scrollbar.java
! src/share/classes/java/awt/SystemColor.java
! src/share/classes/java/awt/SystemTray.java
! src/share/classes/java/awt/TextArea.java
! src/share/classes/java/awt/TextField.java
! src/share/classes/java/awt/Toolkit.java
! src/share/classes/java/awt/WaitDispatchSupport.java
! src/share/classes/java/awt/Window.java
! src/share/classes/java/awt/color/CMMException.java
! src/share/classes/java/awt/color/ColorSpace.java
! src/share/classes/java/awt/color/ICC_ColorSpace.java
! src/share/classes/java/awt/color/ICC_Profile.java
! src/share/classes/java/awt/color/ICC_ProfileGray.java
! src/share/classes/java/awt/color/ICC_ProfileRGB.java
! src/share/classes/java/awt/dnd/DnDEventMulticaster.java
! src/share/classes/java/awt/dnd/DragSourceDropEvent.java
! src/share/classes/java/awt/dnd/DropTarget.java
! src/share/classes/java/awt/event/MouseAdapter.java
! src/share/classes/java/awt/font/FontRenderContext.java
! src/share/classes/java/awt/font/StyledParagraph.java
! src/share/classes/java/awt/geom/AffineTransform.java
! src/share/classes/java/awt/geom/QuadCurve2D.java
! src/share/classes/java/awt/image/BufferStrategy.java
! src/share/classes/java/awt/image/BufferedImage.java
! src/share/classes/java/awt/image/ColorConvertOp.java
! src/share/classes/java/awt/peer/CheckboxPeer.java
! src/share/classes/java/awt/peer/DesktopPeer.java

hg: jdk8/tl/jdk: 8029551: Add value-type notice to java.time classes

[roger . riggs]

Changeset: fe3383582427
Author:rriggs
Date:  2013-12-11 16:52 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/fe3383582427

8029551: Add value-type notice to java.time classes
Summary: Add warning about identity of value types and reference to 
ValueBased.html
Reviewed-by: briangoetz, smarks, scolebourne

! src/share/classes/java/time/Duration.java
! src/share/classes/java/time/Instant.java
! src/share/classes/java/time/LocalDate.java
! src/share/classes/java/time/LocalDateTime.java
! src/share/classes/java/time/LocalTime.java
! src/share/classes/java/time/MonthDay.java
! src/share/classes/java/time/OffsetDateTime.java
! src/share/classes/java/time/OffsetTime.java
! src/share/classes/java/time/Period.java
! src/share/classes/java/time/Year.java
! src/share/classes/java/time/YearMonth.java
! src/share/classes/java/time/ZoneId.java
! src/share/classes/java/time/ZoneOffset.java
! src/share/classes/java/time/ZonedDateTime.java
! src/share/classes/java/time/chrono/HijrahDate.java
! src/share/classes/java/time/chrono/JapaneseDate.java
! src/share/classes/java/time/chrono/MinguoDate.java
! src/share/classes/java/time/chrono/ThaiBuddhistDate.java
! test/java/time/tck/java/time/TCKLocalDateTime.java

hg: jdk8/tl/jdk: 2 new changesets

[roger . riggs]

Changeset: 7186275e6ef1
Author:rriggs
Date:  2013-12-20 13:06 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7186275e6ef1

8030002: Enhance deserialization using readObject
Reviewed-by: sherman, chegar, scolebourne

! src/share/classes/java/time/Duration.java
! src/share/classes/java/time/Instant.java
! src/share/classes/java/time/LocalDate.java
! src/share/classes/java/time/LocalDateTime.java
! src/share/classes/java/time/LocalTime.java
! src/share/classes/java/time/MonthDay.java
! src/share/classes/java/time/OffsetDateTime.java
! src/share/classes/java/time/OffsetTime.java
! src/share/classes/java/time/Period.java
! src/share/classes/java/time/Year.java
! src/share/classes/java/time/YearMonth.java
! src/share/classes/java/time/ZoneId.java
! src/share/classes/java/time/ZoneOffset.java
! src/share/classes/java/time/ZoneRegion.java
! src/share/classes/java/time/ZonedDateTime.java
! src/share/classes/java/time/chrono/AbstractChronology.java
! src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
! src/share/classes/java/time/chrono/ChronoPeriodImpl.java
! src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java
! src/share/classes/java/time/chrono/HijrahChronology.java
! src/share/classes/java/time/chrono/HijrahDate.java
! src/share/classes/java/time/chrono/IsoChronology.java
! src/share/classes/java/time/chrono/JapaneseChronology.java
! src/share/classes/java/time/chrono/JapaneseDate.java
! src/share/classes/java/time/chrono/JapaneseEra.java
! src/share/classes/java/time/chrono/MinguoChronology.java
! src/share/classes/java/time/chrono/MinguoDate.java
! src/share/classes/java/time/chrono/ThaiBuddhistChronology.java
! src/share/classes/java/time/chrono/ThaiBuddhistDate.java
! src/share/classes/java/time/temporal/ValueRange.java
! src/share/classes/java/time/temporal/WeekFields.java
! src/share/classes/java/time/zone/ZoneOffsetTransition.java
! src/share/classes/java/time/zone/ZoneOffsetTransitionRule.java
! src/share/classes/java/time/zone/ZoneRules.java
! test/java/time/tck/java/time/AbstractTCKTest.java
! 
test/java/time/tck/java/time/chrono/serial/TCKChronoLocalDateSerialization.java
! test/java/time/tck/java/time/chrono/serial/TCKChronologySerialization.java
! test/java/time/tck/java/time/serial/TCKDurationSerialization.java
! test/java/time/tck/java/time/serial/TCKInstantSerialization.java
! test/java/time/tck/java/time/serial/TCKLocalDateSerialization.java
! test/java/time/tck/java/time/serial/TCKLocalDateTimeSerialization.java
! test/java/time/tck/java/time/serial/TCKLocalTimeSerialization.java
! test/java/time/tck/java/time/serial/TCKMonthDaySerialization.java
! test/java/time/tck/java/time/serial/TCKOffsetDateTimeSerialization.java
! test/java/time/tck/java/time/serial/TCKOffsetTimeSerialization.java
! test/java/time/tck/java/time/serial/TCKPeriodSerialization.java
! test/java/time/tck/java/time/serial/TCKYearMonthSerialization.java
! test/java/time/tck/java/time/serial/TCKYearSerialization.java
! test/java/time/tck/java/time/serial/TCKZoneOffsetSerialization.java
! test/java/time/tck/java/time/serial/TCKZonedDateTimeSerialization.java
! test/java/time/tck/java/time/temporal/serial/TCKValueRangeSerialization.java
! test/java/time/tck/java/time/temporal/serial/TCKWeekFieldsSerialization.java
! 
test/java/time/tck/java/time/zone/serial/TCKZoneOffsetTransitionRuleSerialization.java
! 
test/java/time/tck/java/time/zone/serial/TCKZoneOffsetTransitionSerialization.java
! test/java/time/tck/java/time/zone/serial/TCKZoneRulesSerialization.java

Changeset: 39a02b18b386
Author:rriggs
Date:  2013-12-20 13:06 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/39a02b18b386

8029909: Clarify equals/hashcode behavior for java.time types
Summary: Document the behavior of equals and hashcode in java.time.chrono date 
types
Reviewed-by: sherman, scolebourne

! src/share/classes/java/time/chrono/HijrahDate.java
! src/share/classes/java/time/chrono/JapaneseDate.java
! src/share/classes/java/time/chrono/MinguoDate.java
! src/share/classes/java/time/chrono/ThaiBuddhistDate.java

hg: jdk8/tl/jdk: 8031103: java.time.Duration has wrong Javadoc Comments in toDays() and toHours()

[roger . riggs]

Changeset: 1b503dd54b95
Author:rriggs
Date:  2014-01-07 11:50 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1b503dd54b95

8031103: java.time.Duration has wrong Javadoc Comments in toDays() and toHours()
Summary: Correct specification for Duration.toDays, toHours
Reviewed-by: lancea, alanb

! src/share/classes/java/time/Duration.java

Re: RFR 9: 8035889: jdk testlibrary - add printing of values of failed assertions

[roger riggs]

Hi Mandy,

I updated the webrev:
http://cr.openjdk.java.net/~rriggs/webrev-testlibrary-asserts-8035889/

Alan suggested copying serviceability-dev so they have a chance to 
review if desired.


I want to investigate if it is possible to use the TestNG Assert classes 
without

the TestNG execution framework.
It would be necessary to compile/run against TestNG.jar but it might not
need the entire mechanism.

Thanks, Roger

On 2/26/2014 10:17 PM, Mandy Chung wrote:

On 2/26/2014 7:09 PM, Roger Riggs wrote:

Hi Mandy,

Yes, it might be more productive to switch the tests to TestNG.
But it did provide support in cases where TestNG could not be used,
for example in a directory of existing tests that had custom reporting.

But I remember there is a problem with TestNG having a dependency for XML
which is not supported in Profile1 and a number of tests had to be 
disabled
in that configuration.  Will XML always be available.  Do we need to 
solve

or work around that problem with TestNG?



This is a good point.   When we want to test just the base module for 
example, how can we run TestNG tests?  We need to address that certainly.


My comment on TestNG is a question for new tests using this Asserts 
class.  Your patch is fine to go (after taking out @library tag if I 
got it correct).


Mandy


Thanks, Roger

On 2/26/14 9:01 PM, Mandy Chung wrote:

Hi Roger,

On 2/26/2014 12:34 PM, roger riggs wrote:
The testlibrary for the jdk should be printing the values in the 
failed

assertions to make debugging easier and quicker.

The webrev adds the printing of the failed assertions and added 
methods

for formatting and unconditional fail methods.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-testlibrary-asserts-8035889/



AssertsTest.java: line 28:  @library doesn't look like it's needed. 
There is no jdk/test/testlibrary directory and I think 
jdk.testlibrary.* are found as relative to $test.src.


Otherwise, the change looks okay.

Now that jtreg supports TestNG and I wonder if this class should 
retire some day (there are only about 10 existing tests using this 
class).  Are you writing new tests using this Asserts class?


Mandy


Bug:
   8035889: jdk testlibrary - add printing of values of failed 
assertions


Thanks, Roger

[1] https://bugs.openjdk.java.net/browse/JDK-8035889

RFR 80044461: Cleanup new Boolean and single character strings

[roger riggs]

A quick sanity check please for these cleanup changes to remove uses of
new Boolean and using chars instead of single character strings contributed
by  Otávio Gonçalves de Santana.

These changes update multiple classes in the jdk9-dev repo (except 
client packages).


webrev: http://cr.openjdk.java.net/~rriggs/webrev-8044461-cleanup/
Issue: https://bugs.openjdk.java.net/browse/JDK-8044461

Thanks, Roger