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
