Comments on the following file attached.
libdladm.h, libwladm.h, liblaadm.c, libdladm.c
--
K. Poon.
[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 :-)
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.
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.
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?
78, 79: Please explain that 15 and 3 are from the newly added
entry to passwd for the user dladm.
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");
...
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.
392-393: Same comment as above on liblaadm.c 73-74.
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.
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().
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.
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?
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?
490: I suppose so_class should be set according to the class
parameter even though right now only DLADM_SECOBJ_CLASS_WEP
is supported.
545: Why is this check?
548: I guess classp should be set according to the returned objp's
so_class.
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?
598: Maybe you can replace malloc() with calloc() so that you don't
need to call memset(0)?
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.
679: Is it better to call it li_val instead? It is really the value,
not the next value, right?
672, 677: Do we need to allocate space for the name? If this is not
needed, please explain why.
695: Should it return an error if a buffer of MAXLINELEN cannot hold
all the data?
714: Please use (lip != NULL).
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.
731: Is it just a memcpy()?
751: Please use (lip != NULL).
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.
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?
784: Please use (lvp != NULL).
817: Should all the lv_nextval malloc()'ed be freed also? Otherwise,
it is a memory leak.
834: Please use (lip != NULL).
846: Please use (lvp != NULL).
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.
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?
900: So only the last error is returned to the caller? Please add a
comment to explain why this behavior is OK.
933: I think it is better to initialize len in the for statement in
941.
943: Can the loop be simplified using strtok_r() or strchr()?
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.
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)
...
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.
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.
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.
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?"
1194: Return B_TRUE?
1198-1207: What is the use of this function? Why does it always return
B_FALSE?
1210-1228: Why does this function always return B_TRUE, even when it
hits an error?
1245: Should the code check for whether tolower(c) is between 'a' and 'f'?
Why can't we use strtol() instead?
1250: I suggest to add a block comment before this function explaining
what is the expected input format of buf.
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.
1297-1317: Same comment as above on 1047-1055.
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.
1367: Same comment as 1106.
_______________________________________________
networking-discuss mailing list
[email protected]
