Re: [12] RFR: 8211961: Broken link in java.util.Locale

[Mandy Chung]

+1

Mandy

On 10/12/18 2:38 PM, naoto.s...@oracle.com wrote:

Hi,

Please review this simple doc fix to the following issue:

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

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8211961/webrev.01/

Although @link is preferred in the javadoc, it is currently not 
working for @serialField (JDK-6344777). Instead, "java.base" module 
name is prepended in the href as a workaround.


Naoto

[12] RFR: 8211961: Broken link in java.util.Locale

[naoto . sato]

Hi,

Please review this simple doc fix to the following issue:

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

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8211961/webrev.01/

Although @link is preferred in the javadoc, it is currently not working 
for @serialField (JDK-6344777). Instead, "java.base" module name is 
prepended in the href as a workaround.


Naoto

RE: RFR (Enhancement): 6194856: Zip Files lose ALL ownership and permissions of the files

[Langer, Christoph]

Hi Sherman,

thanks for your input.

Next week I’ll work on updating my webrev to incorporate your points and I’ll 
also draft a CSR.

Best regards
Christoph

From: Xueming Shen 
Sent: Mittwoch, 10. Oktober 2018 09:17
To: Langer, Christoph 
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR (Enhancement): 6194856: Zip Files lose ALL ownership and 
permissions of the files

Hi Langer,

Here are some of my comments.

ZipEntry:
Optional> getPosixPermissions()
(1) Is "Optional" really necessary here. it's a little inconsistent 
compared to the rest of
methods in the class. a "null" return might be just fine?
(2) Needs a  to separate the first line of the spec from the rest
(3) The wording probably needs more consideration. For example, it might be 
more
 straightforward to use some similar wording from other methods, for 
example


... When read from a ZIP file, this is the posix permission stored in the 
{@code external

file attributes} field of the zip file entry's central directory record, if 
there is

one.

  Also, a @ImplNote might be the better place for "it's not available when 
read from ZIS"?


void setPosixPermissions( Set permissions)

I can see the possible use case of using "null" as a special value to reset 
the
permission field (and my proposal of simply returning a null from 
getPosixPermissions()
when there is no one, brings some symmetry here?)

ZipUtil:
(1) those POSIX_... constants don't need to be public?
(2) I like the "stream" style implementation of permsTo/FromFlags pair, but 
have some
 concerns regarding their cost. "stream" is relative expensive (when 
you take a look at
 those supporting class underneath), as these two are supposed to be 
invoked for every
 entry inside a zip file, they can be hundreds and thousands ... just 
wonder, given most
 of the entries likely to have the "same" permission set inside a 
"normal" zip/jar file, is it
 worth to put in some cache mechanism here, especially for the "get" 
method, is it designed
 to return a read-only set of permission?  (in which we can use some 
mechanism here).
 That said PosixFileAttributes.getPermissions() actually purposely 
returns a modifiable set
 of permissions. It might be worth some discussion here, as the 
ZipEntry.getPosixPermission()
 needs to specify it, if it's going to return a immutable set.

Test: These days it is discouraged to check in a binary zip file as a test 
target. It is preferred
 to create the testing zip file on the fly.

-Sherman



On 9/25/18, 7:57 AM, Langer, Christoph wrote:
Hi all,

I had asked for opinions regarding adding posix permission support to JDK’s zip 
handling libraries and tools [1]. Since I didn’t get clear “no, you can’t do 
this” answers, I’m now concretely proposing some enhancements in the area of 
java.util.zip, jdk.zipfs and jdk.jartool.
I have reopened the long standing bug report (6194856) which had been set to 
“Won’t fix” quite recently for this purpose.

Here are the technical details:
The ZIP format specifications by infozip and pkware ([2] and [3]) do not 
explicitly specify the handling of posix file permissions. But it seems to be 
common sense that if file attribute compatibility is set to “Unix” in the upper 
byte of CEN field “version made by”, the file permissions bits are stored in 
CEN field “external file attributes”, byte 3 and 4. My changes try to honor 
this in the least obtrusive way. 😊

The following changes are proposed:

java.util.zip.ZipEntry:
it will have an additional field “posixPerms”. A value of -1 means “no 
permission information”, positive values will contain the flag values.
There will be 2 new public methods to read/set the permission information:
public Optional> getPosixPermissions()
public void setPosixPermissions(Set 
permissions)
The usage of type “Optional” reflects that posix permissions are not 
necessarily present in a zip file
java.util.zip.ZipFile:
it will have the capability to read the CEN fields and set 
posixPerms if available
java.util.zip.ZipOutputStream:
it will store entries with associated posix permissions as unix 
type in the CEN, together with the bit mask for the permissions


