> /net/zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-cr-changes/webrev
> reflects changes made after the codereview comments received from Seb
> and Dan.
>
> /net/zhadum/export/build1/ws/sshakya/clearview-libdlpi-cr-changes/webrev
> reflects change to the clearview gate.
General:
* If there's really no use for dlpi_attach(), and the only
calls to dlpi_detach() immediatley follow a dlpi_open() call,
it would be simpler to have a DLPI_UNATTACH_STYLE2 flag that
could be passed to dlpi_open().
cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.c:
* 3662, 3975: Extra space between after "name, ".
* 3974, 3980: Seems like the dlpi_open()/dlpi_close()
would be more natural if done inside plumb_one_device()
itself.
* 4068, 4078: Why is `cmd' not `const char *'?
* 4368, 4400: No need for `ppa' here; can just inline the
one use into the snprintf() call.
* 4399: s/-1/ -1/
cmd/cmd-inet/usr.sbin/ifconfig/revarp.c:
* 96, 291: Suggest removing ETHERTYPE_REVARP argument; there's no
other value that makes sense for a rarp_open(). Just pass
ETHERTYPE_REVARP directly on line 309.
* 195-198: Not sure what `dbuf' is for -- why isn't `ans'
directly passed into dlpi_recv()? Seems like we also leak
`dbuf' in all cases, possibly more than once if we end up
looping through `rarp_retry'.
* 216: If `dbuf' is directly used, this memcpy() becomes
needless.
* 324-329: This check and following code doesn't make sense;
di_bcastaddr[] is an array, not a pointer. You need to use
memcpy().
* 331: What's the point of `len'?
* 347: Likewise.
* 363-364: For this to be safe, *mybaddr and *myaddr need to be
initialized to NULL at the top of the function.
* 369: Change `ea' to `physaddr', `ifname' to `linkname', and
`length' to `physaddrlen'; `physaddrlen' should be a uint_t.
* 392-438: This whole function needs to be rewritten since:
o There is no need to dynamically allocate `laddr'.
o There is no need to call dlpi_info().
o The handle is freed twice in the common case (lines
422 and 438).
o It should take `linkname', not `ifname'
I'd recommend:
if (dlpi_open(linkname, &dh, 0) != DLPI_SUCCESS) {
/* Do not report an error */
return;
}
retv = dlpi_get_physaddr(dh, DL_CURR_PHYS_ADDR, physaddr,
&physaddrlen);
dlpi_close(dh);
if (retv != DLPI_SUCCESS) {
Perrdlpi("dlpi_get_physaddr failed", linkname, retv);
return;
}
str = _link_ntoa(laddr, str, physaddrlen, IFT_OTHER);
if (str != NULL) {
switch (dlinfo.di_mactype) {
case DL_IB:
(void) printf("\tipib %s \n", str);
break;
default:
(void) printf("\tether %s \n", str);
break;
}
free(str);
}
cmd/cmd-inet/usr.sbin/snoop/snoop_capture.c:
* 196, 197: If 80 colums are the concern, I'd prefer to see
`linknm' -> `linkname' and `retval' -> `retv'.
* 298, 314, 319, 322: All of this logic with dri_totmsglen looks
incorrect -- scan() expects its second argument to reflect the
number of actual bytes starting at `bufp'. That value is
`msglen'.
* 302: Fix the prototype and remove this cast.
* 319: dri_totmsglen is unsigned; < 0 is impossible.
* 327, 329: This doesn't look right -- we use `dh' on line 329
after we've freed it on line 327.
* 335-337: This code is unreachable -- as per line 316, it is
impossible for `retval == DLPI_SUCCESS && !quitting'.
cmd/cmd-inet/usr.sbin/syncinit.c:
* 123: Why isn't this strlcpy()?
* 130: This isn't cstyle-compliant. Further, why doesn't it just
check if the last character is a number? That's all that really
matters.
cmd/cmd-inet/usr.sbin/syncloop.c:
* 100, 156-161: Why did we change the code for `dnambuf' at all?
Seems to have no relationsship to the DLPI functions.
* 169-171: See syncinit.c:130 comment, above.
* 177, 179, 185: Why is `cnambuf' needed at all? Why not directly
use `portname'?
cmd/cmd-inet/usr.sbin/syncstat.c:
* 115-120: See syncinit.c:130 comment, above.
cmd/dladm/dladm.c:
* 831, 861, 1583, 1808, 1820, 1833: These should all be
DLPI_LINKNAME_MAX, not LIFNAMSIZ.
* 910: If the dlpi_open() succeeded but not the dlpi_info(), we
will leak `dh'. This code should probably be restructured to
something like:
if (dlpi_open(name, &dh, 0) != DLPI_SUCCESS) {
errno = ENOENT;
return (-1);
}
if (dlpi_info(dh, &dlinfo, 0) != DLPI_SUCCESS) {
dlpi_close(dh);
errno = EINVAL;
return (-1);
}
dlpi_close(dh);
*legacy = B_TRUE;
bzero(dlattrp, sizeof (*dlattrp));
dlattrp->da_max_sdu = dlinfo.di_max_sdu;
cmd/iscsi/iscsitgtd/util_ifname.c:
* 351: `return (retval == DLPI_SUCCESS)' will suffice.
lib/libdladm/common/libdladm.c:
* 129: Prefer DLPI_LINKNAME_MAX.
* 136: Remove blank line.
* 140-141: No need for `ppa' here; can just inline the
one use into the snprintf() call.
* 143: Why isn't this strlcpy()?
* 145-150: Seems simpler as:
if (dlpi_open(name, &dh, 0) == DLPI_SUCCESS) {
i_dladm_nt_net_add(arg, name);
dlpi_close(dh);
}
return (DI_WALK_CONTINUE);
--
meem
