Hi Team, I have made the changes as recommended, for the ones that haven't, I have my responses inline.
Webrev have been created against the clearview gate: http://zhadum.east.sun.com/export/build1/ws/sshakya/clearview-libdlpi-cr2-changes/webrev_clearviewgate/ also, against the first round of code review workspace: http://zhadum.east.sun.com/export/build1/ws/sshakya/clearview-libdlpi-cr2-changes/webrevwithround1/ The workspace and cscope database is at: /net/zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-cr2-changes/usr/src -Thanks, Sagun Sebastien Roy wrote: >Sagun, > >> >>http://zhadum.east/export/build1/ws/sshakya/clearview-libdlpi-new/webrev/ >Here are the rest of my comments: > >usr/src/lib/libdlpi/common/libdlpi.c > >60: there are some important MAC types missing from this list, namely the IP > tunneling types DL_IPV4 and DL_IPV6. This doesn't matter, however, > since consumers of this library can't open a tunneling DLPI device yet. > I'll fix that. ;-) > > In order to make it flexible to add MAC types in the future, I added a table- lookup function. >90: is that in bytes? > Yes, added as comment. >131: there are pseudo device nodes in /dev and /dev/net, so this part of the > comment needs clarification. I think just stating the directory in > /devices would make it more clear. Also stating why you need to look > in /devices would be beneficial to the reader as well. > >135: flags (plural) > > Fixed. >142,144: re-using a parameter out of convenience is not appropriate, > especially since you'll be passing in other kinds of flags into > this function in the future, and here you're overwriting the > previous contents of the variable. Also keep in mind that the > flags passed in are not in the same flag space as the open(2) > flags. Moreoever, the open(2) flags are of type "int". You'll > need to define a different local variable to set these flags. > > I realized this issue and changed it in the second round of changes. >146,151: use "sizeof (devname)" > >148,153: I think we should return an error here if the error returned by > open() is something other than ENOENT. Otherwise, you'll be > opening a device other than the one intended, and you'll be > covering up a problem with the filesystem that perhaps the > administrator should be made aware of. > > This has been changed to: return (errno == ENOENT ? DLPI_ENOLINK : DL_SYSERR); >156: use "sizeof (path)" >170: implicitely attached (not bound) > >175: flags (plural) > >182: comment please. What is the meaning of the different calls to > i_dlpi_open()? >197: you don't need "cnt" here, you can use dip->mod_pushed directly. >245: not a boolean > >247: no need for brackets > >252: please fix this so that the line isn't broken up this way > >254: please fix the indentation > All of the above have been fixed. >275: I'm not sure it makes sense to return DLPI_ESTYLE1 to the caller. > Isn't the idea to abstract DLPI device style out of this API? > > The function have been changed a bit. The only function that returns DLPI_ESTYLE1 is i_dlpi_attach. Would it be more appropriate to just return DLPI_FAILURE? >291: this claims that the PPA is returned, but this function returns a DLPI > error. > fixed. >329: this doesn't look right. This doesn't need to be dynamically > allocated, and the allocation is of the wrong size. > Instead of malloc() or statically allocating the DLPI primitive, I've used alloca() for the comment above and throughout the library. alloca() is being used as it ensures alignment and proper sizing of DLPI request buffers. Also, the functions do not have to worry about freeing up the allocated memory. >348: This description needs help. > Changed to: * Common routine invoked by all DLPI control routines. >349-356: it's not very useful to put only the types of the arguments in this > comment; their names would make the comment more meaningful. Same > comment throughout. > > Names have been added for each function. >360: instead of "const void *", couldn't the second arg. be of type "const > union DL_primitives *"? Similar comment for dlreplyp. > This will require casting for each call to dlpi_msg_common(), so it remains as a "const void *". >361: dlreplyprim's type is wrong, it should be t_uscalar_t (unfortunately, > if you're exposing the primitive values to the caller, then you also > need to expose their type. > Fixed to be the right type. >374: cstyle, spaces around '|' > >378: no need for brackets > > fixed. >390: Why is DL_SYSERR appropriate here? If we get a DL_ERROR_ACK in > response to our primitive, then errno will not be set... > > The DL_ERROR_ACK received in response to the primitive would be a part of the `dlreplyp' primitive. A DL_SYSERR would be returned only if getmsg() succeeds. However the function has evolved so the error returned is the error returned by strgetmsg(). >413: Please bring this back up above i_dlpi_open() so that it can diff > against the previous version of this function. In general, there seems > to be lots of gratuitous movement of functions that could have been > more easily reviewed if they had stayed in their original locations. > I've moved functions back align with the original but due to functions being shorter or removed few function are not aligned. >425: Does this work if the endpoint is in DLIOCRAW mode? In that case, > shouldn't we either set the ctl pointer to NULL or use plain old > write()? I think DLIOCRAW mode needs more work, as its handling in > dlpi_send() also seems broken (see other comments on that function). > > This was a bug and has been been fixed in the second round of changes. ctl pointer is set to NULL when in DLIOCRAW mode. Accordingly, dlpi_send() and dlpi_recv() have also been fixed. >460: This can block indefinitely. How will this ensure that we're not > blocking for a longer period of time than what a higher level function > expects? For example, if the poll() in dlpi_msg_common() returns with > some data to read and strgetmsg() reads the message and it's not the > expected primitive, we may then go back into getmsg() with no data to > read forever. > This certainly would block indefinitely. The function has been changed to address this issue. >467: How do you know that they're large enough to hold any DLPI response? > > > It is probably incorrect to say; I had kept the old comment and since DLMAXBUF is defined to be very large I thought it was valid to keep. >468: I don't see the implementation of this comment anywhere in the code. > You always read the additional control information when MORECTL is set > in the return code. > Fixed comment to be: * If MORECTL or MOREDATA is set, we are not interested in the * remainder of the message. Temporary buffers are used to * drain the remainder of this message. >478: If DLMAXBUF is in bytes, then these buffers are 8 times larger than you > intended. > Fixed to be uint8_t. >481: This must be true, since you're testing this as the condition of the > while loop. > Ah..yes..removed this test. >487,488: cstyle: brackets around this block > >494: This is not at all obvious. Please add a comment explaining what's > going on here. I cannot tell what this block of code is attempting to > do. > comment has been added. >494: Shouldn't we be appending additionally read data to the end of the > already read data? Same question for the newly read control > information. > We only read data of 'datalenp' length, any data greater than that is indicated via 'totdatalenp'. For the control information reading more than one block of message would not make sense for M_PROTO or P_PCPROTO message, if I understand correctly? >508,509: Fix indentation > > fixed >508: If expecting returns an error perhaps because the message passed to us > is corrupt in some way, then we'll go back to the top of the loop and > back to getmsg() potentially blocking forever. > This is how this function was originally designed. strgetmsg() is expected to return: in two situations: a) it receives an expected primitive/ expected dl_errror_ack b)a higher function timeouts. I've added a comment in strgetmsg(). >522: It seems like expecting() should return a boolean_t. > >561: remove extra space changes made as suggested. >565: s/disable/disables/ > >575,576: These are the same structure, you don't need to separate them. > >578: You don't need reqprimp, just pass a pointer to the dl_enabmulti_req_t > into your dlpi_msg_common() function. fixed and added comment. >589-592: This is unecessary (and you already know what MAX(opsize, 0) will > evaluate to). You have two separate sizes; opsize is the size of > the request, and MAX(sizeof (dl_ok_ack_t), sizeof (dl_error_ack_t)) > is the size of the reply. Why are you combining these sizes when > they're separate buffers? > The implementation has been changed to refer to a table to get the reply/response size. >594: You can just allocate dlemrp directly. Then, the block from 602-614 > gets reduced to: > > dlemrp->dl_primitive = (op == DL_ENABMULTI_REQ) ? > DL_ENABMULTI_REQ : DL_DISABMULTI_REQ; > dlemrp->dl_addr_length = addr_length; > dlemrp->dl_addr_offset = sizeof (dl_enabmulti_req_t); > (void) memcpy(&dlemrp[1], addrp, addr_length); Size issue has been fixed and combined to use dl_enabmulti_req_t as the primitive type. >595: malloc() already sets errno. You should just return DL_SYSERR here > and leave the errno intact. I'm wondering, in general, why you need a > DLPI_ENOMEM when the errno ENOMEM already exists. Same comment > throughout the file where memory allocation occurs. With the use of alloca(), DLPI_ENOMEM is not needed anymore. So I've removed the error code DLPI_ENOMEM. The idea was to let the caller know that enomem was returned in libdlpi. >617: cstyle: brackets around this block. > line has been changed to: retval = dlpi_msg_common(dip, multireqp, DL_ENABMULTI_REQ_SIZE, replyp, DL_OK_ACK, DLPI_ACK_REPLY_SIZE, DL_OK_ACK_SIZE, 0); if (retval != DLPI_SUCCESS) return (retval); >630: errno needs to be set if this is DL_SYSERR (stored in dl_unix_errno). dlpi_msg_common() takes care of this now. >650: I have almost the exact same set of comments for this function than I > did for the multicast function. Changed to used dl_promison_req_t reduced the rest of the redundant code. >727: Comment please. Related question: This function isn't used anywhere in > ON. Was there a specific need to create it? There is no specific need until right now. >745: Fix this comment's grammar and punctuation please. fixed. >750: This error code looks very strange, I think it contains one too many > 'E's. can't think of a simpler one, any suggestions? DLPI_ELINKINVAL would mean something else. >764: It looks like this constant initialization should go into init_hdl(). Since init_hdl() was used only once and the values were being changed right after the call to init_hdl(), removed the init_hdl(). >785: This goto seems gratuitous when you could conditionally do the Style-2 > open conditionally as part of an if expression. Fixed as suggested. >809: You're leaking dip here. dlpi_close() frees up dip. >823: Comment explaining what "opt" is used for. Why does this argument > exist? "opt" is used for future if new fields are added to dlpi_info, and applications can specify which version of dlpi_info it wants. This is explained in dlpi_info(3DLPI). Is it necessary to explain here? >828: Why is this dynamically allocated? > Used alloca(). See reply to line 329. >856: cstyle: brackets > >859: There's error handling missing here. What if the returned primitive is > DL_ERROR_ACK? > > Fixed. >876: This comment is confusing. It's not discarded, as the next line stores > the value into the structure passed back to the caller... Can you > explain how the di_opts field allows for future expantion of > dlpi_info_t (as the comment suggests)? > >879: What is the value in saving this into the structure if it's never used? > Now, if opt value is not zero an error is returned otherwise, opt is ignored. >883: The pointer arithmetic here will send you off into the weeds. The > offset is in bytes, but you're casting dliap to a pointer to a unsigned > int prior to adding the offset. Same comment on line 894. > Interestingly, it's correct on line 907. > > I had this fixed as: (union DL_qos_types *)((caddr_t)infoackp + infoackp->dl_qos_offset); but lint complains about it (E_BAD_PTR_CAST_ALIGN). I'm fixing this problem. >885,896: I'm not convinced that returning an error is the right thing to do > here. Can't the caller still interact with the device even if > we're not sure what kind of QOS support the device implements? > Can't we return the rest of the relevant information in that case? > We are only supporting connectionless mode so forcing to only used the connectionless QoS support. If we think otherwise I can change this. >910: This should say, "The bound SAP precedes the address". > > corrected. >930: dip->saplen was already set on line 902. >943-946: Bring these lines above the comment on line 937 to make the comment > accurate. :-) > Fixed. >945: Remove extra space before '='. > > This is a bug in webrev. There is no space in the file. >974: This doesn't need to be dynamically allocated. > >983: Hmm, shouldn't DLPI_ANY_SAP result in a different value being bound for > token ring (2?) as is suggested in snoop_capture.c? For that matter, > there probably should only be a single call to dlpi_bind() in > snoop_capture.c with a sap of DLPI_ANY_SAP, and this function should > figure out what best value to use as the sap based on the MAC type. > > True, this has been changed in the second cr changes. To avoid calls to dlpi_info, added dli_mactype to the handle. >990: cstyle: brackets > >1001: This comment seems incomplete. I think it would be more clear to say, > "Verify that the bound SAP is equal to the SAP requested" before the > existing comment. Please also explain in this comment the reasoning > behind failing dlpi_bind() in this case, and not when a boundsap > pointer is passed in. > Changed the comments. >1016: This call to dlpi_info() is superfluous. You know what SAP was > bound based on the information in the DL_BIND_ACK. > > I had a sap value field in the handle originally but I thought that value can be retrieved from dlinfo.di_sap instead. So had to add the call to dlpi_info(). dli_sap has been added to the handle. The call to dlpi_info() has been removed. >1020: You need to set errno if the DLPI error is DL_SYSERR. > > dlpi_msg_common() takes care of this. >1050: Doesn't need to be dynamically allocated. Same comment throughout for > other similar functions. > See reply for comments for line 329. >1069: need to set errno if DLPI error is DL_SYSERR. (same comment > ghroughout) > > dlpi_msg_common() takes care of this. >1123: Why should this API care what type is being requested? I think we > should be liberal here and let the underlying driver decide what is a > bogus type. Case in point: the two listed here are not the only two > in existence, and more are coming soon (destination address for > example). > > I put this restriction because the proposed manpage stated that those were the two type supported (DL_CURR_PHYS_ADDR, DL_FACT_PHYS_ADDR). As you point out the underlying driver should decided whether it is a bogus type and let the API just pass the information down. >1191: Let the DLPI provider return an error if an unknown type is being set. > Different types can be passed transparently and harmlessly through > this API. > > For DLPI_SET_PHYSADDR(), type is not an option in DLPI itself, this argument was added for future use, if more that DL_CURR_PHYS_ADDR is allowed to be set. So I think forcing type to be DL_CURR_PHYS_ADDR is OK in this case. >1240: Please rename this; "size" isn't descriptive enough. (other functions > have this same problem) > > Renamed throughout to be reqsz/acksz. >1244: Please rename this; "length" isn't descriptive enough. (other > functions have this same problem) > > Changed to dl_sapaddrlen; >1252: This isn't big enough, you'll be copy data past the end of this buffer > on line 1304. > Added the buffer size for SAPLEN. i.e dl_sapaddrlen >1257: This will cause a DL_INFO_REQ to be passed down to the driver on every > send. Not a good thing. > > Fixed by adding a dli_sap field in the handle. >1281-1283: cstyle inccorect here. If the if block is bracketed, then the > else also needs to be. These lines need to look like: > > } else { > ... > } > >1292: Remove extraneous spaces before '='. > > fixed. >1292: This should be calculated back before 1252 so that you can allocate > enough memory to hold this. > >1304: No need to memcpy() in two steps. Just copy directly into the buffer > on lines 1294-1298. > >1307: DLIOCRAW send is broken here. DL_UNITDATA_REQ is not what we pass > down for raw streams. > > Fixed as suggested. Last one fixed to: if (dip->dli_oflags & DLPI_RAW) { retval = strputmsg(dip->dli_fd, NULL, 0, msgbuf, msglen, 0); return (retval); } >1308: The 3rd argument is incorrect, it should be the length of the control > information, not the data. It should be DL_UNITDATA_REQ_SIZE + > length. In reality, this doesn't need to be computed again, as you > already stored this size in a variable called "size" up on line 1252. > > fixed 3rd argument to be 'reqsz'. >1315: How do you know how big the provided buffer is, and if it's enough to > hold the data you're going to be reading? > > Here, dlpi_recv() expects the caller has passed 'msgbuf' to be atleast 'msglen' as the manpage indicates. Should the api be smarter? e.g check if msglenp== NULL right now strgetmsg() handles the following conditions as: msgbuf msglenp strgetmsg() (if data exists) 1 ptr length data copied to msgbuf <= msglenp msglenp == data actually retrieved 2 NULL length msgbuf == NULL, msglenp == data actually retrieved 3 ptr NULL msgbuf <= DLMAXBUF,msglenp == NULL 4 NULL NULL BOTH is NULL Case 3 isn't right so I think a check for (msglenp != NULL) is needed . Otherwise for 1 it is the callers responsibility to provide the msgbuf of size length. >1340: This won't work for raw streams. We just read raw data for those. > See reply to line 425. >1341: Isn't the size "size", and not DLMAXBUF? > that is correct this should be "size" or now "indsz". >1373-1379: cstyle: brackets around the if and else. same at 1388. > >1380: cstyle: brackets > > Fixed >1395: There's a missing '!' here, dri_isunicast is being set to the opposite > of what it should be. > >1472: No need to dynamically allocate this. > >1624: Remove extraneous spaces in this function. > > Fixed all. >usr/src/lib/libdlpi/common/libdlpi.h > >68: More needs to be said about these in the form of a comment. Please > explain these error's relationship with DLPI errors (from the DL_* > space), and that libdlpi functions return both sets of errors (which is > why DLPI_ERRBASE needs to be larger than any DL_* error. Is that > correct? > >70: I don't see the point of DLPI_EINVAL nor DLPI_ENOMEM. > I've added a comment for libdlpi error codes. DLPI_ENOMEM doesn't exist anymore but DLPI_EINVAL still does. I think it is appropriate to return this error to cover general cases when invalid argument is provided e.g opt != 0 >143,145: cstyle: indent by 4 spaces > > Fixed. >usr/src/lib/libdlpi/common/libdlpi_impl.h > >50: to help with cscope, please prefix the members with something > descriptive of this library. (try doing a cscope for "ppa" for an > example of how difficult it will be to find references to the uses of > this member (it shows up 1216 times). > > members prefixed with "dli_" >59,65,66: you could move these booleans into a bitfield. > > changed 65 and 66 to be bitfield. Thank you, Sagun
