[net-next,2/6] net: dcb: add new common function for set/del of app/rewr entries
Commit Message
In preparation for DCB rewrite. Add a new function for setting and
deleting both app and rewrite entries. Moving this into a separate
function reduces duplicate code, as both type of entries requires the
same set of checks. The function will now iterate through a configurable
nested attribute (app or rewrite attr), validate each attribute and call
the appropriate set- or delete function.
Note that this function always checks for nla_len(attr_itr) <
sizeof(struct dcb_app), which was only done in dcbnl_ieee_set and not in
dcbnl_ieee_del prior to this patch. This means, that any userspace tool
that used to shove in data < sizeof(struct dcb_app) would now receive
-ERANGE.
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
net/dcb/dcbnl.c | 104 +++++++++++++++++++++++-------------------------
1 file changed, 49 insertions(+), 55 deletions(-)
Comments
Daniel Machon <daniel.machon@microchip.com> writes:
> In preparation for DCB rewrite. Add a new function for setting and
> deleting both app and rewrite entries. Moving this into a separate
> function reduces duplicate code, as both type of entries requires the
> same set of checks. The function will now iterate through a configurable
> nested attribute (app or rewrite attr), validate each attribute and call
> the appropriate set- or delete function.
>
> Note that this function always checks for nla_len(attr_itr) <
> sizeof(struct dcb_app), which was only done in dcbnl_ieee_set and not in
> dcbnl_ieee_del prior to this patch. This means, that any userspace tool
> that used to shove in data < sizeof(struct dcb_app) would now receive
> -ERANGE.
Good.
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> ---
> net/dcb/dcbnl.c | 104 +++++++++++++++++++++++-------------------------
> 1 file changed, 49 insertions(+), 55 deletions(-)
>
> diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
> index a76bdf6f0198..6d19564e19a8 100644
> --- a/net/dcb/dcbnl.c
> +++ b/net/dcb/dcbnl.c
> @@ -1099,6 +1099,45 @@ static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb)
> return err;
> }
>
> +/* Set or delete APP table or rewrite table entries. The APP struct is validated
> + * and the appropriate callback function is called.
> + */
> +static int dcbnl_apprewr_setdel(struct nlattr *attr, struct net_device *netdev,
> + int (*setdel)(struct net_device *dev,
> + struct dcb_app *app),
> + int (*ops_setdel)(struct net_device *dev,
> + struct dcb_app *app))
The name makes it look like it's rewrite-specific. Maybe make it
_app_table_? Given both DCB app and DCB rewrite use app table as the
database... Dunno. Not a big deal.
> +{
> + struct dcb_app *app_data;
> + enum ieee_attrs_app type;
> + struct nlattr *attr_itr;
> + int rem, err;
> +
> + nla_for_each_nested(attr_itr, attr, rem) {
> + type = nla_type(attr_itr);
> +
> + if (!dcbnl_app_attr_type_validate(type))
> + continue;
> +
> + if (nla_len(attr_itr) < sizeof(struct dcb_app))
> + return -ERANGE;
> +
> + app_data = nla_data(attr_itr);
> +
> + if (!dcbnl_app_selector_validate(type, app_data->selector))
> + return -EINVAL;
> +
> + if (ops_setdel)
> + err = ops_setdel(netdev, app_data);
> + else
> + err = setdel(netdev, app_data);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> /* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb GET commands. */
> static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
> {
> @@ -1568,36 +1607,11 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
> }
>
> if (ieee[DCB_ATTR_IEEE_APP_TABLE]) {
> - struct nlattr *attr;
> - int rem;
> -
> - nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
> - enum ieee_attrs_app type = nla_type(attr);
> - struct dcb_app *app_data;
> -
> - if (!dcbnl_app_attr_type_validate(type))
> - continue;
> -
> - if (nla_len(attr) < sizeof(struct dcb_app)) {
> - err = -ERANGE;
> - goto err;
> - }
> -
> - app_data = nla_data(attr);
> -
> - if (!dcbnl_app_selector_validate(type,
> - app_data->selector)) {
> - err = -EINVAL;
> - goto err;
> - }
> -
> - if (ops->ieee_setapp)
> - err = ops->ieee_setapp(netdev, app_data);
> - else
> - err = dcb_ieee_setapp(netdev, app_data);
> - if (err)
> - goto err;
> - }
> + err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE],
> + netdev, dcb_ieee_setapp,
> + ops->ieee_setapp);
This could pre-resolve the callback to use and pass one pointer:
err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE], netdev,
ops->ieee_setapp ?: dcb_ieee_setapp);
And the same below.
> + if (err)
> + goto err;
> }
>
> if (ieee[DCB_ATTR_DCB_APP_TRUST_TABLE]) {
> @@ -1684,31 +1698,11 @@ static int dcbnl_ieee_del(struct net_device *netdev, struct nlmsghdr *nlh,
> return err;
>
> if (ieee[DCB_ATTR_IEEE_APP_TABLE]) {
> - struct nlattr *attr;
> - int rem;
> -
> - nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
> - enum ieee_attrs_app type = nla_type(attr);
> - struct dcb_app *app_data;
> -
> - if (!dcbnl_app_attr_type_validate(type))
> - continue;
> - app_data = nla_data(attr);
> -
> - if (!dcbnl_app_selector_validate(type,
> - app_data->selector)) {
> - err = -EINVAL;
> - goto err;
> - }
> -
> - if (ops->ieee_delapp)
> - err = ops->ieee_delapp(netdev, app_data);
> - else
> - err = dcb_ieee_delapp(netdev, app_data);
> - if (err)
> - goto err;
> - }
> + err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE],
> + netdev, dcb_ieee_delapp,
> + ops->ieee_delapp);
> + if (err)
> + goto err;
> }
>
> err:
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> > ---
> > net/dcb/dcbnl.c | 104 +++++++++++++++++++++++-------------------------
> > 1 file changed, 49 insertions(+), 55 deletions(-)
> >
> > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
> > index a76bdf6f0198..6d19564e19a8 100644
> > --- a/net/dcb/dcbnl.c
> > +++ b/net/dcb/dcbnl.c
> > @@ -1099,6 +1099,45 @@ static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb)
> > return err;
> > }
> >
> > +/* Set or delete APP table or rewrite table entries. The APP struct is validated
> > + * and the appropriate callback function is called.
> > + */
> > +static int dcbnl_apprewr_setdel(struct nlattr *attr, struct net_device *netdev,
> > + int (*setdel)(struct net_device *dev,
> > + struct dcb_app *app),
> > + int (*ops_setdel)(struct net_device *dev,
> > + struct dcb_app *app))
>
> The name makes it look like it's rewrite-specific. Maybe make it
> _app_table_? Given both DCB app and DCB rewrite use app table as the
> database... Dunno. Not a big deal.
Yeah, the other shared funcs are called _app_* too, so lets stick with
that naming. Agreed.
> > /* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb GET commands. */
> > static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
> > {
> > @@ -1568,36 +1607,11 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
> > }
> >
> > if (ieee[DCB_ATTR_IEEE_APP_TABLE]) {
> > - struct nlattr *attr;
> > - int rem;
> > -
> > - nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
> > - enum ieee_attrs_app type = nla_type(attr);
> > - struct dcb_app *app_data;
> > -
> > - if (!dcbnl_app_attr_type_validate(type))
> > - continue;
> > -
> > - if (nla_len(attr) < sizeof(struct dcb_app)) {
> > - err = -ERANGE;
> > - goto err;
> > - }
> > -
> > - app_data = nla_data(attr);
> > -
> > - if (!dcbnl_app_selector_validate(type,
> > - app_data->selector)) {
> > - err = -EINVAL;
> > - goto err;
> > - }
> > -
> > - if (ops->ieee_setapp)
> > - err = ops->ieee_setapp(netdev, app_data);
> > - else
> > - err = dcb_ieee_setapp(netdev, app_data);
> > - if (err)
> > - goto err;
> > - }
> > + err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE],
> > + netdev, dcb_ieee_setapp,
> > + ops->ieee_setapp);
>
> This could pre-resolve the callback to use and pass one pointer:
>
> err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE], netdev,
> ops->ieee_setapp ?: dcb_ieee_setapp);
Agreed.
Will make the suggested changes in v2 - thank you Petr.
/Daniel
@@ -1099,6 +1099,45 @@ static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb)
return err;
}
+/* Set or delete APP table or rewrite table entries. The APP struct is validated
+ * and the appropriate callback function is called.
+ */
+static int dcbnl_apprewr_setdel(struct nlattr *attr, struct net_device *netdev,
+ int (*setdel)(struct net_device *dev,
+ struct dcb_app *app),
+ int (*ops_setdel)(struct net_device *dev,
+ struct dcb_app *app))
+{
+ struct dcb_app *app_data;
+ enum ieee_attrs_app type;
+ struct nlattr *attr_itr;
+ int rem, err;
+
+ nla_for_each_nested(attr_itr, attr, rem) {
+ type = nla_type(attr_itr);
+
+ if (!dcbnl_app_attr_type_validate(type))
+ continue;
+
+ if (nla_len(attr_itr) < sizeof(struct dcb_app))
+ return -ERANGE;
+
+ app_data = nla_data(attr_itr);
+
+ if (!dcbnl_app_selector_validate(type, app_data->selector))
+ return -EINVAL;
+
+ if (ops_setdel)
+ err = ops_setdel(netdev, app_data);
+ else
+ err = setdel(netdev, app_data);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
/* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb GET commands. */
static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
{
@@ -1568,36 +1607,11 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
}
if (ieee[DCB_ATTR_IEEE_APP_TABLE]) {
- struct nlattr *attr;
- int rem;
-
- nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
- enum ieee_attrs_app type = nla_type(attr);
- struct dcb_app *app_data;
-
- if (!dcbnl_app_attr_type_validate(type))
- continue;
-
- if (nla_len(attr) < sizeof(struct dcb_app)) {
- err = -ERANGE;
- goto err;
- }
-
- app_data = nla_data(attr);
-
- if (!dcbnl_app_selector_validate(type,
- app_data->selector)) {
- err = -EINVAL;
- goto err;
- }
-
- if (ops->ieee_setapp)
- err = ops->ieee_setapp(netdev, app_data);
- else
- err = dcb_ieee_setapp(netdev, app_data);
- if (err)
- goto err;
- }
+ err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE],
+ netdev, dcb_ieee_setapp,
+ ops->ieee_setapp);
+ if (err)
+ goto err;
}
if (ieee[DCB_ATTR_DCB_APP_TRUST_TABLE]) {
@@ -1684,31 +1698,11 @@ static int dcbnl_ieee_del(struct net_device *netdev, struct nlmsghdr *nlh,
return err;
if (ieee[DCB_ATTR_IEEE_APP_TABLE]) {
- struct nlattr *attr;
- int rem;
-
- nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
- enum ieee_attrs_app type = nla_type(attr);
- struct dcb_app *app_data;
-
- if (!dcbnl_app_attr_type_validate(type))
- continue;
-
- app_data = nla_data(attr);
-
- if (!dcbnl_app_selector_validate(type,
- app_data->selector)) {
- err = -EINVAL;
- goto err;
- }
-
- if (ops->ieee_delapp)
- err = ops->ieee_delapp(netdev, app_data);
- else
- err = dcb_ieee_delapp(netdev, app_data);
- if (err)
- goto err;
- }
+ err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE],
+ netdev, dcb_ieee_delapp,
+ ops->ieee_delapp);
+ if (err)
+ goto err;
}
err: