Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

FYI I’ve updated the webrev/javadoc with latest edits including the 2 new 
ByteBuffer methods:

http://cr.openjdk.java.net/~vinnie/8170769/webrev.09/ 
<http://cr.openjdk.java.net/~vinnie/8170769/webrev.09/>
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.09/api/java.base/java/util/HexFormat.html
 
<http://cr.openjdk.java.net/~vinnie/8170769/javadoc.09/api/java.base/java/util/HexFormat.html>
(NOTE: internal link to HexFormat.Formatter class is currently broken)


If that is an acceptable compromise then I believe all outstanding issues have 
now been addressed.
Thanks to all those who provided review comments.


> On 12 Dec 2018, at 12:32, Vincent Ryan  wrote:
> 
> Thanks for your proposal.
> Comments below.
> 
>> On 12 Dec 2018, at 02:35, Stuart Marks  wrote:
>> 
>> 
>> 
>> On 12/11/18 1:21 PM, Vincent Ryan wrote:
>>> My preference is for PrintStream rather than Writer, for the same reason as 
>>> Roger: it’s more convenient
>>> for handling System.out. Does that address your concern?
>> 
>> PrintStream is fine with me.
>> 
>>> I cannot simply cast 8859-1 characters into UTF-8 because UTF-8 is 
>>> multi-byte charset so some 0x8X characters
>>> Will trigger the multi-byte sequence and will end up being misinterpreted. 
>>> Hence my rather awkward conversion to a String.
>>> Is there a better way?
>> 
>> In toPrintableString(),
>> 
>> 259 StringBuilder printable = new StringBuilder(toIndex - fromIndex);
>> 260 for (int i = fromIndex; i < toIndex; i++) {
>> 261 if (bytes[i] > 0x1F && bytes[i] < 0x7F) {
>> 262 printable.append((char) bytes[i]);
>> 263 } else if (bytes[i] > (byte)0x9F && bytes[i] <= (byte)0xFF) {
>> 264 printable.append(new String(new byte[]{bytes[i]}, 
>> ISO_8859_1));
>> 265
>> 266 } else {
>> 267 printable.append('.');
>> 268 }
>> 269 }
>> 
>> It works to cast ASCII bytes char, because the 7-bit ASCII range overlaps 
>> the low 7 bits of the UTF-16 char range. The bytes values of ISO 8859-1 
>> overlap the low 8 bits of UTF-16, so casts work for them too.
>> 
>> For any other charset, you'd need to do codeset conversion. But you're 
>> cleverly supporting only ISO 8859-1, so you don't have to do any conversion. 
>> :-)
>> 
>>> I’m not sure I’ve addressed your concern regarding IOExceptions - can you 
>>> elaborate?
>> 
>> Taking out the OutputStream overloads addressed my concerns. In at least one 
>> case the code would wrap the OutputStream into a PrintStream, print stuff to 
>> it, and then throw away the PrintStream. If an output error occurred, any 
>> error state in the PrintStream would also be thrown away. The creation of 
>> the PrintStream wrapper would also use the system's default charset instead 
>> of letting the caller control it.
>> 
>> The dump() overloads now all take PrintStream, so it's the caller's 
>> responsibility to ensure that the PrintStream is using the right charset and 
>> to check for errors after. So this is all OK now.
>> 
>> Note that the internal getPrintStream(), to wrap an OutputStream in a 
>> PrintStream, is now obsolete and can be removed.
>> 
>> (Oh, I see Roger has said much the same things. Oh well, the peril of 
>> parallel reviews.)
>> 
>> **
>> 
>>> BTW updated webrev/javadoc available:
>>> http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/
>>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html
>> 
>> Now we have a somewhat unsatisfying asymmetry in the APIs.
>> 
>> There are four kinds of inputs:
>> 
>> 1. byte[]
>> 2. byte[] subrange
>> 3. InputStream
>> 4. ByteBuffer
>> 
>> and two kinds of outputs:
>> 
>> 1. PrintStream
>> 2. Stream
>> 
>> and two variations of formatters:
>> 
>> 1. default formatter
>> 2. custom formatter + chunk size
>> 
>> This is a total of 16 combinations. But there are only eight methods: three 
>> PrintStream methods with choice of input, two stream-output methods using 
>> the default formatter, and three stream-output methods using custom 
>> chunk+formatter.
>> 
>> You don't have to provide ALL combinations, but what's here is an odd subset 
>> with some apparently arbitrary choices. For example, if I have a ByteBuffer 
>> and I want to dump it to System.out using default formatting, I have to g

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks for your proposal.
Comments below.

> On 12 Dec 2018, at 02:35, Stuart Marks  wrote:
> 
> 
> 
> On 12/11/18 1:21 PM, Vincent Ryan wrote:
>> My preference is for PrintStream rather than Writer, for the same reason as 
>> Roger: it’s more convenient
>> for handling System.out. Does that address your concern?
> 
> PrintStream is fine with me.
> 
>> I cannot simply cast 8859-1 characters into UTF-8 because UTF-8 is 
>> multi-byte charset so some 0x8X characters
>> Will trigger the multi-byte sequence and will end up being misinterpreted. 
>> Hence my rather awkward conversion to a String.
>> Is there a better way?
> 
> In toPrintableString(),
> 
> 259 StringBuilder printable = new StringBuilder(toIndex - fromIndex);
> 260 for (int i = fromIndex; i < toIndex; i++) {
> 261 if (bytes[i] > 0x1F && bytes[i] < 0x7F) {
> 262 printable.append((char) bytes[i]);
> 263 } else if (bytes[i] > (byte)0x9F && bytes[i] <= (byte)0xFF) {
> 264 printable.append(new String(new byte[]{bytes[i]}, 
> ISO_8859_1));
> 265
> 266 } else {
> 267 printable.append('.');
> 268 }
> 269 }
> 
> It works to cast ASCII bytes char, because the 7-bit ASCII range overlaps the 
> low 7 bits of the UTF-16 char range. The bytes values of ISO 8859-1 overlap 
> the low 8 bits of UTF-16, so casts work for them too.
> 
> For any other charset, you'd need to do codeset conversion. But you're 
> cleverly supporting only ISO 8859-1, so you don't have to do any conversion. 
> :-)
> 
>> I’m not sure I’ve addressed your concern regarding IOExceptions - can you 
>> elaborate?
> 
> Taking out the OutputStream overloads addressed my concerns. In at least one 
> case the code would wrap the OutputStream into a PrintStream, print stuff to 
> it, and then throw away the PrintStream. If an output error occurred, any 
> error state in the PrintStream would also be thrown away. The creation of the 
> PrintStream wrapper would also use the system's default charset instead of 
> letting the caller control it.
> 
> The dump() overloads now all take PrintStream, so it's the caller's 
> responsibility to ensure that the PrintStream is using the right charset and 
> to check for errors after. So this is all OK now.
> 
> Note that the internal getPrintStream(), to wrap an OutputStream in a 
> PrintStream, is now obsolete and can be removed.
> 
> (Oh, I see Roger has said much the same things. Oh well, the peril of 
> parallel reviews.)
> 
> **
> 
>> BTW updated webrev/javadoc available:
>> http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/
>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html
> 
> Now we have a somewhat unsatisfying asymmetry in the APIs.
> 
> There are four kinds of inputs:
> 
> 1. byte[]
> 2. byte[] subrange
> 3. InputStream
> 4. ByteBuffer
> 
> and two kinds of outputs:
> 
> 1. PrintStream
> 2. Stream
> 
> and two variations of formatters:
> 
> 1. default formatter
> 2. custom formatter + chunk size
> 
> This is a total of 16 combinations. But there are only eight methods: three 
> PrintStream methods with choice of input, two stream-output methods using the 
> default formatter, and three stream-output methods using custom 
> chunk+formatter.
> 
> You don't have to provide ALL combinations, but what's here is an odd subset 
> with some apparently arbitrary choices. For example, if I have a ByteBuffer 
> and I want to dump it to System.out using default formatting, I have to go 
> the Stream.forEachOrdered route AND provide the default chunk size and 
> formatter.
> 
>HexFormat.dumpAsStream(buf, DEFAULT_CHUNK_SIZE, HEXDUMP_FORMATTER)
> .forEachOrdered(System.out::println);
> 
> These aren't huge deals, but they're easily stumbled over.
> 
> One approach to organizing this is to have a HexFormat instance that contains 
> the setup information and then to have instance methods that either update 
> the setup or perform conversion and output. I'd use static factory methods 
> instead of constructors. For example, you could do this:
> 
> static factories methods:
> - from(byte[])
> - from(byte[], fromIndex, toIndex)
> - from(InputStream)
> - from(ByteBuffer)
> 
> formatter setup instance methods:
> - format(chunksize, formatter)
> 
> output:
> - void dump(PrintStream)
> - Stream stream()
> 
> Using this approach my example from above could be performed as follows:
> 
>   HexFormat.from(buf).dump(System.out);
> 
> Note, I'm not saying that you

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks for your review Alan.

I believe I’ve addressed your concerns in this latest webrev/javadoc:
http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/ 
<http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/>
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html
 
<http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html>


> On 11 Dec 2018, at 13:04, Alan Bateman  wrote:
> 
> On 08/12/2018 01:18, Vincent Ryan wrote:
>> Here’s the latest version that addresses all the current open issues:
>> 
>> webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/ 
>> <http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/>
>> javadoc: 
>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html
>>  
>> <http://cr.openjdk.java.net/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html>
>> 
>> CSR: https://bugs.openjdk.java.net/browse/CCC-8170769 
>> <https://bugs.openjdk.java.net/browse/CCC-8170769>
>> 
> I read through the latest javadoc corresponding to webrev.07.
> 
> Overall I think it looks very good except for dumpAsStream(ByteBuffer, 
> fromIndex, toIndex, chunkSize, Formatter) as it's not clear if fromIndex is 
> from the buffer position or an absolute index. If the former then there are a 
> couple of other issues. I see Roger has touched on the advancement of the 
> buffer position to its limit which isn't right unless all remaining bytes in 
> the  buffer are consumer. There is also an issue with the exception as 
> attempting to consume beyond the limit is a BufferUnderflowException. It 
> might be simpler to replace this method with a dumpAsStream(ByteBuffer, 
> chunkSize, Formatter) that can lazily (rather than eagerly) consume the 
> remaining bytes. Additional overloads could be added in the future if needed.
> 
> dumpAsStream(InputStream) specifies "On return, the input stream will be at 
> end-of-stream" but I assume this isn't right as the method is lazy.
> 
> The 3-arg dumpAsStream doesn't specify the exception/behavior for when 
> chunkSize is <= 0.
> 
> fromString(CharSequence) may need clarification on how it behaves when the CS 
> is a CharBuffer. Does it consume all the remaining chars in the buffer so its 
> position be advanced to the limit? The 3-arg version is also not clear on 
> this point.
> 
> In Formatter interface it may be useful to expand the description of the 
> "offset" parameter as readers may not immediately understand that it's just 
> an input for the formatted string rather than a real offset, an example might 
> help. It also doesn't say if the offset can be specified as <= 0L or not.
> 
> A minor comment is that several places refer to a byte array as a "byte 
> buffers. Is this deliberate or can these be replaced with "byte array" to 
> avoid confusion. Another minor comment is that one of the dumpAsStream 
> methods is missing @throws NPE - maybe they should all be removed and 
> replaced with a statement in the class description.
> 
> -Alan

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks Roger. 

I believe that all but one of your concerns are addressed in this latest 
webrev/javadoc:
http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/ 
<http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/>
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html
 
<http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html>

The remaining issue is how best to define the dumpAsStream() method that takes 
a ByteBuffer.
The current approach has dropped the to/from parameters, consumes all the 
buffer and advances
Its position to the buffer limit.
This places the burden on the caller to size the buffer appropriately.

However I also see the merit in a ‘read-only’ approach that does not advance 
the buffer position.

Any further thoughts?



> On 11 Dec 2018, at 16:54, Roger Riggs  wrote:
> 
> Ho Vincent,
> 
> On 12/11/2018 11:34 AM, Vincent Ryan wrote:
>> Responses in-line.
>> 
>>> 
>>>> Its really nice/necessary that examples can be copy/pasted and compile.
>>>>> 
>>>>> 
>>>>>  - dumpAsStream(ByteBuffer, from, to, chunk, Formatter) may be confusing 
>>>>> because it
>>>>>is using the relative methods of ByteBuffer, but the from and to 
>>>>> offsets are within
>>>>>the position .. limit buffer.  That should be explicit.
>>>>>(Or consider switching to the absolute accesses in the ByteBuffer and 
>>>>> not affect the position)
>>>> 
>>>> Is the additional javadoc line added earlier sufficient?
>>> I would bear some reinforcing that the entire remainder of the buffer is 
>>> read
>>> and the from and to are indexes within that remainder.
>>> And I'm not sure that's the desirable semantics.
>>> 
>>> It would make more sense if the from bytes from the buffer are skipped
>>> and only the (to - from) bytes are dumped.  That leaves the ByteBuffer 
>>> reading only the requested bytes.  A natural usage such as:
>>>  dumpAsStream​(ByteBuffer buffer, 0, 256, 16, formatter)
>>> would dump the next 256bytes of the ByteBuffer and position would be
>>> moved by 256.
>> 
>> [As suggested by Alan]
>> How about dropping the fromIndex and toIndex parameters and requiring the 
>> caller
>> to provide a suitably sized buffer? 
>> 
>> public static Stream dumpAsStream(ByteBuffer buffer, int chunkSize, 
>> Formatter formatter) {
>> 
> I'd go the other direction and make dump use absolute offset and limit.
> The current values for position and limit are readily available from the 
> buffer.
> 
> If the dumping code is being added to perform some diagnostics then it would 
> not
> modify the position and not disturb the existing code that is using the 
> buffer.
> 
> Absolute is simpler to explain and implement and has fewer side effects.
> 
>> 
>>> 
>>>> 
>>>> 
>>>>> 
>>>>> - dump(byte[], OutputStream) - I don't think the example is correct, 
>>>>> there is no reference
>>>>>   to a stream, only the PrintStream::println method, which is not static.
>>>> 
>>>> The code is just illustrative. Real values would have to be supplied for 
>>>> the input bytes and the
>>>> chosen print method on the output stream. Typically, that print method 
>>>> will be System.out::println
>>> Examples that don't compile are really confusing and annoying.
>> 
>> Updated the ‘as if’ code to:
>> 
>>  * byte[] bytes = ...
>>  * PrintStream out = ...
>>  * HexFormat.dumpAsStream(bytes, 16,
>>  * (offset, chunk, from, to) ->
>>  * String.format("%08x  %s  |%s|",
>>  * offset,
>>  * HexFormat.toFormattedString(chunk, from, to),
>>  * HexFormat.toPrintableString(chunk, from, to)))
>>  * .forEachOrdered(out::println);
>> 
>> 
> Looks fine, thanks

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks for your review Stuart.

My preference is for PrintStream rather than Writer, for the same reason as 
Roger: it’s more convenient
for handling System.out. Does that address your concern?

I cannot simply cast 8859-1 characters into UTF-8 because UTF-8 is multi-byte 
charset so some 0x8X characters
Will trigger the multi-byte sequence and will end up being misinterpreted. 
Hence my rather awkward conversion to a String.
Is there a better way?

I’m not sure I’ve addressed your concern regarding IOExceptions - can you 
elaborate?


BTW updated webrev/javadoc available:
http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/ 
<http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/>
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html
 
<http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html>



> On 11 Dec 2018, at 02:11, Stuart Marks  wrote:
> 
> On 12/7/18 10:22 AM, Vincent Ryan wrote:
>>> I'm not convinced that the overloads that send output to an OutputStream 
>>> pull their weight. They basically wrap the OutputStream in a PrintStream, 
>>> which conveniently doesn't declare IOException, making it easy to use from 
>>> a lambda passed to forEachOrdered(). If an error writing the output occurs, 
>>> this is recorded by the PrintStream wrapper; however, the wrapper is then 
>>> thrown away, making it impossible for the caller to check its error status.
>> The intent is to support a trivial convenience method call that generates 
>> the well-known hexdump format.
>> Especially for users that are interested in the hexdump data rather than the 
>> low-level details of how to terminate a stream.
>> The dumpAsStream methods are available to support cases that differ from 
>> that format.
>> Have you a suggestion to improve the dump() methods, or you’d like to see 
>> them omitted?
>>> The PrintStream wrapper also uses the platform default charset, and doesn't 
>>> provide any way for the caller to override the charset.
>> Is there a need for that? Originally the requirement was driven by the 
>> hexdump format which is ASCII-only.
>> Recently the class has been enhanced to also support the printable 
>> characters from ISO 8859-1.
>> A custom formatter be supplied to dumpAsStream() to cater for all other 
>> cases?
> 
> OK, let's step back from this a bit. I see this hexdump as a little subsystem 
> that has the following facets:
> 
> 1) a source of bytes
> 2) a converter to hex
> 3) a destination
> 
> The converter is HexDump.Formatter, which converts and formats a subrange of 
> byte[] to a String. Since the user can supply the Formatter function, the 
> result String can contain any unicode character. Thus, the destination needs 
> to handle any unicode character. It can be a Writer, which accepts String 
> data. Or if you want it to write bytes, it can be an OutputStream, which 
> raises the issue of encoding (charset). I would recommend against relying on 
> the platform default charset, as this has been a source of subtle bugs. The 
> preferred approach these days is to default to UTF-8 and provide an overload 
> that takes an explicit charset.
> 
> An alternative is PrintStream. (This overlaps somewhat with your recent 
> exchange with Roger on this topic.) PrintStream also does charset encoding, 
> and the charset it uses depends on how it's created. I think the same 
> approach should be applied as I described above with OutputStream, namely 
> avoid the platform default charset; default to UTF-8; and provide an overload 
> that takes an explicit charset.
> 
> I'm not sure which of these is the right thing. You should decide which is 
> the most convenient for the use cases you expect to see. However, the 
> solution needs to handle charset encoding. (And it should also properly deal 
> with I/O exceptions, per my previous message.)
> 
> **
> 
> ISO 8859-1 comes up in a different place. The toPrintableString() method 
> (used by the default formatter) considers a byte "printable" if it encodes a 
> valid ISO 8859-1 character. The byte is properly decoded to a String, so this 
> is ok. Note this is a distinct issue from the encoding of the OutputStream or 
> PrintStream as described above.
> 
> (As an aside I think that the encoding of ISO 8859-1 matches the 
> corresponding code units of UTF-16, so you don't have to do the new 
> String(..., ISO_8859_1) jazz. You can just cast the byte to a char and append 
> it to the StringBuilder.)
> 
> s'marks

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Responses in-line.

