Thanks for the reply, Cecilia. Just a few follow-ups below.
On Wed, Nov 01, 2006 at 03:55:37PM +0800, Cecilia.hu wrote:
[...]
> 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"
Okay, I wouldn't expect any. But the webrev shows that the spacing on
those lines changed. Maybe just a merge issue.
[...]
> 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 }
No, that wasn't exactly my point. What I was trying to get at was
that once you've found the wifi_field whose name matches sp->s_fields[i],
there's no need to continue searching. If the cmdtype matches, this is
a successful match; if cmdtype does not match, then we know that there
will not be a successful match. So the inner loop should stop as soon
as a name match is found. The cmdtype check simply determines whether
or not there was a successful match.
I would suggest something like:
for (i = 0; i < sp->s_nfields; i++) {
boolean_t good_match = B_FALSE;
for (j = 0; j < WIFI_MAX_FIELDS; j++) {
if (strcasecmp(sp->s_fields[i],
wifi_fields[j].wf_name) == 0) {
good_match = wifi_fields[j].wf_cmdtype & cmdtype;
break;
}
}
if (!good_match)
goto fail;
wf[i] = &wifi_fields[j];
}
> 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.
My point was that an illegal combination of parameters could be used and
not flagged by this code. If the user had options '-s none -k <wepkey>'
this code would effectively ignore the '-s none' bit and simply set the
security mode to wep. I think the conflict in parameters should be noted.
[...]
> 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.
That would be a good start. Might want to get some opinions from security
types on this.
-renee
_______________________________________________
networking-discuss mailing list
[email protected]
