RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field

[Kazunori Ogata]

Hi,

May I have a review for "JDK-8227831: Avoid using volatile for write-once, 
read-many class field"?

In jdk.internal.reflect.ReflectionFactory, there is a private class field 
named "langReflectAccess", which is referenced every time when the library 
handles various reflective operations.  This field is initialized on the 
first access to the ReflectionFactory class.  This field is declared as 
volatile to avoid (or reduce) race condition between initialization and 
references to the field.

On the platforms with weak memory model (i.e, POWER and ARM), reading a 
volatile variable requires memory fence and incurs overhead.  So it is 
preferable to avoid use of volatile for such a write-once, read-many 
variable.

langReflectAccess can be modified only in setLangReflectAccess() method. 
So we can avoid using volatile by modifying setLangReflectAccess() to use 
a synchronized block to avoid race condition.  This change reduced elapsed 
time of a micro benchmark by 9%, which repeatedly invoke 
Class.getMethods().


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

Webrev: http://cr.openjdk.java.net/~ogatak/8227831/webrev/


Regards,
Ogata

Re: RFR (S): JDK-8222778: Packaging Tool (JEP 343) on Linux/AArch64

[Dmitry Chuyko]

Alexander, thanks for having a look,

On 7/17/19 12:30 AM, Alexander Matveev wrote:

Hi Dmitry,

http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp.frames.html 

Why code between lines 215 and 219 was disabled? Not sure what it 
tries to do, if it tries to guarantee NULL termination we should 
probably keep it or allocate buffer with extra null or read 
(sizeof(buffer)-1). I think EOF defined as -1.


gcc 5.4.0 on Linux reports an error:

sandbox/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp: 
In member function ‘bool PosixProcess::ReadOutput()’:


