[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-nio-dev
Subject:    RE: RFR(L): 8167295: Further cleanup to the native parts of libnet/libnio
From:       "Langer, Christoph" <christoph.langer () sap ! com>
Date:       2016-10-10 20:51:48
Message-ID: 1e35891dffd040c9ba582a6c5bef7e01 () DEWDFE13DE11 ! global ! corp ! sap
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]