> 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.
>
yes. we will definitely look closely at our final webrev to make sure
there aren't spurious changes.
> [...]
> > 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];
> }
we'll fix this.
>
> > 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.
>
agreed. will fix this as well.
> [...]
> > 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.
>
ok. will do that.
eric
_______________________________________________
networking-discuss mailing list
[email protected]