sandbox/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp:215:35: 
error: comparison is always false due to limited range of data type 
[-Werror=type-limits]

 if (buffer[count - 1] == EOF) {

Here buffer is char[] and read(int, void *, size_t) is used.

It won't be right to keep it commented, to me it looks like this block 
can be removed. If not, there should be some other fix like adding 
ifdefs for some platforms or using call different from read().


-Dmitry


Otherwise looks fine.

Thanks,
Alexander

On 7/16/2019 12:55 PM, Dmitry Chuyko wrote:

Hello,

Please review a small patch that mostly fixes jpackage test for Linux 
aarch64 and also arm,x86,power. It is prepared for 
'JDK-8200758-branch' branch of open 'sandbox' repo.


There are few parts:

1. LinuxPlatform.cpp and IniFile.cpp got small fixes for compiler 
warnings.


2. LinuxDebBundler.getArch() now maps only x86_64 to amd64, x86 is 
still mapped to i386, and other archs map to themselves.


3. In tests, new method getRpmArch() was added to 
linux/base/Base.java, it maps JVMs os.arch to default rpmbuild arch. 
Multiple tests were modified to use that method instead of "x86_64" 
in rpm file name. Some timeouts were increased.


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

webrev: http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/

testing: test/jdk/tools/jpackage jtreg tests pass on Ubuntu 16.04 
with rpmbuild on x86_64, aarch64, arm, x86 and power, except 
deb/MaintainerTest (fails everywhere similarly to x86_64 because of 
extra "Unknown" name in email).


I didin't cover s390 as we in BellSoft currently don't build on that 
arch. On typical armv7 hw increased or default timeouts are still too 
low, while they are fine for some relatively weak aarch64 machines. 
Deb tests run especially slow because of dpkg-deb itself. I used 
"force-unsafe-io" option in /etc/dpkg/dpkg.cfg, it does reduce 
packaging time but still not enough to have really fast tests.


-Dmitry

Re: RFR (S): JDK-8222778: Packaging Tool (JEP 343) on Linux/AArch64

[Alexander Matveev]

Hi Dmitry,

http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp.frames.html
Why code between lines 215 and 219 was disabled? Not sure what it tries 
to do, if it tries to guarantee NULL termination we should probably keep 
it or allocate buffer with extra null or read (sizeof(buffer)-1). I 
think EOF defined as -1.


Otherwise looks fine.

Thanks,
Alexander

On 7/16/2019 12:55 PM, Dmitry Chuyko wrote:

Hello,

Please review a small patch that mostly fixes jpackage test for Linux 
aarch64 and also arm,x86,power. It is prepared for 
'JDK-8200758-branch' branch of open 'sandbox' repo.


There are few parts:

1. LinuxPlatform.cpp and IniFile.cpp got small fixes for compiler 
warnings.


2. LinuxDebBundler.getArch() now maps only x86_64 to amd64, x86 is 
still mapped to i386, and other archs map to themselves.


3. In tests, new method getRpmArch() was added to 
linux/base/Base.java, it maps JVMs os.arch to default rpmbuild arch. 
Multiple tests were modified to use that method instead of "x86_64" in 
rpm file name. Some timeouts were increased.


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

webrev: http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/

testing: test/jdk/tools/jpackage jtreg tests pass on Ubuntu 16.04 with 
rpmbuild on x86_64, aarch64, arm, x86 and power, except 
deb/MaintainerTest (fails everywhere similarly to x86_64 because of 
extra "Unknown" name in email).


I didin't cover s390 as we in BellSoft currently don't build on that 
arch. On typical armv7 hw increased or default timeouts are still too 
low, while they are fine for some relatively weak aarch64 machines. 
Deb tests run especially slow because of dpkg-deb itself. I used 
"force-unsafe-io" option in /etc/dpkg/dpkg.cfg, it does reduce 
packaging time but still not enough to have really fast tests.


-Dmitry

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[mikhailo . seledtsov]

On 7/16/19 1:32 PM, Igor Ignatyev wrote:

Hi Misha,

I understand that it doesn't alter the host system. my concern is that we move problem of 
host-configuration into tests. let's say tomorrow a new container engine will require 
something from /usr/local/sbin, or /usr/local/Cellar/docker/bin on another OS, or, god 
forbid, C:\Program Files(x86)\podman\bin. it has nothing to do w/ tests, it's a question 
of configuring a host, as I said, should be handled at another level by 
"scripts" run (once) prior test execution.

OK, it makes sense now.

Thank you,

Misha



-- Igor


On Jul 16, 2019, at 1:23 PM, mikhailo.seledt...@oracle.com wrote:

Hi Igor,

In both cases the environment variable is set for the Docker/Podman 
container process, not the host system. This will not affect the host system in 
any way. The docker process has its own namespace for environment variables. 
Does this alleviate your concerns?


Thank you,

Misha

On 7/16/19 11:49 AM, Igor Ignatyev wrote:

Hi Severin,

I don't think that tests (or test libraries for that matter) should be responsible for 
setting correct PATH value, it should be a part of host configuration procedure (tests 
can/should check that all required bins are available though). in other words, I'd prefer 
if you remove 'env.put("PATH", ...)' lines from both DockerTestUtils and 
TestJFREvents. the rest looks good to me.

Thanks,
-- Igor


On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:

Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:

Hi Severin,

   The change looks good to me. Thank you for adding support for Podman
container technology.

Testing: I ran both HotSpot and JDK container tests with your patch;
tests executed on Oracle Linux 7.6 using default container engine (Docker):

 test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by Fedora
and RHEL 8, called podman[1]. It's mostly compatible with docker. It
looks like OpenJDK docker tests can be made podman compatible with a
few little tweaks. One "interesting" one is to not assert "Successfully
built" in the build output but only rely on the exit code, which seems
to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/

Adjustments I've done:
  * Don't assert "Successfully built" in image build output[2].
  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
to work which is in /usr/sbin on Fedora
  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
to be equal to the previous value. I've found those counters to be
slowly increasing, which made the tests unreliable.

Testing:

Running docker tests with docker as engine. Did the same with podman as
engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
passed (non-trivially).

Thoughts?

Thanks,
Severin

[1] https://podman.io/
[2] Image builds with podman look
 like ("COMMIT" over "Successfully built"):
STEP 1: FROM fedora:29
STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
--> Using cache 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED TestMetrics
STEP 5: COMMIT fedora-metrics-11
d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[Severin Gehwolf]

On Tue, 2019-07-16 at 14:11 -0700, Jonathan Gibbons wrote:
> Severin,
> 
> This might be a reasonable update to jtreg, to allow /usr/sbin on the path
> on Unix-like systems.  The intent of jtreg is to protect tests from random
> crufty stuff on the PATH, and /usr/sbin is not in that category.
> 
> I've created
> CODETOOLS-7902505: Consider allowing /usr/sbin on $PATH
> https://bugs.openjdk.java.net/browse/CODETOOLS-7902505
> 
> The short-term workaround is to use the jtreg command-line option
> 
>  -e:PATH
> 
> which should override the default settign for PATH and pass through
> whatever you have set for $PATH.

Thanks, Jon!

Cheers,
Severin

> -- Jon
> 
> On 07/16/2019 02:01 PM, Severin Gehwolf wrote:
> > Hi Igor,
> > 
> > I understand the concern and I guess I could remove it and locally
> > install a wrapper in /bin or /usr/bin for podman which adds
> > /usr/sbin
> > to the path. On the other hand...
> > 
> > This seems to be an issue of code being run through jtreg. Looking
> > at
> > the jtr files I see this:
> > 
> > --rerun:(21/1545)*--
> > cd /disk/openjdk/upstream-sources/openjdk-head/JTwork/scratch && \\
> > DISPLAY=:0 \\
> > HOME=/home/sgehwolf \\
> > LANG=en_US.UTF-8 \\
> > PATH=/bin:/usr/bin \\
> > XMODIFIERS=@im=ibus \\
> > [...]
> > 
> > So jtreg reduces the host's PATH to /bin:/usr/bin, which is
> > insufficient for the podman case. The tag-spec docs[1] for jtreg
> > mention for "shell" tests that it sets the PATH to the above
> > settings.
> > This affects Java tests as well it seems.
> > 
> > ProcessBuilder outside jtreg has a sensible PATH as set up by the
> > system, FWIW.
> > 
> > So while my system is properly set up, jtreg interferes and renders
> > this necessary. Any suggestions as to how to convince jtreg to use
> > the
> > host's PATH setting?
> > 
> > Thanks,
> > Severin
> > 
> > [1] http://openjdk.java.net/jtreg/tag-spec.html
> > 
> > On Tue, 2019-07-16 at 13:32 -0700, Igor Ignatyev wrote:
> > > Hi Misha,
> > > 
> > > I understand that it doesn't alter the host system. my concern is
> > > that we move problem of host-configuration into tests. let's say
> > > tomorrow a new container engine will require something from
> > > /usr/local/sbin, or /usr/local/Cellar/docker/bin on another OS,
> > > or,
> > > god forbid, C:\Program Files(x86)\podman\bin. it has nothing to
> > > do w/
> > > tests, it's a question of configuring a host, as I said, should
> > > be
> > > handled at another level by "scripts" run (once) prior test
> > > execution.
> > > 
> > > -- Igor
> > > 
> > > > On Jul 16, 2019, at 1:23 PM, mikhailo.seledt...@oracle.com
> > > > wrote:
> > > > 
> > > > Hi Igor,
> > > > 
> > > > In both cases the environment variable is set for the
> > > > Docker/Podman container process, not the host system. This will
> > > > not
> > > > affect the host system in any way. The docker process has its
> > > > own
> > > > namespace for environment variables. Does this alleviate your
> > > > concerns?
> > > > 
> > > > 
> > > > Thank you,
> > > > 
> > > > Misha
> > > > 
> > > > On 7/16/19 11:49 AM, Igor Ignatyev wrote:
> > > > > Hi Severin,
> > > > > 
> > > > > I don't think that tests (or test libraries for that matter)
> > > > > should be responsible for setting correct PATH value, it
> > > > > should
> > > > > be a part of host configuration procedure (tests can/should
> > > > > check
> > > > > that all required bins are available though). in other words,
> > > > > I'd
> > > > > prefer if you remove 'env.put("PATH", ...)' lines from both
> > > > > DockerTestUtils and TestJFREvents. the rest looks good to me.
> > > > > 
> > > > > Thanks,
> > > > > -- Igor
> > > > > 
> > > > > > On Jul 16, 2019, at 5:36 AM, Severin Gehwolf <
> > > > > > sgehw...@redhat.com> wrote:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > I believe I still need a *R*eviewer for this. Any takers?
> > > > > > 
> > > > > > Thanks,
> > > > > > Severin
> > > > > > 
> > > > > > On Fri, 2019-07-12 at 15:19 -0700,
> > > > > > mikhailo.seledt...@oracle.com wrote:
> > > > > > > Hi Severin,
> > > > > > > 
> > > > > > >The change looks good to me. Thank you for adding
> > > > > > > support
> > > > > > > for Podman
> > > > > > > container technology.
> > > > > > > 
> > > > > > > Testing: I ran both HotSpot and JDK container tests with
> > > > > > > your
> > > > > > > patch;
> > > > > > > tests executed on Oracle Linux 7.6 using default
> > > > > > > container
> > > > > > > engine (Docker):
> > > > > > > 
> > > > > > >  test/hotspot/jtreg/containers/   AND
> > > > > > > test/jdk/jdk/internal/platform/docker/
> > > > > > > 
> > > > > > > All PASS
> > > > > > > 
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > 
> > > > > > > Misha
> > > > > > > 
> > > > > > > 
> > > > > > > On 7/12/19 11:08 AM, Severin Gehwolf wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > There is an alternative container engine which is being
> > > > > > > > used by Fedora
> > > > > > > > and RHEL 8, called podman

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[Jonathan Gibbons]

Severin,

This might be a reasonable update to jtreg, to allow /usr/sbin on the path
on Unix-like systems.  The intent of jtreg is to protect tests from random
crufty stuff on the PATH, and /usr/sbin is not in that category.

I've created
CODETOOLS-7902505: Consider allowing /usr/sbin on $PATH
https://bugs.openjdk.java.net/browse/CODETOOLS-7902505

The short-term workaround is to use the jtreg command-line option

    -e:PATH

which should override the default settign for PATH and pass through
whatever you have set for $PATH.

-- Jon

On 07/16/2019 02:01 PM, Severin Gehwolf wrote:

Hi Igor,

I understand the concern and I guess I could remove it and locally
install a wrapper in /bin or /usr/bin for podman which adds /usr/sbin
to the path. On the other hand...

This seems to be an issue of code being run through jtreg. Looking at
the jtr files I see this:

--rerun:(21/1545)*--
cd /disk/openjdk/upstream-sources/openjdk-head/JTwork/scratch && \\
DISPLAY=:0 \\
HOME=/home/sgehwolf \\
LANG=en_US.UTF-8 \\
PATH=/bin:/usr/bin \\
XMODIFIERS=@im=ibus \\
[...]

So jtreg reduces the host's PATH to /bin:/usr/bin, which is
insufficient for the podman case. The tag-spec docs[1] for jtreg
mention for "shell" tests that it sets the PATH to the above settings.
This affects Java tests as well it seems.

ProcessBuilder outside jtreg has a sensible PATH as set up by the
system, FWIW.

So while my system is properly set up, jtreg interferes and renders
this necessary. Any suggestions as to how to convince jtreg to use the
host's PATH setting?

Thanks,
Severin

[1] http://openjdk.java.net/jtreg/tag-spec.html

On Tue, 2019-07-16 at 13:32 -0700, Igor Ignatyev wrote:

Hi Misha,

I understand that it doesn't alter the host system. my concern is
that we move problem of host-configuration into tests. let's say
tomorrow a new container engine will require something from
/usr/local/sbin, or /usr/local/Cellar/docker/bin on another OS, or,
god forbid, C:\Program Files(x86)\podman\bin. it has nothing to do w/
tests, it's a question of configuring a host, as I said, should be
handled at another level by "scripts" run (once) prior test
execution.

-- Igor


On Jul 16, 2019, at 1:23 PM, mikhailo.seledt...@oracle.com wrote:

Hi Igor,

In both cases the environment variable is set for the
Docker/Podman container process, not the host system. This will not
affect the host system in any way. The docker process has its own
namespace for environment variables. Does this alleviate your
concerns?


Thank you,

Misha

On 7/16/19 11:49 AM, Igor Ignatyev wrote:

Hi Severin,

I don't think that tests (or test libraries for that matter)
should be responsible for setting correct PATH value, it should
be a part of host configuration procedure (tests can/should check
that all required bins are available though). in other words, I'd
prefer if you remove 'env.put("PATH", ...)' lines from both
DockerTestUtils and TestJFREvents. the rest looks good to me.

Thanks,
-- Igor


On Jul 16, 2019, at 5:36 AM, Severin Gehwolf <
sgehw...@redhat.com> wrote:

Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700,
mikhailo.seledt...@oracle.com wrote:

Hi Severin,

   The change looks good to me. Thank you for adding support
for Podman
container technology.

Testing: I ran both HotSpot and JDK container tests with your
patch;
tests executed on Oracle Linux 7.6 using default container
engine (Docker):

 test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being
used by Fedora
and RHEL 8, called podman[1]. It's mostly compatible with
docker. It
looks like OpenJDK docker tests can be made podman
compatible with a
few little tweaks. One "interesting" one is to not assert
"Successfully
built" in the build output but only rely on the exit code,
which seems
to be OK for my testing. Interestingly the test would be
skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/

Adjustments I've done:
  * Don't assert "Successfully built" in image build
output[2].
  * Add /usr/sbin to PATH as the podman binary relies on
iptables for it
to work which is in /usr/sbin on Fedora
  * Allow for Metrics.getCpuSystemUsage() and
Metrics.getCpuUserUsage()
to be equal to the previous value. I've found those
counters to be
slowly increasing, which made the tests unreliable.

Testing:

Running docker tests with docker as engine. Did the same
with podman as
engine via -Djdk.test.docker.command=podman on Linux
x86_64. Both
passed (non-trivially).

Thoughts?

Thanks,
Severin

[1] https://podman.io/
[2] Image builds with podman look
 like ("COMMIT" over "Successfully built"):
STEP 1: FROM fedora:29
STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf
clean a

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[Severin Gehwolf]

Hi Igor,

I understand the concern and I guess I could remove it and locally
install a wrapper in /bin or /usr/bin for podman which adds /usr/sbin
to the path. On the other hand...

This seems to be an issue of code being run through jtreg. Looking at
the jtr files I see this:

--rerun:(21/1545)*--
cd /disk/openjdk/upstream-sources/openjdk-head/JTwork/scratch && \\
DISPLAY=:0 \\
HOME=/home/sgehwolf \\
LANG=en_US.UTF-8 \\
PATH=/bin:/usr/bin \\
XMODIFIERS=@im=ibus \\
[...]

So jtreg reduces the host's PATH to /bin:/usr/bin, which is
insufficient for the podman case. The tag-spec docs[1] for jtreg
mention for "shell" tests that it sets the PATH to the above settings.
This affects Java tests as well it seems.

ProcessBuilder outside jtreg has a sensible PATH as set up by the
system, FWIW.

So while my system is properly set up, jtreg interferes and renders
this necessary. Any suggestions as to how to convince jtreg to use the
host's PATH setting?

Thanks,
Severin

[1] http://openjdk.java.net/jtreg/tag-spec.html

On Tue, 2019-07-16 at 13:32 -0700, Igor Ignatyev wrote:
> Hi Misha,
> 
> I understand that it doesn't alter the host system. my concern is
> that we move problem of host-configuration into tests. let's say
> tomorrow a new container engine will require something from
> /usr/local/sbin, or /usr/local/Cellar/docker/bin on another OS, or,
> god forbid, C:\Program Files(x86)\podman\bin. it has nothing to do w/
> tests, it's a question of configuring a host, as I said, should be
> handled at another level by "scripts" run (once) prior test
> execution.
> 
> -- Igor
> 
> > On Jul 16, 2019, at 1:23 PM, mikhailo.seledt...@oracle.com wrote:
> > 
> > Hi Igor,
> > 
> >In both cases the environment variable is set for the
> > Docker/Podman container process, not the host system. This will not
> > affect the host system in any way. The docker process has its own
> > namespace for environment variables. Does this alleviate your
> > concerns?
> > 
> > 
> > Thank you,
> > 
> > Misha
> > 
> > On 7/16/19 11:49 AM, Igor Ignatyev wrote:
> > > Hi Severin,
> > > 
> > > I don't think that tests (or test libraries for that matter)
> > > should be responsible for setting correct PATH value, it should
> > > be a part of host configuration procedure (tests can/should check
> > > that all required bins are available though). in other words, I'd
> > > prefer if you remove 'env.put("PATH", ...)' lines from both
> > > DockerTestUtils and TestJFREvents. the rest looks good to me.
> > > 
> > > Thanks,
> > > -- Igor
> > > 
> > > > On Jul 16, 2019, at 5:36 AM, Severin Gehwolf <
> > > > sgehw...@redhat.com> wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > I believe I still need a *R*eviewer for this. Any takers?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > On Fri, 2019-07-12 at 15:19 -0700, 
> > > > mikhailo.seledt...@oracle.com wrote:
> > > > > Hi Severin,
> > > > > 
> > > > >   The change looks good to me. Thank you for adding support
> > > > > for Podman
> > > > > container technology.
> > > > > 
> > > > > Testing: I ran both HotSpot and JDK container tests with your
> > > > > patch;
> > > > > tests executed on Oracle Linux 7.6 using default container
> > > > > engine (Docker):
> > > > > 
> > > > > test/hotspot/jtreg/containers/   AND
> > > > > test/jdk/jdk/internal/platform/docker/
> > > > > 
> > > > > All PASS
> > > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > Misha
> > > > > 
> > > > > 
> > > > > On 7/12/19 11:08 AM, Severin Gehwolf wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > There is an alternative container engine which is being
> > > > > > used by Fedora
> > > > > > and RHEL 8, called podman[1]. It's mostly compatible with
> > > > > > docker. It
> > > > > > looks like OpenJDK docker tests can be made podman
> > > > > > compatible with a
> > > > > > few little tweaks. One "interesting" one is to not assert
> > > > > > "Successfully
> > > > > > built" in the build output but only rely on the exit code,
> > > > > > which seems
> > > > > > to be OK for my testing. Interestingly the test would be
> > > > > > skipped in
> > > > > > that case.
> > > > > > 
> > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
> > > > > > webrev: 
> > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/
> > > > > > 
> > > > > > Adjustments I've done:
> > > > > >  * Don't assert "Successfully built" in image build
> > > > > > output[2].
> > > > > >  * Add /usr/sbin to PATH as the podman binary relies on
> > > > > > iptables for it
> > > > > >to work which is in /usr/sbin on Fedora
> > > > > >  * Allow for Metrics.getCpuSystemUsage() and
> > > > > > Metrics.getCpuUserUsage()
> > > > > >to be equal to the previous value. I've found those
> > > > > > counters to be
> > > > > >slowly increasing, which made the tests unreliable.
> > > > > > 
> > > > > > Testing:
> > > > > > 
> > > > > > Running docker tests with docker as engine. Did the same
> > > > > > with podman 

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[Igor Ignatyev]

Hi Misha,

I understand that it doesn't alter the host system. my concern is that we move 
problem of host-configuration into tests. let's say tomorrow a new container 
engine will require something from /usr/local/sbin, or 
/usr/local/Cellar/docker/bin on another OS, or, god forbid, C:\Program 
Files(x86)\podman\bin. it has nothing to do w/ tests, it's a question of 
configuring a host, as I said, should be handled at another level by "scripts" 
run (once) prior test execution.

-- Igor

> On Jul 16, 2019, at 1:23 PM, mikhailo.seledt...@oracle.com wrote:
> 
> Hi Igor,
> 
>In both cases the environment variable is set for the Docker/Podman 
> container process, not the host system. This will not affect the host system 
> in any way. The docker process has its own namespace for environment 
> variables. Does this alleviate your concerns?
> 
> 
> Thank you,
> 
> Misha
> 
> On 7/16/19 11:49 AM, Igor Ignatyev wrote:
>> Hi Severin,
>> 
>> I don't think that tests (or test libraries for that matter) should be 
>> responsible for setting correct PATH value, it should be a part of host 
>> configuration procedure (tests can/should check that all required bins are 
>> available though). in other words, I'd prefer if you remove 'env.put("PATH", 
>> ...)' lines from both DockerTestUtils and TestJFREvents. the rest looks good 
>> to me.
>> 
>> Thanks,
>> -- Igor
>> 
>>> On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:
>>> 
>>> Hi,
>>> 
>>> I believe I still need a *R*eviewer for this. Any takers?
>>> 
>>> Thanks,
>>> Severin
>>> 
>>> On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:
 Hi Severin,
 
   The change looks good to me. Thank you for adding support for Podman
 container technology.
 
 Testing: I ran both HotSpot and JDK container tests with your patch;
 tests executed on Oracle Linux 7.6 using default container engine (Docker):
 
 test/hotspot/jtreg/containers/   AND
 test/jdk/jdk/internal/platform/docker/
 
 All PASS
 
 
 Thanks,
 
 Misha
 
 
 On 7/12/19 11:08 AM, Severin Gehwolf wrote:
> Hi,
> 
> There is an alternative container engine which is being used by Fedora
> and RHEL 8, called podman[1]. It's mostly compatible with docker. It
> looks like OpenJDK docker tests can be made podman compatible with a
> few little tweaks. One "interesting" one is to not assert "Successfully
> built" in the build output but only rely on the exit code, which seems
> to be OK for my testing. Interestingly the test would be skipped in
> that case.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
> webrev: 
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/
> 
> Adjustments I've done:
>  * Don't assert "Successfully built" in image build output[2].
>  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
>to work which is in /usr/sbin on Fedora
>  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
>to be equal to the previous value. I've found those counters to be
>slowly increasing, which made the tests unreliable.
> 
> Testing:
> 
> Running docker tests with docker as engine. Did the same with podman as
> engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
> passed (non-trivially).
> 
> Thoughts?
> 
> Thanks,
> Severin
> 
> [1] https://podman.io/
> [2] Image builds with podman look
> like ("COMMIT" over "Successfully built"):
> STEP 1: FROM fedora:29
> STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
> --> Using cache 
> 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
> STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
> 269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
> STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
> java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED 
> TestMetrics
> STEP 5: COMMIT fedora-metrics-11
> d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e
> 

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[mikhailo . seledtsov]

Hi Igor,

   In both cases the environment variable is set for the Docker/Podman 
container process, not the host system. This will not affect the host 
system in any way. The docker process has its own namespace for 
environment variables. Does this alleviate your concerns?



Thank you,

Misha

On 7/16/19 11:49 AM, Igor Ignatyev wrote:

Hi Severin,

I don't think that tests (or test libraries for that matter) should be responsible for 
setting correct PATH value, it should be a part of host configuration procedure (tests 
can/should check that all required bins are available though). in other words, I'd prefer 
if you remove 'env.put("PATH", ...)' lines from both DockerTestUtils and 
TestJFREvents. the rest looks good to me.

Thanks,
-- Igor


On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:

Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:

Hi Severin,

   The change looks good to me. Thank you for adding support for Podman
container technology.

Testing: I ran both HotSpot and JDK container tests with your patch;
tests executed on Oracle Linux 7.6 using default container engine (Docker):

 test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by Fedora
and RHEL 8, called podman[1]. It's mostly compatible with docker. It
looks like OpenJDK docker tests can be made podman compatible with a
few little tweaks. One "interesting" one is to not assert "Successfully
built" in the build output but only rely on the exit code, which seems
to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/

Adjustments I've done:
  * Don't assert "Successfully built" in image build output[2].
  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
to work which is in /usr/sbin on Fedora
  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
to be equal to the previous value. I've found those counters to be
slowly increasing, which made the tests unreliable.

Testing:

Running docker tests with docker as engine. Did the same with podman as
engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
passed (non-trivially).

Thoughts?

Thanks,
Severin

[1] https://podman.io/
[2] Image builds with podman look
 like ("COMMIT" over "Successfully built"):
STEP 1: FROM fedora:29
STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
--> Using cache 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED TestMetrics
STEP 5: COMMIT fedora-metrics-11
d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e

RFR (S): JDK-8222778: Packaging Tool (JEP 343) on Linux/AArch64

[Dmitry Chuyko]

Hello,

Please review a small patch that mostly fixes jpackage test for Linux 
aarch64 and also arm,x86,power. It is prepared for 'JDK-8200758-branch' 
branch of open 'sandbox' repo.


There are few parts:

1. LinuxPlatform.cpp and IniFile.cpp got small fixes for compiler warnings.

2. LinuxDebBundler.getArch() now maps only x86_64 to amd64, x86 is still 
mapped to i386, and other archs map to themselves.


3. In tests, new method getRpmArch() was added to linux/base/Base.java, 
it maps JVMs os.arch to default rpmbuild arch. Multiple tests were 
modified to use that method instead of "x86_64" in rpm file name. Some 
timeouts were increased.


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

webrev: http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/

testing: test/jdk/tools/jpackage jtreg tests pass on Ubuntu 16.04 with 
rpmbuild on x86_64, aarch64, arm, x86 and power, except 
deb/MaintainerTest (fails everywhere similarly to x86_64 because of 
extra "Unknown" name in email).


I didin't cover s390 as we in BellSoft currently don't build on that 
arch. On typical armv7 hw increased or default timeouts are still too 
low, while they are fine for some relatively weak aarch64 machines. Deb 
tests run especially slow because of dpkg-deb itself. I used 
"force-unsafe-io" option in /etc/dpkg/dpkg.cfg, it does reduce packaging 
time but still not enough to have really fast tests.


-Dmitry

Re: 8073213: javadoc of PushbackInputStream methods should specify NullPointerExceptions

[Lance Andersen]

Looks fine Brian
> On Jul 16, 2019, at 3:45 PM, Brian Burkhalter  
> wrote:
> 
> I changed … to {@code …} and @exception to @throws in [3]. The 
> delta versus the original version [1] is at [2].
> 
> Thanks,
> 
> Brian
> 
> [1] http://cr.openjdk.java.net/~bpb/8073213/webrev.00/
> [2] http://cr.openjdk.java.net/~bpb/8073213/webrev.00-01/
> [3] http://cr.openjdk.java.net/~bpb/8073213/webrev.01/
> 
>> On Jul 16, 2019, at 10:19 AM, Lance Andersen  
>> wrote:
>> 
>> +1
>>> On Jul 16, 2019, at 12:08 PM, Brian Burkhalter >> > wrote:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8073213 
>>>  
>>> >> >
>>> 
>>> Add javadoc of NPEs missing from two unread() methods; see below.
>>> 
>>> It would not hurt in this class to also change … to {@code …} 
>>> and @exception to @throws but that would clutter up the review.
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: 8073213: javadoc of PushbackInputStream methods should specify NullPointerExceptions

[Brian Burkhalter]

I changed … to {@code …} and @exception to @throws in [3]. The 
delta versus the original version [1] is at [2].

Thanks,

Brian

[1] http://cr.openjdk.java.net/~bpb/8073213/webrev.00/
[2] http://cr.openjdk.java.net/~bpb/8073213/webrev.00-01/
[3] http://cr.openjdk.java.net/~bpb/8073213/webrev.01/

> On Jul 16, 2019, at 10:19 AM, Lance Andersen  
> wrote:
> 
> +1
>> On Jul 16, 2019, at 12:08 PM, Brian Burkhalter > > wrote:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8073213 
>>  
>> > >
>> 
>> Add javadoc of NPEs missing from two unread() methods; see below.
>> 
>> It would not hurt in this class to also change … to {@code …} 
>> and @exception to @throws but that would clutter up the review.

Re: 8131664: Javadoc for PrintStream is now incorrect

[Lance Andersen]

I think this is OK Brian

> On Jul 16, 2019, at 3:16 PM, Brian Burkhalter  
> wrote:
> 
> An updated version which includes the suggested change below is at [1] with a 
> diff versus the reviewed version .00 at [2].
> 
> Thanks,
> 
> Brian
> 
> [1] http://cr.openjdk.java.net/~bpb/8131664/webrev.01/ 
> 
> [2] Version .01 vs. version .00
> 
> --- a/src/java.base/share/classes/java/io/PrintStream.java
> +++ b/src/java.base/share/classes/java/io/PrintStream.java
> @@ -141,7 +141,7 @@
>  *
>  * @param  outThe output stream to which values and objects will 
> be
>  *printed
> - * @param  autoFlush  Whether the output buffer will be flushed
> + * @param  autoFlush  A boolean; if true, the output buffer will be 
> flushed
>  *whenever a byte array is written, one of the
>  *{@code println} methods is invoked, or a newline
>  *character or byte ({@code '\n'}) is written
> @@ -158,7 +158,7 @@
>  *
>  * @param  outThe output stream to which values and objects will 
> be
>  *printed
> - * @param  autoFlush  Whether the output buffer will be flushed
> + * @param  autoFlush  A boolean; if true, the output buffer will be 
> flushed
>  *whenever a byte array is written, one of the
>  *{@code println} methods is invoked, or a newline
>  *character or byte ({@code '\n'}) is written
> @@ -185,7 +185,7 @@
>  *
>  * @param  outThe output stream to which values and objects will 
> be
>  *printed
> - * @param  autoFlush  Whether the output buffer will be flushed
> + * @param  autoFlush  A boolean; if true, the output buffer will be 
> flushed
>  *whenever a byte array is written, one of the
>  *{@code println} methods is invoked, or a newline
>  *character or byte ({@code '\n'}) is written
> 
> 
>> On Jul 15, 2019, at 1:55 PM, Brian Burkhalter  
>> wrote:
>> 
>>> The existing autoflush wording looks  like it could use some wordsmithing 
>>> at some point (as I know we do this a little differently in some of the 
>>> APIs )
>>> 
>>> - “A boolean”  not sure we need to state this as it is obvious from the 
>>> declaration.
>> 
>> It looks like here one could change "A boolean; if true,” to simply 
>> “Whether”.
>> 
>>> -  Some places in the JDK, we say  True if ; false otherwise.  I am not 
>>> sure how many APIs after the really early ones  start  with “if true…”
>>> 
>>> Anyways, something to consider for a future todo list I guess :-) 
>> 
>> Might be worth doing. I’ll after any other comments come in.
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR [14/java.xml] 8176447: javax.xml.validation.Validator validates incorrectly on uniqueness constraint

