On 11 May 2016, at 22:27, Langer, Christoph <christoph.lan...@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.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.
