Re: RFR: 8245194: Unix domain socket channel implementation [v14]
On Mon, 5 Oct 2020 12:58:52 GMT, Erik Joelsson wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> - simplified Copy.gmk to CAT source files directly
>> - renamed net.properties source files to all be net.properties
>
> Build changes look good.
Thanks again Alan. Assume where there is no comment from me below that
suggestions are accepted:
I will push an update based on these changes soon.
Michael.
> _Mailing list message from [Alan Bateman](mailto:alan.bate...@oracle.com) on
> [nio-dev](mailto:nio-...@openjdk.java.net):_
> On 04/10/2020 14:14, Michael McMahon wrote:
> Another round of comments, this time on v14.
>
> src/java.base/share/classes/java/net/StandardProtocolFamily.java
> - the enum constant is "UNIX" and might be better to put "Local" in
> parentheses rather than "Unix domain".
>
> src/java.base/share/classes/java/nio/channels/package-info.java
> - L260 ends with "family only", I think "only" can be dropped.
>
> src/java.base/share/classes/java/net/UnixDomainSocketAddress.java
> - left over "fix message" comments in constructor
> - a few formatting nits in the equals method due to a spurious space in
> the expression at L192, and missing a space at L194.
>
I don't see a missing space at L194 ?
> src/java.base/share/classes/sun/net/util/SocketExceptions.java
> - Can of(IOException e, SocketAddress addr) check enhancedExceptionText
> to avoid the ugly check in the ofXXX methods?
> - Can you explain the inconsistency in the null handling for the address
> types, maybe a left over from an early prototype?
>
The null check was always there for Inet sockets. It is not required for Unix
but would be benign. So, the check can be promoted to the calling method.
> src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java
> src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java
> - I think we need to change the class descriptions of both to start with
> "Base implementations ..."
> - We need to find a clean way to push the socket() method down to the
> InetXXXImpl classes as they are the only classes that should know about
> the socket adaptors.
Only way to do this is to define yet another implXXX method (implSocket)
but I think it is worthwhile as neither the field nor the implementation
belong in the super class
>
> src/java.base/share/classes/sun/nio/ch/UnixDomainServerSocketChannelImpl.java
> - implBind should not loop/retry when a local address is provided
It actually catches the BindException and rethrows it when 'local != null, but
maybe
the comment above the loop is confusing. I will reword it.
> - what behavior is expected when getTempDir returns null?
I was assuming that ${java.io.tmpdir} would always be defined, but I see
getTempDir() could be more robust in dealing with errors in the preceding
search steps. So, in the unlikely event that ${java.io.tmpdir} is not defined
null would be returned. I will change to throw a BindException in that case.
> - Why doesn't this code use SecureRandom, as is done in the Windows
> Selector implementation.
It does use SecureRandom but only uses the Random api.
> - Can supportedOptions be changed to return
> Set.of(StandardSocketOptions.SO_RCVBUF)?
I can simplify that code definitely, but I think for consistency it should
still return a static unmodifiable instance
> - toString has a left over "TODO".
> - The message for the IOException in implBind should start with a
> capital to be consistent with the other exceptions thrown in this area
>
> src/java.base/share/classes/sun/nio/ch/UnixDomainSockets.java
> - The initializer sets the system property
> "jdk.nio.channels.unixdomain.maxnamelength". Looks like it's to help the
> tests so need to find another solution for the tests.
Okay, the tests didn't really need this anyway
> - checkCapability should be renamed to checkPermission as it does a
> security manager check. Would it more be consistent with existing code
> if changed to "if (sm != null) sm.checkPermission(...)"
> - Can inTempDirectory be removed as it doesn't seem to be used and
> raises several questions if it is needed.
> - getTempName should use the path separator rather than "/".? Also I
> don't think "nio" should be in the name.
> - Can the init method be removed, may be a left over from a previous
> iteration.
> - The left breaks in the initialization of UNNAMED and support seem
> unnecessary, maybe they were very long in previous iterations?
> - Several methods are public, is that intentional, or maybe a left over?
>
> src/java.base/unix/native/libnio/ch/UnixDomainSockets.c
> => Is the intention to put the NET_ functions into libnet or libnio? The
> function prototypes are declared in net_util.h but the functions have
> been compiled in libnio.
>
They were probably in libnet originally, but makes more sense to be
in libnio. So, I will move the function prototypes to nio_util.h and rename
them to remove the NET_
Re: RFR: 8245194: Unix domain socket channel implementation [v14]
On Sun, 4 Oct 2020 13:14:49 GMT, Michael McMahon 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: > > - simplified Copy.gmk to CAT source files directly > - renamed net.properties source files to all be net.properties Build changes look good. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v14]
> 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: - simplified Copy.gmk to CAT source files directly - renamed net.properties source files to all be net.properties - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/dc70cd6e..0096645e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=52=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=52=12-13 Stats: 9 lines in 4 files changed: 0 ins; 4 del; 5 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