[Joe Wang]

Thanks Lance!

On 7/16/19 12:20 PM, Lance Andersen wrote:

Looks OK Joe

On Jul 16, 2019, at 12:34 PM, Joe Wang > wrote:


Please review a fix for the uniqueness constraint. The fix is at line 
403 where the variable for tracking matches is reset. Without the 
reset, subsequent matches would have been skipped, thus not catching 
duplicate values.


As the report's workaround indicated, this issue was fixed in Apache 
Xerces. This patch now brings the class up-to-date with the upstream 
source.


All tests passed.

JBS: https://bugs.openjdk.java.net/browse/JDK-8176447
webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8176447/webrev/

Thanks,
Joe




Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR [14/java.xml] 8176447: javax.xml.validation.Validator validates incorrectly on uniqueness constraint

[Lance Andersen]

Looks OK Joe

> On Jul 16, 2019, at 12:34 PM, Joe Wang  wrote:
> 
> Please review a fix for the uniqueness constraint. The fix is at line 403 
> where the variable for tracking matches is reset. Without the reset, 
> subsequent matches would have been skipped, thus not catching duplicate 
> values.
> 
> As the report's workaround indicated, this issue was fixed in Apache Xerces. 
> This patch now brings the class up-to-date with the upstream source.
> 
> All tests passed.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8176447
> webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8176447/webrev/
> 
> Thanks,
> Joe

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: 8131664: Javadoc for PrintStream is now incorrect

