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. Thanks and best regards Christoph > -----Original Message----- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Mittwoch, 11. Mai 2016 12:25 > To: Langer, Christoph <christoph.lan...@sap.com> > Cc: Alan Bateman <alan.bate...@oracle.com>; Dmitry Samersoff > <dmitry.samers...@oracle.com>; net-dev@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.lan...@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.
