Pushed, with a few additional stylistic edits. http://hg.openjdk.java.net/jdk9/dev/jdk/rev/fe3e1508653e
On 12 May 2016, at 12:00, Langer, Christoph <christoph.lan...@sap.com> wrote: > 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 I omitted this from the current set of cleanups. It can be done later, if needed. -Chris. > 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.hega...@oracle.com] >> Sent: Donnerstag, 12. Mai 2016 12:43 >> To: Langer, Christoph <christoph.lan...@sap.com> >> Cc: Alan Bateman <alan.bate...@oracle.com>; Dmitry Samersoff >> <dmitry.samers...@oracle.com>; Mark Sheppard >> <mark.shepp...@oracle.com>; net-dev@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.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. >
