Renee,
Thanks for the review. Below is our response to your comments.
-------------------------------------------------------------------------------
FEEDBACK ON TECHNICAL DOCUMENTATION/CODE
-------------------------------------------------------------------------------
Reviewer Name: Renee Danson
Document/Module Title: wifi-ws
Document/Module Version/Date: 9/26/06
-------------------------------------------------------------------------------
the 16 files not mentioned here: no comments
-------------------------------------------------------------------------------
-------------------------------------------------------------------------------
usr/src/cmd/svc/milestone/net-physical
-------------------------------------------------------------------------------
RD-01 61 Com Should mention secobj in this comment, too.
Might also note that linkprops will be init'd
later, after interfaces are plumbed.
|ACCEPT. modify the comments as below:
| #
| # Bring up link aggregations and initialize security objects.
| # Note that link property initialization is deferred until after
| # IP interfaces are plumbed to ensure that the links will not
| # be unloaded (and the property settings lost).
| #
RD-02 177 Cos Need to resolve XXX
|EXPLAIN: this is aberrant behavior specific to wifi interfaces
|(i.e. the act of plumbing could cause an interface to reset all its
settings)
|We will log a bug on this behavior, and will change XXX to TODO so as to
|remind us this is a defect that should be fixed at some points.
RD-03 179 Func This is highly unlikely (or maybe not even
allowed?), but what if the interface is IPv6-
only? Shouldn't the init-linkprop be moved
after the v6 interfaces are plumbed, too?
|ACCEPT: yes. I agree that the init-linkprop will be moved after plumbing
|the v6 interfaces.
-------------------------------------------------------------------------------
usr/src/lib/libbsm/audit_class.txt
-------------------------------------------------------------------------------
RD-04 28 Func This comment says you also need to edit audit.h.
But, I can't find audit.h. So it's not obvious
to me what exactly what this note means, but
I'd like to confirm that you've either done
what it is telling you to do, or you have a
reason not to.
|EXPLAIN. The audit.h mentioned in comments is <bsm/audit.h> which is at:
| ~/usr/src/uts/common/c2/audit.h.
|That file is used to declare various data structure used by auditing
|modules. For this case, dladm need only audit "as,cy" which will no
|introduce any new data structure, so we don't need to update
|audit.h for this case.
-------------------------------------------------------------------------------
usr/src/lib/libbsm/common/adt_xlate.c
-------------------------------------------------------------------------------
RD-05 919 Func There are only 44 entries in the table, right?
|ACCEPT. Yes, it will be changed to 44.
-------------------------------------------------------------------------------
usr/src/cmd/dladm/dladm.c
-------------------------------------------------------------------------------
RD-06 202-3, 208 Cos Total nit: why is the whitespace change needed?
|EXPLAIN. We have not done any change on (202-203, 208).
| 202-203 " [-l <mode>] [-T <time>]\n"
| " [-u <address>] -d <dev> ... <key>\n"
| 208 " [-l <mode>] [-T <time>] [-u <address>] <key>\n"
RD-07 old 1866 Func Why was this line (call to kstat_close() in
get_single_mac_stat()) removed? Seems like it
shouldn't have been.
|ACCEPT. Yes, it's a mismerge. We will fix it.
RD-08 2057 Func lasts should be initialized to NULL.
|ACCEPT. The lasts pointer will be changed to be initialized.
RD-09 2095-2100 Perf If you find a matching name, but the cmdtype is
wrong, no need to continue looping. I would
separate out the cmdtype check, so if the name
matches but it doesn't you can bail.
|ACCEPT. Check cmdtype first is also ok. It would be like below:
| 2095 for (j = 0; j < WIFI_MAX_FIELDS; j++) {
| 2096 if (strcasecmp(sp->s_fields[i],
| 2097 (wifi_fields[j].wf_cmdtype & cmdtype) &&
| 2098 wifi_fields[j].wf_name) == 0)
| 2099 break;
| 2100 }
RD-10 do_connect_wifi Func I think there's a potential error case missed
here. If the user specifies a security mode
(other than wep) and also specifies a wep key,
the 'k' option handling will override the 's'
option handling, and things will proceed as if
the -s option hadn't been included. That case
should result in an error, I think.
|EXPLAIN.
| By now we only support wep security mode and if we support more mode
in future
| we can check what type is the key specified by "-k" and select the
right mode.
RD-11 2759 Func duplicate of line 2752
|ACCEPT. duplicate will be deleted.
RD-12 convert_secobj() Com There are several seemingly magic numbers in
this function (the checks on lines 3175 and
3184, for example). Some comments would be
useful; using #defines for those numbers could
help, as well.
|ACCEPT. We will add some comments here to indicate what we are doing
RD-13 audit_*() CodeStd audit_create() and audit_delete() are almost
identical; could they be collapsed to one, with
a differentiating parameter?
|ACCEPT. A new parameter (boolean_t)create will be added to the new function
|audit_secobj(...), so audit_*() can be merged to one function.
RD-14 3532, 3620 CodeStd Probably want to or this flag in, rather than
explicitly setting; it's okay for now, as we
only deal with one flag, but if there are more
in the future, this could be a problem.
|ACCEPT.
| Here we would use |= instead of =. It will be changed to:
| flags |= DLADM_OPT_TEMP;
RD-15 3686 Design Do we really intend to leave in the ability to
dump out the secure values? This doesn't seem
good to me.
|DISCUSS.
| We can check uid first if it's not root the secobj's value would not be
| printed out.
Comment type key:
Func comment on functionality
Perf comment on performance
CodeStd comment on coding standards
Design comment on design
Edit editorial comment
Cos cosmetic comment
Com comment on missing comments
--
[EMAIL PROTECTED]
Solaris x86 Engineering, Sun Microsystems
+86-10-82618200 ext. 82947
_______________________________________________
networking-discuss mailing list
[email protected]
