Hi Chris, thanks for the review. We couldn't observe issues in our OpenJDK test systems and most of the change was part of the SAP JVM already for quite some time. So I pushed it just now: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/d4f70e7859c7
Some more cleanup is still to come... :) Best regards Christoph > -----Original Message----- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Montag, 10. Oktober 2016 14:08 > To: Langer, Christoph <christoph.lan...@sap.com>; net-dev@openjdk.java.net > Cc: nio-...@openjdk.java.net > Subject: Re: RFR(L): 8167295: Further cleanup to the native parts of > libnet/libnio > > Hi Christoph, > > On 07/10/16 16:17, Langer, Christoph wrote: > > Hi again, > > > > I have respun my patch a little bit: > > http://cr.openjdk.java.net/~clanger/webrevs/8167295.1/ > > This is a nice cleanup and an improvement to the code. Specifically, > adding 'struct sockaddr sa' to SOCKETADDRESS allows for the removal > of many cast operations. > > One minor double space I noticed when reviewing the changes. > > Net.c > 339 sa.sa4.sin_len = sizeof(struct sockaddr_in); > ^^ > > My eyes hurt from this kind of review, e.g. > addrs->addr.sa6.sin6_addr.s6_addr!!! so I put your patch through > our internal build and test system too. All looks good. > > -Chris. > > > > > The reason is that the naming of the members of SOCKETADDRESS should be > > changed to 'sa' instead of 'him' as the fields are of type 'struct > > sockaddr...'. I also did a careful inspection of the places where > > SOCKADDR_LEN resp. SOCKETADDRESS_LEN(x) used to be used and found > that > > it should really be ok if this define is completely dropped and always > > sizeof(SOCKETADDRESS) at these places. Basic testing showed that the > > changes were ok - I'll add the patch to the queue for our nightly > > build/test runs and check for regressions... > > > > > > > > Thanks > > > > Christoph > > > > > > > > > > > > *From:*Langer, Christoph > > *Sent:* Donnerstag, 6. Oktober 2016 17:44 > > *To:* net-dev@openjdk.java.net > > *Cc:* nio-...@openjdk.java.net > > *Subject:* RFR(L): 8167295: Contribute further changes from SAP to > > native parts of libnet/libnio > > > > > > > > Hi, > > > > > > > > I'm looking to contribute a few of our diffs in the network area to OpenJDK. > > > > > > > > This is the bug: https://bugs.openjdk.java.net/browse/JDK-8167295 > > > > This is the webrev: http://cr.openjdk.java.net/~clanger/webrevs/8167295.0/ > > > > > > > > Besides minor cleanups, initializations and fixes, the main thing that > > I'm proposing is the unification of the union datatype SOCKADDR (unix) > > and SOCKETADDRESS (windows). > > > > > > > > I think the definition for all platforms should basically look like the > > following, of course depending on IPv6 support and the local datatypes: > > > > > > > > typedef union { > > > > struct sockaddr him; > > > > struct sockaddr_in him4; > > > > struct sockaddr_in6 him6; > > > > } SOCKETADDRESS; > > > > > > > > The type 'SOCKADDR' is already defined on Windows so we should > > consistently use 'SOCKETADDRESS'. This move would allow for better > > writing of shared code dealing with sockaddr structures, which we do at > > SAP especially for some tracing code. > > > > > > > > I don't know yet if it's a good idea to get rid of the definitions > > SOCKADDR_LEN resp. SOCKETADDRESS_LEN(x) and fully rely on > > sizeof(SOCKETADDRESS). I've done this in my webrev and it builds. But > > I'm not sure if especially some Windows APIs would behave strangely if > > passed in a longer length values for an AF_INET socket address. Maybe > > you have some comments on that point. > > > > > > > > Apart from that, I think it is a good idea to get rid of > > 'NET_AllocSockaddr' as there is no need to malloc SOCKETADDRESS fields > > at the few places where it was used (libnio unix only). A stack variable > > would probably be better and less error prone. And the declaration of > > NET_Wait can move to the common net_util.h header as the function is > > available everywhere with the same signature. > > > > > > > > Right now I'm just at a stage where my change builds on all platforms > > and I need to do some further testing. But I'm hoping for some early > > comments J > > > > > > > > Thanks and best regards > > > > Christoph > > > > > >
