Thank you Meem for the review. My responses are inline. New webrevs information:
The webrev against the clearview gate is at: http://zhadum.east.sun.com/export/build1/ws/sshakya/clearview-libdlpi-cr2-changes/webrev_clearviewgate/ Against the last round of code review: http://zhadum.east.sun.com/export/build1/ws/sshakya/clearview-libdlpi-cr2-changes/webrev1/ The workspace and cscope database is at: /net/zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-cr2-changes/usr/src/ Peter Memishian wrote: > 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(). > this is the case, so these functions have been moved as internal functions and a flag DLPI_NOATTACH has been provided, where as the name suggests attach is not called. This also allows to remove dlpi_detach(). > cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.c: > > * 3662, 3975: Extra space between after "name, ". corrected. > * 3974, 3980: Seems like the dlpi_open()/dlpi_close() > would be more natural if done inside plumb_one_device() > itself. > yes, and this will also allow to have a DLPI_NOATTACH flag to dlpi_open(). I've moved the dlpi_open/dlpi_close() to plumb_one_device(). > * 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. i've made the changes as suggested above. > * 324-329: This check and following code doesn't make sense; > di_bcastaddr[] is an array, not a pointer. You need to use > memcpy(). instead of dynamically allocating these variables, 'mybaddr' and 'myaddr' are allocated on the stack. > * 331: What's the point of `len'? `len' has been removed and used dlinfo.di_physaddrlen. > * 347: Likewise. > * 363-364: For this to be safe, *mybaddr and *myaddr need to be > initialized to NULL at the top of the function. same as above. > * 369: Change `ea' to `physaddr', `ifname' to `linkname', and > `length' to `physaddrlen'; `physaddrlen' should be a uint_t. > changed as suggested. > * 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' a call to dlpi_info() is needed to get the mac_type. Otherwise, the rest has been fixed. > 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'. fixed as suggested. > * 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 unreachce able -- as per line 316, it is > impossible for `retval == DLPI_SUCCESS && !quitting'. > corrections made as suggested. > cmd/cmd-inet/usr.sbin/syncinit.c: > > * 123: Why isn't this strlcpy()? i've changed it to be 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. i've fixed it to just check if the last character is a number. > 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. > `dnambuf' has no relationship with DLPI. While making the change it seemed that it was unnecessary to do the strncmp() stuff so I changed it. Since, it is not DLPI related I'll leave the code as is. > * 169-171: See syncinit.c:130 comment, above. > > * 177, 179, 185: Why is `cnambuf' needed at all? Why not directly > use `portname'? that is true `portname' can be used directly. I've changed it to be a local variable instead of global as well. > cmd/cmd-inet/usr.sbin/syncstat.c: > > * 115-120: See syncinit.c:130 comment, above. > Fixed as per comment. > 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); > Changed as suggested. Thanks, Sagun
