On Tue, 13 Oct 2020 19:09:44 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 22 additional commits since > the last revision: > - Merge branch 'master' into unixdomainchannels > - - reorganised the channel impls back into SocketChannelImpl and > ServerSocketChannelImpl > - removed the new Unix domain socket events and folded the behavior into > the existing socket events > - implemented other comments from Alan on Oct 11. > - unixdomainchannels: updates from Chris's review 9 Oct 2020 > - unixdomainchannels: > - updated property name > - added JFR unit test > - 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 > - ... and 12 more: > https://git.openjdk.java.net/jdk/compare/bc3cab15...7f677d5a src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java line 63: > 61: /** Return the, possibly empty, set of extended socket options > available. */ > 62: public final Set<SocketOption<?>> unixOptions() { return unixOptions; > } > 63: Could the name of the method - or alternatively its API doc comment - be improved to make it clear that these options are **Unix Domain protocol** specific option (as opposed to Unix OS specific option). For instance, the field and method could be named: `unixDomainOptions` instead of `unixOptions`? src/java.base/share/classes/sun/nio/ch/ServerSocketAdaptor.java line 96: > 94: @Override > 95: public InetAddress getInetAddress() { > 96: InetSocketAddress local = (InetSocketAddress)ssc.localAddress(); Idle wondering as to whether ServerSocketChannelImpl could be parameterized to avoid casts... `... class ServerSocketChannelImpl<SA extends SocketAddress> extends ...` Or maybe that would just make things more complex. Have you considered it and rejected it? src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java line 99: > 97: // Binding > 98: private SocketAddress localAddress; // null => unbound > 99: See my previous comment about parameterization. src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java line 267: > 265: HashSet<SocketOption<?>> set = new HashSet<>(); > 266: set.add(StandardSocketOptions.SO_RCVBUF); > 267: return Collections.unmodifiableSet(set); Wondering: should there be a call to `ExtendedSocketOptions` after line 266 here, even if that call just returns an empty set? src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java line 430: > 428: > 429: protected int implAcceptUnix(FileDescriptor fd, FileDescriptor > newfd, SocketAddress[] addrs) > 430: throws IOException Does this method needs to be `protected` ? I couldn't find a place where it was called outside of this file. By contrast `implAcceptInet` was declared `private` above... src/java.base/share/classes/sun/nio/ch/SocketAdaptor.java line 110: > 108: public InetAddress getInetAddress() { > 109: InetSocketAddress remote = (InetSocketAddress)sc.remoteAddress(); > 110: if (remote == null) { I guess I'll have the same question about parameterizing `SocketChannelImpl`. Was it considered and rejected? src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 152: > 150: this.family = family; > 151: this.fdVal = IOUtil.fdVal(fd); > 152: } If you introduced: private static FileDescriptor socketFor(ProtocolFamily family) throws IOException ... and private SocketChannelImpl(SelectorProvider sp, ProtocolFamily family, FileDescriptor fd) throws IOException { super(sp); this.family = family; this.fd = fd; this.fdVal = IOUtil.fdVal(fd); } then you could move all the checking to `socketFor(ProtocolFamilly)` and write this constructor as: SocketChannelImpl(SelectorProvider sp, ProtocolFamily family) throws IOException { this(sp, family, sockectFor(family)); } which would push all the validation to **before** the object gets constructed. src/java.base/unix/native/libnio/ch/UnixDomainSockets.c line 65: > 63: if (namelen != 0) { > 64: (*env)->SetByteArrayRegion(env, name, 0, namelen, > (jbyte*)sa->sun_path); > 65: } Should the exception status be checked after calling `SetByteArrayRegion`? src/java.base/unix/native/libnio/ch/UnixDomainSockets.c line 76: > 74: sa->sun_family = AF_UNIX; > 75: int ret; > 76: const char* pname = (const char *)(*env)->GetByteArrayElements(env, > path, NULL); Should `pname == NULL` be checked? src/java.base/windows/native/libnio/ch/UnixDomainSockets.c line 48: > 46: if (name != NULL) { > 47: (*env)->SetByteArrayRegion(env, name, 0, namelen, > (jbyte*)sa->sun_path); > 48: } Same remark here - about exception status src/java.base/windows/native/libnio/ch/UnixDomainSockets.c line 66: > 64: jboolean isCopy; > 65: char *pname = (*env)->GetByteArrayElements(env, addr, &isCopy); > 66: ... and here about checking for `pname == NULL` ------------- PR: https://git.openjdk.java.net/jdk/pull/52
