On Fri, 11 Sep 2020 14:41:32 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. >>> >>> >>> >>> >>> >>> > >> _Mailing list message from [Alan Bateman](mailto:alan.bate...@oracle.com) on >> [nio-dev](mailto:nio-...@openjdk.java.net):_ >> On 07/09/2020 13:14, Michael McMahon wrote: >> >> > > 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. >> >> The file system provider shouldn't know anything about Unix domain >> sockets so I prefer not to have "UnixDomain" in the name of this method. >> I also would prefer if null is an exception so that it is consistent >> with the other methods. >> > > I'm not sure what to call the method then. It returns a UTF-8 string > converted to bytes on Windows and the output of getByteArrayForSysCalls on > Unix. > It could be called getByteArrayUTF8 on Windows, but that would not > be right on Unix. > > Since sockets are file system entities, why not have socket in the method > name? > > What about getByteArrayForSocket() ? > >> I think it would be useful to also summarize how the bind/connect works >> on Windows. For example, suppose the working directory is 240 characters >> and the UnixDomainSocketAddress is to a simple relative path of 20 >> characters. If I understand correctly, the proposal will encode the >> simple relative path (not the resolved absolute path) to bytes using >> UTF-8 so it will probably be 20+ bytes in this case. > > Right > >> This hould be okay >> but inconsistent with the rest of? file system implementation which will >> use the long form. Also if the name is long then it won't use the long >> path prefix (\?) but instead rely on failure because the resulting >> sequence of bytes when encoded is > 100, is that correct? >> > > Yes, that is how it is working currently. We check the length in native code > just before calling bind(). Whether the long path prefix is present or not > won't matter because the path will not be used once it is longer than ~100 > bytes. > > Michael. >> -Alan. Hi Alan, Thanks for the comments. Assume all are accepted, except as queried below > _Mailing list message from [Alan Bateman](mailto:alan.bate...@oracle.com) on > [nio-dev](mailto:nio-...@openjdk.java.net):_ > On 24/09/2020 13:17, Michael McMahon wrote: > I've pulled the latest commits from jdk/pull/55 and have another set of > comments. > > src/java.base/share/classes/sun/nio/ch/InetSocketChannelImpl.java > src/java.base/share/classes/sun/nio/ch/InetServerSocketChannelImpl.java > - class level comment is out dated after the split so should be updated > to make it clear that it for Inet implementations. > - the comment "End of field protected by stateLock" has been copied down > from ServerSocketChannelImpl.java and is confusing here so I think > should be removed. > - The existing convention is to declare the final fields at the top so > best to keep consistent after the split. > > src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java > - the change to isStreamOption would be cleaned if the "SO_FLOW_SLA" > case is removed (I think it was missed by JDK-8244582). > > src/java.base/share/classes/sun/net/util/SocketExceptions.java > - it's a bit inconsistent for of(IOException, SocketAddress) to handle > InetSocketAddress types itself, it would be cleaner to handle both > address types or use two helper methods. > - Is the null check in ofUnixDomain left over from a previous iteration? > > src/java.base/share/classes/sun/nio/ch/SelectorProviderImpl.java > - it would be help to split the really long lines to keep it consistent > with the existing code > > src/java.base/share/classes/sun/nio/fs/AbstractFileSystemProvider.java > - it might be cleaner to rename the method to getSunPathForSocketFile. > It doesn't access the file so would be better not to throw IOException. > - for the description it might be clearer to say that it "Returns a path > name as bytes for ..." > > src/java.base/share/native/libnet/net_util.h > - are you sure that struct sockaddr_un is defined on all platforms, > including slightly older VC++ releases. struct sockaddr_un is declared in <afunix.h> on Windows and <sys/un.h> on Unix. So, it is included via net_util_md.h I suspect the definition is only in fairly recent SDKs on Windows, and I'm not sure what I can do about that. If <afunix.h> is present it will have the definition. If it's not present, then there will be a compile error, but I don't know how to tell if it is present.. > - the lines are a bit long here, inconsistent with the existing code. > > src/java.base/unix/classes/sun/nio/ch/UnixDomainHelper.java > - I think this should be removed and the method included in > UnixDomainSockets > - The hardcoding of /tmp and /var/tmp needs to be dropped too. We also > need to find a name name for jdk.nio.channels.tmpdir as this is specific > to Unix domain sockets. The problem here is that the Java impl of UnixDomainHelper is platform specific, but UnixDomainSockets is shared, but with platform specific native methods. If I make UnixDomainSockets.java platform specific then there will be a lot of duplication of code. Maybe we could pick a better name than UnixDomainHelper? As regards hard-coding of /tmp and /var/tmp, I think we need to be very careful to ensure that the directory name used to hold these automatically bound sockets is as short as possible. ${java.io.tmpdir} is sometimes long enough on our test systems to push the names over the ~100 byte limit. I don't see a good alternative to using /tmp or /var/tmp on Unix On Windows %TEMP% seems to be always defined and seems to always refer to a directory with a short name. What about "jdk.nio.unixdomain.tmpdir" for the property name? > - you can avoid the ugly cast by creating a PrivilegedAction and then > return doPrivileged(pa). > > src/java.base/unix/classes/sun/nio/fs/UnixFileSystemProvider.java > - if you rename the parameter to obj and use UnixPath file = > UnixPath.toUnixPath(obj) then it will be consistent with the existing > code. It will also ensure that ProviderMismatchException is thrown. > > src/java.base/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java > - this can use WindowsPath file = WindowsPath.toWindowsPathobj) > > src/java.base/windows/classes/sun/nio/ch/PipeImpl.java > src/java.base/windows/classes/sun/nio/ch/SinkChannelImpl.java > - I think the change to SinkChannelImpl.java should be dropped. Instead, > move the setting TCP_NODELAY to the PipeImpl constructor and it should > be a lot cleaner. It also means the "buffered" field is not needed. > - if you rename tryUnixDomain noUnixDomainSockets you avoid the > volatile-write at initialization time > - createListener would be a clean if the flag is checked before the > try/catch. > > src/java.base/windows/native/libnet/net_util_md.h > - what is the include of afunix.h needed, left over? > Explained above <afunix.h> is Windows only. > src/java.base/windows/native/libnio/ch/Net.c > - are these changed needed? I would only checked localInetAddress and > remoteInetAddress to be called on file descriptor to inet sockets. > I had changed this file, but reverted all changes in the last version. I don't know why the webrev is still seeing changes. After this round, hopefully they will be gone. > src/jdk.net/linux/classes/jdk/net/LinuxSocketOptions.java > src/jdk.net/macosx/classes/jdk/net/MacOSXSocketOptions.java > - IOUtil.makePipe return a long that encodes the two file descriptors > and may be simpler to avoid populating the int[] in JNI code. > > test/jdk/com/sun/nio/sctp/SctpServerChannel/NonBlockingAccept.java > - did you mean to change this test? > > test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/ConnectionPoolTest.java > - is this needed? > > I'm still working through new Unix* code and the tests so more comments > soon. > > -Alan Thanks for the review. I'm making the changes as described, except where noted above for further discussion. This should appear as a new webrev revision today. Michael ------------- PR: https://git.openjdk.java.net/jdk/pull/52
