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

[Chris Hegarty]

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
>
>
>