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(-)

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))
+{
+	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:
-- 
2.34.1

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