RFR: 8245194: Unix domain socket channel implementation [v34]

[Chris Hegarty]

On Wed, 21 Oct 2020 16:31:06 GMT, Michael McMahon <michaelm at openjdk.org> wrote:

>> Continuing this review as a PR on github with the comments below incorporated. I expect there will be a few more iterations before integrating.
>> 
>> On 06/09/2020 19:47, Alan Bateman wrote:
>>> On 26/08/2020 15:24, Michael McMahon wrote:
>>>>
>>>> As I mentioned the other day, I wasn't able to use the suggested method on Windows which returns an absolute path. So, I added a method to WindowsPath which returns the path in the expected UTF-8 encoding as a byte[]. Let me know what you think of that.
>>>>
>>>> There is a fair bit of other refactoring and simplification done also. Next version is at:
>>>>
>>>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>>>
>>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I don't think we should be caching it as the encoding for sun_path is an outlier on Windows. I'm also a bit dubious about encoding a relative path when the resolved path (before encoding) is > 247 chars. The documentation on the MS site isn't very completely and I think there are a number points that require clarification to fully understand how this will work with relative, directly relative and drive relative paths.
>>>
>> 
>> Maybe it would be better to just use the path returned from toString() and do the conversion to UTF-8 externally. That would leave WindowsPath unchanged.
>> 
>>> In the same area, the new PathUtil is a bit inconsistent with the existing provider code. One suggestion is to add a method to AbstractFileSystemProvider instead. That is the base class the platform providers and would be a better place to get the file path in bytes.
>>>
>> 
>> Okay, I gave the method a name that is specific to Unix domain sockets because this UTF-8 strangeness is not likely to be useful by other components.
>> 
>>> One other comment on the changes to the file system provider it should be okay to change UnixUserPrinipals ad its fromUid and fromGid method to be public. This would mean that the patch shouldn't need to add UnixUserGroup (the main issue is that class is that it means we end up with two classes with static factory methods doing the same thing). 
>> 
>> Okay, that does simplify it a bit.
>> 
>> Thanks,
>> Michael.
>> 
>>> -Alan.
>>>
>>>
>>>
>>>
>>>
>>>
>
> Michael McMahon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 36 additional commits since the last revision:
> 
>  - Merge branch 'master' into unixdomainchannels
>  - updated one error in review
>  - further test update from Daniel
>  - forgot to stage updated test files in last commit
>  - update from Daniel's test comments
>  - Merge branch 'master' into unixdomainchannels
>  - final feedback from Alan
>  - Merge branch 'master' into unixdomainchannels
>  - - Alan's review patch from Oct 18
>    - Reverted changes to JFR unit tests
>  - - added NullTest and fix in SocketChannel.open
>    - updated test bug ids
>  - ... and 26 more: https://git.openjdk.java.net/jdk/compare/62cfdb45...01d7cf53

Changes requested by chegar (Reviewer).

test/jdk/java/nio/channels/unixdomain/policy1 line 1:

> 1: grant {

It is often the case that a copyright and license header is added to such policy files, policy1, policy2, policy3

test/jdk/java/nio/channels/unixdomain/NonBlockingAccept.java line 59:

> 57:             serverSocketChannel.bind(null);
> 58:             SocketChannel socketChannel = serverSocketChannel.accept();
> 59:             System.out.println("The socketChannel is : expected Null " + socketChannel);

The null-ness of the returned socket channel be asserted, regardless of whether or not a prior version of the code threw an exception.

test/jdk/java/nio/channels/unixdomain/IOExchanges.java line 49:

> 47: 
> 48: public class IOExchanges {
> 49:     static boolean supported = true;

This should really be `unixDomainSupported` or some such, to indication that it us used to seed the set of protocol families to test with; IP-only, or IP+Unix.  When I wrote this test originally, the intent was that it could operate with channels of different protocol types, IP, rsocket, and now Unix. Happy to see that it is finally being integrated. Maybe update the copyright year range to start at 2018, see https://cr.openjdk.java.net/~chegar/rsocket/IOExchanges/test/jdk/jdk/net/RdmaSockets/rsocket/SocketChannel/IOExchanges.java.html

-------------

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