> 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 18 additional commits since
the last revision:
- Merge branch 'master' into unixdomainchannels
- - update after Alan's review on Oct 4
- includes API change required by JDK-8251952
- original CSR for the overall change will be resubmitted with
all api changes consolidated based on this update
- - simplified Copy.gmk to CAT source files directly
- renamed net.properties source files to all be net.properties
- unixdomainchannels: error in the last commit in
make/modules/java.base/Copy.gmk
- unixdomainchannels:
(1) rename UnixDomainHelper to UnixDomainSocketsUtil
(2) remove hardcoded /tmp and /var/tmp paths from UnixDomainSocketsUtil
(3) replace (2) with documented system/networking properties
(4) Small update to UnixDomainSocketAddress API
(5) CSR for (3) and (4) submitted at JDK-8253930
(6) Update build to generate net.properties from shared net.properties.common
plus platform specific additions.
- Merge branch 'master' into unixdomainchannels
- unixdomainchannels: remove more cruft from Windows Net.c
- unixdomainchannels: change to Net.c was missed after all
- unixdomainchannels: typo in WindowsFileSystemProvider.java
- unixdomainchannels: Update after Alan's review of Sept 29
- ... and 8 more: https://git.openjdk.java.net/jdk/compare/96b742dd...36efb46c
-------------
Changes:
- all: https://git.openjdk.java.net/jdk/pull/52/files
- new: https://git.openjdk.java.net/jdk/pull/52/files/17acf10a..36efb46c
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=15
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=14-15
Stats: 15436 lines in 1715 files changed: 5751 ins; 2807 del; 6878 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
