RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c

[Langer, Christoph]

Thanks Chris.

When you finally wrap it up, maybe you want to have a look at one thing which I didn't dare to clean up. In line 28 you find:

#if defined(_ALLBSD_SOURCE) && defined(__OpenBSD__)
#include <sys/types.h>
#endif

But <sys/types.h> will also be included unconditionally for all platforms in line 34. So maybe this part for OpenBSD could be removed.
It would change the include order on OpenBSD, of course. As I don't have an OpenBSD build environment I decided not to touch this but you could remove it if you have more confidence...

Best regards
Christoph


> -----Original Message-----
> From: Chris Hegarty [mailto:chris.hegarty at oracle.com]
> Sent: Donnerstag, 12. Mai 2016 12:43
> To: Langer, Christoph <christoph.langer at sap.com>
> Cc: Alan Bateman <Alan.Bateman at oracle.com>; Dmitry Samersoff
> <dmitry.samersoff at oracle.com>; Mark Sheppard
> <mark.sheppard at oracle.com>; net-dev at openjdk.java.net
> Subject: Re: RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c
> 
> On 11 May 2016, at 22:27, Langer, Christoph <christoph.langer at sap.com>
> wrote:
> 
> > Hi again,
> >
> > I'm done with the new version:
> http://cr.openjdk.java.net/~clanger/webrevs/8156521.1/
> >
> > I now tried to break lines that are too long and also fixed some other space
> and indentation issues.
> >
> > To incorporate Mark's suggestions regarding plen in enumIPv6Interfaces, I
> consistently renamed it to "prefix" in all places, also in the prefix function for
> BSD. I also reverted back to scanf as int but then cast prefix to short on all
> relevant calls to addif.
> >
> > I verified the build on Linux, AIX, Solaris and MAC and ran a basic
> NetworkInterface test to list interfaces and print addresses and attributes. I
> think I'm done.
> >
> > Chris, would you be so kind to push it when you consider it reviewed? I'm still
> only author.
> 
> I will take a final pass over the updated webrev, do some testing, and then
> push it for you.
> 
> -Chris.
> 
> > Thanks and best regards
> > Christoph
> >
> >
> >> -----Original Message-----
> >> From: Chris Hegarty [mailto:chris.hegarty at oracle.com]
> >> Sent: Mittwoch, 11. Mai 2016 12:25
> >> To: Langer, Christoph <christoph.langer at sap.com>
> >> Cc: Alan Bateman <Alan.Bateman at oracle.com>; Dmitry Samersoff
> >> <dmitry.samersoff at oracle.com>; net-dev at openjdk.java.net
> >> Subject: Re: RFR(S): 8156521: Minor Fixes and cleanups in
> NetworkInterface.c
> >>
> >>
> >> On 11 May 2016, at 10:21, Langer, Christoph <christoph.langer at sap.com>
> >> wrote:
> >>
> >>> Hi,
> >>>
> >>> @Chris: As for your points:
> >>>
> >>>> I agree with the replacement of strcpy with strncpy, but I think we should
> >>>> explicitly null terminate in case the src is greater or equal to the dst
> buffer
> >>>> size. This is done elsewhere in this file too, e.g.
> >>>>
> >>>>  strncpy(buf, input, sizeof(buf) - 1);
> >>>>  buf[sizeof(buf) - 1] = '\0';
> >>>
> >>> There are 2 different patterns of strncpy usage in this code. One is where
> we
> >> would use strncpy giving the full buffer length and eventually setting the last
> >> byte to 0 to make sure the string is terminated. The other pattern is where a
> >> memset to 0 of the buffer was done before. So I thought it's fine here to give
> >> strncpy "buffersize - 1" as length since per its documentation it would copy
> all
> >> characters up to bufferlen and not terminate with 0 if strlen is >= bufferlen.
> >> That makes sense?
> >>
> >> It does. It was not clear to me that we were calling memset for ALL of these
> >> cases
> >> when I made the comment, but I carefully checked all your changes from
> strcpy
> >> to
> >> strncpy, and they do indeed look fine.
> >>
> >>>>>> Apart from quite a few whitespace changes to clean up the coding, I
> went
> >>>>>> through and replaced all occurences of strcpy with strncpy as this was a
> >>>>>> finding of a code scanner that we used. Also in function
> >>>>>> "enumIPv6Interfaces" for Linux the local variable plen was changed
> from
> >>>>>> int to short.
> >>>>
> >>>> Why was this done for plen specifically, and not scope, or others ?
> >>>
> >>> In struct _netaddr the mask field is defined as short and hence short is
> >> expected by the addif function. So plen should be a short in
> enumIPv6Interfaces
> >> for Linux, too.
> >>
> >> Ok, that's fine.
> >>
> >>> While looking again I see that the BSD function "static int prefix(void *val,
> int
> >> size)" should also rather be a "static short prefix(void *val, int size)". Shall I
> >> update this as well?
> >>
> >> Probably.
> >>
> >>> @Alan: As for the line length: If I get the feedback on those 2 points I'll
> >> prepare a new webrev to shorten some lines.
> >>
> >> That would be great, thanks.
> >>
> >> -Chris.