Sagun,
On Wed, 2006-08-16 at 09:58 -0400, Sagun Shakya wrote:
> http://zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-new/webrev/
>
> The workspace is at:
> /net/zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-new
So far so good. :-) I'm still compiling my main usr/src/lib/libdlpi
comments, but here are comments on the ancillary changes to get you
started. I should have the rest of the comments by tonight or tomorrow.
No comments:
usr/src/cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.h
usr/src/cmd/cmd-inet/usr.sbin/snoop/snoop.c
usr/src/cmd/cmd-inet/usr.sbin/snoop/snoop.h
usr/src/cmd/iscsi/iscsitgtd/Makefile.com
usr/src/cmd/iscsi/iscsitgtd/util_ifname.c
usr/src/lib/Makefile
usr/src/lib/libdlpi/Makefile
deleted_files/usr/src/cmd/cmd-inet/usr.sbin/snoop/dlprims.c
usr/src/cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.c
3649: why not use IF_NAMESIZE (the standard maximum interface name
constant)?
3655: please wrap this block around { } brackets.
3659, 3774: A comment is warranted regarding why this needs to be done.
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.
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.
4418: No need for goto here since the label immediately follows it.
usr/src/cmd/cmd-inet/usr.sbin/ifconfig/revarp.c
196: You could use perror() here.
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?
305: cstyle: separate local variables from the body of the function.
352: why are we doing a dlpi_info() again, when we did this on line 313?
357: why are we only setting *myaddr if debug is true?
usr/src/cmd/cmd-inet/usr.sbin/snoop/snoop_capture.c
158: this comment is no longer accurate. You're not "rereading".
187: raw mode was set earlier in dlpi_open(); this comment needs updating.
193: please fix this function definition, it's still using K&R C 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.
332: why not dlpi_recv()?
usr/src/cmd/cmd-inet/usr.sbin/syncinit.c
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.
152: why is dlpi_set_timeout() being used here if there are no dlpi library
calls below it?
usr/src/cmd/cmd-inet/usr.sbin/syncloop.c
191: perror() calls in this file don't look right either.
195: why dlpi_set_timeout()?
usr/src/cmd/cmd-inet/usr.sbin/syncstat.c
General: same comments as for syncinit.c and syncloop.c
130: is this resolved?
usr/src/cmd/dladm/dladm.c
911: if dlpi_open() succeeds but dlpi_info() fails, there should be a
dlpi_close() here.
usr/src/lib/libdladm/common/libdladm.c
30: why is <ctype.h> needed?
137: move the "- 1" to 139 (same comment as in ifconfig.c).
usr/src/lib/libdlpi/Makefile.com
39: short comment explaining why this depends on libinetutil.
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.
-Seb
