RFR(L): 8167295: Further cleanup to the native parts of libnet/libnio

[Langer, Christoph]

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.hegarty at oracle.com]
> Sent: Montag, 10. Oktober 2016 14:08
> To: Langer, Christoph <christoph.langer at sap.com>; net-dev at openjdk.java.net
> Cc: nio-dev at 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 at openjdk.java.net
> > *Cc:* nio-dev at 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
> >
> >
> >