RFR 8245194: Unix domain socket channel implementation

[Michael McMahon]

On 14/09/2020 09:52, Alan Bateman wrote:
> I'm still working through all the changes from the webrev (there are 
> 87 files changed and require a lot of effort to review). I realize the 
> PR has an update, I'll get to that. For now, this mail is mostly on 
> the changes to the existing SocketChannel and ServerSocketChannel 
> implementations.
>
> I don't particularly like the splitting up of SocketChannelImpl and 
> ServerSocketChannelImpl but I agree it's probably the best approach to 
> avoid having duplicate code in two implementations (as I think one of 
> the early iterations had). So let's go with that.
>
> The split means there is a round or two needed of cleanup. The 
> ordering of the XXX and implXXX is inconsistent everywhere so let's 
> try to fix that. Also the imports were copied and I assume should be 
> trimmed. It would be good to use @Override consistently as it's really 
> hard in some case to know what is being overridden in the Inet* and 
> Unix* classes. I don't like it that finishAccept is no longer private 
> but maybe it can be final to avoid mistakes?
>
I was able to make that private again. I think I have addressed the 
method ordering and @overrides also.
> I think UnixDomainNet should be renamed, maybe UnixDomainSockets 
> because it defines static methods to operate on Unix domain sockets. 
> The JNI natives are in UnixNet so I think they can be renamed too 
> (btw: libnio should not be exporting NET_* functions, I'll get to that 
> in the next mail). UnixDomainHelper is also a bit of an outlier, I 
> think we'll need to find a better name or solution for that the 
> charset issue.
>
Okay on UnixDomainNet -> UnixDomainSockets. Haven't changed 
UnixDomainHelper yet


> Why does sun.nio.ch.Net need a static initializer to initialize 
> UnixDomainNet? Maybe this is left over from a previous iteration.
>
Right, no longer needed, after a small adjustment to some tests.
> The Unix version of Net.localAddress has been changed to check for a 
> Unix domain socket, is this needed?
>
No
> The Windows version of Net.c introduces "sockaddrall". Is that still 
> needed? If so then I think it will require broader changes.
>
I didn't really want to burden the Inet channels with the extra memory 
requirement imposed by sockaddr_un.
But, actually it wasn't necessary to define a union there at all. So, I 
have removed it and replaced with
direct uses of sockaddr_un which is a lot clearer.


> That's all for this area. I've been through the Unix domain socket 
> implementation too, I'll try to write down comments on that shortly.
>
Thanks, the latest webrev should appear soon as revision 04 at the PR 
https://github.com/openjdk/jdk/pull/52

- Michael