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. I also changed the > 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. > > > > > > ------------- Commit messages: - unixdomainchannels: fixing compile error on Windows and Alan's review comment this morning - unixdomainchannels: initial commit from hg sandbox with changes from Alan's email 06/09/2020 Changes: https://git.openjdk.java.net/jdk/pull/52/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8245194 Stats: 6960 lines in 87 files changed: 5902 ins; 763 del; 295 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
