Thank you Meem.
I've addressed the comments. Please see inline for some issues not
addressed.
For the rest I've addressed as suggested.
The webrev against the ONNV gate is at:
http://zhadum.east.sun.com/export/build1/ws/sshakya/clearview-libdlpi-onnv-review3/libdlpi-webrev-baseon/
To see the changes made after your comments below:
http://zhadum.east.sun.com/export/build1/ws/sshakya/clearview-libdlpi-onnv-review3/libdlpi-webrev-review2/
Peter Memishian wrote:
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
Fixed this for Perrdlpi(). If this should reflect on Perror0() and
Perrror2(), I make those changes also to be the same format.
* 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.
Used function name - dlpi_parselink() in this case above.
* 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'?
changed to use '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.
this comment has mostly been rewritten as the code has been modified to
avoid the use of label 'rarp_retry'.
* 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?
Fixed as part of rewriting the code as explained above.
* 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.
replaced 'fail' with 'out' and decided to preserve the out label.
* 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?
No changes made. returning a dedicated error code will not miss the
actual system errors so returning DL_SYSERR would be more accurate.
* 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.
Didn't change for errors that would be returned as a result of system error.
* 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.
Reset the default time to be 5.
Thanks,
Sagun
_______________________________________________
networking-discuss mailing list
[email protected]
