RE: RFR(L): 8167295: Further cleanup to the native parts of libnet/libnio
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 ; 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
> >
> >
> >
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
RE: RFR(L): 8167295: Further cleanup to the native parts of libnet/libnio
Hi again,
I have respun my patch a little bit:
http://cr.openjdk.java.net/~clanger/webrevs/8167295.1/
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 :)
Thanks and best regards
Christoph
