Sagun,
On Wed, 2006-08-16 at 09:58 -0400, Sagun Shakya wrote:
> Here is the link to the webrev of the changes made to libdlpi and the
> applications that are being ported.
>
> 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. ;-)
90: is that in bytes?
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)
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.
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.
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
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?
291: this claims that the PPA is returned, but this function returns a DLPI
error.
329: this doesn't look right. This doesn't need to be dynamically
allocated, and the allocation is of the wrong size.
348: This description needs help.
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.
360: instead of "const void *", couldn't the second arg. be of type "const
union DL_primitives *"? Similar comment for dlreplyp.
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.
374: cstyle, spaces around '|'
378: no need for brackets
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...
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.
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).
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.
467: How do you know that they're large enough to hold any DLPI response?
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.
478: If DLMAXBUF is in bytes, then these buffers are 8 times larger than you
intended.
481: This must be true, since you're testing this as the condition of the
while loop.
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.
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.
508,509: Fix indentation
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.
522: It seems like expecting() should return a boolean_t.
561: remove extra space
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.
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?
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);
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.
617: cstyle: brackets around this block.
630: errno needs to be set if this is DL_SYSERR (stored in dl_unix_errno).
650: I have almost the exact same set of comments for this function than I
did for the multicast function.
727: Comment please. Related question: This function isn't used anywhere in
ON. Was there a specific need to create it?
745: Fix this comment's grammar and punctuation please.
750: This error code looks very strange, I think it contains one too many
'E's.
764: It looks like this constant initialization should go into init_hdl().
785: This goto seems gratuitous when you could conditionally do the Style-2
open conditionally as part of an if expression.
809: You're leaking dip here.
823: Comment explaining what "opt" is used for. Why does this argument
exist?
828: Why is this dynamically allocated?
856: cstyle: brackets
859: There's error handling missing here. What if the returned primitive is
DL_ERROR_ACK?
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?
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.
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?
910: This should say, "The bound SAP precedes the address".
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. :-)
945: Remove extra space before '='.
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.
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.
1016: This call to dlpi_info() is superfluous. You know what SAP was
bound based on the information in the DL_BIND_ACK.
1020: You need to set errno if the DLPI error is DL_SYSERR.
1050: Doesn't need to be dynamically allocated. Same comment throughout for
other similar functions.
1061: cstyle: brackets (same comment throughout)
1069: need to set errno if DLPI error is DL_SYSERR. (same comment
ghroughout)
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).
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.
1240: Please rename this; "size" isn't descriptive enough. (other functions
have this same problem)
1244: Please rename this; "length" isn't descriptive enough. (other
functions have this same problem)
1252: This isn't big enough, you'll be copy data past the end of this buffer
on line 1304.
1257: This will cause a DL_INFO_REQ to be passed down to the driver on every
send. Not a good thing.
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 '='.
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.
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.
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?
1340: This won't work for raw streams. We just read raw data for those.
1341: Isn't the size "size", and not DLMAXBUF?
1373-1379: cstyle: brackets around the if and else. same at 1388.
1380: cstyle: brackets
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.
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.
143,145: cstyle: indent by 4 spaces
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).
59,65,66: you could move these booleans into a bitfield.
-Seb
