kacheong,
sorry for the late reply.
below are my responses.
---------------------------------------------------------
[EMAIL PROTECTED]
A Meta comment. I think that while most of the functions and
data structures are quite self-explanatory, it is still a
good idea to have some comments explaining how things work,
what do all those parameters to a function mean, etc. It is
quite "interesting" to read through all those files and
find that at most there are a handful of comments :-)
| ACCEPT
| Will try to add more comments
libdladm.h:
No comment.
libwladm.h:
38-39: It would be good if there is a comment explaining that
this length is defined in the 802.11 protocol, not just
some number we come up with.
| ACCEPT
| will add comment.
|
41: It would be good if there is a comment explaining what
speed really is and where do all those magic numbers,
12, 9, 6, and 3 come from.
| ACCEPT
| This is because drivers return a value ranging from 0-15.
| we are just splitting this into five subranges
| ( 0-2 = very weak, 3-5 = weak, 6-8 = good, 9-11 = very good,
| 12 = excellent)
|
liblaadm.c:
73-74: Does it make sense to add a DLADM_DIR macro (/etc/dladm/)
for all files under /etc/dladm instead of including
"/etc/dladm" in all those macros?
| REJECT
| you'll end up having to define something like:
| #define LAADM_DB DLADM_DIR "/aggregation.conf"
|
| I think the current code is clearer than the above.
| DLADM_DIR is a little harder to type and takes up just
| as much spaces.
|
78, 79: Please explain that 15 and 3 are from the newly added
entry to passwd for the user dladm.
| ACCEPT
|
libdladm.c:
273-317: Do we really need to pass in buf for this simple
function? Is there a problem to just do something
like
switch (status) {
case DLADM_STATUS_OK:
return ("ok");
...
| EXPLAIN
| I agree this looks better.
| but the intention was to make this function consistent with
| other *2str() interfaces, which do need to take a buf argument.
|
346: Please write a comment explaining that there is not a
1-1 mapping between dladm and wladm error codes. This
is not clear in reading the code in this function as
what it does seems to be changing WLxxx to DLxxx...
Please also add a comment explaining why this is needed
and how it is used. I guess it is also good to add a
comment in libwladm.h explaining the wladm_status_t enum.
| ACCEPT
|
392-393: Same comment as above on liblaadm.c 73-74.
| REJECT
| same response as above.
|
398-399: Why do we need DLADM_DB_OWNER and LAADM_DB_OWNER? Do
they need to be different? If they don't need to be
different, I'd suggest removing one of them and put the
#define in the appropriate header file. If they need
to be different, I'd suggest putting a comment explaining
this.
| EXPLAIN
| they don't need to be different.
| the problem with putting the #define in one place is that
| the libraries are structured such that libdladm can depend on liblaadm
| but not the other way around (to avoid circular dependency). because
| of this, we can't put the #define in libdladm.h. (technically we
| can simply have liblaadm include libdladm.h but not link with
| libdladm. but I don't think this is usual practice)
| it also doesn't make sense to put the #define in liblaadm.h that would
| mean that the generic db processing code needs to depend on something
| media specific.
|
413: I understand that right now only wladm supports setting link
propoerty. But it does not seem right to directly call
wladm_is_valid(). Should there be a call to get the link
type and then depending on the type, we call different
property setting routine? This will avoid the need to call
open_link() in both wladm_is_valid() and wladm_set_prop().
The above comment also applies to dladm_walk_prop() and
dladm_get_prop().
| EXPLAIN
| In order to implement the function to determine the link type, I'd
| have to put media-specific code (e.g. wifi specific ioctls) in
| libdladm; this is what I want to avoid because libdladm is meant
| to be media independent.
| If in the future we want to support another link type, the code
| will probably look somewhat like:
| dladm_set_prop(link, ...)
| {
| if (wladm_is_valid(link)) {
| wladm_set_prop(link,...);
| ...
| }
| if (ether_is_valid(link)) {
| ether_set_prop(link,...);
| ...
| }
| }
| can you think of a better way?
|
| also, you can't really avoid the open_link() within wladm_set_prop()
| because libwladm is meant to be usable outside of libdladm as well.
| I'd prefer to not expose the file descriptors.
|
445: I understand that DLADM_PROP_VAL_PERSISTENT is not applicable
to wl link. But will it be applicable to other links in
future? If it will, I'd suggest to re-arranage the code to
reflect this.
| EXPLAIN
| No it won't. the intention is to centralize all db manipulation
| code within libdladm. (note that liblaadm still maintains its
| own db, but it's not part of this project so we're not converting
| it to use our new code)
|
453-464: A question on the design of wladm_prop_type_t. Will it
always be a subset of dladm_prop_type_t? If it will, do
we need to define wladm_prop_type_t? How about other
link types in future? Does each of them need to define
their own XXadm_prop_type_t?
| EXPLAIN
| wladm_prop_type_t can in fact be a superset of dladm_prop_type_t
| it's just that libdladm will only use the types it's aware of.
| I agree it's simpler to not require each link type to define
| their own *_prop_type_t; but libwladm (and other future libraries)
| are designed to allow their interfaces to be used independently
| of libdladm. that's why defining these duplicate types is
| unavoidable. I think this is a small price to pay for more
| flexibility.
483: I understand that right now there is only the WEP security
object. But I don't think it is a good idea to directly
check for DLADM_SECOBJ_CLASS_WEP here. Maybe adding a macro
or a function such as dladm_check_secobj_class() to check for
the validity of the class parameter?
| ACCEPT
| will add your suggested function
|
490: I suppose so_class should be set according to the class
parameter even though right now only DLADM_SECOBJ_CLASS_WEP
is supported.
| ACCEPT
| I will likely add a function that converts a DLADM_SECOBJ_CLASS_WEP
| into DLD_SECOBJ_CLASS_WEP.
|
545: Why is this check?
| ACCEPT
| This was just a sanity check to ensure the kernel only returns
| class types that libdladm is aware of. I'll remove it.
|
548: I guess classp should be set according to the returned objp's
so_class.
| ACCEPT
| right. I'll add a conversion function like 490 above.
|
585: Does it make sense to have another ioctl() to get the size
of all objects such that we don't need to hard code the buffer
size here?
| REJECT
| I think it's simpler to not have to add another ioctl.
| also, between the time you got the count and the time you invoke
| the GET ioctl, someone else could have inserted other objects.
| my code passes in the maximum allowable buffer (64k) for I_STR
| ioctls. this ensures that it will always get everything.
|
598: Maybe you can replace malloc() with calloc() so that you don't
need to call memset(0)?
| ACCEPT
|
671-693: I think it is better to move all these up to the beginning
of the file. Please also add some comments explaining what
they are and how they will be used.
| DISCUSS
| I can add some comments for these.
| but do you think it's better to keep these closer to the actual
| code? the reason these are kept down here is that they are
| tied to the internals of the db logic and are not meant to
| be consumed by anything else. I did expose some interfaces that
| are used by the upper half of the file at 58-68. If I also move
| all the internal data structures up here, the clutter will make
| this part of the code less clean.
| another thing I can do is to move the linkprop and secobj code
| along with data structures to separate files linkprop.c and secobj.c,
| would this be better?
|
679: Is it better to call it li_val instead? It is really the value,
not the next value, right?
| ACCEPT
| will change it to li_val.
| li_val is the first of a chain of linkprop_val_t.
|
672, 677: Do we need to allocate space for the name? If this is not
needed, please explain why.
| EXPLAIN
| these point back to the per-line buffer at parsing time.
| will add a comment to indicate that.
|
695: Should it return an error if a buffer of MAXLINELEN cannot hold
all the data?
| ACCEPT
| will restructure function to do that.
|
714: Please use (lip != NULL).
| ACCEPT
|
714-730: I think the loop should also check if ptr has reached lim or
not, otherwise 723 needs to be fixed. Note that snprintf()
will return the number of bytes that would have been written
regardless of the value of second argument. So it means that
ptr can go beyond lim.
| ACCEPT
| will add check to see if lim is reached.
|
731: Is it just a memcpy()?
| EXPLAIN
| no. this appends newline and trailing NUL.
|
751: Please use (lip != NULL).
| ACCEPT
|
767-770: I guess there should also be a block comment before this
function explaining this behavior. So if listp passed in
is NULL, the buf will still be modified yet no new entry
is created.
| EXPLAIN
| no. an entry will be created. this NULL case is meant to be
| used by 1118-1126. i.e. we have walked the file but failed
| to find the specified link. a new line has to be added.
| will add comments to indicate that.
|
780-790: So if later in 797 the malloc() fails, the old property
will still be deleted. I guess this should be pointed out
in the block comment before the function. And is this a
desirable side effect?
| EXPLAIN
| this only deletes the values referenced by this list.
| the file is untouched because we neither call generate_linkprop_line()
| or truncate the buffer by doing buf[0] = '\0'.
|
784: Please use (lvp != NULL).
| ACCEPT
|
817: Should all the lv_nextval malloc()'ed be freed also? Otherwise,
it is a memory leak.
| ACCEPT
| will fix the leak.
834: Please use (lip != NULL).
846: Please use (lvp != NULL).
| ACCEPT
|
851, 856: Does it mean that ls_valcntp can only get smaller and
smaller if process_link_prop_get() is called multiple times
on the same listp? This does not sound right. If this is
the intended behavior, please add a comment to explain.
| EXPLAIN
| process_linkprop_get() will only get called once when
| process_linkprop_line() finds a matching link.
| the returning of B_FALSE at the end of process_linkprop_get()
| ensures that.
| lsp->ls_valcntp is an in/out argument initially passed in
| to indicate the user-specified array size for ls_propval;
| as an out argument, lsp->ls_valcntp is set to the actual
| number of values for this particular property.
| anyway, will add comments to explain the above.
|
884: So even though the malloc(), the function still returns B_TRUE?
And it seems that the function always returns B_TRUE. Is this
the intended behavior?
| EXPLAIN
| yes. I wanted init-linkprop to continue to initialize other properties
| even if failures are encountered.
|
900: So only the last error is returned to the caller? Please add a
comment to explain why this behavior is OK.
| ACCEPT
| no. this is not intended. I'll fix the ENOMEM case above.
|
933: I think it is better to initialize len in the for statement in
941.
| ACCEPT
| I'd prefer not within the for (...) but immediately before it at 940.
|
943: Can the loop be simplified using strtok_r() or strchr()?
| REJECT
| strtok_r() is not possible because it clobbers any matching
| character it finds, which is not good because I need to know
| what matched.
| strchr() can possibly work but you'd end up having to call
| strchr() 3 times. this is less compact than what I have
| (match = (c == '=' || c == ',' || c == ';'))
| also the loop can't be avoided, so strchr() doesn't save
| you much.
|
976: So if the buf is ""foo= ,;" lv_name will point to " ". Is this
the intention? Is the function supposed to do some error checking?
If it is assumed that the buf passed in is in the correct format,
I guess we don't need to handle those error cases as in 969. If
we cannot assume that the buf is in correct format, I guess we need
to do more error handling in this function.
| EXPLAIN
| yes. we can assume that buf is in correct format because it's read from
| a config file we generated. of course, if the file is corrupted, this
| code can handle that gracefully and not crash.
|
1047, 1055: I guess the reason to use strncmp() and then isspace()
is that we don't know the link name in str. I think
1047 and 1055 can be combined to make the code cleaner.
Does the following work?
if (strtok_r(str, " \n\t", &lasts) == NULL)
goto fail;
if (lsp->ls_link != NULL && strcmp(str, lsp->ls_link) != 0)
return (B_TRUE);
if ((str = strtok_r(NULL, " \n\t", &lasts)) == NULL)
...
| REJECT
| the strncmp() and isspace() are used because I don't want the
| buf to be modified in case the specified link doesn't match.
| if the buf gets modified, the fputs() at 1109 will end up
| truncating lines for links we're not interested in.
| the strtok_r() at 1055 is ok because the ls_link == NULL
| case is meant to be used by read-only operations like linkprop_get
| and linkprop_init().
| I'll document the above more clearly.
|
1101-1104: I think the comment is not clear. What is meant by process
and transform? I suggest to elaborate more on exactly what
the loop does.
| ACCEPT
| I agree it's a bit terse. I'll change it to be:
| /*
| * This loop processes each line of the configuration file.
| * buf can potentially be modified by process_linkprop_line().
| * If this is a write operation and buf is not truncated, buf will
| * be written to disk. process_linkprop_line() will no longer be
| * called after it returns B_FALSE; at which point the remainder
| * of the file will continue to be read and, if necessary, written
| * to disk as well.
| */
1106: So once cont is B_FALSE, the loop will never call
process_linkprop_line() again and it will keep reading the
file and writing to nfp. It does not sound right. Is this the
intention? If it is, please add a comment to explain it.
| EXPLAIN
| yes. this is intentional.
| we only want to modify the line we're interested in and leave
| everything else untouched.
| note that we have to do this because we are constructing a temporary
| copy of the config file that will later be renamed to the actual
| config file.
|
1134-1155: I suggest to move these lines to the beginnin of the file
and add comments explaining what they are and how they are
used.
| DISCUSS
| same reasoning as 671-693 above.
|
1169, 1170, 1173, 1175: Same comment as above on the return value of
snprintf(). What tests have been done to verify
that the code runs fine with "misc error input?"
| ACCEPT
| will fix the above to ensure no overflow happens.
| the config file is assumed to be sane so we haven't tested lots of
| error cases. I think as long as we do not crash that's sufficient.
|
1194: Return B_TRUE?
| REJECT
| we got here because we found the secobj we're interested in.
| there is no need to continue.
|
1198-1207: What is the use of this function? Why does it always return
B_FALSE?
| EXPLAIN
| it truncates the buffer. which has the effect of deleting the line.
| we only allow one deletion at a time so this returns B_FALSE.
|
1210-1228: Why does this function always return B_TRUE, even when it
hits an error?
| EXPLAIN
| because malloc()..etc in this case are non-fatal errors. we could
| return whatever we managed to allocate back to the caller.
| but I agree it's better to at least set the statusp so that
| the caller can choose to ignore the error or not.
|
1245: Should the code check for whether tolower(c) is between 'a' and 'f'?
Why can't we use strtol() instead?
| ACCEPT
| I think I can change 1267 to use strtol instead of this function.
|
1250: I suggest to add a block comment before this function explaining
what is the expected input format of buf.
| ACCEPT
| I'll explain that this has to be a hex string prefixed by 0x and
| has an even number of hex digits.
|
1250-1272: I'm puzzled by the code. What is the reason why si_val
needs to be in this format? It is very important to add
some comments explaining this.
| ACCEPT
| see previous comment.
1297-1317: Same comment as above on 1047-1055.
| REJECT
| same response.
|
1363-1365: I think the comment is not clear. What is meant by process
and transform? I suggest to elaborate more on exactly what
the loop does.
| ACCEPT
| my comment will be similar to 1101 above.
|
1367: Same comment as 1106.
| EXPLAIN
| same response as above.
|
libwladm.c:
233: Is the reason to do a do_get_bsstype() to check if the link
is indeed wireless? Please add a comment.
| ACCEPT
| will add:
| /*
| * We invoke this ioctl to check if the link is wireless.
| */
|
245: It seems that there is no need to have the local variable
status. Instead of return (status), just return the mode.
| ACCEPT
|
327: I suppose there is no need to unset WLADM_WLAN_ATTR_CHANNEL as
it should not be set anyway.
| ACCEPT
|
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.
| ACCEPT
| there are certain drivers that automatically associates with
| an AP after a scan operation. this code is a workaround.
| but I think it's best to fix it in the driver.
|
393-402: I suggest to move these to the beginning of the file and
add comments explaining what they are.
| ACCEPT
| I can add comments.
| but don't you think it's better to keep the code/structs
| self-contained? these structs have no use outside of this function.
|
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.
| ACCEPT
| I think attr_compare() is better in case we want to compare
| other attrs in the future.
| will add comments.
|
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()...
| ACCEPT
| will add comments.
| but I'll keep the connect_cb() name since it's used specifically
| for connect for filtering wlans.
|
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.
| ACCEPT
| will make this a #define.
| I don't think this needs to be made a tunable because shortening
| will likely not improve performance in any way and lengthening
| will hurt performance.
|
562: 1000000000 can be replaced by the standard #define NANOSEC.
| ACCEPT
|
588, 591, 601, 630: Should the doc be updated to include these
errors or the return value be changed?
| ACCEPT
| will fix the api specs. unfortunately there are quite a few
| error codes not mentioned there.
|
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.
| ACCEPT
|
660: I think there should be a comment explaining why after a
connect fails, the code needs to do a disconnect.
| ACCEPT
| this is because some drivers will continue with the connect even
| after certain ioctl fails.
| will add comments.
|
687: Should the doc be updated to include this error or the return
value be changed?
| ACCEPT
|
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?
| ACCEPT
| again, this is a driver problem.
| will fix driver code and remove this workaround if possible.
|
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.
| ACCEPT
| I think I can restructure this to return an error.
|
766: Should the doc be updated to include this error or the return
value be changed?
| ACCEPT
|
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.
| EXPLAIN
| correct. it's needs to free the wlist.
| will add comment.
|
800, 803: Should the doc be updated to include these errors or
the return value be changed?
| ACCEPT
|
861: Should WLADM_SPEED2STRENGTH be called WLADM_RSSI2STRENGTH
instead? What does "SPEED" mean in WLADM_SPEED2STRENGTH?
| ACCEPT
| will call this WLADM_SIGNAL2STRENGTH instead.
| I agree SPEED is the wrong name.
|
959, 964, 970: I think it is simpler to just do return (error)
in these cases.
| ACCEPT
|
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.
| ACCEPT
| will update doc.
|
981: I think it is simpler to just do free(vdp) and return (error).
| ACCEPT
| I think the done label is not needed after this and the above changes.
|
988: If the above are done, then only return (WLADM_STATUS_OK);
is needed. In fact, the local variable status can be removed.
| ACCEPT
|
996: The prototype is different in the doc. There is no val_cnt
parameter in the prototype wladm_set_prop() in the doc.
| ACCEPT
| will fix 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.
| ACCEPT
| yes. it can be NULL. it's used when resetting all properties of
| this link to their defaults.
| will update doc.
|
1023-1057: Is there a reason why these lines need to be inside the
for loop?
| EXPLAIN
| yes. because of the need to reset all props at once as mentioned
| above. but I think the current code won't work because of the
| break at 1057. this will be removed.
1070: The doc says that this function may return WLADM_STATUS_FAILED.
What is the reason?
| ACCEPT
| will fix doc.
|
1109-1146: Is there a reason why these lines need to be inside the
for loop?
| ACCEPT
| you're right. these need not be.
|
1094, 1097, 1121, 1128: Should the doc be updated to include these
errors or the return value be changed?
| ACCEPT
|
1111: Since gbuf is only needed for this case, maybe the malloc()
can be done in this case only.
| REJECT
| I prefer the code to be structured this way because in the future
| the WLADM_PROP_VAL_MODIFIABLE properties could be retrieved via
| ioctls.
|
1157, 1172: Can the function just return a boolean_t value instead
of an int?
| REJECT
| it can. but I think returning int is more flexible in case
| there are other errors to return in the future.
|
1199: Can _link_ntoa() be used in this function?
| ACCEPT
| yes. that's possible.
|
1201: Is it correct to pass WLADM_STRSIZE to snprintf() every time
inside the loop?
| ACCEPT
| this will be fixed.
|
1209: Is this function supposed to be used externally? It is not
mentioned in the doc.
| ACCEPT
| no it's not meant for external use. I'll make it a static.
|
1214: If it cannot be found, should the function return NULL to
notify the caller this problem?
| EXPLAIN
| all our wladm_*2str() functions are designed to not fail even
| if someone passes in invalid data. this is done so that the
| caller can use it in printf()..etc right away without having
| to check if the return value is NULL.
|
1282: Maybe strlcpy() is better?
| ACCEPT
|
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?
| ACCEPT
| no particular reason.
| will change everything to use switch for consistency.
|
1720: What are 5 and 13? Please add a comment.
| ACCEPT
| these are the legal lengths for wep keys.
| will add comments
|
1738: Is there a specific reason why 0x0 is used instead of plain 0?
| ACCEPT
| will use 0 instead.
|
1742: Should strlcpy() be used for safety reason?
| ACCEPT
|
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.
| ACCEPT
| will fix the above.
|
1892: Is there a particular reason why memset() is used to initialize
a boolean_t value?
| ACCEPT
| will get rid of the memset() since r doesn't have to be initialized.
|
This message posted from opensolaris.org
_______________________________________________
networking-discuss mailing list
[email protected]