jdk.jartool:
I propose to add and option "--preserve-posix" or short "-o" to honor the posix 
bits that may be stored inside zip/jar files. By default the option is not set 
and hence posix permissions are ignored. If the flag is set and the file system 
that the jar tool is running on supports posix, posix file permissions that 
exist in the file system will be stored in newly created/update archives or 
restored to the file system if such information is present in the archive.


jdk.zipfs:
I added the capability for posix file permissions in the 
implementation. I decided to support PosixFileAttributes by subclassing 
ZipFileAttr

Re: RFR(JDK 12/NIO) 8202285: (fs) Add a method to Files for comparing file contents

[Joe Wang]

Hi all,

Here's an update based on all of the great reviews and comments (thanks 
all!):


JBS: https://bugs.openjdk.java.net/browse/JDK-8202285
CSR: https://bugs.openjdk.java.net/browse/JDK-8202302

Current version:
specdiff: 
http://cr.openjdk.java.net/~joehw/jdk12/8202285/specdiff_02/java/nio/file/Files.html

webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v02/

Previous version:
specdiff: 
http://cr.openjdk.java.net/~joehw/jdk12/8202285/specdiff_v01/java/nio/file/Files.html

webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v01/

It's been a while, so here's a summary of what's changed:

Spec: Alan's patch to fix inconsistencies in wording/terminology
Impl: s/mismatchByAttrs and etc/isSameFile;
 Stick with the simple implementation (InputStream/Buffer) for 
now. Further improvement may be done if there's a need;
 The impl is smaller than the previous version, merged the code 
into Files, removed FilesMismatch.java;


Test: more readable variables instead of the array of test files
 more comments on the testcases
 improved the private methods used for generating the test files

Thanks,
Joe

On 9/21/18, 6:49 PM, Joe Wang wrote:


On 9/20/18, 2:46 PM, Stuart Marks wrote:



On 9/19/18 11:48 AM, Joe Wang wrote:
After much discussion and 10 iterations of reviews, this proposal 
has evolved from what was the original isSameContent method to a 
mismatch method. API-wise, a compare method was also considered as 
it looked like just a short step forward from mismatch, however, it 
was eventually dropped since there is no convincing use case 
comparing files lexicographically by contents. Impl-wise, extensive 
performance benchmarking has been done to compare a buffered reading 
vs memory mapping, the result was that a simple buffered reading 
performed better among small files, and those with the mismatched 
byte closer to the beginning of files. Since the proposed method's 
targeted files are small ones, the impl currently does a buffered 
reading only.


Hi Joe,

Thanks for being persistent with this one!


Thanks for the help, and the "lot of stuff" with clear indicators 
(e.g. line numbers and etc.)!  From JDK 11 to 12, hopefully wont be 
deferred to 13 :-)


A very small spec nitpick:

specdiff: 
http://cr.openjdk.java.net/~joehw/jdk12/8202285/specdiff/java/nio/file/Files.html


1544  *  {@link #isSameFile(Path, Path) isSameFile(path, 
path2)} returns true,


I would add "or" after the trailing comma. This makes it similar to 
the two-item list that follows.


Done.



webrev: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev/


A couple minor comments on FilesMismatch.java:

If mismatchByAttrs() is replaced with a call to isSameFile(), as Alan 
suggested, this simplifies things considerably. It looks like the 
mismatch() implementation will be reduced to around ~40 lines of 
code. Does it deserve its own file, or can it be placed directly into 
Files.java? That file has over 4,000 lines already though.


I merged the private methods after removing mismatchByAttrs(), that 
reduced the code to 33 lines, and then with the change you suggested 
below, it's further reduced to 29 lines. I'll put them in Files for now.


