> http://cr.grommit.com/~sshakya/libdlpi-baseon-webrev/
Here are my comments with the exception of libdlpi.c itself. (I'm
delaying review of libdlpi.c until the issues we discussed on Friday have
been addressed. Perhaps the review in general should be delayed until
those have been addressed and retesting has been done?)
cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.c:
* The error message handling in here is flawed. Right now it
will produce cryptic error messages like:
ifconfig: rarp_open: dlpi_open failed: foo invalid DLPI linkname
... or in other cases:
ifconfig: failed to open: foo invalid DLPI linkname
Neither of these are clear. To correct this, Perrdlpi() should
put quotes around the link name and a colon afterwards, and the
error messages passed to Perrdlpi() need to be carefully cleaned
up (considering *exactly* how it will look once Perrdlpi()
outputs it). Minimally, something like this would be tolerable:
ifconfig: cannot open link "foo": invalid DLPI linkname
* 3665: As per above: "cannot parselink" is an indecipherable
error message; either name the function that failed or
(if possible) describe the failure in plain English. Do
likewise for all Perrdlpi*() call sites.
* 3941: Why is `dev_fd' still here? (While ifconfig isn't lint-
clean, please ensure that lint is no worse after your changes.)
* 4072: See earlier bullet on error message handling. Feel free
to introduce additional functions to make the error message
handling clear.
* 4390: Remove blank line.
* 4395, 4397: Remove braces.
cmd/cmd-inet/usr.sbin/ifconfig/revarp.c:
* 66: Wouldn't `ifaddrlen' make more sense as `physaddrlen'?
* 70: Why `rh' when everywhere else uses `dh'?
* 80, 82: Remove braces.
* 83: While you're changing this line, change to strlcpy().
* 86: While you're here, it would worth fixing an existing bug:
`s' is never close()'d.
* 157-166: This comment needs to be rewritten; the code no longer
uses poll() directly. Also, while you're here, RARP_TIMEOUT
should be changed to RARPTIMEOUT to match the code.
* 169: Remove currenttime and inline the gethrtime() call.
* 172-182, 187-195: Why is this code here twice? This shouldn't
be necessary, and it's wrong to not call dlpi_recv() just
because waitsec is `0' -- what if the message is sitting there,
ready to be read?
* 247-250: This code should be replaced with a `goto out', `fail'
changed to `out' on line 259, and line 263 changed to return
a variable containing the return value.
* 258: This /* NOTREACHED */ should be removed.
* 298: Cast to `(void)' missing.
* 310-317: This debug code should be folded into the block at
lines 321-329, and _link_ntoa() should be used rather than
a for() loop to print out the broadcast address.
* 334: Needless cast to `(void)'; dlpi_close() does not return
a value.
* 365: No need to initialize `str'.
cmd/cmd-inet/usr.sbin/snoop/snoop.c:
* 749: `errno' needs to be saved before *any* other functions
are called here -- assign it at line 744.
cmd/cmd-inet/usr.sbin/snoop/snoop_capture.c:
* 211, 215: The error messages should be more descriptive --
as it stands, DL_PROMISC_SAP, DL_PROMISC_MULTI, and
DL_PROMISC_PHYS failures all return the same error.
On a related note: perhaps libdlpi should have a dedicated
error code indicating that it could not enable raw mode on
the link?
* 224, 233: Not completely due to your changes, but these
messages are vague and inconsistent -- something like:
snoop: cannot push "pfmod" on "bge0": <error>
... would be preferable.
* 236, 238, 241, 243, 246, 248: Remove braces.
* 291-293: Would prefer:
if (msglen != 0)
scan(bufp, msglen, filter, 0, 0, proc, 0, 0, flags);
cmd/cmd-inet/usr.sbin/syncinit.c:
* This is not cstyle-compliant:
if (strlcpy(cnambuf, argv[0], sizeof (cnambuf))
>= sizeof (cnambuf)) {
Use:
if (strlcpy(cnambuf, argv[0], sizeof (cnambuf)) >=
sizeof (cnambuf)) {
cmd/cmd-inet/usr.sbin/syncstat.c:
* While you're here, please rename `ifname' and `ifdevice' to
`sername' and `serdevice' or the like.
cmd/dladm/dladm.c:
* 1312: While you're here, change `index' to `ppa'.
cmd/iscsi/iscsitgtd/util_ifname.c:
* 351-355: This seems wrong -- grab_address() should only allocate
memory if it returns success, but if dlpi_get_physaddr() fails,
it doesn't free `*addr'.
Moreover, we're preserving a needlessly complicated interface.
Just have if_find_mac() allocate a buffer of DLPI_PHYSADDR_MAX
and then simply pass that into grab_address() (along with its
size). Then grab_address() can be shortened to:
static boolean_t
grab_address(const char *linkname, uchar_t *addr, size_t *addrlenp)
{
dlpi_handle_t dh;
int retval;
if (dlpi_open(linkname, &dh, 0) != DLPI_SUCCESS)
return (B_FALSE);
retval = dlpi_get_physaddr(dh, DL_CURR_PHYS_ADDR, addr, addrlenp);
dlpi_close(dh);
return (retval == DLPI_SUCCESS);
}
lib/libdladm/common/libdladm.c:
* 134, 136: Remove unnecessary braces.
lib/libdlpi/common/libdlpi_impl.h:
* 39: This comment is incorrect; libdlpi does not have a maximum
buffer size.
* 49: This comment is incorrect; DLPI_MODS_MAX is the maximum
number of modules that can be pushed onto a stream and has
nothing to do with the length of their names.
* 62, 68: Macro arguments need to be wrapped in ()'s as usual.
* 73, 81, 89: All of these comments seem to miss the mark: the
comment should say what the structure is for, not enumerate its
contents. The purpose of each field should be in a comment to
the right of the field name.
* 115: Strictly speaking, this is an array of mods, not a table.
* 116, 117: Comments should be unindented one level.
lib/libdlpi/common/libdlpi.h:
* 45: s/terminating NULL/terminating NUL/
* 50: "Constant for dlpi_bind()" is a meaningless comment.
* 55: This comment should make it clear that not all of these
flags are public -- e.g.:
/*
* Flags for dlpi_open(); those not documented in dlpi_open(3DLPI)
* are Consolidation Private and subject to change or removal.
*/
* 67: DLPI_DEF_TIMEOUT is set to 3 here, but the manpage says that
any value less than 5 may be unreliable. This needs to be
reconciled.
--
meem
_______________________________________________
networking-discuss mailing list
[email protected]