> On 10 Dec 2018, at 21:38, Roger Riggs  wrote:
> 
> Hi Vincent,
> 
> On 12/10/2018 03:59 PM, Vincent Ryan wrote:
>> Comments in-line.
>> Thanks.
>> 
>>> On 10 Dec 2018, at 16:59, Roger Riggs >> <mailto:roger.ri...@oracle.com>> wrote:
>>> 
>>> Hi,
>>> 
>>> Looks good, though some points to clarify.
>>> 
>>> - The methods that use ByteBuffers should be clear that accesses to the 
>>> ByteBuffers
>>>are relative and use and modify the position and ByteBuffer exceptions 
>>> may be thrown.
>> 
>> Added the line:
>> 'Access to the ByteBuffer is relative and its position gets set to the 
>> buffer's limit.'
> ok
>> 
>>> 
>>> - The methods that write output (Strings) to an output stream might be more 
>>> general
>>>   if the argument was a PrintStream, allowing the hex formatted output to 
>>> be 
>>>   assembled with other localized output.
>>>   I do see that as long as HexFormat is only writing hex digits, the effect 
>>> would be almost the same.
>>>   (It would also eliminate, the inefficient code in getPrintStream that 
>>> creates a PrintStream on demand).
>> 
>> I had wanted to provide flexibility by favouring OutputStream over 
>> PrintStream.
>> It is convenient for the common case of dumping to a file using 'new 
>> FileOutputStream’.
>> As you note it complicates assembly with other output.
>> 
>> I guess it’s fine to change the OutputStream args to PrintStream.
> ok, if the caller has a direct OutputStream, a PrintStream could be created.
> But I think the PrintStream is more common.
>> 
>> 
>>> 
>>>  - toString(byte[], from, to) and other methods there's no need for parens 
>>> around the note about fromIndex==toIndex.
>> 
>> OK
>> 
>>> 
>>>  - fromString methods: The optional prefix of "0x": add the phrase  "is 
>>> ignored".
>>> The IAE, does not mention the optional prefix, I'm not sure if that 
>>> allows some confusion.
>> 
>> Added the line:
>> 'The optional prefix of {@code "0x"} is ignored.'
> ok
>> 
>>> 
>>> - dumpAsStream(InputStream) does not have a throws NPE for in == null.
>>>   A catch all in the class javadoc could cover that. 
>>>   " Unless otherwise noted, passing a null argument to a constructor or 
>>> method in any class or 
>>>interface in this package will cause a NullPointerException to be 
>>> thrown. “
>> 
>> Added an @throws to the method.
> ok
>> 
>>> 
>>>  - dumpAsStream methods, the formatter argument is described as being used 
>>> "if present";  
>>>it might be clearer as "if not null”.
>> 
>> OK
>> Its really nice/necessary that examples can be copy/pasted and compile.
>>> 
>>> 
>>>  - dumpAsStream(ByteBuffer, from, to, chunk, Formatter) may be confusing 
>>> because it
>>>is using the relative methods of ByteBuffer, but the from and to offsets 
>>> are within
>>>the position .. limit buffer.  That should be explicit.
>>>(Or consider switching to the absolute accesses in the ByteBuffer and 
>>> not affect the position)
>> 
>> Is the additional javadoc line added earlier sufficient?
> I would bear some reinforcing that the entire remainder of the buffer is read
> and the from and to are indexes within that remainder.
> And I'm not sure that's the desirable semantics.
> 
> It would make more sense if the from bytes from the buffer are skipped
> and only the (to - from) bytes are dumped.  That leaves the ByteBuffer 
> reading only the requested bytes.  A natural usage such as:
>  dumpAsStream​(ByteBuffer buffer, 0, 256, 16, formatter)
> would dump the next 256bytes of the ByteBuffer and position would be
> moved by 256.

[As suggested by Alan]
How about dropping the fromIndex and toIndex parameters and requiring the caller
to provide a suitably sized buffer? 

