Thanks you Dan for the comments. My responses are inline. I have created a new workspace that reflects the comments made by Seb and Dan. Along with that some other changes have also been made to reflect comments I got from Meem.
webrev: /net/zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-cr-changes/webrev reflects changes made after the codereview comments received from Seb and Dan. /net/zhadum/export/build1/ws/sshakya/clearview-libdlpi-cr-changes/webrev_clearviewgate reflects change to the clearview gate. The workspace is at: /net/zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-cr-changes/webrev cscope database are at: /net/zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-cr-changes/usr/src Dan Groves wrote: > - usr/src/cmd/cmd-inet/usr.sbin/ifconfig/revarp.c > > * lines 318-327 - I think you leaked whatever mybaddr pointed to. > At this point, both mybaddr and my_addr point to NULL so I didn't free anything. > * lines 341 & 357 - another potential memory leak > Why would this be a potential memory leak as 'fail' would take care of this? Or is there something that I am missing? > - usr/src/cmd/cmd-inet/usr.sbin/snoop/snoop_capture.c > > * line 165 - The types don't match in the comparison. > interface->mac_type is an unsigned integer, and dlinfo.di_mactype is > an unsigned char. Minor since I'd hope the compiler would do the > right thing in this case. > the compiler is happy I have left it as. > * line 171-172 - The l format specifier isn't needed because the > data type is an unsigned char. fixed. > > * line 245 - sizeof(linkname) gets you the size of a pointer. I > thikn you should use DLPI_LINKNAME_MAX instead. > That's true but I've changed from a char * to char[] so that takes care of the issue. > - usr/src/cmd/cmd-inet/usr.sbin/syncstat.c > * Line 130 - I really don't think that comment should be there. > :) yes, that's been fixed. > - usr/src/cmd/iscsi/iscsitgtd/util_ifname.c > > * The new version is much easier to understand. > Certainly and hope the rest of the application as similar. > - usr/src/lib/libdlpi/amd64/Makefile > > * Shouldn't the version in the ident string be 1.1? > Will be fixed when I collapse the deltas. > - usr/src/lib/libdlpi/common/libdlpi.c > > * lines 182 - 185 - Could this be condensed? For example, > something along the lines of: > > retval = i_dlpi_open(dip->modcnt ? dip->provider : dip->linkname, > &fd, flag); > Fixed. > * lines 187-192 - Which case is going to be more common? I'd put > that first. On Intel (and maybe AMD, I couldn't find any > documentation on AMD's branch prediction) chips, putting the most > common case first in an if-else statement leads to better performance > (see http://www.intel.com/cd/ids/developer/asmo-na/eng/66779.htm?prn=Y > if you're interested to see why). > Thank you for the data. I think STYLE1 would be the more common case. So I've reversed the order. > * line 312 - Is it possible to pass in a linkname that is not NULL > terminated? It seems to me to be impossible, but in case it is, this > line could cause memory corruption. > Since the linkname passed is strlcpy'd, which is always NULL terminated, I don't think this would be the case. > * line 873 - You should check to make sure infop is not NULL before > calling memset on it. > I think a check if infop is NULL should be done at the start of the function so added it at the beginning of the function. > * 1444-1445 - sizeof(provider) is going to return the size of a > pointer on the architecture you're running on. Is this really what > you want here? > Yes, I found this out the hardway. It's been fixed to sizeof (DLPI_LINKNAME_MAX). > - usr/src/lib/libdlpi/common/libdlpi_impl.h > > * Shouldn't the version in the ident string be 1.1? > - usr/src/lib/libdlpi/sparcv9/Makefile > > * Shouldn't the version in the ident string be 1.1? > Will be fixed when I collapse the deltas. thank you, Sagun
