diff mbox series

[PATCH/RFC,2/3] net/sched: act_tunnel_key: add extended ack support

Message ID 20180306170805.19500-3-simon.horman@netronome.com
State RFC, archived
Delegated to: David Miller
Headers show
Series introduce Geneve options support in TC tunnel key | expand

Commit Message

Simon Horman March 6, 2018, 5:08 p.m. UTC 
Add extended ack support for the tunnel key action by using NL_SET_ERR_MSG
during validation of user input.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 net/sched/act_tunnel_key.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Jiri Benc March 9, 2018, 11:22 a.m. UTC | #1 
On Tue,  6 Mar 2018 18:08:04 +0100, Simon Horman wrote:
> -	if (!tb[TCA_TUNNEL_KEY_PARMS])
> +	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
> +		NL_SET_ERR_MSG(extack, "Missing tunnel key parameter");

"parameters" (it's not just one parameter)

> @@ -107,6 +109,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>  		break;
>  	case TCA_TUNNEL_KEY_ACT_SET:
>  		if (!tb[TCA_TUNNEL_KEY_ENC_KEY_ID]) {
> +			NL_SET_ERR_MSG(extack, "Missing tunnel key enc id");

This is not much helpful to the user. What's "enc"? I guess "Missing
tunnel key id" would be enough and better.

> @@ -144,6 +147,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>  						      0, flags,
>  						      key_id, 0);
>  		} else {
> +			NL_SET_ERR_MSG(extack, "Missing both ipv4 and ipv6 enc src and dst");

We don't need both but only one of them. And again, "enc" does not have
a clear meaning.

"Missing either IPv4 or IPv6 source and destination address"?

In addition, it makes little sense to me to add extacks to just some of
the errors in the tunnel_key_init function. Please add extacks to all
of them.

Thanks,

 Jiri
Simon Horman March 22, 2018, 11:49 a.m. UTC | #2 
On Fri, Mar 09, 2018 at 12:22:48PM +0100, Jiri Benc wrote:
> On Tue,  6 Mar 2018 18:08:04 +0100, Simon Horman wrote:
> > -	if (!tb[TCA_TUNNEL_KEY_PARMS])
> > +	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
> > +		NL_SET_ERR_MSG(extack, "Missing tunnel key parameter");
> 
> "parameters" (it's not just one parameter)
> 
> > @@ -107,6 +109,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
> >  		break;
> >  	case TCA_TUNNEL_KEY_ACT_SET:
> >  		if (!tb[TCA_TUNNEL_KEY_ENC_KEY_ID]) {
> > +			NL_SET_ERR_MSG(extack, "Missing tunnel key enc id");
> 
> This is not much helpful to the user. What's "enc"? I guess "Missing
> tunnel key id" would be enough and better.
> 
> > @@ -144,6 +147,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
> >  						      0, flags,
> >  						      key_id, 0);
> >  		} else {
> > +			NL_SET_ERR_MSG(extack, "Missing both ipv4 and ipv6 enc src and dst");
> 
> We don't need both but only one of them. And again, "enc" does not have
> a clear meaning.
> 
> "Missing either IPv4 or IPv6 source and destination address"?

Sure, I'll work on making the messages clearer.

> In addition, it makes little sense to me to add extacks to just some of
> the errors in the tunnel_key_init function. Please add extacks to all
> of them.

At the time I wrote the patch I tried to cover those errors that could
result from user-input. I can extend the coverage if that is preferred.
diff mbox series

Patch

diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 7ce95538177b..c0bbdec50022 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -90,12 +90,14 @@  static int tunnel_key_init(struct net *net, struct nlattr *nla,
 		return -EINVAL;
 
 	err = nla_parse_nested(tb, TCA_TUNNEL_KEY_MAX, nla, tunnel_key_policy,
-			       NULL);
+			       extack);
 	if (err < 0)
 		return err;
 
-	if (!tb[TCA_TUNNEL_KEY_PARMS])
+	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
+		NL_SET_ERR_MSG(extack, "Missing tunnel key parameter");
 		return -EINVAL;
+	}
 
 	parm = nla_data(tb[TCA_TUNNEL_KEY_PARMS]);
 	exists = tcf_idr_check(tn, parm->index, a, bind);
@@ -107,6 +109,7 @@  static int tunnel_key_init(struct net *net, struct nlattr *nla,
 		break;
 	case TCA_TUNNEL_KEY_ACT_SET:
 		if (!tb[TCA_TUNNEL_KEY_ENC_KEY_ID]) {
+			NL_SET_ERR_MSG(extack, "Missing tunnel key enc id");
 			ret = -EINVAL;
 			goto err_out;
 		}
@@ -144,6 +147,7 @@  static int tunnel_key_init(struct net *net, struct nlattr *nla,
 						      0, flags,
 						      key_id, 0);
 		} else {
+			NL_SET_ERR_MSG(extack, "Missing both ipv4 and ipv6 enc src and dst");
 			ret = -EINVAL;
 			goto err_out;
 		}