Sagun Shakya wrote:
Hi Folks,
This is a code review request for a public DLPI library being provided
as part of a support work for the Clearview components. I'd like to set
the timer for comments to October 6th, 2006. As part of this effort,
current consumers of the Consolidation Private DLPI library are also
being ported. Thus the changes comprises of both the new libdlpi library
and the applications being ported.
If you're off the SWAN webrev is located at:
http://cr.grommit.com/~sshakya/libdlpi_webrev/
If you're on the SWAN webrev is located at:
http://zhadum.east.sun.com/export/build1/ws/sshakya/clearview-libdlpi-onnv/webrev/
My comments on libdlpi.c, libdlpi.h. I have no comments about
libdlpi_impl.h.
libdlpi.c:
line 178: It seems a little strange to me to cast a const void* to a char*.
line 334: Typo in the comment, "open doesn' succes" should be "open
doesn't return success".
lines 517-518, elsewhere: What happens if alloca fails (is such a thing
possible)? The man page indicates that its behavior is undefined if
there is not enough space available to service the request, and also
includes a warning that its use is strongly discouraged.
line 570: Check to make sure linkname is not NULL.
line 574-577: It's a shame there is not a version of strlen that will
stop searching for a NULL character after counting n characters. What
you're doing here is the best you can do given what's available, but it
would be nice to have such a function. For instance, if provider is not
NULL terminated, there's no guarantee that strlen will ever return,
though chances are it will return at some point. It boggles my mind
that nobody thought of this. I could write something to do this if
people out there feel it's of value.
lines 1212-1215: Could these be combined to something like:
return (dip == NULL ? -1 : dip->dli_fd);
(make sure you adjust the operands so that the most common case comes
first)?
lines 1231-1237: You should check to make sure dip is not NULL before
dereferencing it.
lines 1258-1297: If both dlreqp and dlreplyp are NULL, then retval is
never set and you will return some undefined value. It looks like this
could not happen given the current code, but I wanted to bring it up
just in case this was unintentional. This could be an issue if someone
later modifies the libdlpi code without knowing about this edge case.
libdlpi.h:
69-74: Why did we decide to start libdlpi specific error codes at
10000? I'm just curious because my first thought on reading the comment
was "Will 10000 always be a good number to start libdlpi specific error
codes at? What happens if 10000 is no longer a good number to start
libdlpi specific error codes at?"
The workspace used to generate the webrev is at (sorry SWAN only, also
includes cscope database):
/net/zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-onnv
For architecture document and API manpages please see:
http://www.opensolaris.org/os/project/clearview/libdlpi/
If any additional information is needed during the review, please let me
know.
Thanks,
Sagun
_______________________________________________
networking-discuss mailing list
[email protected]
