On Tue, 2018-10-09 at 22:47 +0200, Johannes Berg wrote:
> On Tue, 2018-10-09 at 13:42 -0700, James Prestwood wrote:
>
> > > In general, I guess what would work is to be able to *restrict*
> > > the
> > > advertised features over what's currently the case, but I suspect
> > > that's not something that would be so very much useful for you
> > > (unless we
> > > also add more features to hwsim).
> >
> > Actually this would work perfectly. Just being able to
> > disable/restrict
> > features will work fine. I can change the attribute to take a mask
> > of
> > disabled/enabled features, but only features that hwsim actually
> > supports.
>
> Well I guess you really only need *disabled* ones since the default
> would remain to have all enabled, and you can't change them on the
> fly,
> only set them at radio creation, right?
Yes, disabling features is really what we needed ultimately. And
changing them on the fly is not needed.
>
> > The reason I did it this way was to follow how NL80211 packaged up
> > iftype support (NL80211_ATTR_SUPPORTED_IFTYPES).
>
> Ok, but I still think a bitmap would make more sense, back then for
> some
> reason I/we didn't (like to) do bitmaps, but these days I think it's
> the
> much better representation.
>
> > We could model this
> > after how we want the feature support, where we only allow
> > disabling/enabling features/iftypes that are already supported in
> > the
> > default configuration. So, for iftypes, my code could remain the
> > same
> > where I build up a mask of iftypes, but then AND that with a the
> > default configuration.
>
> Sort of.
>
> Yes, I think we would want to have a list/mask of *enabled*
> (extended)
> features/interface types, unlike what I said above to just have a
> mask
> of disabled features; if we do a mask of disabled ones then one
> basically has to list all current and *future ones* to have
> predictable
> output, that's not going to be feasible.
>
> So yes, we want a list of *enabled* ones.
>
> However, I think we want to reject configurations that list more
> enabled
> features than we currently support, because otherwise again you end
> up
> with unpredictability if the hwsim version changes underneath your
> tool
> or test case.
Ok, yeah this makes sense.
>
> So I think the algorithm should be:
>
> valid_features = <current list>;
> enabled_features = nla_get_whatever(...);
>
> if (enabled_features & ~valid_features) {
> NL_SET_ERR_ATTR_MSG(...);
> return -EINVAL;
> }
>
> // use enabled_features instead of the current list later
Ok, thanks for your help/suggestions. Ill get a patch together with the
talked about changes.
>
> johannes
