Thank you Jim for the comments.
I have fixed the nits and other suggested fix and left out the comments.
Response/questions for the remaining are inline.
I've left the Synchronous Serial line special handling and left out the
comment.
James Carlson wrote:
Sagun Shakya writes:
http://zhadum.east.sun.com/export/build1/ws/sshakya/clearview-libdlpi-onnv/webrev/
ifconfig/ifconfig.c
3642: is this argument ever something other than the global 'name'
array? If not, why pass it in?
The argument is always the global 'name'. I will change it to use the
global 'name' array instead.
3652: the "noattach" mode might need a comment here; actual DLPI
dance is done by the IP and ARP modules instead of libdlpi.
Comment will be added before both line 3652, 3776.
* *
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.
I'll make the error messages changes for places where the possible error
is due to usage.
3657: strange that 'name' is used here when 'devname' is used
elsewhere.
Not intentional. This intent was to use 'devname' but now as the global
'name' is going to be used; 'name' will be used instead.
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.
I've removed the second call to dlpi_parselink and debug message.
4403-4413: this code could be simplified by the use of 'else' -- the
goto could be removed.
4406: indenting is wrong.
I've simplified with the use of 'else' and removed use of goto.
ifconfig/revarp.c
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.
This was changed to represent the correct name as a datalink name.
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?
I have fixed this back. I hadn't tested RARP with a server and client.
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.)
Since gethrtime() returns time in nanosecondes wouldn't this require
further conversion from nanoseconds to milliseconds for dlpi_recv() and
if the rarp_timeout is to be calculated in seconds than a conversion
from nanoseconds to seconds?
170: why did waittime change to waitsec?
This was changed to reflect variable waittime was in seconds.
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.)
Fixed as suggested.
snoop/snoop_capture.c
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.
That is correct.
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.
Instead of the comment I went with the approach of copying the link name
before nuking the handle. I don't think passing the linkname in
pr_errdlpi() in itself is any problem.
229,235,240,245,250,255,263: error messages have stray colons at the
end.
The colons were left to keep the error message format of :
(which could be done my adding a ':' in the call to pr_err() in
pr_errdlpi() itself)
snoop: cmd: linkname: error string
but I think it is better to have the format of:
snoop: cmd linkname: error string
290: why is the letter 'k' here?
Old code which was overlooked.
311: this test isn't needed. The only way to get here when quitting
is false is if retval is not success.
Couldn't we get here if it breaks out of the loop in line 299 due to
retval is not success but quitting is false? Or I'm not being able to
follow the logic ?
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've removed CR 4842175 from list.
dladm/dladm.c
1592-1593: it looks to me like the original code checked for "aggr"
first on purpose. Maybe. :-/
This change should done a strcmp on 'drv' instead of 'dev' which would
need the dlpi_parselink() called first.
1592 if (dlpi_parselink(dev, drv, &index) != DLPI_SUCCESS ||
1593 strcmp(drv, "aggr") == 0 || index >= 1000 ||
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 ...)
It is needed but dlpi_open() does the test for loopback.
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?
The iSCSI testsuite was ran against bfu bits. I will see why this was
not caught.
libdlpi/Makefile
34: just curious: what 64 bit user space objects link against this
library?
common/libdlpi.c
There are no known ones.
119,122: look like const to me, or even just macro usage.
This will be converted to a macro.
218: no guarantee that bufc (which becomes ctlbuf) is properly
aligned; this may drop core on platforms (such as SPARC) that
require alignment.
Using the -misalign flag at compile time would not be a solution to
this. How should this be solved?
238,525: I'd recommend against gettimeofday. It drifts and isn't
reliable enough for an application like this. gethrtime() is
better.
I'll use gethrtime and here a conversion from nanoseconde to
milliseconds will be needed?
242,254: something's wrong here. poll(2) takes milliseconds, but
tv_usec is microseconds. What's going on?
I got confused and was thinking tv_usec was in milliseconds, thus was
using it. Will fix as with the use of gethrtime.
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.)
Understood. Will you use gethttime.
305: will crash if ctlbuf is NULL.
Added :
if (!no_ctl)
(void) memcpy(ctlbuf, bufc,
ctl.len);
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?
Sorry, this was done as an effort to keep the diffs aligned as much as
possible. I will be gathering each friends in one place.
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?
This test is for the case when raw message are requested. line 933. It
was not intentional to loop forever when both ctlbuf and databuf are null.
This will be fixed.
372: what does the stat() call here do that the open() does not?
I had left this as from the previous (consolidation private) version of
libdlpi.
385: this function doesn't check for null dip ... why?
I will add a check for null dip.
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.
Curious isn't the opt field in some sense to serve the purpose of the
sizeof(dlpi_info_t) since if
the sturcture size change can be detected from the different opt field?
411: why is this needed? dl_info_req_t has a fixed known size.
This is to keep the dlpi_primsizes table consistent with all the
primitives used to have the corresponding 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?]
I have added the validation. If I understood your question properly do
you mean how do I check the size of the returned reply primitive. In
that case expecting does the check.
432: what guarantees that &infop->di_qos_sel is big enough to hold
all infoackp->dl_qos_length bytes?
Is it safe after adding a check as below line 432 and dlpi_info_t
structure defines di_qos_sel as a dl_qos_cl_sel1_t?
438-439,450-451,472-473: needs more checking.
Adding a check for offset as well
infoackp->dl_addr_offset > sizeof(dl_info_ack_t)
Added check for :
432: (infoackp->dl_qos_length < sizeof(dl_qos_cl_sel1_t)))
438-439: (infoackp->dl_qos_range_length < sizeof(dl_qos_cl_range1_t))) }
450-451: (infoackp->dl_addr_length < DLPI_PHYSADDR_MAX)
472-473: see comment for line 464
440: what guarantees that &infop->di_qos_range is big enough to hold
all infoackp->dl_qos_range_length bytes?
Same question as for comment of line 432.
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?
Flag is set when 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?
For the destination buffer since infop->di_physaddr is an array of
DLPI_PHYSADDR_MAX isn't it safe to assume the size?
I've added a check (takes care of the later comment as well):
if (infop->di_physaddrlen > DLPI_PHYSADDR_MAX ||
infop->di_bcastaddrlen != infop->di_physaddrlen
Do you suggest a check of offset and a check of dl_sap_length?
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.
Would you suggest if it's not equal to return DL_BADADDR and same for
above?
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.
Not needed except to keep it consistent as the others. But in that case
dlpi_passive(), dlpi_unbind() could be done the same.
527-542: this function belongs nearer its friend -- dlpi_mactype.
Will move it closer to its friend.
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 ...)
I've changed this to:
return (i < nprimsizes ? dlpi_primsizes[i].dp_primsz : 0);
548: consider adding a provider buffer length field here.
570: consider having linkname buffer length passed in.
Added buffer length fields here.
576: should probably be ">=".
579: will access one off the beginning of the array if "" is passed
in for provider.
Added a check of:
if (provlen = 0 || provlen >= DLPI_LINKNAME_MAX)
return (DLPI_ELINKNAMEINVAL);
668: should this function do something about dip->dli_sap?
Probably but I'm not sure what can be done since setting it to zero also
has a meaning.
696: what guarantees that the addr_length value from the user is
sane (and doesn't cause a buffer overrun)?
There is not guarantee. I've added a check of:
if (addr_length > DLPI_PHYSADDR_MAX)
return (DLPI_EINVAL);
784-785: should do more checking here.
Added a check for dl_addr_length:
if ((physackp->dl_addr_offset != 0) &&
(physackp->dl_addr_length != 0) &&
(physackp->dl_addr_length <= DLPI_PHYSADDR_MAX)) {
adding a check for offset as well.
physackp->dl_addr_offset > DL_PHYS_ADDR_ACK_SIZE;
790: destination buffer size check?
Added a 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.
Set *addrlenp to zero if driver returns a zero-length address.
821: how do we know that addrlen is safe?
A check of addrlen > DLPI_PHYSADDR_MAX has been added. to line 810.
830: what does "an active primitive" mean? (Does it mean "passive
mode has already been activated?")
I was trying to comment more as this:
* Enable DLPI passive mode on a DLPI handle. We intentionally do not care
* if this request fails, as this indicates the underlying DLPI device does
* not support link aggregation and thus implicitly supports passive mode,
* which is primitives listed in dlpi(7p), like DL_BIND_REQ will not fail
* with DL_SYSERR/EBUSY).
888: check on daddrlen validity.
I've add a check.
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.
Still working on this.
939: 'else' here is misleading. Entire rest of function is actually
the 'else' clause, due to line 938.
I've removed the 'else' .
953-954: should check more.
Added a check for:
(udatap->dl_src_addr_length <= DLPI_PHYSADDR_MAX))
Do you suggest to add a check for the offset as well?
udatap->dl_src_addr_offset >= DL_UNITDATA_REQ_SIZE;
958,962: how do we know saddrp is big enough?
975,979: same for recvp->dri_destaddr.
The approach was, as will be in the manpage for dlpi_recv(), the caller
needs to ensure saddrp is atleast DLPI_PHYSADDR_MAX.
As in other places will add a check for saddrp.
For recvp->dri_destaddr, the structure member is defined to be a an
array of DLPI_PHYSADDR_MAX. Will that ensure the size otherwise will add
a check of sizeof(recvp->dri_destaddr) >= DLPI_PHYSADDR_MAX.
983: I'd suggest passing the dl_group_address value back to the
caller unmolested. Values other than 0 and 1 may be significant.
Isn't the purpose of dri_isunicast just to indicate if it is a unicast
address or multicast/broadcast address?
1009: should this also return failure if i == -1 (that is, if the
input link name was either _all_ digits or just "")?
Yes, otherwise this would be a problem.
1026-1031: this could probably use a more detailed comment.
I can add more details but this code is soon to be whacked by the tunnel
work. So didn't put in much time.
1033-1038: seems a bit misleading; consider reformatting.
I 've reformatted this to:
if (retval != DLPI_SUCCESS) {
dip->dli_mod_pushed = 0;
return (retval);
} else {
dip->dli_fd = fd;
}
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?
As mentioned above in comment for line 1026, this code was left as the
previous code, except the variable name changes, since it will be
removed by the IP Tunnel work.
The code here for example for ifconfig ip.tun0 plumb does is
line 1047:dli_mod_pushed++ is done so that the pushing of the last
module is done in i_dlpi_style2_open.
Line 1087 is to bring the dli_mod_pushed to the actual value (bumped in
line 1047) so the ioctl call in line 1094 is done.
I can add a comment and put a note that this code is temporary and will
be simplified with the IP tunnel work.
1236: all the other user interface functions have checks for NULL
dip. Why not this one?
This has been fixed.
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. :-<)
Sorry I don't have a better solution. If DL_NOTIFY_IND is to be added,
the notification will have to probably be more complicated. Like the api
dlpi_notify() could be added a call back function and callback argument
mechanism as proposed in the PSARC fasttrack?
1291-1294: what does this represent? Can it ever happen?
Probably not, it is a default condition if anything unexpected happens.
1351-1408: should be at top of file, or static to the function that
uses it.
Will fix in the new webrev.
1359: this string isn't reachable; could just be NULL.
Would this be just a matter of preference if left with as the error text?
1407: why is Style 1 an error? (Maybe error text is confusing.)
This is an error returned when a STYLE2 is expected. Would
DLPI_ENOTSTYLE2 with error test of "Not Style 2 DLPI device" easy to
understand?
1416: will drop core if err is less than zero.
Added a check for >= to zero.
common/libdlpi.h
77: why not just have DLPI_SUCCESS = 10000 and remove DLPI_ERRBASE?
In dlpi_strerror() wanted to do the calculation 'err - DLPI_ERRBASE'
instead of DLPI_SUCCESS. I've changed ithave it to be:
enum {
DLPI_ERRBASE = 10000, /* starting libdlpi error code */
DLPI_SUCCESS = DLPI_ERRBASE, /* libdlpi success */
common/libdlpi_impl.h
74: why not milliseconds (which is what poll(2) uses)?
Except for dlpi_recv(), the timeout values for other APIs is in
seconds therefore I chose to set it to seconds.
79: is this actually t_uscalar_t?
Are you asking because dl_provider_style is t_uscalar_t?
81: this is used elsewhere as uint_t; inconsistent.
I will fix this to be a uint16_t elsewhere. The reason for that is
uint16_t is what the drivers use for the sap value.
84,85: probably cannot be < 0.
Yes will fix to be uint_t.
Thanks,
Sagun
_______________________________________________
networking-discuss mailing list
[email protected]
