On Mon, 19 Oct 2020 22:13:40 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 incrementally with one > additional commit since the last revision: > > final feedback from Alan The tests mostly look good to me. There's a general cleanup needed to make sure channels get closed properly in case of exception (that is - use try-with-resources whenever possible). test/jdk/java/net/UnixDomainSocketAddress/AddressTest.java line 43: > 41: public class AddressTest { > 42: > 43: static UnixDomainSocketAddress addr; This static seems to be unused? test/jdk/java/net/UnixDomainSocketAddress/AddressTest.java line 29: > 27: * @compile ../../nio/file/spi/TestProvider.java AddressTest.java > 28: * @run testng/othervm AddressTest > 29: */ Could add `@summary` - IIUC this test checks that a Path that doesn't come from the build in filesystem is rejected by `UnixDomainSocketAddress`? test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/Launcher.java line 85: > 83: sc2.close(); > 84: Files.delete(addr.getPath()); > 85: ssc.close(); Should this use nested try-with-resources to close `ssc` and `sc2`? test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/Launcher.java line 107: > 105: sc2.close(); > 106: ssc.close(); > 107: return sc1; Here again nested try-with-resources could be used to close sc2 and ssc. test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/Launcher.java line 136: > 134: ssc.close(); > 135: InetSocketAddress isa = new > InetSocketAddress(InetAddress.getLocalHost(), port); > 136: return SocketChannel.open(isa); dito. test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/Launcher.java line 144: > 142: var addr = ssc.getLocalAddress(); > 143: launch(className, null, null, Util.getFD(ssc)); > 144: ssc.close(); Use try-with-resource? test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/Launcher.java line 170: > 168: String[] > options, > 169: String... > args) > 170: throws IOException { Using try-with-resource or try-finally to make sure `dc` is closed properly should be envisaged. test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/StateTest.java line 172: > 170: /* > 171: * Launch service with a SocketChannel (tcp nowait) > 172: */ try-with-resources or try-finally could be used in this file too to ensure that everything gets properly closed. test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/UnixDomainChannelTest.java line 56: > 54: ByteBuffer buf = > ByteBuffer.wrap(result.getBytes(ISO_8859_1)); > 55: sc.write(buf); > 56: } Should the test fail if `channel` is not an instance of `SocketChannel`? Or at least print some kind of debug information? Do we want to verify that it's a Unix Domain Socket Channel - e.g. by looking at the local address? Same question for the server socket case below... test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/UnixDomainChannelTest.java line 78: > 76: // Test with a named connected socket > 77: private static void test1() throws Exception { > 78: ServerSocketChannel listener = ServerSocketChannel.open(UNIX); Maybe try with resources or try-finally could be used to ensure that all channels are closed. test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/UnixSocketTest.java line 53: > 51: public static class Child2 { > 52: public static void main(String[] args) throws Exception { > 53: ServerSocketChannel server = > (ServerSocketChannel)System.inheritedChannel(); Maybe use try-with-resources test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/UnixSocketTest.java line 41: > 39: public static class Child1 { > 40: public static void main(String[] args) throws Exception { > 41: SocketChannel chan = (SocketChannel)System.inheritedChannel(); try-with-resources? test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/UnixSocketTest.java line 68: > 66: > 67: public static void main(String args[]) throws Exception { > 68: SocketChannel sc = > Launcher.launchWithUnixSocketChannel("UnixSocketTest$Child1"); try-with-resources? test/jdk/java/nio/channels/unixdomain/Bind.java line 118: > 116: client.close(); > 117: if (accept1 != null) > 118: accept1.close(); Maybe each of these `close()` calls should be in its own try-catch. test/jdk/java/nio/channels/unixdomain/Bind.java line 132: > 130: static void assertAddress(SocketAddress a, UnixDomainSocketAddress > a1, String s) { > 131: if (!(a instanceof UnixDomainSocketAddress)) { > 132: throw new RuntimeException("wrong address type"); It would be useful to which wrong address type was observed. test/jdk/java/nio/channels/unixdomain/Bind.java line 136: > 134: UnixDomainSocketAddress ua = (UnixDomainSocketAddress)a; > 135: if (!a.equals(a1)) > 136: throw new RuntimeException("this is not the " + s + " > address"); Could you consider turning this test into a `testng` test so that asssertEquals can be used? Alternatively if you want to retain the ability to run the test as a standalone java program out of the repository then consider modifying the assertXxxx methods so that the exception message prints both parts: `<what>` is not equal to `<what>` test/jdk/java/nio/channels/unixdomain/Bind.java line 140: > 138: > 139: static void assertEquals(Object a, Object b) { > 140: if (!a.equals(b)) Should this call Objects.equals instead? test/jdk/java/nio/channels/unixdomain/NonBlockingAccept.java line 61: > 59: System.out.println("The socketChannel is : expected Null " + > socketChannel); > 60: // exception could be thrown otherwise > 61: } Do we have/need a test that goes the whole way down? Register for OP_CONNECT with a selector and wait for the connect to finish? test/jdk/java/nio/channels/unixdomain/Security.java line 127: > 125: final UnixDomainSocketAddress saddr = > UnixDomainSocketAddress.of(servername); > 126: final ServerSocketChannel server = > ServerSocketChannel.open(UNIX); > 127: final SocketChannel client = SocketChannel.open(UNIX); Maybe try-with-resources should be used and a finally too to ensure that the file is deleted... test/jdk/java/nio/channels/unixdomain/Security.java line 145: > 143: final UnixDomainSocketAddress saddr = > UnixDomainSocketAddress.of(servername); > 144: final ServerSocketChannel server = > ServerSocketChannel.open(UNIX); > 145: final SocketChannel client = SocketChannel.open(UNIX); Same remark as above test/jdk/java/nio/channels/unixdomain/Security.java line 163: > 161: s1.bind(saddr); > 162: var s2 = ServerSocketChannel.open(UNIX); > 163: s2.bind(null); same remark as above test/jdk/java/nio/channels/unixdomain/SocketOptions.java line 59: > 57: s.bind(null); > 58: UnixDomainSocketAddress addr = > (UnixDomainSocketAddress)s.getLocalAddress(); > 59: SocketChannel c = SocketChannel.open(addr); try-with-resources? ------------- PR: https://git.openjdk.java.net/jdk/pull/52