public static Stream dumpAsStream(ByteBuffer buffer, int chunkSize, 
Formatter formatter) {


> 
>> 
>> 
>>> 
>>> - dump(byte[], OutputStream) - I don't think the example is correct, there 
>>> is no reference
>>>   to a stream, only the PrintStream::println method, which is not static.
>> 
>> The code is just illustrative. Real values would have to be supplied for the 
>> input bytes and the
>> chosen print method on the output stream. Typically, 

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Comments in-line.
Thanks.

> On 10 Dec 2018, at 16:59, Roger Riggs  wrote:
> 
> Hi,
> 
> Looks good, though some points to clarify.
> 
> - The methods that use ByteBuffers should be clear that accesses to the 
> ByteBuffers
>are relative and use and modify the position and ByteBuffer exceptions may 
> be thrown.

Added the line:
'Access to the ByteBuffer is relative and its position gets set to the buffer's 
limit.'

> 
> - The methods that write output (Strings) to an output stream might be more 
> general
>   if the argument was a PrintStream, allowing the hex formatted output to be 
>   assembled with other localized output.
>   I do see that as long as HexFormat is only writing hex digits, the effect 
> would be almost the same.
>   (It would also eliminate, the inefficient code in getPrintStream that 
> creates a PrintStream on demand).

I had wanted to provide flexibility by favouring OutputStream over PrintStream.
It is convenient for the common case of dumping to a file using 'new 
FileOutputStream’.
As you note it complicates assembly with other output.

I guess it’s fine to change the OutputStream args to PrintStream.


> 
>  - toString(byte[], from, to) and other methods there's no need for parens 
> around the note about fromIndex==toIndex.

OK

> 
>  - fromString methods: The optional prefix of "0x": add the phrase  "is 
> ignored".
> The IAE, does not mention the optional prefix, I'm not sure if that 
> allows some confusion.

Added the line:
'The optional prefix of {@code "0x"} is ignored.'

> 
> - dumpAsStream(InputStream) does not have a throws NPE for in == null.
>   A catch all in the class javadoc could cover that. 
>   " Unless otherwise noted, passing a null argument to a constructor or 
> method in any class or 
>interface in this package will cause a NullPointerException to be thrown. “

Added an @throws to the method.

> 
>  - dumpAsStream methods, the formatter argument is described as being used 
> "if present";  
>it might be clearer as "if not null”.

OK

> 
>  - dumpAsStream(ByteBuffer, from, to, chunk, Formatter) may be confusing 
> because it
>is using the relative methods of ByteBuffer, but the from and to offsets 
> are within
>the position .. limit buffer.  That should be explicit.
>(Or consider switching to the absolute accesses in the ByteBuffer and not 
> affect the position)

Is the additional javadoc line added earlier sufficient?


> 
> - dump(byte[], OutputStream) - I don't think the example is correct, there is 
> no reference
>   to a stream, only the PrintStream::println method, which is not static.

The code is just illustrative. Real values would have to be supplied for the 
input bytes and the
chosen print method on the output stream. Typically, that print method will be 
System.out::println


> 
> Regards, Roger
> 
> 
> 
> On 12/07/2018 08:18 PM, Vincent Ryan wrote:
>> Here’s the latest version that addresses all the current open issues:
>> 
>> webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/ 
>> <http://cr.openjdk.java.net/%7Evinnie/8170769/webrev.07/>
>> javadoc: 
>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html
>>  
>> <http://cr.openjdk.java.net/%7Evinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html>
>> 
>> CSR: https://bugs.openjdk.java.net/browse/CCC-8170769 
>> <https://bugs.openjdk.java.net/browse/CCC-8170769>
>> 
>> 
>>> On 7 Dec 2018, at 19:51, Vincent Ryan >> <mailto:vincent.x.r...@oracle.com>> wrote:
>>> 
>>> Even shorter. I’ll add that instead.
>>> Thanks.
>>> 
>>>> On 7 Dec 2018, at 19:04, Roger Riggs >>> <mailto:roger.ri...@oracle.com>> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> I don't think this is performance sensitive and less code is better.
>>>> 
>>>> Use java.lang.Character.digit(ch, 16) to convert the char to an int.
>>>> 
>>>> Roger
>>>> 
>>>> On 12/07/2018 01:49 PM, Kasper Nielsen wrote:
>>>>> Hi,
>>>>> 
>>>>> I don't know if performance is an issue. But if it is, I think it world
>>>>> make sense to use a precompute array. And replace
>>>>> 
>>>>> private static int hexToBinary(char ch) {
>>>>>if ('0' <= ch && ch <= '9') {
>>>>>return ch - '0';
>>>>>}
>>>>>if ('A' <= ch && ch <= 'F') {
>>>>>return ch - 'A' 

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Here’s the latest version that addresses all the current open issues:

webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/ 
<http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/>
javadoc: 
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html
 
<http://cr.openjdk.java.net/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html>

CSR: https://bugs.openjdk.java.net/browse/CCC-8170769 
<https://bugs.openjdk.java.net/browse/CCC-8170769>


> On 7 Dec 2018, at 19:51, Vincent Ryan  wrote:
> 
> Even shorter. I’ll add that instead.
> Thanks.
> 
>> On 7 Dec 2018, at 19:04, Roger Riggs  wrote:
>> 
>> Hi,
>> 
>> I don't think this is performance sensitive and less code is better.
>> 
>> Use java.lang.Character.digit(ch, 16) to convert the char to an int.
>> 
>> Roger
>> 
>> On 12/07/2018 01:49 PM, Kasper Nielsen wrote:
>>> Hi,
>>> 
>>> I don't know if performance is an issue. But if it is, I think it world
>>> make sense to use a precompute array. And replace
>>> 
>>> private static int hexToBinary(char ch) {
>>>if ('0' <= ch && ch <= '9') {
>>>return ch - '0';
>>>}
>>>if ('A' <= ch && ch <= 'F') {
>>>return ch - 'A' + 10;
>>>}
>>>if ('a' <= ch && ch <= 'f') {
>>>return ch - 'a' + 10;
>>>}
>>>return -1;
>>> }
>>> 
>>> with
>>> 
>>> private static int hexToBinary(char ch) {
>>>return ch <= 'f' ? staticPrecomputedArray[ch] : -1;
>>> }
>>> 
>>> where int[] staticPrecomputedArray is computed in a static initializer
>>> block.
>>> 
>>> /Kasper
>>> 
>>> On Fri, 23 Nov 2018 at 14:51, Vincent Ryan 
>>> wrote:
>>> 
>>>> Hello,
>>>> 
>>>> Please review this proposal for a new API to conveniently generate and
>>>> display binary data using hexadecimal string representation.
>>>> It supports both bulk and stream operations and it can also generate the
>>>> well-known hexdump format [1].
>>>> 
>>>> This latest revision addresses review comments provided earlier.
>>>> These include adding methods to support for data supplied in a
>>>> ByteBuffer, exposing the default formatter implementation as a public
>>>> static and dropping the superfluous 'Hex' term from several method names.
>>>> 
>>>> Thanks
>>>> 
>>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>>>> API:
>>>> 
>>>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.06/api/java.base/java/util/Hex.html
>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.06/
>>>> 
>>>> 
>>>> 
>>>> [1] https://docs.oracle.com/cd/E88353_01/html/E37839/hexdump-1.html
>>>> 
>> 
> 

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Even shorter. I’ll add that instead.
Thanks.

> On 7 Dec 2018, at 19:04, Roger Riggs  wrote:
> 
> Hi,
> 
> I don't think this is performance sensitive and less code is better.
> 
> Use java.lang.Character.digit(ch, 16) to convert the char to an int.
> 
> Roger
> 
> On 12/07/2018 01:49 PM, Kasper Nielsen wrote:
>> Hi,
>> 
>> I don't know if performance is an issue. But if it is, I think it world
>> make sense to use a precompute array. And replace
>> 
>> private static int hexToBinary(char ch) {
>> if ('0' <= ch && ch <= '9') {
>> return ch - '0';
>> }
>> if ('A' <= ch && ch <= 'F') {
>> return ch - 'A' + 10;
>> }
>> if ('a' <= ch && ch <= 'f') {
>> return ch - 'a' + 10;
>> }
>> return -1;
>> }
>> 
>> with
>> 
>> private static int hexToBinary(char ch) {
>>     return ch <= 'f' ? staticPrecomputedArray[ch] : -1;
>> }
>> 
>> where int[] staticPrecomputedArray is computed in a static initializer
>> block.
>> 
>> /Kasper
>> 
>> On Fri, 23 Nov 2018 at 14:51, Vincent Ryan 
>> wrote:
>> 
>>> Hello,
>>> 
>>> Please review this proposal for a new API to conveniently generate and
>>> display binary data using hexadecimal string representation.
>>> It supports both bulk and stream operations and it can also generate the
>>> well-known hexdump format [1].
>>> 
>>> This latest revision addresses review comments provided earlier.
>>> These include adding methods to support for data supplied in a
>>> ByteBuffer, exposing the default formatter implementation as a public
>>> static and dropping the superfluous 'Hex' term from several method names.
>>> 
>>> Thanks
>>> 
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>>> API:
>>> 
>>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.06/api/java.base/java/util/Hex.html
>>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.06/
>>> 
>>> 
>>> 
>>> [1] https://docs.oracle.com/cd/E88353_01/html/E37839/hexdump-1.html
>>> 
> 

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

I’ll add that replacement method. Thanks.

> On 7 Dec 2018, at 18:49, Kasper Nielsen  wrote:
> 
> Hi,
> 
> I don't know if performance is an issue. But if it is, I think it world make 
> sense to use a precompute array. And replace
> 
> private static int hexToBinary(char ch) {
> if ('0' <= ch && ch <= '9') {
> return ch - '0';
> }
> if ('A' <= ch && ch <= 'F') {
> return ch - 'A' + 10;
> }
> if ('a' <= ch && ch <= 'f') {
> return ch - 'a' + 10;
> }
> return -1;
> }
> 
> with
> 
> private static int hexToBinary(char ch) {
> return ch <= 'f' ? staticPrecomputedArray[ch] : -1;
> }
> 
> where int[] staticPrecomputedArray is computed in a static initializer block.
> 
> /Kasper
> 
> On Fri, 23 Nov 2018 at 14:51, Vincent Ryan  <mailto:vincent.x.r...@oracle.com>> wrote:
> Hello,
> 
> Please review this proposal for a new API to conveniently generate and 
> display binary data using hexadecimal string representation.
> It supports both bulk and stream operations and it can also generate the 
> well-known hexdump format [1].
> 
> This latest revision addresses review comments provided earlier.
> These include adding methods to support for data supplied in a 
> ByteBuffer, exposing the default formatter implementation as a public
> static and dropping the superfluous 'Hex' term from several method names.
> 
> Thanks
> 
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769 
> <https://bugs.openjdk.java.net/browse/JDK-8170769>
> API: 
> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.06/api/java.base/java/util/Hex.html
>  
> <http://cr.openjdk.java.net/~vinnie/8170769/javadoc.06/api/java.base/java/util/Hex.html>
> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.06/ 
> <http://cr.openjdk.java.net/~vinnie/8170769/webrev.06/>
> 
> 
> 
> [1] https://docs.oracle.com/cd/E88353_01/html/E37839/hexdump-1.html 
> <https://docs.oracle.com/cd/E88353_01/html/E37839/hexdump-1.html>

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks for your review Stuart.
Comments below.


> On 6 Dec 2018, at 02:18, Stuart Marks  wrote:
> 
> Hi Vinnie,
> 
> Roger Riggs wrote:
>>> The 'forEachOrdered' should not be necessary and may raise questions about 
>>> why.
>>> if there's no good reason, use 'forEach’.
> 
> Using forEachOrdered() is necessary. The dumpAsStream() method produces a 
> stream; presumably it has a defined order that's the same as its input. The 
> semantics of Stream.forEach() are extremely relaxed, and in particular it's 
> not required to process stream elements in order, even if the stream is 
> ordered. (In practice this is noticeable if the stream is run in parallel.)
> 
> I'd use forEachOrdered() both in the examples and also where you use it in 
> implementations.

I’ve retained forEachOrdered() throughout.

> 
> The methods that return streams should specify the important characteristics. 
> Probably the ones most important one are that the returned stream is ordered 
> and sequential. For the overloads that take fixed-size input, the resulting 
> stream might also be SIZED.

I’ve updated the javadoc @return tag for the Stream-returning methods.
Let me know if you’d prefer the stream characteristics to also appear in the 
method’s first sentence.

> 
> I'm not convinced that the overloads that send output to an OutputStream pull 
> their weight. They basically wrap the OutputStream in a PrintStream, which 
> conveniently doesn't declare IOException, making it easy to use from a lambda 
> passed to forEachOrdered(). If an error writing the output occurs, this is 
> recorded by the PrintStream wrapper; however, the wrapper is then thrown 
> away, making it impossible for the caller to check its error status.

The intent is to support a trivial convenience method call that generates the 
well-known hexdump format.
Especially for users that are interested in the hexdump data rather than the 
low-level details of how to terminate a stream.
The dumpAsStream methods are available to support cases that differ from that 
format.

Have you a suggestion to improve the dump() methods, or you’d like to see them 
omitted?

> 
> The PrintStream wrapper also uses the platform default charset, and doesn't 
> provide any way for the caller to override the charset.

Is there a need for that? Originally the requirement was driven by the hexdump 
format which is ASCII-only.
Recently the class has been enhanced to also support the printable characters 
from ISO 8859-1.
A custom formatter be supplied to dumpAsStream() to cater for all other cases?

> 
> Instead, you can just provide the Stream-returning methods, and let the user 
> send the output to a PrintStream using forEachOrdered() as in your examples.
> 
> It might be nice to provide convenience APIs to send output elsewhere, but 
> the problem is that it seems difficult to do so without losing control over 
> things like error handling or charsets. In particular, since the hex 
> formatter is producing strings, it seems like there should be an option to 
> send the output to a Writer. Unfortunately it's difficult to do so from a 
> Stream, because all the Writer methods throw IOException. However, solving 
> this isn't hexdump's problem.

It’s a little more effort but those cases can always be handled by a custom 
formatter.


> 
> s'marks

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

is, 16, (o, c, s, e) -> {
> sb.setLength(indent.length());
> for (int i = s; i < e; i++) {
> sb.append(c[i]);
> sb.append(", ");
> }
> sb.append(System.lineSeparator());
> return sb.toString();
> }).forEach(s -> System.out.print(s));
> }
> System.out.print("}\n");
> 
> 
> On 11/23/2018 09:51 AM, Vincent Ryan wrote:
>> Hello, 
>> 
>> Please review this proposal for a new API to conveniently generate and 
>> display binary data using hexadecimal string representation. 
>> It supports both bulk and stream operations and it can also generate the 
>> well-known hexdump format [1]. 
>> 
>> This latest revision addresses review comments provided earlier. 
>> These include adding methods to support for data supplied in a ByteBuffer, 
>> exposing the default formatter implementation as a public 
>> static and dropping the superfluous 'Hex' term from several method names. 
>> 
>> Thanks 
>> 
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769 
>> <https://bugs.openjdk.java.net/browse/JDK-8170769> 
>> API: 
>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.06/api/java.base/java/util/Hex.html
>>  
>> <http://cr.openjdk.java.net/~vinnie/8170769/javadoc.06/api/java.base/java/util/Hex.html>
>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.06/ 
>> <http://cr.openjdk.java.net/~vinnie/8170769/webrev.06/> 
>> 
>> 
>>  
>> [1] https://docs.oracle.com/cd/E88353_01/html/E37839/hexdump-1.html 
>> <https://docs.oracle.com/cd/E88353_01/html/E37839/hexdump-1.html> 
> 

RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Hello,

Please review this proposal for a new API to conveniently generate and 
display binary data using hexadecimal string representation.
It supports both bulk and stream operations and it can also generate the 
well-known hexdump format [1].


This latest revision addresses review comments provided earlier.
These include adding methods to support for data supplied in a 
ByteBuffer, exposing the default formatter implementation as a public

static and dropping the superfluous 'Hex' term from several method names.

Thanks


Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
API: 
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.06/api/java.base/java/util/Hex.html

Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.06/



[1] https://docs.oracle.com/cd/E88353_01/html/E37839/hexdump-1.html

Re: [11] RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

> On 10 May 2018, at 01:08, Weijun Wang <weijun.w...@oracle.com> wrote:
> 
> 
> 
>> On May 10, 2018, at 6:49 AM, Vincent Ryan <vincent.x.r...@oracle.com> wrote:
>> 
>>> 
>>> - As Max observes, being able to supply the delimiters might be a good 
>>> addition.  (I'm thinking IP addresses too).
>> 
>> Sure. Add another toHexString method that takes a delimiter character?
> 
> Good idea.
> 
> Still, I am not sure about all those methods taking InputStream as an input. 
> Most likely I would use this class for debugging purpose, so any side effect 
> (advancing inside an InputStream or ByteBuffer) might not be desirable. 
> 
> Another question, I just noticed that sun.security.HexDumpEncoder emits 
> uppercase letters (ABCDEF) and your code emits lowercase letters. Do we need 
> to provide user a choice here?

The hexdump format uses lowercase but it is trivial to generate uppercase using 
a custom formatter.


> 
> Thanks
> Max
> 

Re: [11] RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks for your comments Max.

> On 9 May 2018, at 03:34, Weijun Wang <weijun.w...@oracle.com> wrote:
> 
> Nice tool.
> 
> However, I am not sure how toFormattedHexString() and toPrintableString() are 
> useful, seems only for providing a customizable dump format which is, 
> actually, not very customizable.

These provide implementations only to generate the human-readable hexdump 
format. 
They are not designed to be customizable. Custom formats should be supplied via 
Hex.Formatter.


> 
> For me, toHexString and fromHexString are of course the most useful methods. 
> As for dump, I can only think of
> 
> 1. The existing sun.security.HexDumpEncoder format, when I want to dump a lot 
> of bytes as a block
> 2. "00:11:22:33:AA:BB:CC" which fits in one line and also easy to read, when 
> I want inline debugging output
> 
> If the customizable dump method is both powerful and simple enough to create 
> 2) above, I'll be happy. Otherwise, I can live with 
> toHexString().replaceAll("(..)(?=.)", "$1:”).

How about adding another toHexString method that takes a delimiter character?

> 
> Thanks
> Max
> 
>> On May 4, 2018, at 4:22 AM, Vincent Ryan <vincent.x.r...@oracle.com> wrote:
>> 
>> Hello,
>> 
>> Please review this proposal for a new API to conveniently generate and 
>> display binary data using hex string representation.
>> It supports both bulk and stream operations and it can also generate the 
>> well-known hexdump format [1].
>> 
>> Thanks
>> 
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>> API: 
>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.05/api/java.base/java/util/Hex.html
>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.05/
>> 
>> 
>> [1] https://docs.oracle.com/cd/E86824_01/html/E54763/hexdump-1.html
> 

Re: [11] RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks Roger for your comments.

The main motivator for this class is to provide a basic hex. encoder/decoder 
for smaller amounts of binary data
and to provide a hexdump encoder for larger amounts of binary data, while 
recognising the need to cater for
custom formats too.

The class does not attempt to satisfy every custom format. Instead it provides 
some basic formatting methods
and functions from which a wide variety of custom formats can be constructed. 
Developers can also write their
own formatting functions to use with this class.

More comments in-line below.


> On 9 May 2018, at 16:20, Roger Riggs <roger.ri...@oracle.com> wrote:
> 
> Hi Vinnie,
> 
> On the API and spec, a few comments:
> 
>  - Expanding the printable string from ASCII to ISO-8859-1 would make it a 
> bit more useful in more cases.
>That might suggest using the Charset converter to do the work (less 
> optimized but more functional).

Yes adding support for additional charsets could be useful but it is a tradeoff 
against cluttering the API.
I think ASCII is sufficient for the common case.


> 
> - There is no API support for ByteBuffers, another common source of bytes, 
> that would make a good addition
>for completeness.  John Rose suggested a ByteSequence interface in the 
> context of file processing but
>that hasn't settled down.

Support for byte buffers was requested before and I suggested wrapping in an 
I/O stream.
Is that acceptable?


> 
> - The class name "Hex" might be a bit more evocative as HexDump or 
> HexConverter.

I tried to keep the class name short as many of its method names are long.


> 
> - Method names;  the "Hex" in method names might be unnecessary/redundant 
> since, as static methods,
>   they would frequently appear in code as "Hex.fromHexString" and a simple 
> "Hex.fromString" would be fine.
>   Ditto,  toHexString(bytes) -> toString(bytes)…

I agree the repetition is ugly. Shortening to fromString() and toString() is 
appealing except for the possible
confusion with Object.toString


> 
> - There are not many forms that allow the formatter to be supplied, for 
> example, dump(in, out) might be
>   a case where a formatter would be desired.

The 3 dump methods are just convenience functions around the stream generators. 
The dumpAsStream
methods are the expected entry points for customizers.


> 
> - Hex.Formatter interface could have a default method that provides the 
> default formatting or as
>   a static method so it can be used with a method reference.

Sure. What do you suggest?


> 
> - On the example in the class javadoc, I would use the implementation of the 
> default formatter with both hex and ascii
>   to show how that works.

Can you give me an example?


> 
>  - As Max observes, being able to supply the delimiters might be a good 
> addition.  (I'm thinking IP addresses too).

Sure. Add another toHexString method that takes a delimiter character?


> 
> It looks quite good and very useful.
> 
> Thanks, Roger
> 
> On 5/8/2018 10:34 PM, Weijun Wang wrote:
>> Nice tool.
>> 
>> However, I am not sure how toFormattedHexString() and toPrintableString() 
>> are useful, seems only for providing a customizable dump format which is, 
>> actually, not very customizable.
>> 
>> For me, toHexString and fromHexString are of course the most useful methods. 
>> As for dump, I can only think of
>> 
>> 1. The existing sun.security.HexDumpEncoder format, when I want to dump a 
>> lot of bytes as a block
>> 2. "00:11:22:33:AA:BB:CC" which fits in one line and also easy to read, when 
>> I want inline debugging output
>> 
>> If the customizable dump method is both powerful and simple enough to create 
>> 2) above, I'll be happy. Otherwise, I can live with 
>> toHexString().replaceAll("(..)(?=.)", "$1:”).
>> 
>> Thanks
>> Max
>> 
>>> On May 4, 2018, at 4:22 AM, Vincent Ryan <vincent.x.r...@oracle.com> wrote:
>>> 
>>> Hello,
>>> 
>>> Please review this proposal for a new API to conveniently generate and 
>>> display binary data using hex string representation.
>>> It supports both bulk and stream operations and it can also generate the 
>>> well-known hexdump format [1].
>>> 
>>> Thanks
>>> 
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>>> API: 
>>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.05/api/java.base/java/util/Hex.html
>>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.05/
>>> 
>>> 
>>> [1] https://docs.oracle.com/cd/E86824_01/html/E54763/hexdump-1.html
> 

[11] RFR: 8170769 Provide a simple hexdump facility for binary data

[Vincent Ryan]

Hello,

Please review this proposal for a new API to conveniently generate and display 
binary data using hex string representation.
It supports both bulk and stream operations and it can also generate the 
well-known hexdump format [1].

Thanks


Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
API: 
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.05/api/java.base/java/util/Hex.html
Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.05/


[1] https://docs.oracle.com/cd/E86824_01/html/E54763/hexdump-1.html

Re: Charsets for hex/base64

[Vincent Ryan]

FYI here’s the javadoc for a draft of the Hex API that Alan mentioned below:
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.05/api/java.base/java/util/Hex.html

Thanks.


> On 2 May 2018, at 10:55, Jonas Konrad  wrote:
> 
> I did not know about the old HexDumpEncoder. It extends an internal class 
> `CharacterEncoder` which seems to be pretty similar purpose-wise to what I am 
> suggesting with CharsetEncoder. There is also the good old 
> `DatatypeConverter.printHexBinary`, though it can't stream.
> 
> But this is not really what I mean. The interesting part about doing this as 
> a charset is the unified API aspect of it, the fact that you can reuse 
> existing code utilizing Charset for hex operations.
> 
> - Jonas
> 
> On 05/02/2018 11:44 AM, Alan Bateman wrote:
>> On 02/05/2018 09:35, Jonas Konrad wrote:
>>> Hi,
>>> 
>>> Conceptually, a 'charset' (in java) is a pair of transformations from bytes 
>>> to utf-16 code units and vice versa. Could it be useful to have charset 
>>> implementations that convert from bytes to the hex (or base64) 
>>> representations of those? The idea is as follows:
>>> 
>>> "0a0b0c".getBytes(HexCharset.getInstance()) = new byte[] { 0x0a, 0x0b, 0x0c 
>>> }
>>> new String(new byte[] { 0x0a, 0x0b, 0x0c }, HexCharset.getInstance()) = 
>>> "0a0b0c"
>>> 
>>> The motivation behind this idea is that there are lots of APIs that provide 
>>> efficient transformations between chars and bytes using charsets, but 
>>> converting to/from hex for debugging is usually more involved. One example 
>>> of this is netty ByteBuf.toString ( 
>>> https://netty.io/4.0/api/io/netty/buffer/ByteBuf.html#toString-int-int-java.nio.charset.Charset-
>>>  ) - it would be really convenient to be able to just plug in a hex charset 
>>> to get nice debug output.
>>> 
>>> Of course this is only one example and there are other ways to get hex 
>>> output from ByteBuf in netty in particular, but I still think that a hex 
>>> charset would be an interesting tool to reuse code. I understand this is a 
>>> somewhat dubious use of a Charset, but I'd like to hear other thoughts on 
>>> it.
>>> 
>> Have you looked at the Hex API proposed in JDK-8170769? It needs to be 
>> dusted off but the last iteration discussed here got to the point where it 
>> provides conversion of binary data to/from hex string representation. It 
>> also supported both bulk and stream operations.
>> -Alan

Re: [9] RFR 8170769: Provide a simple hexdump facility for binary data

[Vincent Ryan]

Unfortunately this feature has arrived a little too late so I’m withdrawing it 
from consideration for JDK 9.
Thanks again to those who took time to review it.


> On 10 Dec 2016, at 01:02, Vincent Ryan <vincent.x.r...@oracle.com> wrote:
> 
> Thanks to those who provided review comments.
> 
> I have incorporated most of them and updated the webrev:
>http://cr.openjdk.java.net/~vinnie/8170769/webrev.01/
> 
> The main changes are to tighten up the javadoc spec and to add three 
> additional methods
> to support byte array ranges. The total number of public methods is now 7.
> 
> I have not added support for InputStream or ByteBuffer. I think what we have 
> in place is
> sufficient for now and HexDump can be enhanced later if there is demand.
> 
> The other issue is related to streams implementation and I’d like to defer 
> that until later.
> 
> 
> I think the API is in good shape now but please let me know if there are any 
> must-fix API issues.
> Otherwise I will consider the API finalised and move this forward.
> 
> Thanks.
> 

Re: [9] RFR 8170769: Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks to those who provided review comments.

I have incorporated most of them and updated the webrev:
http://cr.openjdk.java.net/~vinnie/8170769/webrev.01/

The main changes are to tighten up the javadoc spec and to add three additional 
methods
to support byte array ranges. The total number of public methods is now 7.

I have not added support for InputStream or ByteBuffer. I think what we have in 
place is
sufficient for now and HexDump can be enhanced later if there is demand.

The other issue is related to streams implementation and I’d like to defer that 
until later.


I think the API is in good shape now but please let me know if there are any 
must-fix API issues.
Otherwise I will consider the API finalised and move this forward.

Thanks.

Re: [9] RFR 8170769: Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks for your review comments Paul.
Responses below.


> On 8 Dec 2016, at 20:08, Paul Sandoz <paul.san...@oracle.com> wrote:
> 
> Hi,
> 
> It may take a few iterations to get the API settled.
> 
> There are other byte sources we may want to consider such as InputStream and 
> ByteBuffer.

I’d prefer to keep this class simple for now but I’m happy to add additional 
methods later if there is demand.

Adding support for InputStream is awkward because of its blocking nature.
It’s simpler to allow the user to handle that aspect, populate a byte array, 
and present it to the HexDump methods.

Some ByteBuffers can be converted to a byte[] without the penalty of a copy 
operation.
If performance becomes an issue then we can re-visit supporting ByteBuffer.


> 
> Comments below.
> 
> Paul.
> 
>> On 7 Dec 2016, at 08:32, Vincent Ryan <vincent.x.r...@oracle.com> wrote:
>> 
>> A hexdump facility has been available for many, many years via an 
>> unsupported class: sun.misc.HexDumpEncoder.
>> Although that class was always unsupported, it was still accessible. That 
>> accessibility changes with Jigsaw so I’m proposing
>> a very simple replacement in a new and supported class: java.util.HexDump.
>> 
>> Thanks.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.00/
>> 
> 
>  55 if (bytes == null) {
>  56 throw new NullPointerException();
>  57 }
> 
> Objects.requireNonNull

OK

> 
> 
>  78  * @throws IllegalArgumentException if {@code hexString} has an odd 
> number
>  79  * of digits
>  80  * @throws NullPointerException if {@code hexString} is {@code null}
>  81  */
>  82 public static byte[] fromHexString(String hexString) {
> 
> This should accept a CharSequence, plus also have bounded range equivalent.

toHexString and fromHexString are expected to operate on reasonably short byte 
arrays.

I have added range equivalents for dump and dumpToStream where longer byte 
arrays
are expected.


> 
> Also throws IAE if the hexString contains non-convertible characters.
> 

OK

> 
> 106 /**
> 107  * Generates a stream of hexadecimal strings, in 16-byte chunks,
> 108  * from the contents of a binary buffer.
> 109  *
> 110  * @param bytes a binary buffer
> 111  * @return a stream of hexadecimal strings
> 112  * @throws NullPointerException if {@code bytes} is {@code null}
> 113  */
> 114 public static Stream dumpToStream(byte[] bytes) {
> 115 if (bytes == null) {
> 116 throw new NullPointerException();
> 117 }
> 118 return StreamSupport.stream(
> 119 Spliterators.spliteratorUnknownSize(
> 120 new HexDumpIterator(bytes),
> 121 Spliterator.ORDERED | Spliterator.NONNULL),
> 122 false);
> 123 }
> 
> I suspect there is no need for a HexDumpIterator and you can do:
> 
>  return IntStream.range(0, roundUp(bytes.length, 16)).mapToObject(…);
> 
> and in the map function take care of the trailing bytes based on the byte[] 
> length. That’s potentially simple enough there might be no need for a stream  
> method. Where it gets more complicated is if the source is of unknown size 
> such as InputStream, where the stream returning method probably has more 
> value.
> 
> Ideally what we want to return is Stream<[Long, String]>, then it’s really 
> easy for others for format, but alas the current form would be ugly and 
> inefficient. So i suspect the dump method has some legs but it’s real value 
> may be for a fully streaming solution.
> 
> 
> 140 public static String dump(byte[] bytes) {
> 141 if (bytes == null) {
> 142 throw new NullPointerException();
> 143 }
> 144
> 145 int[] count = { -16 };
> 146 return dumpToStream(bytes).collect(Collector.of(
> 147 StringBuilder::new,
> 148 (stringBuilder, hexString) ->
> 149 stringBuilder
> 150 .append(String.format("%08x  %s%n",
> 151 (count[0] += CHUNK_LENGTH),
> 152 explode(hexString))),
> 153 StringBuilder::append,
> 154 StringBuilder::toString));
> 155 }
> 
> The encoding of the count and exploding could also append directly into the 
> main string builder, reducing memory churn.
> 
> And i think you can also start from IntStream.range(0, roundUp(bytes.length, 
> 16)) avoiding the count state.

Thanks for those suggested improvements. They look good but I'd prefer to make 
implementation changes in a follow-up.


> 
> 
> 192 if (b < ' ' || b > '~') {
> 193 sb.append(".”);
> 
> Use ‘.’

OK

> 
> 
> 194 } else {
> 195 sb.append(new String(new byte[]{ b }));
> 
> Cast the byte to a char instead.

OK

> 
> 
> 196 }
> 
> 
> 
> 
> 
> 
> 
> 
> 

Re: [9] RFR 8170769: Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks for your review comments Alan.
Responses below.


> On 8 Dec 2016, at 19:44, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 07/12/2016 16:32, Vincent Ryan wrote:
> 
>> A hexdump facility has been available for many, many years via an 
>> unsupported class: sun.misc.HexDumpEncoder.
>> Although that class was always unsupported, it was still accessible. That 
>> accessibility changes with Jigsaw so I’m proposing
>> a very simple replacement in a new and supported class: java.util.HexDump.
>> 
>> Thanks.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.00/
> I'm happy to see a proposal for an API in Java SE for this, it's always sad 
> to bump into code that is using a utility method exposed in java.xml.bind or 
> the JDK internal hex dumper.

Yes it’s long overdue.

> 
> I've skimmed the API. The class name and package look okay. I assume you can 
> make HexDump final (I realize the ctor is private).

OK.

> 
> The toHexString/fromHexString methods are really useful but I think the 
> javadoc needs to be fleshed out a bit. For example, fromHexString doesn't 
> make it clear whether the A-F need to be in a specific case, the IAE 
> description doesn't make it clear that it is thrown when there is invalid 
> input. The description of toHexString can be clearer on the length of the 
> returned string, the reader might wonder if there is a "0x" prepended for 
> example.

OK.

> 
> I like dump(byte[]) but I could imagine calls to "customize" the returned 
> string in many ways. After reading dumpToStream then I wonder if it might be 
> better to drop dump and let users do their own layout. If you keep it then 
> the javadoc needs work - the reader will have many questions. What is 
> "non-printable", what is the character in the output that splits the lines, 
> how is the last line handled when the input is not a multiple of 16 bytes. 
> Given dumpToStream then I wonder if might be better to drop this method.

The behaviour of the dump method can also be achieved by manipulating the 
stream generated by the dumpToStream
method but that requires additional programming. The dump method does all that 
work for you so I’d prefer to keep it as a convenience method. 

> 
> dumpToStream(byte[]) is interesting too but again needs a lot more javadoc. I 
> assume the spliterator will be replaced eventually as the size is known so 
> you can do a good implementation.

Yes. Paul has provided guidance on implementation details that and I will pick 
that up later once this class has been integrated.

> 
> It would be useful to put in some @see references from methods like 
> Integer.toHexString.

Where? Just on toHexString ?

> 
> I don't have comments on the implementation/test at this time.
> 
> -Alan.

Re: [9] RFR 8170769: Provide a simple hexdump facility for binary data

[Vincent Ryan]

> On 8 Dec 2016, at 12:10, Vincent Ryan <vincent.x.r...@oracle.com> wrote:
> 
> 
>> On 8 Dec 2016, at 07:22, David Holmes <david.hol...@oracle.com> wrote:
>> 
>> On 8/12/2016 2:32 AM, Vincent Ryan wrote:
>>> A hexdump facility has been available for many, many years via an 
>>> unsupported class: sun.misc.HexDumpEncoder.
>>> Although that class was always unsupported, it was still accessible. That 
>>> accessibility changes with Jigsaw so I’m proposing
>>> a very simple replacement in a new and supported class: java.util.HexDump.
>>> 
>>> Thanks.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.00/
>> 
>> googling for "hexdump java" reveals quite a few hits - including 
>> javax.xml.bind.DatatypeConverter.
> 
> DatatypeConverter is no longer directly accessible in JDK 9 as it is in the 
> java.xml.bind module.
> So an ‘—add-modules’ flag would be required.
> 
> 
>> 
>> Does this sun.* class really meet the acceptance criteria for inclusion in 
>> java.util? That bar is intentionally set very high.
> 
> We considered other locations for these methods (including java.lang.String 
> !) but settled on java.util.HexString
s/HexString/HexDump

> as a good balance between visibility and controversy.
> 
> 
>> 
>> Just wondering ...
>> 
>> David
>> 
> 

Re: [9] RFR 8170769: Provide a simple hexdump facility for binary data

[Vincent Ryan]

> On 8 Dec 2016, at 07:22, David Holmes <david.hol...@oracle.com> wrote:
> 
> On 8/12/2016 2:32 AM, Vincent Ryan wrote:
>> A hexdump facility has been available for many, many years via an 
>> unsupported class: sun.misc.HexDumpEncoder.
>> Although that class was always unsupported, it was still accessible. That 
>> accessibility changes with Jigsaw so I’m proposing
>> a very simple replacement in a new and supported class: java.util.HexDump.
>> 
>> Thanks.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.00/
> 
> googling for "hexdump java" reveals quite a few hits - including 
> javax.xml.bind.DatatypeConverter.

DatatypeConverter is no longer directly accessible in JDK 9 as it is in the 
java.xml.bind module.
So an ‘—add-modules’ flag would be required.


> 
> Does this sun.* class really meet the acceptance criteria for inclusion in 
> java.util? That bar is intentionally set very high.

We considered other locations for these methods (including java.lang.String !) 
but settled on java.util.HexString
as a good balance between visibility and controversy.


> 
> Just wondering ...
> 
> David
> 

Re: [9] RFR 8170769: Provide a simple hexdump facility for binary data

[Vincent Ryan]

> On 8 Dec 2016, at 10:41, Ulf Zibis <ulf.zi...@cosoco.de> wrote:
> 
> Hi,
> 
> I would prefer a "normal" class instead a convolut of static methods. Via a 
> normal constructor, we could pass some custom parameters e.g. 
> capital/uppercase letters for "abcdef", prefix a header line, width of the 
> index counter, bytes per line, i.e. have all the parameters, you have 
> hardcoded, variable.

The dumpToStream method is designed to cater for all forms of customisation.
You can see in the implementation that the dump method simply calls 
dumpToStream with a custom collector.

I think that approach is more flexible than a constructor taking a fixed set of 
parameters.


> 
> Additionally I would like to see a method with variable start and end:
> 
> String dump(byte[] bytes, int start, int length)
> 

That makes sense, I will add that. And  matching one for dumpToStream ?


> 
> -Ulf
> 
> Am 07.12.2016 um 17:32 schrieb Vincent Ryan:
>> A hexdump facility has been available for many, many years via an 
>> unsupported class: sun.misc.HexDumpEncoder.
>> Although that class was always unsupported, it was still accessible. That 
>> accessibility changes with Jigsaw so I’m proposing
>> a very simple replacement in a new and supported class: java.util.HexDump.
>> 
>> Thanks.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.00/
>> 
>> 
> 

Re: [9] RFR 8170769: Provide a simple hexdump facility for binary data

[Vincent Ryan]

> On 8 Dec 2016, at 09:34, Volker Simonis <volker.simo...@gmail.com> wrote:
> 
> Hi Vincent,
> 
> the bug is closed and can't be looked at outside Oracle.
> Can you please make it visible for everybody.

Sorry. I’ve just corrected that.

> 
> Also, this is a change of the public API so it requires a CCC 
> request/approval.

I’ve a CCC prepared which I will submit if there is consensus on the proposed 
API.


> 
> Finally, this seems to be clearly a feature and not a bug so it
> requires an FC extension request [1]

Right. I’ve begun that too.


> 
> Thanks,
> Volker
> 
> [1] http://openjdk.java.net/projects/jdk9/fc-extension-process
> 
> On Wed, Dec 7, 2016 at 5:32 PM, Vincent Ryan <vincent.x.r...@oracle.com> 
> wrote:
>> A hexdump facility has been available for many, many years via an 
>> unsupported class: sun.misc.HexDumpEncoder.
>> Although that class was always unsupported, it was still accessible. That 
>> accessibility changes with Jigsaw so I’m proposing
>> a very simple replacement in a new and supported class: java.util.HexDump.
>> 
>> Thanks.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.00/
>> 

[9] RFR 8170769: Provide a simple hexdump facility for binary data

[Vincent Ryan]

A hexdump facility has been available for many, many years via an unsupported 
class: sun.misc.HexDumpEncoder.
Although that class was always unsupported, it was still accessible. That 
accessibility changes with Jigsaw so I’m proposing
a very simple replacement in a new and supported class: java.util.HexDump.

Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.00/

Re: Process to add some default methods to javax.naming.Context?

[Vincent Ryan]

Please file an RFE for this issue so that the core-libs team can evaluate.
While this seems like a good use for default methods, it may be difficult to 
justify including a convenience method
at this late stage in the JDK 9 schedule.

Thanks.


> On 14 Oct 2016, at 22:21, Laird Nelson <ljnel...@gmail.com> wrote:
> 
> On Fri, Oct 14, 2016 at 7:19 AM Vincent Ryan <vincent.x.r...@oracle.com 
> <mailto:vincent.x.r...@oracle.com>> wrote:
> The problem of having to cast the result of Context.lookup 
> <http://download.java.net/java/jdk9/docs/api/javax/naming/Context.html#lookup-javax.naming.Name->
>  can be solved using an existing method:
> InitialContext.doLookup 
> <http://download.java.net/java/jdk9/docs/api/javax/naming/InitialContext.html#doLookup-javax.naming.Name->
> 
> That solves the simple casting problem from an initial Context, but not for 
> all Contexts, and the simple addition of a relatively trivial default method 
> like this in the Context interface it seems to me would open the door for 
> Context implementers to reimplement it to do more sophisticated—and 
> deliberate—type conversion.
> 
> Best,
> Laird

Re: Process to add some default methods to javax.naming.Context?

[Vincent Ryan]

Hello Laird,

The problem of having to cast the result of Context.lookup 

 can be solved using an existing method:
InitialContext.doLookup 


Although it might not suit all uses because no JNDI environment properties are 
passed to the InitialContext
constructor that gets created within the method.



> On 13 Oct 2016, at 18:14, Laird Nelson  wrote:
> 
> Hello; I am new to this mailing list and its conventions; if I misstep I am
> happy to be pointed in the right direction.  I was directed here by Mark
> Reinhold.
> 
> Disclaimer in case it matters: I work for Oracle.
> 
> I have long wanted there to be methods in javax.naming.Context like the
> following:
> 
> // typed from memory off the cuff
> default  T lookup(final String name, final Class type) {
>  return type.cast(this.lookup(name));
> }
> // do similar things for other lookup* methods here
> 
> What would be the steps I would next need to take to help with this?
> 
> Thanks in advance for help, time and guidance,
> Best,
> Laird
> --
> http://about.me/lairdnelson

Re: RFR : 8139965:Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Vincent Ryan]

IIRC pausing the receiver was introduced to prevent an LDAP client (with 
limited memory) from being overwhelmed
by server results and having no opportunity to transmit an abandon request. The 
80% value was chosen to balance
maximising queue capacity against the risk of an OOME.

If that behaviour is maintained via a BlockingQueue then all is well.


> On 5 Aug 2016, at 16:56, Roger Riggs  wrote:
> 
> Hi Sean,
> 
> I agree that the 80% reflects the previous coding and is ok.
> 
> I don't have enough background on this code to know whether the 80% is now 
> spurious and confusing and could be removed.
> 
> Roger
> 
> 
> On 8/5/2016 11:46 AM, Seán Coffey wrote:
>> Sorry - cc'ed the wrong Pavel earlier. Corrected.
>> 
>> I thought the consumer/producer model was best served by delegating to the 
>> BlockingQueue. Is there a need to synchronize as a result ?
>> 
>> The 80% logic is only implemented in the rare case where the 
>> com.sun.jndi.ldap.search.replyQueueSize ldap context property is set. It 
>> seems to be legacy behaviour from the old code where the reader thread was 
>> paused at 80% capacity. Setting the BlockingQueue to the same '80%' size 
>> should have the same net effect.
>> 
>> Regards,
>> Sean.
>> 
>> On 05/08/16 15:50, Roger Riggs wrote:
>>> Hi Sean,
>>> 
>>> Looks like a cleaner solution to synchronize writer and readers.
>>> 
>>> I don't quite understand the 80% capacity value.  It is related to the 
>>> obsolete highWatermark
>>> but that does not seem relevant with the update.
>>> 
>>> If the caller is going to specify a replyQueueCapacity then why should it 
>>> be downgraded to 80%?
>>> 
>>> Roger
>>> 
>>> 
>>> 
>>> On 8/5/2016 10:05 AM, Seán Coffey wrote:
 Hoping to get a review on this issue that's been sitting on my plate for a 
 long while. Pavel drew up the bulk of the edits for this one (Thanks)
 
 The fix basically delegates polling and timeout management to the 
 BlockingQueue.poll(timeout.. ) method. As a result it makes Connection 
 readReply logic much easier to handle.
 
 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8139965.9/webrev/
 bug report : https://bugs.openjdk.java.net/browse/JDK-8139965
 
>>> 
>> 
> 

Re: RFR: 8132455 - com/sun/jndi/ldap/LdapTimeoutTest.java fails at handleNamingException

[Vincent Ryan]

That looks fine for now.
Thanks.

> On 11 Nov 2015, at 15:11, Rob McKenna  wrote:
> 
> So unfortunately this isn't a real fix for this failing test. (intermittent, 
> certain platforms only) However since I have enough information to 
> investigate this problem separately I'd like to silence this specific failure 
> in the mean time. I'll investigate the actual failure under 
> https://bugs.openjdk.java.net/browse/JDK-8141370
> 
> http://cr.openjdk.java.net/~robm/8132455/webrev.01/
> 
>   -Rob

Re: RFR: 8129957 - Deadlock in JNDI LDAP implementation when closing the LDAP context

[Vincent Ryan]

Your proposed fix looks fine Rob.
Thanks.


> On 14 Sep 2015, at 16:25, Rob McKenna  wrote:
> 
> Hi folks,
> 
> So on further investigation it looks like we could get away with reducing the 
> amount of locking in LdapClient. Here is a proposed fix followed by a 
> description:
> 
> http://cr.openjdk.java.net/~robm/8129957/webrev.02/
> 
> processConnectionClosure():
> - Remove the synchronization from processConnectionClosure and handle it 
> further down in notifyUnsolicited
> 
> removeUnsolicited():
> - Remove the synchronized block in removeUnsolicited as its redundant. 
> Vectors are synchronized already.
> 
> processUnsolicited()
> - Remove the initial synchronized block in processUnsolicited and limit it to 
> the area around the unsolicited size check / notice creation. (again, due to 
> the notifyUnsolicited changes)
> - Remove the redundant unsolicited.size check from the end of 
> processUnsolicited
> 
> notifyUnsolicited():
> - Synchronize on the unsolicited vector in order to create a copy of that 
> vector and empty it if e is a NamingException.
> - Outside the notifyUnsolicited synchronize block, loop through the copy of 
> unsolicited and call fireUnsolicited on each element.
> - The main potential problem with this fix would be if an LdapCtx became 
> unsolicited before we got to it in the for loop. However since both 
> LdapCtx.fireUnsolicited and LdapCtx.removeUnsolicited sync on eventSupport 
> and LdapCtx.fireUnsolicited double checks to make sure it is still 
> unsolicited, that should be fine.
> 
>   -Rob
> 
> 
> On 10/08/15 14:06, Rob McKenna wrote:
>> Hi folks,
>> 
>> We have a hang between LdapClient / Ctx due to the fact that
>> Connection.cleanup() calls LdapClient.processConnectionClosure which
>> locks the unsolicited vector and in turn calls LdapCtx.fireUnsolicited
>> which locks the eventSupport object. Unfortunately when an
>> LdapCtx.close() occurs at the same time, its removeUnsolicited method
>> locks the eventSupport object first and then attempts to call
>> LdapClient.removeUnsolicited which attempts to lock the unsolicited vector.
>> 
>> So:
>> 
>> thread 1:
>> 
>> Connection.cleanup ->
>> LdapClient.processConnectionClosure (LOCK VECTOR) ->
>> LdapCtx.fireUnsolicited (LOCK EVENTSUPPORT)
>> 
>> (LdapClient is looping through LdapCtx objects in the unsolicited vector)
>> 
>> thread 2:
>> 
>> LdapCtx.close (LOCK LDAPCTX) ->
>> LdapCtx.removeUnsolicited (LOCK EVENTSUPPORT) ->
>> LdapClient.removeUnsolicited (LOCK VECTOR)
>> 
>> (A single LdapCtx removes itself from its LdapClient unsolicited list)
>> 
>> 
>> My proposed solution is to have both threads lock the LdapClient before
>> locking either the unsolicited vector or the eventSupport object.
>> 
>> Webrev at: http://cr.openjdk.java.net/~robm/8129957/webrev.01/
>> 
>> -Rob

Re: RFR (XS) 8074761: Empty optional parameters of LDAP query are not interpreted as empty

[Vincent Ryan]

Your fix looks good to me. Thanks for contributing the patch.


 On 25 Mar 2015, at 14:47, Stanislav Baiduzhyi baiduzhyi.de...@gmail.com 
 wrote:
 
 On Tuesday 10 March 2015 17:18:06 Stanislav Baiduzhyi wrote:
 Hi All,
 
 Please review the patch for LdapURL. Patch fixes the parsing of query part
 of LDAP URL, leaving empty optional fields as null instead of .
 
 Webrev:
 http://goo.gl/OO0V38
 
 Bug:
 https://bugs.openjdk.java.net/browse/JDK-8074761
 
 JTreg:
 http://goo.gl/ermmoh
 
 Details:
 
 RFC 2255 [1] allows any of the query parameters to be empty. Current
 implementation of parsing method extracts substring without checking for
 length, leaving empty fields as  instead of null. But the code under
 com.sun.jndi.ldap package checks only for null when handling optional
 fields. So the patch modifies the parsing method to avoid substring
 operations on empty fields and leaving them as null instead.
 
 In proposed patch, I was not able to generalize the code, so using similar
 code blocks to make it obvious if additional changes will be required later.
 
 It would be even better to use java.util.Optional for this, but it will be
 compatibility-breaking change, I'm not sure it worth doing even under
 com.sun.* packages.
 
 There is wider test case in RedHat Bugzilla [2], includes OpenDS setup and
 small client app, shows the difference in results between openldap-clients
 with Java-based implementation. Proposed patch fixes the java client
 behaviour.
 
 [1]: https://tools.ietf.org/html/rfc2255#section-3
 [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1194226
 
 A small reminder, please review the webrev quoted above, the patch is 
 extremely small, and passes the TCK.
 
 -- 
 Regards,
Stas
 

Re: RFR: 8065238 - javax.naming.NamingException after upgrade to JDK 8

[Vincent Ryan]

Changes look fine to me Rob.
Thanks.


On 5 Dec 2014, at 05:17, Rob McKenna rob.mcke...@oracle.com wrote:

 Just updated the test to workaround a seemingly unrelated platform specific 
 issue. (that only manifests on older kernels)
 
 http://cr.openjdk.java.net/~robm/8065238/webrev.02/
 
-Rob
 
 On 03/12/14 16:21, Rob McKenna wrote:
 Hi folks,
 
 Looking to fix a regression caused by 8042857. Basically the behaviour in 
 8042857 is incorrect. This fix reverts to the previous behaviour and 
 attempts to beef up the tests a little around Ldap timeouts.
 
 http://cr.openjdk.java.net/~robm/8065238/webrev.01/
 
 The test itself looks quite complex but isn't really. There are two executor 
 pools. (scheduled and fixed) The fixed pool is for running tests 
 concurrently. The scheduled pool is for killing tests that test ldap 
 connects / reads where no timeout is set. (according to the spec these 
 should wait forever)
 
 For these long running timeout tests, we schedule a thread to interrupt the 
 test after waiting for 20s. There are 3 of these long running tests in 
 total, hence the decision to run the tests in parallel.
 
 I'm not averse to breaking this out into separate tests and a library for 
 the helper classes if people think it makes more sense to leave the 
 concurrency up to the test framework.
 
-Rob
 
 

Re: RFR JDK-8059311: com/sun/jndi/ldap/LdapTimeoutTest.java fails with exit_code == 0

[Vincent Ryan]

All looks good.
Thanks.


On 7 Nov 2014, at 12:14, Pavel Rappo pavel.ra...@oracle.com wrote:

 Hi everyone,
 
 Could you please review my change for JDK-8059311?
 
 http://cr.openjdk.java.net/~prappo/8059311/webrev.00/
 
 
 It looks like one of the killSwitches went off and executed System.exit(0). 
 The problem is jtreg doesn't like System.exit and treats it as a test failure 
 (http://openjdk.java.net/jtreg/faq.html#question7.1):
 
 ...Tests are not allowed to call System.exit because the test must have the 
 ability to run in the same JVM as the harness. Calling System.exit while the 
 test is running in this manner would cause the harness itself to exit...
 
 Moreover, the purpose of the killSwitch is not clear. It seems to be 
 ensuring the test won't hang by killing JVM after a certain timeout.
 First of all, if the test hangs -- it means we have a problem to investigate 
 and the killSwitch is just masking it. If it doesn't hang -- there's no 
 need for a killSwitch. 
 
 If the test hangs it will be noticed by jtreg 
 (http://openjdk.java.net/jtreg/tag-spec.txt):
 
 .../timeout=seconds
 
 Specify the timeout value.  The default timeout is two minutes.  If an action
 does not finish before the timeout expires, it fails...
 
 Thus if we remove the killSwitch we treat the problem of slow systems 
 (where killSwitch timeout was too small) and at the same time will be 
 notified if test hangs.
 
 Thanks
 -Pavel

Re: RFR JDK-8051991: Flatten VersionHelper hierarchies

[Vincent Ryan]

Look fine Pavel.
Thanks.


On 25 Jul 2014, at 13:59, Pavel Rappo pavel.ra...@oracle.com wrote:

 Hi everyone,
 
 Could you please review my change for JDK-8051991?
 
 http://cr.openjdk.java.net/~prappo/8051991/webrev.00/
 
 -Pavel

Re: RFR JDK-8054158: Fix typos in JNDI-related packages

[Vincent Ryan]

Thanks for fixing those typso.

On 1 Aug 2014, at 16:50, Pavel Rappo pavel.ra...@oracle.com wrote:

 Hi everyone,
 
 Could you please review my change for JDK-8054158?
 
 http://cr.openjdk.java.net/~prappo/8054158/webrev.00/
 
 -Pavel

Re: RFR JDK-8048175: Remove redundant use of reflection on core classes from JNDI

[Vincent Ryan]

The LDAP changes look fine to me.
Thanks.

On 15 Jul 2014, at 15:07, Mark Sheppard mark.shepp...@oracle.com wrote:

 Hi Pavel,
   the changes look ok  I've run some relevant jck tests (naming 
 management rmi) and the CORBA regression suite
 for the changeset, with no perceptible issues
 
 regards
 Mark
 
 On 14/07/2014 12:43, Pavel Rappo wrote:
 Hi everyone,
 
 Could you please review my change for JDK-8048175?
 
 http://cr.openjdk.java.net/~prappo/8048175/webrev.00/
 
 Summary
 
 1. com.sun.jndi.ldap.Connection imports both java.net.InetSocketAddress and 
 javax.net.SocketFactory which are available since 1.4
 2. com.sun.jndi.toolkit.corba.CorbaUtils defines 3 imports from java.rmi and 
 java.rmi.CORBA. Justification: CorbaUtils is used by the 
 com.sun.jndi.cosnaming package which resides in CORBA module. Which in turn 
 has a dependency on RMI module.
 3. com.sun.jndi.cosnaming.RemoteToCorba.getStateToBind method doesn't handle 
 ClassNotFoundException internally. There's no need for that now (see 2).
 4. com.sun.jndi.ldap.VersionHelper instantiates only the 
 com.sun.jndi.ldap.VersionHelper12. Justification: the rudimentary test for 
 jdk version is no longer needed as JNDI is a part of the platform and 
 com.sun.jndi.ldap.VersionHelper11 is no longer available (at least since 
 1.3).
 
 All corresponding reflection usages have been removed.
 
 Thanks
 -Pavel
 

Re: RFR JDK-8049884: Reduce possible timing noise in com/sun/jndi/ldap/LdapTimeoutTest.java

[Vincent Ryan]

This change looks fine Pavel and eliminates another intermittent test failure.
Thanks.


On 11 Jul 2014, at 14:59, Pavel Rappo pavel.ra...@oracle.com wrote:

 Hi everyone,
 
 Could you please review my change for JDK-8049884?
 
 http://cr.openjdk.java.net/~prappo/8049884/webrev.00/
 
 Thanks
 -Pavel

Re: [9] request for review 8047353: Improve error message when a JAR with invalid signatures is loaded

[Vincent Ryan]

Hello Aaron,

I considered using your testcase that manually generates the necessary 
malformed JAR
but as there was a suitable signed JAR already in the test suite I decided to 
re-use that.

I think it makes sense to re-work the test as a Java program. Unfortunately 
I’ll be on vacation
from today but I’ll return to this issue when I get back.

Thanks.



On 20 Jun 2014, at 11:00, Aaron Digulla digu...@hepe.com wrote:

 Am Donnerstag, 19. Juni 2014 23:49 CEST, Joe Darcy joe.da...@oracle.com 
 schrieb:
 
 I'd prefer to see the CheckJarSigError.sh as a Java program.
 
 There original bug report contains a full self-contained test case in Java. 
 Why was that split into several files?
 
 I'm also a bit uneasy about the just show the file name. I have thousands 
 of JARs with the same name on my harddisk (several Maven repos, target 
 folders, you name it). If you strip the path from the error message, then I 
 have to somehow figure out the classpath which was used.
 
 That might work when I run Java from the command line but when I use complex 
 frameworks like OSGi or Maven which do all kinds of magic to determine which 
 JARs they might want to load, then this doesn't help much.
 
 
 At least add a command line option / system property which allows to see the 
 full path.
 
 Regards,
 
 --
 Aaron Optimizer Digulla a.k.a. Philmann Dark
 It's not the universe that's limited, it's our imagination.
 Follow me and I'll show you something beyond the limits.
 http://blog.pdark.de/

[9] request for review 8047353: Improve error message when a JAR with invalid signatures is loaded

[Vincent Ryan]

Please review the following simple changeset to identify the offending JAR file 
following a signature verification error.
Previously, only the offending entry in the JAR was identified.

This helps during troubleshooting when several JAR files being processed.

The request was originally submitted by Aaron Digulla.


Bug: https://bugs.openjdk.java.net/browse/JDK-8047353
Webrev: http://cr.openjdk.java.net/~vinnie/8047353/webrev.00/

Re: [9] request for review 8047353: Improve error message when a JAR with invalid signatures is loaded

[Vincent Ryan]

I shortened the output to display only the JAR filename to avoid leaking 
filesystem information.
I’ve updated the webrev in-place.

Thanks.


On 19 Jun 2014, at 17:59, Vincent Ryan vincent.x.r...@oracle.com wrote:

 Please review the following simple changeset to identify the offending JAR 
 file following a signature verification error.
 Previously, only the offending entry in the JAR was identified.
 
 This helps during troubleshooting when several JAR files being processed.
 
 The request was originally submitted by Aaron Digulla.
 
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8047353
 Webrev: http://cr.openjdk.java.net/~vinnie/8047353/webrev.00/
 
 

Re: RFR JDK-8047062: Improve diagnostic output in com/sun/jndi/ldap/LdapTimeoutTest.java

[Vincent Ryan]

Looks fine Pavel.

On 17 Jun 2014, at 14:06, Pavel Rappo pavel.ra...@oracle.com wrote:

 Hi everyone,
 
 Could you please review my change for JDK-8047062?
 
 http://cr.openjdk.java.net/~prappo/8047062/webrev.00/
 
 Thanks
 -Pavel

Re: RFR: 8044059: Memory Leak in Elliptic Curve Private Key Generation

[Vincent Ryan]

It’s difficult to develop a useful test that is cross-platform so let’s skip 
the testcase for this fix.


On 28 May 2014, at 17:21, Jeremy Manson jeremyman...@google.com wrote:

 You can defer the change if you need to.  I'm going on vacation from the 6th 
 to the 16th, so there are timing issues around that.
 
 At Google, we plug in a different version of malloc that checks to make sure 
 that you've freed everything you've allocated.  It's a bit painful to write 
 this from scratch, but I might be able to hack something together for you 
 (that only runs on Linux), if you want.
 
 Jeremy
 
 
 On Wed, May 28, 2014 at 8:52 AM, Vincent Ryan vincent.x.r...@oracle.com 
 wrote:
 I’ll begin reviewing your changeset and get back to you.
 Thanks.
 
 On 28 May 2014, at 16:28, Jeremy Manson jeremyman...@google.com wrote:
 
 Ah, okay.  Thanks, Iris.  I should already know known that - Martin has 
 explained it to me before.
 
 Vincent - let me know how you want to proceed.
 
 Jeremy
 
 
 On Tue, May 27, 2014 at 10:00 PM, Iris Clark iris.cl...@oracle.com wrote:
 Hi, Jeremy.
 
 As an Author, you can create a changeset but you can't push to the repo 
 until you're a Committer.  Additional details about the differences 
 between Author and Committer may be found here [1,2].
 
 The diffs to create a changeset are (of course) in your webrev.  Your 
 Sponsor can use that along with the hg commit -u jmanson ... to list you 
 as the changeset Author.  Alternatively, your Sponsor may want you to just 
 send the output of hg export -g which would preserve information in the 
 changeset header including the author, commit comment, etc.
 
 Thanks,
 iris
 
 [1]: http://openjdk.java.net/projects/#project-author
 [2]: http://openjdk.java.net/projects/#project-committer
 
 -Original Message-
 From: Jeremy Manson [mailto:jeremyman...@google.com]
 Sent: Tuesday, May 27, 2014 2:19 PM
 To: Vincent Ryan
 Cc: OpenJDK; core-libs-dev
 Subject: Re: RFR: 8044059: Memory Leak in Elliptic Curve Private Key 
 Generation
 
 Thanks, Vincent.  This would be the first change I've made as an author, so 
 I'm not sure what the process is.  I think I just need someone to do the 
 review, and then I can check it in?
 
 Jeremy
 
 
 On Tue, May 27, 2014 at 1:55 PM, Vincent Ryan vincent.x.r...@oracle.com
 wrote:
 
  Thanks Jeremy. I believe you have JDK 9 Author status so can sponsor
  your fix if you wish.
 
  On 27 May 2014, at 21:09, Jeremy Manson jeremyman...@google.com wrote:
 
   Just like the title says - every time you generate a new private
   key, you leak a little memory.  Webrev here:
  
   http://cr.openjdk.java.net/~jmanson/8044059/webrev/
  
   This is probably the wrong mailing list for this bug, but I expect
  someone
   on this list will point me in the right direction.
  
   Jeremy
 
 
 
 
 

Re: RFR: 8044059: Memory Leak in Elliptic Curve Private Key Generation

[Vincent Ryan]

Thanks Jeremy. I believe you have JDK 9 Author status so can sponsor your fix 
if you wish.

On 27 May 2014, at 21:09, Jeremy Manson jeremyman...@google.com wrote:

 Just like the title says - every time you generate a new private key, you
 leak a little memory.  Webrev here:
 
 http://cr.openjdk.java.net/~jmanson/8044059/webrev/
 
 This is probably the wrong mailing list for this bug, but I expect someone
 on this list will point me in the right direction.
 
 Jeremy

Re: RFR - 8041451: com.sun.jndi.ldap.Connection:ReadTimeout should abandon ldap request

[Vincent Ryan]

That fix looks fine. 
Thanks.

On 23 May 2014, at 18:08, Rob McKenna rob.mcke...@oracle.com wrote:

 Hi folks,
 
 Very simple fix to ensure that a slow server isn't left waiting around for a 
 request we have already given up on.
 
 http://cr.openjdk.java.net/~robm/8041451/webrev.01/
 
-Rob
 

Re: RFR - 8042857: 14 stuck threads waiting for notification on LDAPRequest

[Vincent Ryan]

Fix looks fine Rob.
Thanks.

On 16 May 2014, at 17:00, Rob McKenna rob.mcke...@oracle.com wrote:

 Hi folks,
 
 A simple fix to eliminate a possible infinite loop when an Ldap Connection 
 cannot contact the server.
 
 http://cr.openjdk.java.net/~robm/8042857/webrev.01/
 
-Rob
 

Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64

[Vincent Ryan]

On 22 Mar 2014, at 03:22, Mandy Chung mandy.ch...@oracle.com wrote:

 Good catch Sherman.
 
 Vinnie - what's your recommendation for this LDAP change having both 
 encode/decode uses the platform default charset (rather than retaining the 
 old interop issue)?  It's an incompatible change and I'll file a CCC to track 
 this.

Yes. I think that’s the right approach as the compatibility risk is low.


 
 Mandy
 
 On 3/21/2014 2:23 AM, Vincent Ryan wrote:
 You’re right but we’ve never received a report of any charset interop. 
 issues.
 Probably such a scenario has never been encountered by customers.
 
 
 On 21 Mar 2014, at 05:54, Xueming Shen xueming.s...@oracle.com wrote:
 
 Obj.java:#482
It appears sun.misc.BASE64Decoder.decodeBuffer(String) uses String's 
 deprecated
String.getBytes(int srcBegin, int srcEnd, byte[] dst, int dstBegin). The 
 proposed change
now uses the jvm's default charset. It might trigger incompatible 
 behavior if the default
charset is not an ASCII compatible charset.  But if the Java object in 
 LDAP was encoded
with the platform default charset (as the new comment suggested), the 
 old implementation
actually did not work on platform that the default encoding is not ASCII 
 compatible, such
as the IBM ebcdic.
 
 -Sherman
 
 On 3/20/14 3:48 PM, Mandy Chung wrote:
 On 3/19/14 12:28 PM, Xueming Shen wrote:
 On 03/19/2014 11:37 AM, Mandy Chung wrote:
 https://bugs.openjdk.java.net/browse/JDK-8035807
 
 Webrev at:
 http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/
 
 This patch converts the last 2 references to 
 sun.misc.BASE64Encoder/Decoder from the jdk repo with java.util.Base64.  
  We should also update the tests and I have filed JDK-8037873 for that.
 
 Thanks
 Mandy
 The sun.misc.BASE64En/Decoder is MIME type, so it outputs the \r\n per 76
 characters during encoding, and ignores/skip \r or \n when decoding. The 
 new
 Base64.getEncoder/Decoder() returns the basic Base64 coder, which it 
 never
 inserts line separator when output, and throws exception for any 
 non-base64-
 alphabet character, including \r and \n.
 
 The only disadvantage/incompatibility (j.u.Base64.getMimeDecoer() vs
 sun.misc.BASE64Decoder) of switching to j.u.Base64 MIME type en/decoder
 is that the Base64 Mime decoder ignores/skips any non-base64-alphabet
 (including \r and \n), while sun.misc.BASE64Decoder appears to simply
 use the init value -1 for any non-base64-alphabet character for 
 decoding.
 
 I'm not familiar with the use scenario of ldap's Obj class, so I'm not 
 sure if
 it matters (if it ever outputs/inputs  76 character data, or even it 
 does,if
 the difference matters).
 
 Btw, except getMimeEncoder(int ...) all other Base64.getXXXEn/Decoder()
 returns singleton, so the de/encoder cache might not be necessary.
 Thanks Sherman.  Vinnie confirms that it should retain the current 
 behavior as there could be long-lived Java object in LDAP encoded with JDK 
 8 for example and then retrieved with JDK 9.
 
 Here is the updated webrev:
 http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.01/
 
 Thanks
 Mandy
 

Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64

[Vincent Ryan]

You’re right but we’ve never received a report of any charset interop. issues.
Probably such a scenario has never been encountered by customers.


On 21 Mar 2014, at 05:54, Xueming Shen xueming.s...@oracle.com wrote:

 Obj.java:#482
It appears sun.misc.BASE64Decoder.decodeBuffer(String) uses String's 
 deprecated
String.getBytes(int srcBegin, int srcEnd, byte[] dst, int dstBegin). The 
 proposed change
now uses the jvm's default charset. It might trigger incompatible behavior 
 if the default
charset is not an ASCII compatible charset.  But if the Java object in 
 LDAP was encoded
with the platform default charset (as the new comment suggested), the old 
 implementation
actually did not work on platform that the default encoding is not ASCII 
 compatible, such
as the IBM ebcdic.
 
 -Sherman
 
 On 3/20/14 3:48 PM, Mandy Chung wrote:
 On 3/19/14 12:28 PM, Xueming Shen wrote:
 On 03/19/2014 11:37 AM, Mandy Chung wrote:
 https://bugs.openjdk.java.net/browse/JDK-8035807
 
 Webrev at:
 http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/
 
 This patch converts the last 2 references to 
 sun.misc.BASE64Encoder/Decoder from the jdk repo with java.util.Base64.   
 We should also update the tests and I have filed JDK-8037873 for that.
 
 Thanks
 Mandy
 
 The sun.misc.BASE64En/Decoder is MIME type, so it outputs the \r\n per 76
 characters during encoding, and ignores/skip \r or \n when decoding. The new
 Base64.getEncoder/Decoder() returns the basic Base64 coder, which it never
 inserts line separator when output, and throws exception for any non-base64-
 alphabet character, including \r and \n.
 
 The only disadvantage/incompatibility (j.u.Base64.getMimeDecoer() vs
 sun.misc.BASE64Decoder) of switching to j.u.Base64 MIME type en/decoder
 is that the Base64 Mime decoder ignores/skips any non-base64-alphabet
 (including \r and \n), while sun.misc.BASE64Decoder appears to simply
 use the init value -1 for any non-base64-alphabet character for decoding.
 
 I'm not familiar with the use scenario of ldap's Obj class, so I'm not sure 
 if
 it matters (if it ever outputs/inputs  76 character data, or even it 
 does,if
 the difference matters).
 
 Btw, except getMimeEncoder(int ...) all other Base64.getXXXEn/Decoder()
 returns singleton, so the de/encoder cache might not be necessary.
 
 Thanks Sherman.  Vinnie confirms that it should retain the current behavior 
 as there could be long-lived Java object in LDAP encoded with JDK 8 for 
 example and then retrieved with JDK 9.
 
 Here is the updated webrev:
 http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.01/
 
 Thanks
 Mandy
 

Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64

[Vincent Ryan]

Fix looks fine. You just need to update the import statements in Obj.java


On 19 Mar 2014, at 18:37, Mandy Chung mandy.ch...@oracle.com wrote:

 https://bugs.openjdk.java.net/browse/JDK-8035807
 
 Webrev at:
 http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/
 
 This patch converts the last 2 references to sun.misc.BASE64Encoder/Decoder 
 from the jdk repo with java.util.Base64.   We should also update the tests 
 and I have filed JDK-8037873 for that.
 
 Thanks
 Mandy

Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64

[Vincent Ryan]

OK. Thanks.

On 19 Mar 2014, at 19:06, Mandy Chung mandy.ch...@oracle.com wrote:

 On 3/19/2014 12:03 PM, Vincent Ryan wrote:
 Fix looks fine. You just need to update the import statements in Obj.java
 
 Fixed.  Not sure how it's missed in the webrev :)
 
 thanks
 Mandy
 
 
 On 19 Mar 2014, at 18:37, Mandy Chung mandy.ch...@oracle.com wrote:
 
 https://bugs.openjdk.java.net/browse/JDK-8035807
 
 Webrev at:
 http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/
 
 This patch converts the last 2 references to sun.misc.BASE64Encoder/Decoder 
 from the jdk repo with java.util.Base64.   We should also update the tests 
 and I have filed JDK-8037873 for that.
 
 Thanks
 Mandy
 

Re: RFR [6968459] JNDI timeout fails before timeout is reached

[Vincent Ryan]

Hello Ivan,

Thanks for the updated patch. I would like to see a testcase along with this 
fix, since it is
modifying a critical component of the LDAP client code. An LDAP server may not 
even be
required in order to exercise the timeouts.

Thanks.


On 3 Dec 2013, at 14:35, Ivan Gerasimov ivan.gerasi...@oracle.com wrote:

 Hi Peter!
 
 Thank you for your review!
 
 You are right, the patch changed the behavior of the code.
 I've reverted back all the unnecessary changes. This should minimize the risk.
 
 I've also made another correction: After decrementing the remaining timeOut, 
 the startTime should be set to currTime.
 
 Would you please help review the updated webrev:
 http://cr.openjdk.java.net/~igerasim/6968459/2/webrev/
 
 Sincerely yours,
 Ivan
 
 
 From quick look I notice a danger that Connection.readReply() pauses (for 
 the timeOut time) in spite of the fact that a reply is ready and enqueued 
 before waiting.
 
 Imagine a situation where the 1st try of obtaining result returns null:
 
 450 // Get the reply if it is already available.
 451 BerDecoder rber = ldr.getReplyBer();
 
 
 ...after that, but before reaching the following lines:
 
 471 synchronized (ldr) {
 472 ldr.wait(timeOut);
 473 }
 
 
 The other thread enqueues a reply (LdapRequest.addReplyBer()) because the 
 code in readReply() is executing without holding a lock on ldr. When 
 this thread (executing readReply()) finally enters synchronized block and 
 waits, it will wait until wait() times out or another reply is enqueued 
 which will issue another notify(). This can lead to unnecessary pauses. Old 
 code does not exhibit this problem, because it re-checks the readiness of a 
 reply immediately after entering the synchronized block.
 
 
 Regards, Peter
 
 
 
 

[8] RfR 8027567: JDK8 build failure: the correct version of GNU make is being rejected

[Vincent Ryan]

Please review this fix to correct the JDK8 build Configure script.
It reverts a recent change to common/autoconf/basics.m4 that was causing a 
build failure on Solaris.

Bug: https://bugs.openjdk.java.net/browse/JDK-8027567
Webrev: http://cr.openjdk.java.net/~vinnie/8027567/

Re: [8] RfR 8027567: JDK8 build failure: the correct version of GNU make is being rejected

[Vincent Ryan]

Thanks Chris. 

So reverting to the previous version of builds.m4 will fix this issue on 
Solaris but will undo
the fix for 8026528 on Windows. Maybe Eric can advise.



On 30 Oct 2013, at 15:05, Chris Hegarty chris.hega...@oracle.com wrote:

 Hi Vinnie,
 
 I have seen this issue myself, kind of funny ;-)
 
 ...
 configure: Found GNU make at /java/devtools/i386/bin/make, however this is 
 not version 3.81 or later. (it is: GNU Make 3.81). Ignoring.
 configure: error: Cannot find GNU make 3.81 or newer! Please put it in the 
 path, or add e.g. MAKE=/opt/gmake3.81/make as argument to configure.
 configure exiting with result code 1
 
 I originally thought that the problem was with a bad change in tl, but when 
 reviewing your change I noticed that it is as a result of a changes that went 
 into jdk8/build [1], and we see it in tl after a sync up with master.
 
 I guess we can still resolve the problem in tl, and sync back into master, 
 but Erik should ensure that the original issue is still fixed, 8026528: 
 [build] configure does not recognize newer make in cygwin
 
 -Chris.
 
 [1] http://hg.openjdk.java.net/jdk8/build/rev/1a853fac18ff
 
 On 30/10/2013 14:40, Vincent Ryan wrote:
 Please review this fix to correct the JDK8 build Configure script.
 It reverts a recent change to common/autoconf/basics.m4 that was causing a 
 build failure on Solaris.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8027567
 Webrev: http://cr.openjdk.java.net/~vinnie/8027567/
 

Re: [8] RfR 8027567: JDK8 build failure: the correct version of GNU make is being rejected

[Vincent Ryan]

Thanks Erik. We’re seeing the issue in TL so it’d be great to get this fixed 
today.
Would you like to take ownership of this issue or shall I push your fix to TL?

On 30 Oct 2013, at 16:00, Erik Joelsson erik.joels...@oracle.com wrote:

 I found a solution that's also more readable. Posting inline since it's so 
 small a change:
 
 diff -r 4f2011496393 common/autoconf/basics.m4
 --- a/common/autoconf/basics.m4
 +++ b/common/autoconf/basics.m4
 @@ -514,7 +514,7 @@
 if test x$IS_GNU_MAKE = x; then
   AC_MSG_NOTICE([Found potential make at $MAKE_CANDIDATE, however, this 
 is not GNU Make. Ignoring.])
 else
 -  IS_MODERN_MAKE=`$ECHO $MAKE_VERSION_STRING | $GREP 
 '\(3\.8[[12]]\)\|\(4\.\)'`
 +  IS_MODERN_MAKE=`$ECHO $MAKE_VERSION_STRING | $GREP -e '3\.8[[12]]' -e 
 '4\.'`
   if test x$IS_MODERN_MAKE = x; then
 AC_MSG_NOTICE([Found GNU make at $MAKE_CANDIDATE, however this is not 
 version 3.81 or later. (it is: $MAKE_VERSION_STRING). Ignoring.])
   else
 
 /Erik
 
 On 2013-10-30 16:51, Erik Joelsson wrote:
 It seems the problem is with the grep tool used to parse the version string. 
 /usr/xpg4/bin/grep doesn't handle '\(3\.8[12]\)\|\(4\.\)' the same as gnu 
 grep. In jprt it finds /usr/sfw/bin/ggrep which works better. I will see if 
 I can figure out something that works with both.
 
 /Erik
 
 On 2013-10-30 16:28, Vincent Ryan wrote:
 Thanks Chris.
 
 So reverting to the previous version of builds.m4 will fix this issue on 
 Solaris but will undo
 the fix for 8026528 on Windows. Maybe Eric can advise.
 
 
 
 On 30 Oct 2013, at 15:05, Chris Hegarty chris.hega...@oracle.com wrote:
 
 Hi Vinnie,
 
 I have seen this issue myself, kind of funny ;-)
 
 ...
 configure: Found GNU make at /java/devtools/i386/bin/make, however this is 
 not version 3.81 or later. (it is: GNU Make 3.81). Ignoring.
 configure: error: Cannot find GNU make 3.81 or newer! Please put it in the 
 path, or add e.g. MAKE=/opt/gmake3.81/make as argument to configure.
 configure exiting with result code 1
 
 I originally thought that the problem was with a bad change in tl, but 
 when reviewing your change I noticed that it is as a result of a changes 
 that went into jdk8/build [1], and we see it in tl after a sync up with 
 master.
 
 I guess we can still resolve the problem in tl, and sync back into master, 
 but Erik should ensure that the original issue is still fixed, 8026528: 
 [build] configure does not recognize newer make in cygwin
 
 -Chris.
 
 [1] http://hg.openjdk.java.net/jdk8/build/rev/1a853fac18ff
 
 On 30/10/2013 14:40, Vincent Ryan wrote:
 Please review this fix to correct the JDK8 build Configure script.
 It reverts a recent change to common/autoconf/basics.m4 that was causing 
 a build failure on Solaris.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8027567
 Webrev: http://cr.openjdk.java.net/~vinnie/8027567/
 
 
 

Re: [8] RfR 8027567: JDK8 build failure: the correct version of GNU make is being rejected

[Vincent Ryan]

Sure. I’ll re-run autogen.sh and submit an updated webrev.
Thanks.


On 30 Oct 2013, at 16:15, Erik Joelsson erik.joels...@oracle.com wrote:

 I'm leaving for the day pretty soon, so feel free to push this to tl if you 
 have time.
 
 If you do, don't forget to run common/autoconf/autogen.sh and to also submit 
 the closed generated-configure.
 
 /Erik
 
 On 2013-10-30 17:07, Vincent Ryan wrote:
 Thanks Erik. We’re seeing the issue in TL so it’d be great to get this fixed 
 today.
 Would you like to take ownership of this issue or shall I push your fix to 
 TL?
 
 On 30 Oct 2013, at 16:00, Erik Joelsson erik.joels...@oracle.com wrote:
 
 I found a solution that's also more readable. Posting inline since it's so 
 small a change:
 
 diff -r 4f2011496393 common/autoconf/basics.m4
 --- a/common/autoconf/basics.m4
 +++ b/common/autoconf/basics.m4
 @@ -514,7 +514,7 @@
 if test x$IS_GNU_MAKE = x; then
   AC_MSG_NOTICE([Found potential make at $MAKE_CANDIDATE, however, this 
 is not GNU Make. Ignoring.])
 else
 -  IS_MODERN_MAKE=`$ECHO $MAKE_VERSION_STRING | $GREP 
 '\(3\.8[[12]]\)\|\(4\.\)'`
 +  IS_MODERN_MAKE=`$ECHO $MAKE_VERSION_STRING | $GREP -e '3\.8[[12]]' 
 -e '4\.'`
   if test x$IS_MODERN_MAKE = x; then
 AC_MSG_NOTICE([Found GNU make at $MAKE_CANDIDATE, however this is 
 not version 3.81 or later. (it is: $MAKE_VERSION_STRING). Ignoring.])
   else
 
 /Erik
 
 On 2013-10-30 16:51, Erik Joelsson wrote:
 It seems the problem is with the grep tool used to parse the version 
 string. /usr/xpg4/bin/grep doesn't handle '\(3\.8[12]\)\|\(4\.\)' the same 
 as gnu grep. In jprt it finds /usr/sfw/bin/ggrep which works better. I 
 will see if I can figure out something that works with both.
 
 /Erik
 
 On 2013-10-30 16:28, Vincent Ryan wrote:
 Thanks Chris.
 
 So reverting to the previous version of builds.m4 will fix this issue on 
 Solaris but will undo
 the fix for 8026528 on Windows. Maybe Eric can advise.
 
 
 
 On 30 Oct 2013, at 15:05, Chris Hegarty chris.hega...@oracle.com wrote:
 
 Hi Vinnie,
 
 I have seen this issue myself, kind of funny ;-)
 
 ...
 configure: Found GNU make at /java/devtools/i386/bin/make, however this 
 is not version 3.81 or later. (it is: GNU Make 3.81). Ignoring.
 configure: error: Cannot find GNU make 3.81 or newer! Please put it in 
 the path, or add e.g. MAKE=/opt/gmake3.81/make as argument to configure.
 configure exiting with result code 1
 
 I originally thought that the problem was with a bad change in tl, but 
 when reviewing your change I noticed that it is as a result of a changes 
 that went into jdk8/build [1], and we see it in tl after a sync up with 
 master.
 
 I guess we can still resolve the problem in tl, and sync back into 
 master, but Erik should ensure that the original issue is still fixed, 
 8026528: [build] configure does not recognize newer make in cygwin
 
 -Chris.
 
 [1] http://hg.openjdk.java.net/jdk8/build/rev/1a853fac18ff
 
 On 30/10/2013 14:40, Vincent Ryan wrote:
 Please review this fix to correct the JDK8 build Configure script.
 It reverts a recent change to common/autoconf/basics.m4 that was 
 causing a build failure on Solaris.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8027567
 Webrev: http://cr.openjdk.java.net/~vinnie/8027567/
 
 

Re: [8] RfR 8027567: JDK8 build failure: the correct version of GNU make is being rejected

[Vincent Ryan]

Thanks for the review Chris.

For the record, here’s the updated webrev:
  http://cr.openjdk.java.net/~vinnie/8027567/webrev.01/


On 30 Oct 2013, at 16:57, Chris Hegarty chris.hega...@oracle.com wrote:

 The source changes look fine to me, I'm happy to be listed as a reviewer for 
 this. ( generated-configure.sh, I just assume is ok once 
 common/autoconf/autogen.sh is run ).
 
 -Chris.
 
 On 30/10/2013 16:24, Vincent Ryan wrote:
 Sure. I’ll re-run autogen.sh and submit an updated webrev.
 Thanks.
 
 
 On 30 Oct 2013, at 16:15, Erik Joelssonerik.joels...@oracle.com  wrote:
 
 I'm leaving for the day pretty soon, so feel free to push this to tl if you 
 have time.
 
 If you do, don't forget to run common/autoconf/autogen.sh and to also 
 submit the closed generated-configure.
 
 /Erik
 
 On 2013-10-30 17:07, Vincent Ryan wrote:
 Thanks Erik. We’re seeing the issue in TL so it’d be great to get this 
 fixed today.
 Would you like to take ownership of this issue or shall I push your fix to 
 TL?
 
 On 30 Oct 2013, at 16:00, Erik Joelssonerik.joels...@oracle.com  wrote:
 
 I found a solution that's also more readable. Posting inline since it's 
 so small a change:
 
 diff -r 4f2011496393 common/autoconf/basics.m4
 --- a/common/autoconf/basics.m4
 +++ b/common/autoconf/basics.m4
 @@ -514,7 +514,7 @@
 if test x$IS_GNU_MAKE = x; then
   AC_MSG_NOTICE([Found potential make at $MAKE_CANDIDATE, however, 
 this is not GNU Make. Ignoring.])
 else
 -  IS_MODERN_MAKE=`$ECHO $MAKE_VERSION_STRING | $GREP 
 '\(3\.8[[12]]\)\|\(4\.\)'`
 +  IS_MODERN_MAKE=`$ECHO $MAKE_VERSION_STRING | $GREP -e '3\.8[[12]]' 
 -e '4\.'`
   if test x$IS_MODERN_MAKE = x; then
 AC_MSG_NOTICE([Found GNU make at $MAKE_CANDIDATE, however this is 
 not version 3.81 or later. (it is: $MAKE_VERSION_STRING). Ignoring.])
   else
 
 /Erik
 
 On 2013-10-30 16:51, Erik Joelsson wrote:
 It seems the problem is with the grep tool used to parse the version 
 string. /usr/xpg4/bin/grep doesn't handle '\(3\.8[12]\)\|\(4\.\)' the 
 same as gnu grep. In jprt it finds /usr/sfw/bin/ggrep which works 
 better. I will see if I can figure out something that works with both.
 
 /Erik
 
 On 2013-10-30 16:28, Vincent Ryan wrote:
 Thanks Chris.
 
 So reverting to the previous version of builds.m4 will fix this issue 
 on Solaris but will undo
 the fix for 8026528 on Windows. Maybe Eric can advise.
 
 
 
 On 30 Oct 2013, at 15:05, Chris Hegartychris.hega...@oracle.com  
 wrote:
 
 Hi Vinnie,
 
 I have seen this issue myself, kind of funny ;-)
 
 ...
 configure: Found GNU make at /java/devtools/i386/bin/make, however 
 this is not version 3.81 or later. (it is: GNU Make 3.81). Ignoring.
 configure: error: Cannot find GNU make 3.81 or newer! Please put it in 
 the path, or add e.g. MAKE=/opt/gmake3.81/make as argument to 
 configure.
 configure exiting with result code 1
 
 I originally thought that the problem was with a bad change in tl, but 
 when reviewing your change I noticed that it is as a result of a 
 changes that went into jdk8/build [1], and we see it in tl after a 
 sync up with master.
 
 I guess we can still resolve the problem in tl, and sync back into 
 master, but Erik should ensure that the original issue is still fixed, 
 8026528: [build] configure does not recognize newer make in cygwin
 
 -Chris.
 
 [1] http://hg.openjdk.java.net/jdk8/build/rev/1a853fac18ff
 
 On 30/10/2013 14:40, Vincent Ryan wrote:
 Please review this fix to correct the JDK8 build Configure script.
 It reverts a recent change to common/autoconf/basics.m4 that was 
 causing a build failure on Solaris.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8027567
 Webrev: http://cr.openjdk.java.net/~vinnie/8027567/
 
 
 

Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs

[Vincent Ryan]

The JAAS and JGSS changes look fine too.


On 7 Oct 2013, at 09:23, Chris Hegarty wrote:

 Alan,
 
 I checked the httpsever and sctp changes. All look good to me.
 
 -Chris.
 
 On 10/06/2013 09:03 PM, Alan Bateman wrote:
 
 As a follow-up to Joe Darcy's rename of jdk.Supported to jdk.Exported,
 I'd like to have another attempt at adding the annotation to a number of
 JDK specific APIs that are long standing exported, documented and
 supported APIs. Specifically, the following APIs:
 
 - Java Debug Interface (com.sun.jdi)
 - Attach API (com.sun.tools.attach)
 - SCTP API (com.sun.nio.sctp)
 - HTTP server API (com.sun.net.httpserver)
 - Management extensions (com.sun.management)
 - JConsole Plugin API (com.sun.tools.jconsole)
 - JDK-specific API to JAAS (com.sun.security.auth)
 - JDK-specific JGSS API (com.sun.security.jgss)
 
 (The javadoc for each of these APIs is currently generated in the build)
 
 The webrev with the proposed update is here:
   http://cr.openjdk.java.net/~alanb/8008662/webrev.02/
 
 As per the original webrev, I've added package-info.java to a number of
 packages that didn't have any description. In a few cases, I've had to
 rename the legacy package.html to package-info.java.
 
 For the review then the intention is that @jdk.Exported be added to the
 package-info and all public/protected types in these APIs. The only
 exceptions are two cases where I've added @jdk.Exported(false),
 specifically:
 
 - com.sun.management.OSMBeanFactory as it clearly documents to stay away
 - com.sun.security.auth.callback.DialogCallbackHandler as it for the
 chop (see JEP 162)
 
 Thanks,
 
 Alan.

Re: Fake DNS query result inside a test

[Vincent Ryan]

Are you suggesting to run a local DNS server? If so then it is easy to access 
that
via a JNDI context.

If you're proposing developing a basic JNDI service provider then that would be
more effort.


On 7 Nov 2012, at 01:05, Weijun Wang wrote:

 Hi Vinnie
 
 I want to write a regression test so that
 
   Context ctx = NamingManager.getURLContext(dns, new Hashtable(0));
   Attributes attrs =((DirContext)ctx).getAttributes(
 _kerberos._udp.ASDF.COM., SRV_RR_ATTR);
 
 can return some entries without querying a real external server.
 
 Is this possible by registering a local name server provider or else?
 
 Thanks
 Max

Re: Code review request: 6961765: Double byte characters corrupted in DN for LDAP referrals

[Vincent Ryan]

Your fix looks fine.


On 03/ 6/12 08:32 AM, Weijun Wang wrote:

Hi Vinnie

This bug is about using UrlUtil.decode() to decode a URL that is not
fully encoded, i.e. including non-ASCII characters.

The webrev is at

http://cr.openjdk.java.net/~weijun/6961765/webrev.00/

It simply delegates the call to URLDecoder.decode().

LDAP URL (RFC 4516 2.1) specifies that only reserved, unreserved,
and pct-encoded chars can be used, which do not include general
non-ASCII unicode. So precisely the user input in the bug report is
illegal, but since it's already a valid URL/URI in Java, we can somehow
be more friendly.

In fact, the javadoc of URLDecoder [1] also only allows these
characters, but at the same time it says --

There are two possible ways in which this decoder could deal with
illegal strings. It could either leave illegal characters alone or
it could throw an IllegalArgumentException. Which approach the
decoder takes is left to the implementation.

Now the Oracle implementation of the class leave illegal characters
alone. In this sense, UrlUtil is not as good as URLDecoder. It neither
leaves them alone nor throws an exception.

To be more correct, I think we can update URLDecoder so that it leaves
Unicode in the other category (non-control, non-whitespace non-ASCII
Unicode chars, as described in URI's spec) unchanged, and throw an
exception otherwise (that is, non-ASCII, and control or space). But I'll
leave that to another RFE.

Thanks
Max


 Original Message 
*Change Request ID*: 6961765
*Synopsis*: Double byte characters corrupted in DN for LDAP referrals


=== *Description*

SYNOPSIS

Double byte characters corrupted in DN for LDAP referrals

OPERATING SYSTEM

All

FULL JDK VERSION

All

DESCRIPTION
---

If the DN component of an LDAP URL contains double byte characters, it
is corrupted by com.sun.jndi.toolkit.url.UrlUtil.decode(). This
corruption leads to application level failures.

Consider the following scenario:

1. Application connects to an LDAP server and searches for the string
uid=???,??? (where ??? are double byte characters)

2. JNDI code receives a referral, for example:
ldap://www.test.com/uid=???,???,ou=people,ou=test,ou=test,o=test

3. The referral is then parsed to split the hostname, port number and
the DN element of the URI via
com.sun.jndi.ldap.LdapURL.parsePathAndQuery()

4. The DN element is decoded using
com.sun.jndi.toolkit.url.UrlUtil.decode()

5. This method expects the characters to be ASCII. If the characters
are non-ASCII, as in our example, then those characters are not
converted properly.

6. This corrupted DN is then passed to the LDAP server, resulting in an
unexpected failure.

TESTCASE

This testcase does not represent normal application code. It highlights
the problem by calling into com.sun.* internal classes directly. This
allows the problem to be demonstrated without setting up an LDAP server.

import java.net.URI;
import java.net.URLDecoder;
import com.sun.jndi.ldap.LdapURL;

public class LdapURLTest {
public static void main (String args[]) throws Exception {
String testString =
(ldap://www.test.com/uid=\u3070\u3073\u3076,\u3079\u307C\u307E,ou=test,ou=test,ou=test,o=test;);

LdapURL ldURL = new LdapURL(testString);
System.out.println( LDAP URL String:  + testString);
System.out.println( decoded DN:  + ldURL.getDN());

// suggested fix demonstration
String DN;
String path = new URI(testString).getPath();

DN = path.startsWith(/) ? path.substring(1) : path;
String proposedDN = URLDecoder.decode(DN, UTF8);

System.out.println(\nDN from proposed fix:  + proposedDN);
}
}

SUGGESTED FIX
-
Use java.net.URLDecoder rather than com.sun.jndi.toolkit.url.UrlUtil to
conduct the URL decoding in parsePathAndQuery().

Specifically, change the line that decodes the DN element in
com.sun.jndi.ldap.LdapURL.parsePathAndQuery() from:

DN = path.startsWith(/) ? path.substring(1) : path;
if (DN.length()  0) {
-- DN = UrlUtil.decode(DN, UTF8); --
}

to:

DN = path.startsWith(/) ? path.substring(1) : path;
if (DN.length()  0) {
-- DN = URLDecoder.decode(DN, UTF8); --
}


=== *Evaluation*
=
The URL in the testcase has an invalid encoding. Its Unicode characters
must be encoded in UTF-8. For example,

\u3070 - \e3\81\b0 - %5Ce3%5C81%5Cb0

Re: 7056489: test/com/sun/jndi/ldap/ReadTimeoutTest.java loops or times out

[Vincent Ryan]

Apologies for overlooking the core-libs audience.

On 06/20/11 23:42, Joe Darcy wrote:
 Vincent Ryan wrote:
 On 06/20/11 16:08, Alan Bateman wrote:
  
 Vincent Ryan wrote:

 :
 Thanks Alan. JNDI applications are encouraged to call close() rather
 than
 rely on GC to free resources.

 I guess we can support both behaviours: long-lived JNDI applications
 can
 continue to use try blocks and explicitly call close(), when finished.
 Short-lived JNDI applications can use try-with-resources blocks and
 benefit
 from the automatic resource management.

 It really is just a 1-line change:

 marino% diff old/javax/naming/Context.java
 new/javax/naming/Context.java
 281c281
  public interface Context {
 ---
  public interface Context extends AutoCloseable {
 marino%
 
 That's all it should be. Even where try/finally it used today there is
 still the possibility of close failing and supplanting a more
 interesting exception. Overall it sounds like this is worth doing.

 -Alan.
 

 Right. I've filed an RFE and targeted it to jdk8.
   http://monaco.us.oracle.com/detail.jsf?cr=7057061

 FYI NamingEnumeration also supports a close() method to explicitly
 release
 resources, for example when discarding the remainder of a search result.
 Under normal circumstances, the final iteration of NamingEnumeration will
 cause its resources to be released automatically. So close() is only used
 when exiting an iteration prematurely.

 I don't think NamingEnumeration is a suitable candidate for
 try-with-resources
 because the common case is to complete the iteration and _not_ to call
 close().
   
 Hello.
 
 Vinnie, please reply about this rfe and your assessment of it to the
 core-libs list.
 
 Thanks,
 
 -Joe
 

Re: 7056489: test/com/sun/jndi/ldap/ReadTimeoutTest.java loops or times out

[Vincent Ryan]

Your fix looks good.

On 18/06/2011 20:39, Alan Bateman wrote:


I need a reviewer for a small fix to a test that I caught looping on my
syst.em. The test creates a server thread that emulates a non responding
LDAP server. Unfortunately the thread goes into a loop reading from the
connection once it is closed. I'll bet the original author only tested
this on Solaris where it would have completed as intended due to
now-disabled and legacy interruptible I/O support. While I was there I
eliminated the hard coded port that would cause it to fail if that port
were already in use. The test could be improved in other ways too but
for now the webrev with the proposed changes is here:

http://cr.openjdk.java.net/~alanb/7056489/webrev/

Thanks,
Alan.

hg: jdk7/tl/jdk: 2 new changesets

[vincent . ryan]

Changeset: 117b245b5bb9
Author:vinnie
Date:  2010-01-21 23:59 +
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/117b245b5bb9

6763530: Cannot decode PublicKey (Proider SunPKCS11, curve prime256v1)
Reviewed-by: andrew

! src/share/classes/sun/security/pkcs11/P11ECKeyFactory.java
! src/share/classes/sun/security/pkcs11/P11Key.java

Changeset: c94ac5522d01
Author:vinnie
Date:  2010-01-22 00:02 +
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/c94ac5522d01

Merge

hg: jdk7/tl/jdk: 6876158: Remove dependencies on Signer, Certificate, Identity, IdentityScope classes from java.security pkg

[vincent . ryan]

Changeset: 327adb1c2224
Author:vinnie
Date:  2009-12-07 17:06 +
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/327adb1c2224

6876158: Remove dependencies on Signer, Certificate, Identity, IdentityScope 
classes from java.security pkg
Reviewed-by: alanb, mullan

! src/share/classes/com/sun/security/auth/PolicyFile.java
! src/share/classes/sun/security/pkcs/PKCS10.java
- src/share/classes/sun/security/provider/IdentityDatabase.java
! src/share/classes/sun/security/provider/PolicyFile.java
- src/share/classes/sun/security/provider/SystemIdentity.java
- src/share/classes/sun/security/provider/SystemSigner.java
! src/share/classes/sun/security/tools/JarSigner.java
! src/share/classes/sun/security/tools/KeyTool.java
! src/share/classes/sun/security/x509/CertAndKeyGen.java
- src/share/classes/sun/security/x509/X500Signer.java
- src/share/classes/sun/security/x509/X509Cert.java
- src/share/classes/sun/tools/jar/JarVerifierStream.java

hg: jdk7/tl/jdk: 6906854: SSL/Krb5 testcase should not use a fixed port number

[vincent . ryan]

Changeset: bc12627832e0
Author:vinnie
Date:  2009-12-03 21:30 +
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/bc12627832e0

6906854: SSL/Krb5 testcase should not use a fixed port number
Reviewed-by: alanb

! test/ProblemList.txt
! test/sun/security/krb5/auto/SSL.java

hg: jdk7/tl/jdk: 2 new changesets

[vincent . ryan]

Changeset: 561186928899
Author:vinnie
Date:  2009-12-02 17:06 +
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/561186928899

6906510: Fix testcase for 6894643: Separate out dependency on Kerberos
Reviewed-by: weijun

! test/sun/security/krb5/auto/SSL.java

Changeset: 79d91585d7d7
Author:vinnie
Date:  2009-12-02 17:34 +
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/79d91585d7d7

Merge

- make/tools/CharsetMapping/DoubleByte-X.java
- make/tools/CharsetMapping/SingleByte-X.java
- src/share/classes/sun/util/CoreResourceBundleControl-XLocales.java
- src/share/classes/sun/util/LocaleDataMetaInfo-XLocales.java
- test/java/util/Formatter/Basic-X.java
- test/sun/tools/native2ascii/test2
- test/tools/launcher/SolarisDataModel.sh
- test/tools/launcher/SolarisRunpath.sh
- test/tools/launcher/libraryCaller.c
- test/tools/launcher/libraryCaller.h
- test/tools/launcher/libraryCaller.java

hg: jdk7/tl/jdk: 6885204: JSSE should not require Kerberos to be present

[vincent . ryan]

Changeset: 54118c8e0ebe
Author:vinnie
Date:  2009-10-05 23:42 +0100
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/54118c8e0ebe

6885204: JSSE should not require Kerberos to be present
Reviewed-by: wetmore, alanb

! 
src/share/classes/com/sun/net/ssl/internal/www/protocol/https/DelegateHttpsURLConnection.java
! src/share/classes/sun/net/www/protocol/https/HttpsClient.java
! src/share/classes/sun/security/ssl/CipherSuite.java
! src/share/classes/sun/security/ssl/JsseJce.java

hg: jdk7/tl/jdk: 6884175: CR cleanup for 6840752: Provide out-of-the-box support for ECC algorithms

[vincent . ryan]

Changeset: 845fefff00a4
Author:vinnie
Date:  2009-09-21 23:01 +0100
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/845fefff00a4

6884175: CR cleanup for 6840752: Provide out-of-the-box support for ECC 
algorithms
Reviewed-by: wetmore

! make/sun/security/ec/Makefile
! make/sun/security/other/Makefile
! src/share/classes/sun/security/ec/ECDHKeyAgreement.java
! src/share/classes/sun/security/ec/ECDSASignature.java
! src/share/classes/sun/security/ec/ECKeyPairGenerator.java
! src/share/classes/sun/security/ec/SunEC.java
! src/share/classes/sun/security/ec/SunECEntries.java
! src/share/native/sun/security/ec/ECC_JNI.cpp
- src/share/native/sun/security/ec/ec.c
+ src/share/native/sun/security/ec/impl/ec.c
+ src/share/native/sun/security/ec/impl/ec.h
+ src/share/native/sun/security/ec/impl/ec2.h
+ src/share/native/sun/security/ec/impl/ec2_163.c
+ src/share/native/sun/security/ec/impl/ec2_193.c
+ src/share/native/sun/security/ec/impl/ec2_233.c
+ src/share/native/sun/security/ec/impl/ec2_aff.c
+ src/share/native/sun/security/ec/impl/ec2_mont.c
+ src/share/native/sun/security/ec/impl/ec_naf.c
+ src/share/native/sun/security/ec/impl/ecc_impl.h
+ src/share/native/sun/security/ec/impl/ecdecode.c
+ src/share/native/sun/security/ec/impl/ecl-curve.h
+ src/share/native/sun/security/ec/impl/ecl-exp.h
+ src/share/native/sun/security/ec/impl/ecl-priv.h
+ src/share/native/sun/security/ec/impl/ecl.c
+ src/share/native/sun/security/ec/impl/ecl.h
+ src/share/native/sun/security/ec/impl/ecl_curve.c
+ src/share/native/sun/security/ec/impl/ecl_gf.c
+ src/share/native/sun/security/ec/impl/ecl_mult.c
+ src/share/native/sun/security/ec/impl/ecp.h
+ src/share/native/sun/security/ec/impl/ecp_192.c
+ src/share/native/sun/security/ec/impl/ecp_224.c
+ src/share/native/sun/security/ec/impl/ecp_256.c
+ src/share/native/sun/security/ec/impl/ecp_384.c
+ src/share/native/sun/security/ec/impl/ecp_521.c
+ src/share/native/sun/security/ec/impl/ecp_aff.c
+ src/share/native/sun/security/ec/impl/ecp_jac.c
+ src/share/native/sun/security/ec/impl/ecp_jm.c
+ src/share/native/sun/security/ec/impl/ecp_mont.c
+ src/share/native/sun/security/ec/impl/logtab.h
+ src/share/native/sun/security/ec/impl/mp_gf2m-priv.h
+ src/share/native/sun/security/ec/impl/mp_gf2m.c
+ src/share/native/sun/security/ec/impl/mp_gf2m.h
+ src/share/native/sun/security/ec/impl/mpi-config.h
+ src/share/native/sun/security/ec/impl/mpi-priv.h
+ src/share/native/sun/security/ec/impl/mpi.c
+ src/share/native/sun/security/ec/impl/mpi.h
+ src/share/native/sun/security/ec/impl/mplogic.c
+ src/share/native/sun/security/ec/impl/mplogic.h
+ src/share/native/sun/security/ec/impl/mpmontg.c
+ src/share/native/sun/security/ec/impl/mpprime.h
+ src/share/native/sun/security/ec/impl/oid.c
+ src/share/native/sun/security/ec/impl/secitem.c
+ src/share/native/sun/security/ec/impl/secoidt.h
! test/sun/security/ec/TestEC.java
+ test/sun/security/ec/certs/sunlabscerts.pem
+ test/sun/security/ec/keystore
+ test/sun/security/ec/truststore
! test/sun/security/pkcs11/ec/ReadCertificates.java
! test/sun/security/pkcs11/sslecc/CipherTest.java

hg: jdk7/tl/jdk: 6872048: bad private keys are generated for 2 specific ECC curves

[vincent . ryan]

Changeset: dd997cc0c823
Author:vinnie
Date:  2009-08-24 18:37 +0100
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/dd997cc0c823

6872048: bad private keys are generated for 2 specific ECC curves
Reviewed-by: wetmore

! src/share/native/sun/security/ec/ec.c
! test/sun/security/ec/TestEC.java