Search Linux Wireless

Re: [PATCH V9 2/2] cfg80211/nl80211: Enable drivers to implement MAC address based ACL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I'm applying this because I've sat on it for such a long time, but I
think you can do better. I've fixed these issues with your patch:

> + * @NL80211_ATTR_ACL_POLICY: ACL policy, see &enum nl80211_acl_policy_attr.

You missed documenting the data type of this, I've changed it to u32 and
added the documentation.

> +/**
> + * enum nl80211_acl_policy_attr - access control policy
> + *
> + * Access control policy is applied on a MAC list set by
> + * %NL80211_CMD_START_AP and %NL80211_CMD_SET_MAC_ACL, to
> + * be used with %NL80211_ATTR_ACL_POLICY.
> + *
> + * @NL80211_ACL_POLICY_ACCEPT_UNLESS_LISTED: Deny stations which are
> + *	listed in ACL, i.e. allow all the stations which are not listed
> + *	in ACL to authenticate.
> + * @NL80211_ACL_POLICY_DENY_UNLESS_LISTED: Allow the stations which are listed
> + *	in ACL, i.e. deny all the stations which are not listed in ACL.
> + * @__NL80211_ACL_POLICY_AFTER_LAST: Internal use
> + * @NL80211_ACL_POLICY_MAX: Highest acl policy attribute
> + */
> +enum nl80211_acl_policy_attr {
> +	NL80211_ACL_POLICY_ACCEPT_UNLESS_LISTED,
> +	NL80211_ACL_POLICY_DENY_UNLESS_LISTED,

You're modelling this like an attribute, yet it's really just an enum
value, you should model it like enum nl80211_connect_failed_reason
(which you even touched the docs for.)

No need for all the overhead necessary for attributes.

Oh, and IF it actually had been attribute, then you'd have to reserve
index 0, so you'd have it wrong.


> +       if (WARN_ON(wiphy->max_acl_mac_addrs &&
> +                   (!(wiphy->flags & WIPHY_FLAG_HAVE_AP_SME) ||
> +                   !rdev->ops->set_mac_acl)))
> +               return -EINVAL;

bad indentation

> +       if ((dev->wiphy.flags & WIPHY_FLAG_HAVE_AP_SME) &&
> +           nla_put_u32(msg, NL80211_ATTR_MAC_ACL_MAX,
> +                       dev->wiphy.max_acl_mac_addrs))
> +               goto nla_put_failure;

(Almost?) all attributes are left out if they're not supported, so this
should be too.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux