RE: RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c
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 > Cc: Alan Bateman ; Dmitry Samersoff > ; 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 > 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.
Re: RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c
the declaration in of plen (also scope and dad_status) as an int in enumIPv6Interfaces would appear to be done to align with its use in fscanf, even though it is a two character conversion. So would it not be more appropriate to cast it as a short when passed to addif ? the name plen would also convey a variable containing a length, but that doesn't appear to be the purpose of plen. a more appropriate name would be more informative in the code regards Mark On 11/05/2016 09:50, Chris Hegarty wrote: Hi Christoph, On 11 May 2016, at 08:43, Dmitry Samersoff wrote: ... bugreport: https://bugs.openjdk.java.net/browse/JDK-8156521 webrev: http://cr.openjdk.java.net/~clanger/webrevs/8156521.0/ I think this is mainly fine, and good to have such cleanup in this area. 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'; 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 ? -Chris.
Re: RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c
On 11 May 2016, at 10:21, Langer, Christoph 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.
RE: RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c
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? > >> 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. 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? @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. @Dmitry: Will also work in your findings in the update... Thanks for all the feedback Christoph
Re: RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c
On 11/05/2016 09:50, Chris Hegarty wrote: Hi Christoph, On 11 May 2016, at 08:43, Dmitry Samersoff wrote: ... bugreport: https://bugs.openjdk.java.net/browse/JDK-8156521 webrev: http://cr.openjdk.java.net/~clanger/webrevs/8156521.0/ I think this is mainly fine, and good to have such cleanup in this area. I agree on the clean-up. It would be great if we could use the opportunity to clean-up the annoying long lines that have crept into this code recently, this would avoid too much horizontal scrolling when looks at the changes side-by-side. -Alan.
Re: RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c
Hi Christoph, On 11 May 2016, at 08:43, Dmitry Samersoff wrote: >> ... >> bugreport: https://bugs.openjdk.java.net/browse/JDK-8156521 >> >> webrev: http://cr.openjdk.java.net/~clanger/webrevs/8156521.0/ I think this is mainly fine, and good to have such cleanup in this area. 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'; >> 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 ? -Chris.
Re: RFR 8154234: Examine if netdoc Handler can be removed
On 11 May 2016, at 07:44, vyom wrote: > Hi All, > > Please review the following simple fix for the > issue(https://bugs.openjdk.java.net/browse/JDK-8154234). > >hg rm src/java.base/share/classes/sun/net/www/protocol/netdoc/Handler.java This seems fine to me Vyom. Thanks. -Chris.
Re: RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c
Christoph, Looks good for me (Reviewed). Few more space nits (no need to re-review) 1141 Missed space around = in i=0 1890 extra space after ( 2068 wrong indent -Dmitry On 2016-05-10 12:54, Langer, Christoph wrote: > Hi all, > > > > can I please get a review for a change to NetworkInterface.c > > > > bugreport: https://bugs.openjdk.java.net/browse/JDK-8156521 > > webrev: http://cr.openjdk.java.net/~clanger/webrevs/8156521.0/ > > > > 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. > > > > I ran builds on Linux, AIX, Solaris Sparc and Darwin to make sure > nothing broke. > > > > Thanks in advance > > Christoph > > > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