[Brian Burkhalter]

An updated version which includes the suggested change below is at [1] with a 
diff versus the reviewed version .00 at [2].

Thanks,

Brian

[1] http://cr.openjdk.java.net/~bpb/8131664/webrev.01/ 

[2] Version .01 vs. version .00

--- a/src/java.base/share/classes/java/io/PrintStream.java
+++ b/src/java.base/share/classes/java/io/PrintStream.java
@@ -141,7 +141,7 @@
  *
  * @param  outThe output stream to which values and objects will be
  *printed
- * @param  autoFlush  Whether the output buffer will be flushed
+ * @param  autoFlush  A boolean; if true, the output buffer will be flushed
  *whenever a byte array is written, one of the
  *{@code println} methods is invoked, or a newline
  *character or byte ({@code '\n'}) is written
@@ -158,7 +158,7 @@
  *
  * @param  outThe output stream to which values and objects will be
  *printed
- * @param  autoFlush  Whether the output buffer will be flushed
+ * @param  autoFlush  A boolean; if true, the output buffer will be flushed
  *whenever a byte array is written, one of the
  *{@code println} methods is invoked, or a newline
  *character or byte ({@code '\n'}) is written
@@ -185,7 +185,7 @@
  *
  * @param  outThe output stream to which values and objects will be
  *printed
- * @param  autoFlush  Whether the output buffer will be flushed
+ * @param  autoFlush  A boolean; if true, the output buffer will be flushed
  *whenever a byte array is written, one of the
  *{@code println} methods is invoked, or a newline
  *character or byte ({@code '\n'}) is written


> On Jul 15, 2019, at 1:55 PM, Brian Burkhalter  
> wrote:
> 
>> The existing autoflush wording looks  like it could use some wordsmithing at 
>> some point (as I know we do this a little differently in some of the APIs )
>> 
>> - “A boolean”  not sure we need to state this as it is obvious from the 
>> declaration.
> 
> It looks like here one could change "A boolean; if true,” to simply “Whether”.
> 
>> -  Some places in the JDK, we say  True if ; false otherwise.  I am not 
>> sure how many APIs after the really early ones  start  with “if true…”
>> 
>> Anyways, something to consider for a future todo list I guess :-) 
> 
> Might be worth doing. I’ll after any other comments come in.

DnsClient TCP socket timeout

[Milan Mimica]

Hello list

Have you ever considered the problem reported here:
https://stackoverflow.com/questions/14603553/dns-query-freezes-with-dnscontextfactory-in-java

A DNS query gets stuck easily if read timeout is not set on the
socket. The proposed solution is trivial.


