Hi Sagun,
My comments are below. Files that I don't mention I don't have any
comments on. I only have a few comments, most are minor.
- usr/src/cmd/cmd-inet/usr.sbin/ifconfig/revarp.c
* lines 318-327 - I think you leaked whatever mybaddr pointed to.
* lines 341 & 357 - another potential memory leak
- 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.
* line 171-172 - The l format specifier isn't needed because the
data type is an unsigned char.
* line 245 - sizeof(linkname) gets you the size of a pointer. I
thikn you should use DLPI_LINKNAME_MAX instead.
- usr/src/cmd/cmd-inet/usr.sbin/syncstat.c
* Line 130 - I really don't think that comment should be there.
- usr/src/cmd/iscsi/iscsitgtd/util_ifname.c
* The new version is much easier to understand.
- usr/src/lib/libdlpi/amd64/Makefile
* Shouldn't the version in the ident string be 1.1?
- 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);
* 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).
* 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.
* line 873 - You should check to make sure infop is not NULL before
calling memset on it.
* 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?
- 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?
Sagun Shakya wrote:
> Hi,
>
> I've made a few changes and I have regenerated a webrev at the same
> workspace:
>
> http://zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-new/webrev/
>
> I'm including the email I had sent out earlier regarding the test/bugs
> and cscope location.
>
> Thanks,
>
> Sagun
>
> The workspace is at:
> /net/zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-new
>
> cscope database at :
> /net/zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-new/usr/src
>
> How the changes have been tested so far:
> *used the bits to punchin
> *tested with gldv3 testsuite.
> The summary of test is at
> file:///home/ss150715/tmp/gldv3-test-status
> * ran iscsi test-suite for the iscsi changes
>
> *syncinit.c/syncstat.c/syncloop.c are not working as it should so I will
> send an update when that is fixed.
>
> What is different than the proposal:
>
> * DLPI_ENOLINK is not returned in dlpi_open(), I had planned to return
> his error code when opening of device failed but i_dlpi_open() returns
> DL_SYSERR instead with the current code. The reason for this is opening
> of a link could fail for more than just the link not existing. like EPERM.
>
> * add DLPI_ESTYLE1/2 error codes.
>
> I have to file a RFE that will act as a blanket rfe for all these changes.
>
> Sagun Shakya wrote:
>> Hi Seb and Dan,
>>
>> I have been making a few changes to the workspace, so I wanted you to
>> hold off on the code review while I make the changes. I apologize if
>> you have spent sometime reviewing the code. I will let you know once I
>> have made the changes and generated a new webrev.
>
