Thank you for you comments, Seb. 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; I've included a summary of those
comments. Please see separate email for replys to Seb and Dan's comments.
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
Sebastien Roy wrote:
>usr/src/cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.c
>
>3649: why not use IF_NAMESIZE (the standard maximum interface name
> constant)?
>
>
Using a standard constant makes more sense. I've changed it to be a
IF_NAMESIZE.
>3655: please wrap this block around { } brackets.
>
>
>
Done.
>3659, 3774: A comment is warranted regarding why this needs to be done.
>
Before adding comments, I wanted to get opinions about making
dlpi_detach a flag to dlpi_open() instead of providing it as a
Consolidation private interface.
Right now the only consumer of dlpi_detach is ifconfig.c, in order to
make this change the function signature of plumb_one_device() would need
to be changed as well. This would also simplify plumb_one_device() and
inetplumb().
static void
-plumb_one_device(dlpi_if_attr_t *diap, int ip_fd, int af)
*+plumb_one_device(int af)*
Also, since dlpi_attach() is not explicitly called by any applications
there is no need to provide this api as a consolidation private interface.
>3663: dlpi_fd(dh_ip) is called quite a few times in this function. You
> could just call it once at the start and store the fd for later ioctl
> calls.
>
>
Accepted and also made changes similarly for dlpi_fd(dh_arp).
>4395: This is not the length of the provider string (as the name "provlen"
> would suggest), it's one less than that. If you want to have a
> variable named provlen, then set its value to the true string length,
> and then subtract one from that when you want to use it as an index
> into the string. Also, "strlen(provider)" is already done on line
> 4389. Please set provlen above that, and use provlen there as well.
>
>
Changes made as suggested.
>4418: No need for goto here since the label immediately follows it.
>
>
Sorry. removed the goto.
>usr/src/cmd/cmd-inet/usr.sbin/ifconfig/revarp.c
>
>
Found one error at line 201:assignment operator instead of equality
operator. Fixed.
>196: You could use perror() here.
>
>
Accepted. Also, changed for line 319, 342,. 420.
>212: How do you know if errno is set here? In general, I'm confused by the
> errors returned by the libdlpi functions. Why, in some cases, are the
> DLPI_ errors duplicating the existing errno values? How does the
> caller know when to call dlpi_strerror(), and when to call perror() or
> strerror()? If debug is turned on, it would seem that this line is
> redundant as a similar call is made immediately after on line 218.
> Also, why do you use Perrdlpi() is some places in this function, and a
> combination of fprintf() and dlpi_strerror() in others?
>
It should have been a Perrdlpi().
My approach was to use Perrdlpi_exit/Perrdlpi (depending if exit() needs
to be called). It should have been Perrdlpi() instead of the fprintf()
and dlpi_strerror().
As for when debug is turned on, I didn't think this through. Unless I'm
missing something, I don't think the error message is needed since the
error message gets printed above anyways. Removed the redundant debug
error message.
>305: cstyle: separate local variables from the body of the function.
>
>
Fixed.
>352: why are we doing a dlpi_info() again, when we did this on line 313?
>
This was done to get di_physaddr. When the handle is in DL_UNBOUND state
the physaddr is unspecified. So a dlpi_info() was needed after dlpi_bind().
However, I've changed the code to do a bind after the open and removed
the second dlpi_info() call.
>357: why are we only setting *myaddr if debug is true?
>
>
that's an error. Fixed.
>usr/src/cmd/cmd-inet/usr.sbin/snoop/snoop_capture.c
>
>158: this comment is no longer accurate. You're not "rereading".
>
>
Fixed.
>187: raw mode was set earlier in dlpi_open(); this comment needs updating.
>
>
>
Comment has been updated. Also, comment for check_device() line 84 fixed.
>193: please fix this function definition, it's still using K&R C syntax.
>
>
Fixed. Also, other functions that used this syntax.
>251: that's a lot of calls to dlpi_fd(). It would be better to stash the fd
> into a local variable and use that as arguments to various ioctl()
> calls.
>
>
Accepted.
>332: why not dlpi_recv()?
>
>
Yes, that was the whole point of having a dlpi_recv(). I've changed it
to use dlpi_rev() but would like to test and confirm as well.
>usr/src/cmd/cmd-inet/usr.sbin/syncinit.c
>
>
As mentioned earlier, I didn't have sync* stuff working.
It has been fixed now and in order to fix that changes to dlpi_open()
had to be made. The main problem was dlpi_info() was called, while the
synchronous serial line driver isn't actually a dlpi device. Thus the
work around was to introduced a special flag DLPI_SYNC.
>148: perror() doesn't look right here nor on 153. In general, please look
> through the code and make sure that the error handling is correct and
> consistent.
>
I realized this while fixing the sync* stuff. The error handling has
been modified to call fprintf() along with dlpi_strerror().
>152: why is dlpi_set_timeout() being used here if there are no dlpi library
> calls below it?
>
Not needed. I've removed it as well as the constant MAXWAIT.
>usr/src/cmd/cmd-inet/usr.sbin/syncloop.c
>
>191: perror() calls in this file don't look right either.
>
>
as comment above for usr/src/cmd/cmd-inet/usr.sbin/syncinit.c .
>195: why dlpi_set_timeout()?
>
>
as comment above for usr/src/cmd/cmd-inet/usr.sbin/syncinit.c .
>usr/src/cmd/cmd-inet/usr.sbin/syncstat.c
>
>
as comment above for usr/src/cmd/cmd-inet/usr.sbin/syncinit.c .
>General: same comments as for syncinit.c and syncloop.c
>
>130: is this resolved?
>
>
YES.
>usr/src/cmd/dladm/dladm.c
>
>911: if dlpi_open() succeeds but dlpi_info() fails, there should be a
> dlpi_close() here.
>
I've added a call to dlpi_close() if dh is not null.
>usr/src/lib/libdladm/common/libdladm.c
>
>30: why is <ctype.h> needed?
>
>
To use isdigit().
>137: move the "- 1" to 139 (same comment as in ifconfig.c).
>
>
Fixed.
>usr/src/lib/libdlpi/Makefile.com
>
>39: short comment explaining why this depends on libinetutil.
>
>
Comment added.
# this library uses ifparse_ifspec() in libinetutil to validate a linkname
>usr/src/lib/libdlpi/amd64/Makefile
>
>General: something is wrong with the webrev or with your workspace. This is
>a new file, but the webrev doesn't think it is.
>
>
I'm not sure why the webrev is doing this, it isn't the case in the
parent workspace. When I create the webrev it also lists the files as
new. Same goes for the other two file.
usr/src/lib/libdlpi/sparcv9/Makefile
usr/src/lib/libdlpi/common/libdlpi_impl.h
Thank you,
sagun