--
Milan Mimica

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[Igor Ignatyev]

Hi Severin,

I don't think that tests (or test libraries for that matter) should be 
responsible for setting correct PATH value, it should be a part of host 
configuration procedure (tests can/should check that all required bins are 
available though). in other words, I'd prefer if you remove 'env.put("PATH", 
...)' lines from both DockerTestUtils and TestJFREvents. the rest looks good to 
me.

Thanks,
-- Igor

> On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:
> 
> Hi,
> 
> I believe I still need a *R*eviewer for this. Any takers?
> 
> Thanks,
> Severin
> 
> On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:
>> Hi Severin,
>> 
>>   The change looks good to me. Thank you for adding support for Podman 
>> container technology.
>> 
>> Testing: I ran both HotSpot and JDK container tests with your patch; 
>> tests executed on Oracle Linux 7.6 using default container engine (Docker):
>> 
>> test/hotspot/jtreg/containers/   AND 
>> test/jdk/jdk/internal/platform/docker/
>> 
>> All PASS
>> 
>> 
>> Thanks,
>> 
>> Misha
>> 
>> 
>> On 7/12/19 11:08 AM, Severin Gehwolf wrote:
>>> Hi,
>>> 
>>> There is an alternative container engine which is being used by Fedora
>>> and RHEL 8, called podman[1]. It's mostly compatible with docker. It
>>> looks like OpenJDK docker tests can be made podman compatible with a
>>> few little tweaks. One "interesting" one is to not assert "Successfully
>>> built" in the build output but only rely on the exit code, which seems
>>> to be OK for my testing. Interestingly the test would be skipped in
>>> that case.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
>>> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/
>>> 
>>> Adjustments I've done:
>>>  * Don't assert "Successfully built" in image build output[2].
>>>  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
>>>to work which is in /usr/sbin on Fedora
>>>  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
>>>to be equal to the previous value. I've found those counters to be
>>>slowly increasing, which made the tests unreliable.
>>> 
>>> Testing:
>>> 
>>> Running docker tests with docker as engine. Did the same with podman as
>>> engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
>>> passed (non-trivially).
>>> 
>>> Thoughts?
>>> 
>>> Thanks,
>>> Severin
>>> 
>>> [1] https://podman.io/
>>> [2] Image builds with podman look
>>> like ("COMMIT" over "Successfully built"):
>>> STEP 1: FROM fedora:29
>>> STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
>>> --> Using cache 
>>> 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
>>> STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
>>> 269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
>>> STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
>>> java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED 
>>> TestMetrics
>>> STEP 5: COMMIT fedora-metrics-11
>>> d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e
>>> 
> 

