Thanks for the quick reply, Eric! Sounds good.
-renee
On Fri, Nov 03, 2006 at 02:39:47PM -0800, Eric Cheng wrote:
> > 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]
