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