Re: [PATCH] nl80211: Add set/get for frag/rts threshold and retry limits

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On Fri, 2009-04-17 at 22:38 +0300, Jouni Malinen wrote: 
> On Fri, Apr 17, 2009 at 09:29:33PM +0200, Johannes Berg wrote:
> > > +	if (changed & WIPHY_PARAM_RTS_THRESHOLD) {
> > > +		if (local->ops->set_rts_threshold)
> > > +			local->ops->set_rts_threshold(local_to_hw(local),
> > > +						      wiphy->rts_threshold);
> > > +	}
> > 
> > That could return an error. (so do the changes the other way around)
> 
> I noticed, but ended up following the existing behavior in wext.c. While
> that may not be the ideal source for good behavior, at least this is
> consistent. Should both of them be changed? What exactly should happen
> on error?

Well if you do the rts change first, and return early, you change none
of the other parameters when that has an error.

> > > +		result = rdev->ops->set_wiphy_params(&rdev->wiphy, changed);
> > > +		if (result)
> > > +			goto bad_res;
> > > +	}
> > 
> > If that returns an error we need to roll back the values?
> 
> I thought about this, but ended up not doing that because the existing
> (wext) code seemed to behave in the same way. It is unclear whether the
> error there would indicate that some of the parameters were taken into
> use, but not all. Unless we provide mechanism for returning that
> information (e.g., separate calls for each parameter), I'm not sure what
> exactly should be done here. Just leaving the parameters (which were
> validated before) in struct wiphy seemed like the safest (and well,
> certainly easiest ;-) alternative.

And then here you can just mandate that it's either-or and returning an
error means that you haven't done any of the requested changes...

Or you just remove the ability to return an error and let whoever needs
that ability for their hardware deal with it :P

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]