Files is too big,  ~180K, that compares with the next one ~45K among 
files in the nio/file package. Unfortunately, the great majority of 
them are specs, probably 90%.


 106 if (nRead1 == 0 || nRead2 == 0) {
 107 if (nRead1 == 0 && nRead2 == 0) {
 108 // both files reach EOF
 109 return -1;
 110 } else {
 111 // one of the files reaches EOF
 112 return totalRead;
 113 }
 114 } else if (nRead1 != nRead2) {

I think it reads slightly better if the nested 'if' at lines 107-113 
were flattened into the else-if chain, like so:


if (nRead1 == 0 && nRead2 == 0) {
// both files reached EOF
return -1;
} else if (nRead1 == 0 || nRead2 == 0) {
// one but not both files reached EOF
return totalRead;
} else if (nRead1 != nRead2) {

There are a couple places where lines can be joined. The resulting 
lines are longer than 80 characters but are still less than 100 
characters, which I think is OK. (I don't think we have a strict 
limit of 80 anymore.)


  97 private static long mismatch(InputStream in1, InputStream in2)
  98 throws IOException {

 117 return totalRead +
 118 Arrays.mismatch(buffer1, 0, len, buffer2, 0, 
len);


Done. 95 characters :-)



Comments on tests in Comparison.java:

* This file, and several names within this file, should probably 
should be renamed to focus on mismatch instead of comparison.


Will rename to "mismatch". Previously, it covers both mismatch and 
compare.


* I'm finding it quite difficult to read the t

Review Request: 6443578: Continuation lines in JAR manifests do not follow RFC-822

[Philipp Kunz]

Hi,

Attached is a patch with a proposal to fix bug 6443578 about Manifest
inserting line breaks in between bytes of characters encoded in utf-8 with more 
than one byte and a test for it.


The Issue

The current Manifest implementation places line breaks for breaking and
continuing values onto the next line always at 72 bytes from the start
of the line without paying attention to characters encoded in utf-8
with more than one byte and to all bytes of one character together. It
occurs that a character encoded in utf-8 with more than one byte is
partially written at the end of one line and then after a line break
for a continuation line ("\r\n ") continued on the next line resulting
in an invalid utf-8 byte sequence where "\r\n " is inserted in between
the multiple bytes of the byte sequence a character is encoded in utf-8 
with.

That cannot happen with characters encoded with one byte in utf-8 which
are used most often. It only affects characters with more than one byte
encoded in utf-8. It also does not affect header names (the keys)
because header names are limited to {A-Z} | {a-z} | {0-9} | - | _ [1]
which all happen to be characters encoded in utf-8 with one byte.

The Manifest implementation removes these continuation line delimiters
("\r\n ") again in [2] and [3] (mind the --) and [4] (mind the 1 as
second parameter to arraycopy skipping the space) and concatenates the
parts from individual continuation lines together in [5] operating
still on a sequence of bytes before decoding the remaining bytes of the
value back into the string value applying the utf-8 decoding in [6].
The process is very similar also for names of named sections, see [7],
[8], [9], and [10]. Manifests written with the current implementation
are read again correctly.

Not only the specification says that sequences of bytes of the same
utf-8 encoded character cannot be broken apart [1]
> value: SPACE *otherchar newline *continuation
> continuation: SPACE *otherchar newline
> otherchar: any UTF-8 character except NUL, CR and LF
but line break and space can occur only between whole utf-8 encoded
characters and also jar manifests cannot be viewed with utf-8 aware
file viewers correctly. Manifests with values with characters illegally
broken onto a new line look for example simplified and with a shortened
line width limit similar like this one in any of my favorite file viewers:

> Manifest-Version: 1.0
> Key: Gänsefüßchen in Übergr?
> ?ße

where ? and ? should in fact be a German o umlaut ö. A file viewer may
understand and decode utf-8 well but has no way to know that the two
bytes, one before and one after the continuation line break actually
constitute the utf-8 encoded byte sequence of one single character.


Don't Put Continuation Line Breaks in Front of UTF-8 Continuation Bytes

The approach chosen in my patch is to prevent putting continuation line
breaks before a utf-8 continuation byte. UTF-* continuation bytes can
be detected with a bit mask and comparison, see [11]. An alternative
would have been to write one character (unicode codepoint and not Java
16-bit char) after another computing the length in number of bytes in
utf-8 encoded form for each and checking if it still fits on the same
line before writing it with or without an addition continuation line
break. When working with strings in Java, an additional challenge would
have been to work with surrogate pairs. I figure the bitmask comparison
fits the purpose here best, it is also used elsewhere in the jdk, see
[12] and [13] among other places, and presumably involves the least
resource usage and performance penalty, unfortunately none of which
accessible (private).


Tricky Strings of Actual Bytes

The current Manifest implementation does some kind of strange
processing when writing and encoding manifests with respect to using
strings with bytes as already described and mentioned in [14] and [15]
among probably more. The values are converted into strings with the
invocation of the deprecated constructor [17] in [16]:
> value = new String(vb, 0, 0, vb.length);
which converts the utf-8 encoded bytes into a string of chars each of
which is set to a byte value in its lower byte and zero in the higher
byte [18]. Each char of that string actually holds a byte and its
length corresponds to the number of bytes of the utf-8 encoded value.
After inserting line breaks in make72Safe [19] the string is written to
the output stream disregarding the higher bytes of the chars by
applying a lossy conversion in [20] thereby restoring the original utf-
8 byte sequence with line breaks added which also don't use higher
bytes.

I'll be glad to also solve bug 8066619 by replacing these weird strings
with byte arrays or anyhow else but prefer to do that in a separate
effort and patch and for this patch here the strings are encoded as
they always were. Above paragraph explains why my patch's
isUTF8ContinuationByte takes a parameter of type char which looks weird
at first glance but after st

Re: JDK 12 RFR of JDK-8058202 : AnnotatedType implementations don't override toString(), equals(), hashCode()

[Werner Dietl]

Hi Joe, all,

the logic looks good to me.

In the tests I'm wondering whether to include an annotated wildcard
bound. There is:

307 public @AnnotType(11) Set<@AnnotType(13) ? extends Number>
fooNumberSet2() {return null;}

but nothing like

Set fooNumberSet2() {return null;}

I wouldn't expect problems for this, but a test would exercise some of
the code that gets added.

Best,
cu, WMD.




On Wed, Oct 10, 2018 at 2:40 AM Joel Borggrén-Franck
 wrote:
>
> Hej Joe,
>
> New version looks good!
>
> Thanks for the explanations, makes sense to me.
>
> Cheers
> /Joel
>
> On Wed, 10 Oct 2018 at 08:27, joe darcy  wrote:
>>
>> Hi Joel,
>>
>> Thanks for the review; slightly revised version at
>>
>>  http://cr.openjdk.java.net/~darcy/8058202.3/
>>
>> Comments below.
>>
>>
>> On 10/9/2018 11:00 AM, Joel Borggrén-Franck wrote:
>> > Hi Joe,
>> >
>> > Good to see this happening. In general looks good to me, a few nits below.
>> >
>> > AnnotatedTypeBaseImpl contains comments indicating from which
>> > superclass or interface the overridden methods comes. I'd either add
>> > // Object at line 207 or delete line 145 and 177 for consistency.
>>
>> Okay; comments added to follow that local convention.
>>
>> >
>> > Even though this isn't performance critical by far the allocation at
>> > line 215 still bugs me a bit given that the common case will most
>> > certainly be no annotations.
>>
>> Sure; refactored to avoid the allocation.
>>
>> >
>> > Why the inverted logic line 250-253, IIRC you should be able to test
>> > if it is an AnnotatedBaseTypeImpl, or?
>>
>> I was aiming to avoid testing for the impl class directly to not
>> directly tie the classes to this particular implementation of the
>> AnnotatedType hierarchy. However, inter-operability based on the specs
>> would need the equals and hashCode behavior to be defined.
>>
>> >
>> > For equalsTypeAndAnnotations and baseHashCode lines 232-244 equals
>> > test for owner type equality while hashcode doesn't include owner
>> > type. I must confess I no longer remember the equals-hashCode contract
>> > but I assume this is intentional.
>>
>> Good catch; equals and hashCode checks made consistent.
>>
>> Some refactoring and hardening of the test included too.
>>
>> Delta-diffs below.
>>
>> Thanks,
>>
>> -Joe
>>
>> diff
>> webrev.2/raw_files/new/src/java.base/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java
>> webrev/raw_files/new/src/java.base/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java
>>
>> 2c2
>> <  * Copyright (c) 2013, 2015, Oracle and/or its affiliates. All rights
>> reserved.
>> ---
>>  >  * Copyright (c) 2013, 2018, Oracle and/or its affiliates. All rights
>> reserved.
>> 207c207
>> < @Override
>> ---
>>  > @Override // java.lang.Object
>> 215d214
>> < StringJoiner sj = new StringJoiner(" ");
>> 216a216
>>  > StringJoiner sj = new StringJoiner(" ");
>> 228,229c228,231
>> < }
>> < return sj.toString();
>> ---
>>  > return sj.toString();
>>  > } else {
>>  > return "";
>>  > }
>> 244c246,247
>> < Objects.hash((Object[])getAnnotations());
>> ---
>>  > Objects.hash((Object[])getAnnotations()) ^
>>  > Objects.hash(getAnnotatedOwnerType());
>>
>> diff
>> webrev.2/raw_files/new/test/jdk/java/lang/annotation/typeAnnotations/TestObjectMethods.java
>> webrev/raw_files/new/test/jdk/java/lang/annotation/typeAnnotations/TestObjectMethods.java
>>
>> 142d141
>> <
>> 157,168c156,158
>> < AnnotatedType annotType1 = m.getAnnotatedReturnType();
>> < AnnotatedType annotType2 = m.getAnnotatedReturnType();
>> <
>> < boolean valid = annotType1.equals(annotType2);
>> <
>> < if (!valid) {
>> < errors++;
>> < System.err.println(annotType1);
>> < System.err.println(" is not equal to ");
>> < System.err.println(annotType2);
>> < System.err.println();
>> < }
>> ---
>>  > checkTypesForEquality(m.getAnnotatedReturnType(),
>>  >   m.getAnnotatedReturnType(),
>>  >   true);
>> 171a162,185
>>  > private static void checkTypesForEquality(AnnotatedType annotType1,
>>  >   AnnotatedType annotType2,
>>  >   boolean expected) {
>>  > boolean comparison = annotType1.equals(annotType2);
>>  >
>>  > if (comparison) {
>>  > int hash1 = annotType1.hashCode();
>>  > int hash2 = annotType1.hashCode();
>>  > if (hash1 != hash2) {
>>  > errors++;
>>  > System.err.format("Equal AnnotatedTypes with unequal hash
>> codes: %n%s%n%s%n",
>>  >   annotType1.toString(), annotType2.toString());
>>  > }
>>  > }
>>  >
>>  > if (comparison != expected) {
>>  > errors++;
>>  > System.err.println(annotType1);
>>  > System

Re: CODETOOLS-7902290 breaks all JTreg tests which use @requires vm.opt.*

[Jonathan Gibbons]

On 10/12/18 8:15 AM, Volker Simonis wrote:

On Thu, Oct 11, 2018 at 11:09 PM Igor Ignatyev  wrote:

Hi Volker,

vm.opt.* options are set by jtreg (RegressionContext::processVMOptions), and their value 
is expected to be "null" if a flag hasn't been passed to JDK under tests via 
-javaoptions or -vmoptions. I don't think there is something to fix (at least not in 
jtreg).


Well, if "vm.opt.*" options are indeed only options which are taken
from the command line (i.e. "-javaoption/-vmoption"), than there's
definitely something we have to fix! Because, as I described before,
after CODETOOLS-7902290, all test which check such an option in a
@requires clause (negatively or positively) will now fail if that
option is not given explicitly on the command line.

If "vm.opt.*" options are really only the options given on the command
line (please confirm this and please document it somewhere
prominently!) I think many tests which use them are broken because
they simply check such options like this:

@requires vm.opt.TieredCompilation == true

This would be wrong and would have to be replaced by:

@requires vm.opt.TieredCompilation == null | vm.opt.TieredCompilation == true

to also account for the case where -XX:TieredCompilation was not
explicitly set on the command line. Notice that in in such a case, the
value of "TieredCompilation" in the VM could be either "true" or
"false" (depending on the platform and VM build configuration), but it
wouldn't be possible to test that by using "vm.opt.TieredCompilation".

That said, there are three "vm.opt.final.*" options which are set by VMProps:

 vmOptFinalFlag(map, "ClassUnloading");
 vmOptFinalFlag(map, "UseCompressedOops");
 vmOptFinalFlag(map, "EnableJVMCI");

Notice that these options are set to the values of the corresponding
flags in the agent VM . For tests which run in "othervm" mode (or
which start new VMs with ProcessBuilder or
jdk.test.lib.process.ProcessTools), that doesn't necessarily
corresponds to the values of these flags in the testee VM.

Finally, options specified with "-javaoption/-vmoption" and reflected
in the corresponding "vm.opt.*" flags, may be over-ridden by command
line options specified for tests which run in "othervm" mode (so
checking them with a @requires clause is of limited usefulness as
well).

All this is quite complex and I think we should*really*  document it
in a prominent place like the "jtreg: Command Line Options" page [1]
AND the
"The JDK Test Framework: Tag Language Specification" page [2].

Regards,
Volker

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



Volker,

I will look  at what can be done to improve the documentation,
especially with regard to documenting the limitations of the
@requires mechanism.

That being said, most of the mechanism is provided outside
of jtreg, by the extraPropDefns  extension mechanism specified
in TEST.ROOT.  It was a deliberate design choice to decouple
these classes from jtreg, so that they can evolve faster and
separately from jtreg promotions, according to the needs of
the test suite. So, without diminishing the need for additional
documentation, I'll note that detailed documentation may not
belong in the jtreg pages you specified.

Finally, you are right to observe the inadequacies of the
vm.opt.* mechanism. IMO, it is better to use custom
properties defined by the extraPropDefns mechanism
that provide the result of analyzing the options, as
compared to checking the options themselves. My
prime example of this, when I was writing the jtreg support,
was the "-server" and "-client" options, which are so-called
supported options on all system, but may be no-ops
on some platforms. Therefore, it is more important to
check the internal settings in the VM than to check the
options given to the VM.

-- Jon

Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

[Sean Mullan]

On 10/12/18 10:33 AM, Langer, Christoph wrote:

Sean, what is your take on this?


Sorry, I haven't had time to look at this in more detail yet. But, let's 
take a step back first. Can you or Matthias explain in more detail why 
this fix is necessary? What are the use cases and motivation? The bug 
report doesn't go into any detail about that and there isn't anything 
in the initial RFR email that explains why this change is useful or 
necessary. As a general guideline or advice, RFEs should include this 
type of information so that Reviewers understand more of the context and 
motivation behind the change.


Thanks,
Sean

Re: CODETOOLS-7902290 breaks all JTreg tests which use @requires vm.opt.*

[Volker Simonis]

On Thu, Oct 11, 2018 at 11:09 PM Igor Ignatyev  wrote:
>
> Hi Volker,
>
> vm.opt.* options are set by jtreg (RegressionContext::processVMOptions), and 
> their value is expected to be "null" if a flag hasn't been passed to JDK 
> under tests via -javaoptions or -vmoptions. I don't think there is something 
> to fix (at least not in jtreg).
>

Well, if "vm.opt.*" options are indeed only options which are taken
from the command line (i.e. "-javaoption/-vmoption"), than there's
definitely something we have to fix! Because, as I described before,
after CODETOOLS-7902290, all test which check such an option in a
@requires clause (negatively or positively) will now fail if that
option is not given explicitly on the command line.

If "vm.opt.*" options are really only the options given on the command
line (please confirm this and please document it somewhere
prominently!) I think many tests which use them are broken because
they simply check such options like this:

@requires vm.opt.TieredCompilation == true

This would be wrong and would have to be replaced by:

@requires vm.opt.TieredCompilation == null | vm.opt.TieredCompilation == true

to also account for the case where -XX:TieredCompilation was not
explicitly set on the command line. Notice that in in such a case, the
value of "TieredCompilation" in the VM could be either "true" or
"false" (depending on the platform and VM build configuration), but it
wouldn't be possible to test that by using "vm.opt.TieredCompilation".

That said, there are three "vm.opt.final.*" options which are set by VMProps:

vmOptFinalFlag(map, "ClassUnloading");
vmOptFinalFlag(map, "UseCompressedOops");
vmOptFinalFlag(map, "EnableJVMCI");

Notice that these options are set to the values of the corresponding
flags in the agent VM . For tests which run in "othervm" mode (or
which start new VMs with ProcessBuilder or
jdk.test.lib.process.ProcessTools), that doesn't necessarily
corresponds to the values of these flags in the testee VM.

Finally, options specified with "-javaoption/-vmoption" and reflected
in the corresponding "vm.opt.*" flags, may be over-ridden by command
line options specified for tests which run in "othervm" mode (so
checking them with a @requires clause is of limited usefulness as
well).

All this is quite complex and I think we should *really* document it
in a prominent place like the "jtreg: Command Line Options" page [1]
AND the
"The JDK Test Framework: Tag Language Specification" page [2].

Regards,
Volker

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

> -- Igor
>
>
> > On Oct 11, 2018, at 1:59 PM, Volker Simonis  
> > wrote:
> >
> > Jonathan Gibbons  schrieb am Do. 11. Okt. 2018
> > um 19:16:
> >
> >> Volker,
> >>
> >> It's a typo/oops on my part.  I'll fix it.
> >>
> >
> > Hi John,
> >
> > thanks for looking at this issue!
> >
> > What do you mean  with “ typo”? Returning null instead of the string “null”?
> >
> > That would “fix” the corresponding tests in the sense that they would run
> > again, however still with bogus results for all the @requires guards which
> > check the “vm.opt.*” options. Shouldn’t we fix that as well?
> >
> > Regards,
> > Volker
> >
> >
> >
> >> -- Jon
> >>
> >>
> >> On 10/11/18 10:13 AM, Jonathan Gibbons wrote:
> >>> Volker,
> >>>
> >>> Thanks for the report.  I'll take a look shortly.
> >>>
> >>> -- Jon
> >>>
> >>>
> >>> On 10/11/18 10:01 AM, Volker Simonis wrote:
>  Hi,
> 
>  the change "CODETOOLS-7902290: introduce error state of @requires
>  properties" [1] which was recently pushed to the codetools repo,
>  breaks all JTreg tests which have a require clause that queries a non
>  final (i.e. no "vm.opt.final") VM option.
> 
>  For example the test
>  "hotspot/jtreg/runtime/ReservedStack/ReservedStackTest.java" which
>  uses "@requires vm.opt.DeoptimizeALot != true" will now fail with:
> 
>  test result: Error. Error evaluating expression: name not defined:
>  vm.opt.DeoptimizeALot
> 
>  This is because
>  com.sun.javatest.regtest.config.RegressionContext.get() was changed to
>  return null instead of the string "null" for unknown properties.
>  Looking at requires.VMProps, I realized that "vm.opt.DeoptimizeALot"
>  is indeed not defined (as many other "vm.opt" properties, see below).
>  So it turns out that all the tests which @required a "vm.opt" option
>  did run in a faulty configuration until now.
> 
>  I could for example easily fix
>  "hotspot/jtreg/runtime/ReservedStack/ReservedStackTest.java" by adding
>  the corresponding option in VMProps.java:
> 
>  $ hg diff
>  diff -r 7a1e2d7ac55a test/jtreg-ext/requires/VMProps.java
>  --- a/test/jtreg-ext/requires/VMProps.java  Thu Oct 11 10:11:18
>  2018 -0400
>  +++ b/test/jtreg-ext/requires/VMProps.java  Thu Oct 11 18:51:37
>  2018 +0200
>  @@ -266,6 +266,8 @@
>

RE: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

[Langer, Christoph]

Hi Matthias,

I generally support this enhancement of IOExceptions to include path 
information.

I also think that we should protect this with the java.security property 
"jdk.includeInExceptions" and agree that "path" would be a good choice since it 
is generic enough to be used in other places where Exceptions are thrown that 
might include path names. As for some comment from security-folks: Sean, what 
is your take on this?

As for the implementation: We should definitely get rid of the upcall into Java 
from JNU_ThrowIOExceptionWithLastErrorAndPath and build the exception message 
in that function only, just using the " jstring path" argument.

The reason for having the current implementation of ExceptionUtils. 
createExceptionTextWithPath, as you suggested in your webrev was that we were 
chasing a very specific customer issue where cwd and user.dir diverged (e.g. 
because of some 3rd party native library which changed the cwd). And this code 
is somehow still around.
So, user.dir should definitely not be part of the exception message. And 
whether the current cwd should be contained should be reviewed in detail again. 
Maybe at some places it is interesting but on other places it isn't at all.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Baesken, Matthias
> Sent: Freitag, 12. Oktober 2018 15:19
> To: Alan Bateman ; security-
> d...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Subject: [CAUTION] RE: RFR: 8211752:
> JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions
> with path causing the issue
> 
> Hi Alan, Goetz,
> 
> >There are several issues with this proposal. The security concern is the main
> one and I think we should wait for comment from security-dev.
>   .
> 
> >If this is required, I would choose a more generic tag
> 
> >so it can be reused in other places.
> 
> >I would just use "path".
> 
> >
> 
> >Best regards,
> 
> >  Goetz.
> 
> Thanks for  the comments .   Sure,  I can use path  for the category naming  
> as
> well, but would be good to hear the  opinions of  the security-dev guys on
> this .
> 
> > The second concern is that the patch is incomplete and inconsistent in that
> it changes the exception throw by two methods,
> >it ignores other file I/O methods that throw exceptions.
> 
> We  have these extended exceptions for quite some time in our JVM;   we
> decided to   enhance exceptions where we have a path already "at hand" ,
>   so it was easy to extend the exceptions .
> In case you think this can be done with some others , that's fine with me -  
> do
> you have some concrete examples ?
> 
> A lot of IO exceptions currently thrown that use
> JNU_ThrowIOExceptionWithLastError  do not have a path (at least not where
> the exception is thrown).
> 
> 
> Best regards ,
> Matthias
> 
> 
> 
> From: Alan Bateman 
> Sent: Freitag, 12. Oktober 2018 11:18
> To: Baesken, Matthias ; security-
> d...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath -
> enhance some IOExceptions with path causing the issue
> 
> On 12/10/2018 09:12, Baesken, Matthias wrote:
> 
> Ping ...  any reviews / comments ?
> 
> Should I add  a category  , for example"ioExceptionsWithPath" to the
> java.security - file  to control  the enabling/disabling of the  enhanced
> exception ?
> 
>   java.security
> 
> #
> # Enhanced exception message information
> ...
> #jdk.includeInExceptions=hostInfo,jar,ioExceptionsWithPath
> 
> There are several issues with this proposal. The security concern is the main
> one and I think we should wait for comment from security-dev. The second
> concern is that the patch is incomplete and inconsistent in that it changes 
> the
> exception throw by two methods, it ignores other file I/O methods that
> throw exceptions. Finally there is the concerns with the patch itself that 
> both
> Roger and I commented on.
> 
> -Alan

RE: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

[Baesken, Matthias]

Hi Alan, Goetz,

>There are several issues with this proposal. The security concern is the main 
>one and I think we should wait for comment from security-dev.
  .

>If this is required, I would choose a more generic tag

>so it can be reused in other places.

>I would just use "path".

>

>Best regards,

>  Goetz.

Thanks for  the comments .   Sure,  I can use path  for the category naming  as 
well, but would be good to hear the  opinions of  the security-dev guys on this 
.

> The second concern is that the patch is incomplete and inconsistent in that 
> it changes the exception throw by two methods,
>it ignores other file I/O methods that throw exceptions.

We  have these extended exceptions for quite some time in our JVM;   we decided 
to   enhance exceptions where we have a path already "at hand" ,
  so it was easy to extend the exceptions .
In case you think this can be done with some others , that's fine with me -  do 
you have some concrete examples ?

A lot of IO exceptions currently thrown that use 
JNU_ThrowIOExceptionWithLastError  do not have a path (at least not where the 
exception is thrown).


Best regards ,
Matthias



From: Alan Bateman 
Sent: Freitag, 12. Oktober 2018 11:18
To: Baesken, Matthias ; 
security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance 
some IOExceptions with path causing the issue

On 12/10/2018 09:12, Baesken, Matthias wrote:

Ping ...  any reviews / comments ?

Should I add  a category  , for example"ioExceptionsWithPath" to the 
java.security - file  to control  the enabling/disabling of the  enhanced 
exception ?

  java.security

#
# Enhanced exception message information
...
#jdk.includeInExceptions=hostInfo,jar,ioExceptionsWithPath

There are several issues with this proposal. The security concern is the main 
one and I think we should wait for comment from security-dev. The second 
concern is that the patch is incomplete and inconsistent in that it changes the 
exception throw by two methods, it ignores other file I/O methods that throw 
exceptions. Finally there is the concerns with the patch itself that both Roger 
and I commented on.

-Alan

RE: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

[Lindenmaier, Goetz]

Hi,

If this is required, I would choose a more generic tag
so it can be reused in other places.
I would just use "path".

Best regards,
  Goetz.


> -Original Message-
> From: core-libs-dev  On Behalf
> Of Baesken, Matthias
> Sent: Freitag, 12. Oktober 2018 10:13
> To: security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Subject: [CAUTION] RE: RFR: 8211752:
> JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions
> with path causing the issue
> 
> Ping ...  any reviews / comments ?
> 
> Should I add  a category  , for example"ioExceptionsWithPath" to the
> java.security - file  to control  the enabling/disabling of the  enhanced
> exception ?
> 
>   java.security
> 
> #
> # Enhanced exception message information
> ...
> #jdk.includeInExceptions=hostInfo,jar,ioExceptionsWithPath
> 
> 
> Thanks, Matthias
> 
> 
> From: Baesken, Matthias
> Sent: Dienstag, 9. Oktober 2018 14:12
> To: security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Cc: Alan Bateman (alan.bate...@oracle.com) 
> Subject: FW: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath -
> enhance some IOExceptions with path causing the issue
> 
> Hello, Alan commented on it :
> 
> 
>   *   This proposal will require a security review as it leaks sensitive
> information into exceptions.
> 
> So I forward it to security-dev as well.
> 
> If needed,  we might  use something similar to  JDK-8207768  where a
> category  has been added  for  enhanced exception messages to the
> java.security file .
> 
> Best regards, Matthias
> 
> From: Baesken, Matthias
> Sent: Dienstag, 9. Oktober 2018 13:40
> To: core-libs-dev@openjdk.java.net d...@openjdk.java.net>
> Cc: Langer, Christoph
> mailto:christoph.lan...@sap.com>>;
> Lindenmaier, Goetz
> mailto:goetz.lindenma...@sap.com>>
> Subject: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath -
> enhance some IOExceptions with path causing the issue
> 
> 
> Hello, please review the following change .
> It enhances a number of JNU_ThrowIOExceptionWithLastError  calls with
> path and current working directory information.
> For this, a new function JNU_ThrowIOExceptionWithLastErrorAndPath is
> added.
> 
> bug and webrev :
> 
> https://bugs.openjdk.java.net/browse/JDK-8211752
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8211752.0/
> 
> Thanks, Matthias

Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

[Alan Bateman]

On 12/10/2018 09:12, Baesken, Matthias wrote:


Ping …  any reviews / comments ?

Should I add  a category  , for example    “ioExceptionsWithPath” 
to the java.security – file  to control  the enabling/disabling of 
the  enhanced exception ?


  java.security

#

# Enhanced exception message information

…

#jdk.includeInExceptions=hostInfo,jar,ioExceptionsWithPath


There are several issues with this proposal. The security concern is the 
main one and I think we should wait for comment from security-dev. The 
second concern is that the patch is incomplete and inconsistent in that 
it changes the exception throw by two methods, it ignores other file I/O 
methods that throw exceptions. Finally there is the concerns with the 
patch itself that both Roger and I commented on.


-Alan

RE: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

[Baesken, Matthias]

Ping ...  any reviews / comments ?

Should I add  a category  , for example"ioExceptionsWithPath" to the 
java.security - file  to control  the enabling/disabling of the  enhanced 
exception ?

  java.security

#
# Enhanced exception message information
...
#jdk.includeInExceptions=hostInfo,jar,ioExceptionsWithPath


Thanks, Matthias


From: Baesken, Matthias
Sent: Dienstag, 9. Oktober 2018 14:12
To: security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
Cc: Alan Bateman (alan.bate...@oracle.com) 
Subject: FW: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance 
some IOExceptions with path causing the issue

Hello, Alan commented on it :


  *   This proposal will require a security review as it leaks sensitive 
information into exceptions.

So I forward it to security-dev as well.

If needed,  we might  use something similar to  JDK-8207768  where a category  
has been added  for  enhanced exception messages to the
java.security file .

Best regards, Matthias

From: Baesken, Matthias
Sent: Dienstag, 9. Oktober 2018 13:40
To: core-libs-dev@openjdk.java.net
Cc: Langer, Christoph 
mailto:christoph.lan...@sap.com>>; Lindenmaier, Goetz 
mailto:goetz.lindenma...@sap.com>>
Subject: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some 
IOExceptions with path causing the issue


Hello, please review the following change .
It enhances a number of JNU_ThrowIOExceptionWithLastError  calls with path and 
current working directory information.
For this, a new function JNU_ThrowIOExceptionWithLastErrorAndPath is added.

bug and webrev :

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

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

Thanks, Matthias