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

[Langer, Christoph]

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.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.