Comments on libwladm.c attached.
--
K. Poon.
[EMAIL PROTECTED]
libwladm.c:
233: Is the reason to do a do_get_bsstype() to check if the link
is indeed wireless? Please add a comment.
245: It seems that there is no need to have the local variable
status. Instead of return (status), just return the mode.
327: I suppose there is no need to unset WLADM_WLAN_ATTR_CHANNEL as
it should not be set anyway.
377-384: It is unclear what the intention of this code is. If
before the scan the link is connected, then the code
will not do anything. But if the link is not connected
before the scan and is now connected, the code will
do the disconnect. What is the resaon for doing the
above? So suppose there is one thread trying to
do a connect while another thread is doing the scanning.
So does it mean that the scanning thread will disconnect
the other thread's connect? Please add a comment.
393-402: I suggest to move these to the beginning of the file and
add comments explaining what they are.
405: I think the name should somehow indicate that this function
is just comparing strength and speed, not all attributes. If
the intention is to have a more general attribute comparison
function, I suggest to add a comment explaining the current
limitation. I also suggest to add a comment explaining why
signal strength is more important than speed when doing the
comparison.
427: It would be good to add a block comment before this function
to expplain that this is to be used as the callback to
wladm_scan() in wladm_connect(). So it will filter out (i.e.
return B_TRUE to continue the scan) all the APs with a
different attribute set. Maybe it should be called scan_cb()...
550: Should this sleep time be configurable instead of hard coded
as 200000000 ns? Maybe it should be dynamically calculated
based on the given timeout? I guess at least it should be
#define.
562: 1000000000 can be replaced by the standard #define NANOSEC.
588, 591, 601, 630: Should the doc be updated to include these
errors or the return value be changed?
652: A nit. If the return value of attr_compare() is reveresed,
I guess the code does not need to try from the end of the
array.
660: I think there should be a comment explaining why after a
connect fails, the code needs to do a disconnect.
687: Should the doc be updated to include this error or the return
value be changed?
701-714: I think there should be a comment explaining why
do_disconnect() returns OK, yet the code needs to check
the link status again to see if it is really disconnected.
Is there a time delay between disconnect starts and finishes?
If there is, should the code wait for longer? Is this
behavior specific to a driver?
746: Should this error be returned to the caller? Otherwise,
the caller may conclude that there is simply no link available.
But this is not true and there is only a problem so that some
links are not returned.
766: Should the doc be updated to include this error or the return
value be changed?
775: If (*func)() returns B_FALSE, shouldn't the loop be stopped
immediately? I guess it is because it needs to free wlist.
Please add a comment.
800, 803: Should the doc be updated to include these errors or
the return value be changed?
861: Should WLADM_SPEED2STRENGTH be called WLADM_RSSI2STRENGTH
instead? What does "SPEED" mean in WLADM_SPEED2STRENGTH?
959, 964, 970: I think it is simpler to just do return (error)
in these cases.
958: I think there is no such return value explained in the doc.
I suggest to check all the caller of do_check_prop() and update
their respective return value in the doc.
981: I think it is simpler to just do free(vdp) and return (error).
988: If the above are done, then only return (WLADM_STATUS_OK);
is needed. In fact, the local variable status can be removed.
996: The prototype is different in the doc. There is no val_cnt
parameter in the prototype wladm_set_prop() in the doc.
1017: Can prop_name be NULL? If it cannot be, I suggest checking
its value before the loop and return an error to the caller
if it is NULL. If it can be NULL, I think the doc needs to
be updated to explain what is the semantics of wladm_set_prop()
with a NULL prop_name argument.
1023-1057: Is there a reason why these lines need to be inside the
for loop?
1070: The doc says that this function may return WLADM_STATUS_FAILED.
What is the reason?
1109-1146: Is there a reason why these lines need to be inside the
for loop?
1094, 1097, 1121, 1128: Should the doc be updated to include these
errors or the return value be changed?
1111: Since gbuf is only needed for this case, maybe the malloc()
can be done in this case only.
1157, 1172: Can the function just return a boolean_t value instead
of an int?
1199: Can _link_ntoa() be used in this function?
1201: Is it correct to pass WLADM_STRSIZE to snprintf() every time
inside the loop?
1209: Is this function supposed to be used externally? It is not
mentioned in the doc.
1214: If it cannot be found, should the function return NULL to
notify the caller this problem?
1282: Maybe strlcpy() is better?
1562-1700: Is there a specific reason why in some functions a
switch() is used but in other functions, a chain of
if-else is used?
1720: What are 5 and 13? Please add a comment.
1738: Is there a specific reason why 0x0 is used instead of plain 0?
1742: Should strlcpy() be used for safety reason?
1759, 1764: Isn't it simpler to just return the error?
1783: Isn't it simpler to just free(vdp) and return the error?
1791: If the above is done, then just a return (WLADM_STATUS_OK) is
needed here.
1892: Is there a particular reason why memset() is used to initialize
a boolean_t value?
_______________________________________________
networking-discuss mailing list
[email protected]
