Sagun Shakya writes: > http://zhadum.east.sun.com/export/build1/ws/sshakya/clearview-libdlpi-onnv/webrev/
I've looked at all of the files, so for the ones not mentioned, I reviewed that file and have no comments. ifconfig/ifconfig.c 3642: is this argument ever something other than the global 'name' array? If not, why pass it in? 3652: the "noattach" mode might need a comment here; actual DLPI dance is done by the IP and ARP modules instead of libdlpi. 3654,3858, et seq.: I'd prefer it if we avoided using internal function names in error messages, particularly error messages that might result from simple usage errors. 3657: strange that 'name' is used here when 'devname' is used elsewhere. 3725: nit: as long as you're here, please remove the stray space before \n. 3781-3782: can this return a different answer than the first time it was called? Why does it need to be called again? 3784-3788: looks like stray code to me; please remove. 4403-4413: this code could be simplified by the use of 'else' -- the goto could be removed. 4406: indenting is wrong. ifconfig/revarp.c 53: this looks unnecessary. 55: I'm curious: why was 'ifname' changed to 'linkname'? It seems like it's the same thing, and the latter requires a lot of rototilling. 141: this doesn't work. &req[1] gives you a pointer of type 'struct arphdr *'. Adding in IPADDRL and the rest results in those offsets being multiplied by sizeof(struct arphdr), which means that RARP is now broken. Did you test RARP? 169: I'd recommend using something that isn't tied to system time of day, like gethrtime(). (And you can then do work directly in milliseconds rather than multiplying seconds by 1000 for the call to dlpi_recv.) 170: why did waittime change to waitsec? 171: nit: indenting. snoop/snoop.c 746,750,752: pr_errdlpi should copy linkname before doing dlpi_close. It could even get the link name out of the dh handle itself. (Note that dlpi_close() doesn't handle NULL, so it's guaranteed here that there's a valid handle, or snoop is going to drop core.) 749-752: doesn't seem to be necessary, as dlpi_strerror handles the DL_SYSERR case properly. snoop/snoop_capture.c 185: doesn't look necessary to me. 191,195: the reason for the copy of the linkname (rather than just using the returned dlpi_linkname(dh) pointer directly) looks obscure to me. I _think_ the reason this is being done is that pr_errdlpi() closes the handle before printing anything. I think this at least needs a comment. Better yet, pr_errdlpi() could do the necessary link name copying before nuking the handle, and you could perhaps remove the linkname parameter to pr_errdlpi as well. (Not sure about that last part.) Forcing all of the callers to do this copy seems mean. 229,235,240,245,250,255,263: error messages have stray colons at the end. 290: why is the letter 'k' here? 311: this test isn't needed. The only way to get here when quitting is false is if retval is not success. 611,639: as long as you're cleaning up, these could be const char *. usr.sbin/syncinit.c usr.sbin/syncloop.c usr.sbin/syncstat.c Change these files if you must for your work, but the changes here do *NOT* fix CR 4842175. I'd very much rather that this code _not_ be ported over to the new DLPI library. This isn't actually DLPI! It's just lousy code that bogusly reuses the DL_ATTACH_REQ message format (and _NOTHING_ else from DLPI). Instead, it should be set adrift on a scow with its own private DL_ATTACH_REQ garbage. dladm/dladm.c 1592-1593: it looks to me like the original code checked for "aggr" first on purpose. Maybe. :-/ iscsitgtd/util_ifname.c 344: why was the original test for loopback removed? Is it not needed now? (I'm guessing that it's not ...) 347: this looks broken to me. The original code did a malloc for *addr. That's why the argument was a uchar_t **, rather than just a uchar_t *. This new code doesn't appear capable of doing any memory allocation. Did you test iSCSI? See also if_find_mac() in this file. libdlpi/Makefile 34: just curious: what 64 bit user space objects link against this library? libdlpi/amd64/Makefile 25: nit: pragma not needed in non-C source. common/libdlpi.c 52,88: nit: "is composed of a" or "comprises a". 55,91: table should be const, so that it ends up in .rodata. 56: prefer braces around individual structure initializers. 65,71: why are these needed? They were once needed in the old code, as it was a direct array map rather than a look-up table. But in converting to a table, "unknown" entries shouldn't be necessary. 119,122: look like const to me, or even just macro usage. 218: no guarantee that bufc (which becomes ctlbuf) is properly aligned; this may drop core on platforms (such as SPARC) that require alignment. 238,525: I'd recommend against gettimeofday. It drifts and isn't reliable enough for an application like this. gethrtime() is better. 242,254: something's wrong here. poll(2) takes milliseconds, but tv_usec is microseconds. What's going on? 254: ignoring tv_sec means that if you're unlucky enough to span a second boundary, msec will become a huge negative number. (Gethrtime gets rid of that problem.) 305: will crash if ctlbuf is NULL. 320: I find it very confusing to jump from top to bottom of file and back again to find these closely-related functions. Can they be gathered together? 318: what is this testing? It looks like the function loops forever if both ctlbuf and databuf are NULL on input. Is that intentional? Why would it be necessary? 329: nit: s/funciton/function/ 330-331: remove special case for sync serial lines. They shouldn't come through here. 334: nit: s/doesn' succes/doesn't succeed/ 348: remove 372: what does the stat() call here do that the open() does not? 385: this function doesn't check for null dip ... why? 394: as a design rule, having the user allocate the data structure to be used within a library is dangerous. The dlpi_info_t structure should be allocated by a library function, or this interface should require the user to pass in sizeof(dlpi_info_t) in case things change in the future. 411: why is this needed? dl_info_req_t has a fixed known size. 431: should do more validation here. dl_qos_offset had better not be less than sizeof (*infoackp). dl_qos_length should not be greater than the size of the buffer returned. Incidentally, given the way dlpi_msg_common is defined, how do you get at the returned message length for checking? 432: what guarantees that &infop->di_qos_sel is big enough to hold all infoackp->dl_qos_length bytes? 438-439,450-451,472-473: needs more checking. 440: what guarantees that &infop->di_qos_range is big enough to hold all infoackp->dl_qos_range_length bytes? 457,460: why is this done only in the case where there's an address? Why not set this flag when you're computing the abs value? 464,466,475: same questions about destination buffer size: how do we know that the buffer can hold what we're copying? What if the message from the driver is corrupt? 470: if you later assume this to be so, then why not check it here? If dl_brdcst_addr_length is non-zero, then check that it's also equal to infop->di_physaddrlen. If it's not, then something's wrong. 505-506: this code is unreachable; there's no way that dip could be NULL here, as you'd have dropped core well before that. None of the callers of this static function can handle a NULL dip pointer. 508-513: remove 515-518: why is this needed? Why not just use "dl_attach_req_t attachreq;" and "dl_ok_ack_t ack;" on the stack, rather than calling these extra functions and using alloca? There doesn't seem to be a benefit in this case. 527-542: this function belongs nearer its friend -- dlpi_mactype. 541: if 'prim' is invalid, this accesses memory off the end of the array. (Not that I care for a static function always called with constants, but ...) 548: consider adding a provider buffer length field here. 552: please don't compare booleans against B_TRUE or B_FALSE; just use as boolean. 570: consider having linkname buffer length passed in. 576: should probably be ">=". 579: will access one off the beginning of the array if "" is passed in for provider. 583: strlcpy? 610: nit: s/types(SAP/types (SAP/ 631: nit: s/then/than/ 632: nit: s/the case, if/the case where/ 637,638: join as else-if. 640,641: coding standard requires use of 'else' or "?:" in this case. 668: should this function do something about dip->dli_sap? 696: what guarantees that the addr_length value from the user is sane (and doesn't cause a buffer overrun)? 784-785: should do more checking here. 790: destination buffer size check? 793: this looks harmful. Returning DLPI_SUCCESS but not at _least_ setting *addrlenp to zero in the case where the driver returns a zero-length address looks hazardous to users. 821: how do we know that addrlen is safe? 830: nit: s/aleady/already/ 830: what does "an active primitive" mean? (Does it mean "passive mode has already been activated?") 888: check on daddrlen validity. 899,904: this works only on little endian (x86) machines and when dli_saplen happens to be exactly 4 (i.e., never). Something a bit more complex will be needed to make this work on all platforms. 939: 'else' here is misleading. Entire rest of function is actually the 'else' clause, due to line 938. 953-954: should check more. 958,962: how do we know saddrp is big enough? 975,979: same for recvp->dri_destaddr. 983: I'd suggest passing the dl_group_address value back to the caller unmolested. Values other than 0 and 1 may be significant. 1009: should this also return failure if i == -1 (that is, if the input link name was either _all_ digits or just "")? 1026-1031: this could probably use a more detailed comment. 1033-1038: seems a bit misleading; consider reformatting. 1047: at least needs a comment. What's going on here? If I_PUSH fails, we can't have pushed anything, can we? (I think the idea here is to force i_dlpi_style2_open to start over, but I don't think it necessarily works right unless dli_mod_pushed == dip->dli_mod_cnt - 1 happens to be true.) 1087: what? 1097: this may clobber errno. 1119: please don't compare boolean against B_TRUE. 1153-1166: remove 1177: indenting is off. 1190-1191: combine using &&. 1236: all the other user interface functions have checks for NULL dip. Why not this one? 1254: the design seems somewhat fragile here. It'll discard unexpected asynchronous messages rather than dispatching them, which may well be harmful for things like DL_NOTIFY_IND. (No, I don't have a good solution that doesn't involve significant complexity for the application. :-<) 1288: why is this needed? 'dlp->error_ack.' should work fine -- dlp is defined as the union of all primitives. 1289: missing blank line after variable declaration. 1291-1294: what does this represent? Can it ever happen? 1351-1408: should be at top of file, or static to the function that uses it. 1359: this string isn't reachable; could just be NULL. 1389: could be const. 1407: why is Style 1 an error? (Maybe error text is confusing.) 1416: will drop core if err is less than zero. common/libdlpi.h 61: should go. 77: why not just have DLPI_SUCCESS = 10000 and remove DLPI_ERRBASE? 160: nit: could use a blank line here. common/libdlpi_impl.h 74: why not milliseconds (which is what poll(2) uses)? 79: is this actually t_uscalar_t? 81: this is used elsewhere as uint_t; inconsistent. 83: used as uint_t elsewhere. 84,85: probably cannot be < 0. 89: uint_t might be better for flags. libdlpi/sparcv9/Makefile 25: nit: remove "pragma" 32: nit: stray blank line -- James Carlson, KISS Network <[EMAIL PROTECTED]> Sun Microsystems / 1 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677 _______________________________________________ networking-discuss mailing list [email protected]
