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

From: Jiri Benc <jbenc@redhat.com>
To: Simon Horman <simon.horman@netronome.com>
Cc: Jiri Pirko <jiri@mellanox.com>,
	Jamal Hadi Salim <jhs@mojatatu.com>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	netdev@vger.kernel.org, oss-drivers@netronome.com,
	Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Subject: Re: [PATCH/RFC 2/3] net/sched: act_tunnel_key: add extended ack support
Date: Fri, 9 Mar 2018 12:22:48 +0100	[thread overview]
Message-ID: <20180309122248.1979e00f@redhat.com> (raw)
In-Reply-To: <20180306170805.19500-3-simon.horman@netronome.com>

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