Thank you Dan for reviewing the changes.
Dan Groves wrote:
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/
libdlpi.c:
line 178: It seems a little strange to me to cast a const void* to a
char*.
I agree and strputmsg() could be changed to take a void * instead of
const void *, which would be a safe thing to do. But when calling
strputmsg() in line 1267 the same strangeness would apply, casting to
'dlp' a 'void *'.
line 334: Typo in the comment, "open doesn' succes" should be "open
doesn't return success".
Comment has been fixed to be:
" * If open doesn't succeed and link doesn't exist (ENOENT), this function
* returns DLPI_ENOLINK, otherwise returns DL_SYSERR. "
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.
I also had the same question when I got the suggestion to use allloca()
instead of doing malloc(). As Meem has explained in his reply to this
comment I also got the same explanation and thus was used alloca().
line 570: Check to make sure linkname is not NULL.
Added a check.
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.
Yes such a function would be nice. Probably you could file an RFE for this?
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)?
I've combined this to be: return (dip != NULL ? dip->dli_fd : -1);
lines 1231-1237: You should check to make sure dip is not NULL before
dereferencing it.
I've fixed this to be:
return (dip != NULL ? dip->dli_linkname : NULL);
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.
Good point. 'retval' is initialized to DLPI_FAILURE and returning this
error code would be appropriate since passing dlpi_msg_common with both
dlreqp and dlreplyp as NULL doesn't make sense.
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?"
10000 is a arbitrary value selected under the basis that dlpi error
return values (DL_ERROR_ACK) will not reach that high of a value. As
long as the "DL_ERROR_ACK" return values are not overlapping I think it
is safe to say that the number is good, unless there is something I am
missing.
-Sagun
_______________________________________________
networking-discuss mailing list
[email protected]
