RFR: 8245194: Unix domain socket channel implementation [v17]
Chris Hegarty
chegar at openjdk.java.net
Fri Oct 9 09:18:23 UTC 2020
On Tue, 6 Oct 2020 19:53:18 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 incrementally with one additional commit since the last revision:
>
> unixdomainchannels:
> - updated property name
> - added JFR unit test
src/java.base/share/classes/java/net/UnixDomainSocketAddress.java line 2:
> 1: /*
> 2: * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
Can the copyright year range for 2020 be added please.
src/java.base/share/classes/java/net/UnixDomainSocketAddress.java line 139:
> 137:
> 138: /**
> 139: * Create a UnixDomainSocketAddress from the given path string.
Create*S* a ...
src/java.base/share/classes/java/net/UnixDomainSocketAddress.java line 156:
> 154:
> 155: /**
> 156: * Create a UnixDomainSocketAddress for the given path.
Create*S* a ...
src/java.base/share/classes/java/net/UnixDomainSocketAddress.java line 173:
> 171:
> 172: /**
> 173: * Return this address's path.
Return*S* this ...
test/jdk/java/nio/channels/unixdomain/AddressTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
Can the copyright year range for 2020 be added please.
test/jdk/java/nio/channels/unixdomain/AddressTest.java line 49:
> 47: throw new RuntimeException("Expected illegal path exception");
> 48: } catch (IllegalArgumentException e) {}
> 49: }
Would be a good candidate for a testng test, using assertThrows or similar.
test/jdk/java/nio/channels/unixdomain/IOExchanges.java line 113:
> 111: SPINBAccep_NBConn_NBIO_WR_11a
> 112: SPINBAccep_NBConn_NBIO_RW_12a
> 113: */
I recognise this test ;-) I thought there was a version for IP-specific channels, but cannot find it now. I was going
to ask if these could be merged or abstracted out somehow.
-------------
PR: https://git.openjdk.java.net/jdk/pull/52
More information about the hotspot-jfr-dev
mailing list