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

[Chris Hegarty]

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.