On Wed, Mar 8, 2023 at 5:07 PM Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
>
> On Wed, 2023-03-08 at 08:00 +0000, Jaewan Kim wrote:
> > >
> > > > +static int parse_pmsr_capa(const struct nlattr *pmsr_capa, struct cfg80211_pmsr_capabilities *out,
> > > > + struct genl_info *info)
> > > > +{
> > > > + struct nlattr *tb[NL80211_PMSR_ATTR_MAX + 1];
> > > > + struct nlattr *nla;
> > > > + int size;
> > > + int ret = nla_parse_nested(tb, NL80211_PMSR_ATTR_MAX, pmsr_capa,
> > > > + hwsim_pmsr_capa_policy, NULL);
> > > > +
> > > > + if (ret) {
> > > > + NL_SET_ERR_MSG_ATTR(info->extack, pmsr_capa, "malformed PMSR capability");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (tb[NL80211_PMSR_ATTR_MAX_PEERS])
> > > > + out->max_peers = nla_get_u32(tb[NL80211_PMSR_ATTR_MAX_PEERS]);
> > > > + out->report_ap_tsf = !!tb[NL80211_PMSR_ATTR_REPORT_AP_TSF];
> > > > + out->randomize_mac_addr = !!tb[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR];
> > > > +
> > > > + if (!tb[NL80211_PMSR_ATTR_TYPE_CAPA]) {
> > > > + NL_SET_ERR_MSG_ATTR(info->extack, tb[NL80211_PMSR_ATTR_TYPE_CAPA],
> > > > + "malformed PMSR type");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + nla_for_each_nested(nla, tb[NL80211_PMSR_ATTR_TYPE_CAPA], size) {
> > > > + switch (nla_type(nla)) {
> > > > + case NL80211_PMSR_TYPE_FTM:
> > > > + parse_ftm_capa(nla, out, info);
> > > > + break;
> > > > + default:
> > > > + WARN_ON(1);
> > >
> > > WARN_ON doesn't seem right here. I suspect that the following is more fitting.
> > >
> > > NL_SET_ERR_MSG_ATTR(...);
> > > return -EINVAL;
> > >
> >
> > Not using NL_SET_ERR_MSG_ATTR(...) is intended to follow the pattern
> > of net/wireless/pmsr.c,
> > where unknown type isn't considered as an error.
>
> NL80211_PMSR_ATTR_TYPE_CAPA is normally NLA_REJECT (not sent by
> userspace), you just use it here for the hwsim capabilities which makes
> sense, but it feels better to just reject unknown types.
>
> If you're thinking of actually using it we still have in pmsr.c this
> code:
>
> nla_for_each_nested(treq, req[NL80211_PMSR_REQ_ATTR_DATA], rem) {
> switch (nla_type(treq)) {
> case NL80211_PMSR_TYPE_FTM:
> err = pmsr_parse_ftm(rdev, treq, out, info);
> break;
> default:
> NL_SET_ERR_MSG_ATTR(info->extack, treq,
> "unsupported measurement type");
> err = -EINVAL;
> }
>
>
> johannes
Done, and uploaded the next patchset for this. Thank you for the review.
--
Jaewan Kim (김재완) | Software Engineer in Google Korea |
jaewan@xxxxxxxxxx | +82-10-2781-5078
