On Wed, 21 Oct 2020 16:31:06 GMT, Michael McMahon <micha...@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/8c9391a4...01d7cf53 Changes requested by chegar (Reviewer). test/jdk/java/nio/channels/unixdomain/Bind.java line 51: > 49: if (!supported()) { > 50: System.out.println("Unix domain channels not supported"); > 51: return; Here it would be better to throw the `jtreg.SkippedException`, rather than silently returning. Same comment for any other tests that check supported-ness before continuing to execute. test/jdk/java/nio/channels/unixdomain/Bind.java line 66: > 64: SocketChannel.open(StandardProtocolFamily.UNIX).close(); > 65: } catch (UnsupportedOperationException e) { > 66: return false; In this case `ServerSocketChannel.open(UNIX)` should also throw UOE. ------------- PR: https://git.openjdk.java.net/jdk/pull/52