Re: 8073213: javadoc of PushbackInputStream methods should specify NullPointerExceptions

[Lance Andersen]

+1
> On Jul 16, 2019, at 12:08 PM, Brian Burkhalter  
> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8073213 
> 
> 
> Add javadoc of NPEs missing from two unread() methods; see below.
> 
> It would not hurt in this class to also change … to {@code …} 
> and @exception to @throws but that would clutter up the review.
> 
> Thanks,
> 
> Brian
> 
> @@ -213,14 +213,15 @@
>  * of the pushback buffer.  After this method returns, the next byte to be
>  * read will have the value b[off], the byte after that will
>  * have the value b[off+1], and so forth.
>  *
>  * @param b the byte array to push back.
>  * @param off the start offset of the data.
>  * @param len the number of bytes to push back.
> + * @exception NullPointerException If b is 
> null.
>  * @exception IOException If there is not enough room in the pushback
>  *buffer for the specified number of bytes,
>  *or this input stream has been closed by
>  *invoking its {@link #close()} method.
>  * @since 1.1
>  */
> public void unread(byte[] b, int off, int len) throws IOException {
> @@ -235,14 +236,15 @@
> /**
>  * Pushes back an array of bytes by copying it to the front of the
>  * pushback buffer.  After this method returns, the next byte to be read
>  * will have the value b[0], the byte after that will have 
> the
>  * value b[1], and so forth.
>  *
>  * @param b the byte array to push back
> + * @exception NullPointerException If b is 
> null.
>  * @exception IOException If there is not enough room in the pushback
>  *buffer for the specified number of bytes,
>  *or this input stream has been closed by
>  *invoking its {@link #close()} method.
>  * @since 1.1
>  */
> public void unread(byte[] b) throws IOException {

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: 8227587: Add internal privileged System.loadLibrary

[Mandy Chung]

On 7/16/19 6:48 AM, Claes Redestad wrote:

Hi,

refactored to use a BootLoader::loadLibrary API that is only visible
within the java.base module:

http://cr.openjdk.java.net/~redestad/8227587/open.03/



Looks good.

Nit:  in JavaLangAccess
   321 void loadLibrary(Class klass, String library);

s/klass/caller/


I've retained the bridge to ClassLoader::loadLibrary for performance,
but without any magic or privilege escalation involved. Moving sys_path
/ systemNativeLibraries out of ClassLoader seems quite complicated, so
I've not attempted that refactoring for this RFE.


Yes, it is non-trivial.  I will file a RFE for this possible clean up.

Mandy

RFR [14/java.xml] 8176447: javax.xml.validation.Validator validates incorrectly on uniqueness constraint

[Joe Wang]

Please review a fix for the uniqueness constraint. The fix is at line 
403 where the variable for tracking matches is reset. Without the reset, 
subsequent matches would have been skipped, thus not catching duplicate 
values.


As the report's workaround indicated, this issue was fixed in Apache 
Xerces. This patch now brings the class up-to-date with the upstream source.


All tests passed.

JBS: https://bugs.openjdk.java.net/browse/JDK-8176447
webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8176447/webrev/

Thanks,
Joe

8073213: javadoc of PushbackInputStream methods should specify NullPointerExceptions

[Brian Burkhalter]

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


Add javadoc of NPEs missing from two unread() methods; see below.

It would not hurt in this class to also change … to {@code …} and 
@exception to @throws but that would clutter up the review.

Thanks,

Brian

@@ -213,14 +213,15 @@
  * of the pushback buffer.  After this method returns, the next byte to be
  * read will have the value b[off], the byte after that will
  * have the value b[off+1], and so forth.
  *
  * @param b the byte array to push back.
  * @param off the start offset of the data.
  * @param len the number of bytes to push back.
+ * @exception NullPointerException If b is null.
  * @exception IOException If there is not enough room in the pushback
  *buffer for the specified number of bytes,
  *or this input stream has been closed by
  *invoking its {@link #close()} method.
  * @since 1.1
  */
 public void unread(byte[] b, int off, int len) throws IOException {
@@ -235,14 +236,15 @@
 /**
  * Pushes back an array of bytes by copying it to the front of the
  * pushback buffer.  After this method returns, the next byte to be read
  * will have the value b[0], the byte after that will have the
  * value b[1], and so forth.
  *
  * @param b the byte array to push back
+ * @exception NullPointerException If b is null.
  * @exception IOException If there is not enough room in the pushback
  *buffer for the specified number of bytes,
  *or this input stream has been closed by
  *invoking its {@link #close()} method.
  * @since 1.1
  */
 public void unread(byte[] b) throws IOException {

RFR : 8227737: avoid implicit-function-declaration on AIX

[Baesken, Matthias]

Hello, please review the following  AIX related change .
It fixes a number of  missing inclusions  leading to  
implicit-function-declaration  warnings when compiling  with the recent xlc16 
/xlclang .


At various places in the native C coding in jdk, we miss header inclusions on 
AIX.
This leads to warnings like :

/nightly/jdk/src/java.base/unix/native/libjava/childproc.c:99:5: warning: 
implicitly declaring library function 'snprintf'
with type 'int (char *, unsigned long, const char *, ...)' 
[-Wimplicit-function-declaration]
snprintf(aix_fd_dir, 32, "/proc/%d/fd", getpid());
^
/nightly/jdk/src/java.base/unix/native/libjava/childproc.c:99:5: note: include 
the header  or explicitly provide a declaration for 'snprintf'

or

/nightly/jdk/src/java.base/aix/native/libjli/java_md_aix.c:38:5: warning: 
implicitly declaring library function 'memset'
with type 'void *(void *, int, unsigned long)' [-Wimplicit-function-declaration]
memset((void *)info, 0, sizeof(Dl_info));

/nightly/jdk/src/java.base/aix/native/libjli/java_md_aix.c:38:5: note: include 
the header  or explicitly provide a declaration for 'memset'



Bug/webrev :

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

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


Thanks, Matthias

Re: RFR: 8227587: Add internal privileged System.loadLibrary

[Roger Riggs]

+1

On 7/16/19 10:52 AM, Chris Hegarty wrote:

On 16 Jul 2019, at 14:48, Claes Redestad  wrote:

Hi,

refactored to use a BootLoader::loadLibrary API that is only visible
within the java.base module:

http://cr.openjdk.java.net/~redestad/8227587/open.03/

I think that this is good Claes. Reviewed.

-Chris.

Re: RFR: 8227587: Add internal privileged System.loadLibrary

[Chris Hegarty]

> On 16 Jul 2019, at 14:48, Claes Redestad  wrote:
> 
> Hi,
> 
> refactored to use a BootLoader::loadLibrary API that is only visible
> within the java.base module:
> 
> http://cr.openjdk.java.net/~redestad/8227587/open.03/

I think that this is good Claes. Reviewed.

-Chris.

Re: RFR: 8227587: Add internal privileged System.loadLibrary

[Claes Redestad]

Hi,

refactored to use a BootLoader::loadLibrary API that is only visible
within the java.base module:

http://cr.openjdk.java.net/~redestad/8227587/open.03/

I've retained the bridge to ClassLoader::loadLibrary for performance,
but without any magic or privilege escalation involved. Moving sys_path
/ systemNativeLibraries out of ClassLoader seems quite complicated, so
I've not attempted that refactoring for this RFE.

/Claes

On 2019-07-12 20:03, Mandy Chung wrote:

Just to recap what we discussed offline:

We could introduce BootLoader::loadLibrary to load a system native
library for boot loader.  sys_path and systemNativeLibraries in
ClassLoader implementation are solely for boot loader and it seems
cleaner for BootLoader to handle loading of system native libraries
where it can be optimized for boot loader.  This would require
some refactoring.

One possible solution is to add a shared secret to simply call
package-private ClassLoader::loadLibrary(Class fromClass, String name)
method and have BootLoader::loadLibrary to check if SM is present
and wrap the call with doPriv; otherwise, just call the shared
secret loadLibrary method.

Then we can investigate in the future to refactor the native library
loading implementation to jdk.internal.loader.

what do you think?

Mandy

On 7/12/19 8:25 AM, Mandy Chung wrote:

Claes,

Thanks for exploring this.  I would like to see if we can avoid 
introducing

an internal @CS method (keep @CSM only for public APIs will help security
analysis).

There are other alternatives to improve the footprint performance.

One idea is java.base and java.desktop each has its own utility method
to load library.  No change is needed for java.net since only one 
loadLibrary

call.

Another idea is to add BootLoader.loadLibrary that skips the security
permission overhead and only for boot loader use.  It would require
refactoring.

I think java.base and java.desktop having its own utility method is
a simpler solution.

Mandy

On 7/12/19 7:55 AM, Claes Redestad wrote:

Hi,

I'm dropping the java.desktop changes, and Mandy has asked me to explore
options that does not add a new @CallerSensitive entry point, even to a
strongly encapsulated API like JavaLangAccess

Easiest of these is to explicitly provide the class context we're
calling from - with proper assertions. This is marginally more
performant due avoiding the Reflection.getCallerClass, but slightly less
convenient.

http://cr.openjdk.java.net/~redestad/8227587/open.01/

/Claes

On 2019-07-12 15:27, Roger Riggs wrote:

Hi Claes,

Looks fine.

Thanks for the updates, Roger


On 7/11/19 10:39 AM, Claes Redestad wrote:

Hi Roger,

On 2019-07-11 16:10, Roger Riggs wrote:

Hi Claes,

JavaLangAccess.java:
316: Add @param tag


done.



System.java:  2282, 2287
Runtime.loadLibrary0 makes a second check for a security manager.
Is there any potential gain by calling ClassLoader.loadLibrary 
directly?

None of the internal uses would have a separatorChar.


Most of the gain comes from not having to load one-off PA classes or
lambdas, but of course avoiding the indexOf and a few indirections are
marginally helpful to startup. We should at least assert that the
library names are sane, though.



I expect most of the files need a copyright update.
The script in /make/scripts/update_copyright_year.sh should 
do the work for modified files.


Fixed copyrights and updated in place: 
http://cr.openjdk.java.net/~redestad/8227587/open.00


Thanks!

/Claes

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[Severin Gehwolf]

Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:
> Hi Severin,
> 
>The change looks good to me. Thank you for adding support for Podman 
> container technology.
> 
> Testing: I ran both HotSpot and JDK container tests with your patch; 
> tests executed on Oracle Linux 7.6 using default container engine (Docker):
> 
>  test/hotspot/jtreg/containers/   AND 
> test/jdk/jdk/internal/platform/docker/
> 
> All PASS
> 
> 
> Thanks,
> 
> Misha
> 
> 
> On 7/12/19 11:08 AM, Severin Gehwolf wrote:
> > Hi,
> > 
> > There is an alternative container engine which is being used by Fedora
> > and RHEL 8, called podman[1]. It's mostly compatible with docker. It
> > looks like OpenJDK docker tests can be made podman compatible with a
> > few little tweaks. One "interesting" one is to not assert "Successfully
> > built" in the build output but only rely on the exit code, which seems
> > to be OK for my testing. Interestingly the test would be skipped in
> > that case.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/
> > 
> > Adjustments I've done:
> >   * Don't assert "Successfully built" in image build output[2].
> >   * Add /usr/sbin to PATH as the podman binary relies on iptables for it
> > to work which is in /usr/sbin on Fedora
> >   * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
> > to be equal to the previous value. I've found those counters to be
> > slowly increasing, which made the tests unreliable.
> > 
> > Testing:
> > 
> > Running docker tests with docker as engine. Did the same with podman as
> > engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
> > passed (non-trivially).
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 
> > [1] https://podman.io/
> > [2] Image builds with podman look
> >  like ("COMMIT" over "Successfully built"):
> > STEP 1: FROM fedora:29
> > STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
> > --> Using cache 
> > 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
> > STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
> > 269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
> > STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
> > java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED 
> > TestMetrics
> > STEP 5: COMMIT fedora-metrics-11
> > d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e
> >