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

[Michael McMahon]

> 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 31 additional commits since
the last revision:

 - 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
 - - use String.isEmpty where appropriate
 - fix white space error
 - - test updates from Daniel's review 14 Oct 2020
 - src changes from Daniel's review. Tests will come later
 - Merge branch 'master' into unixdomainchannels
 - ... and 21 more: https://git.openjdk.java.net/jdk/compare/73958dea...053bffee

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/52/files
  - new: https://git.openjdk.java.net/jdk/pull/52/files/36623a45..053bffee

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=28
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=27-28

  Stats: 27994 lines in 425 files changed: 18001 ins; 8154 del; 1839 mod
  Patch: https://git.openjdk.java.net/jdk/pull/52.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52

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