[PATCH net-next v1] flow_offload: improve extack msg for user when adding invalid filter

[PATCH net-next v1] flow_offload: improve extack msg for user when adding invalid filter

[Baowen Zheng]

Add extack message to return exact message to user when adding invalid
filter with conflict flags for TC action.

In previous implement we just return EINVAL which is confusing for user.

Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
---
 net/sched/act_api.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index ca03e72..eb0d7bd 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1446,6 +1446,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 				continue;
 			if (skip_sw != tc_act_skip_sw(act->tcfa_flags) ||
 			    skip_hw != tc_act_skip_hw(act->tcfa_flags)) {
+				NL_SET_ERR_MSG(extack,
+					       "Conflict occurs for TC action and filter flags");
 				err = -EINVAL;
 				goto err;
 			}
-- 
2.5.0

Re: [PATCH net-next v1] flow_offload: improve extack msg for user when adding invalid filter

[Roi Dayan]

On 2022-02-28 12:44 PM, Baowen Zheng wrote:
> Add extack message to return exact message to user when adding invalid
> filter with conflict flags for TC action.
> 
> In previous implement we just return EINVAL which is confusing for user.
> 
> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> ---
>   net/sched/act_api.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index ca03e72..eb0d7bd 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1446,6 +1446,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
>   				continue;
>   			if (skip_sw != tc_act_skip_sw(act->tcfa_flags) ||
>   			    skip_hw != tc_act_skip_hw(act->tcfa_flags)) {
> +				NL_SET_ERR_MSG(extack,
> +					       "Conflict occurs for TC action and filter flags");
>   				err = -EINVAL;
>   				goto err;
>   			}

Reviewed-by: Roi Dayan <roid@nvidia.com>

Re: [PATCH net-next v1] flow_offload: improve extack msg for user when adding invalid filter

[Jakub Kicinski]

On Mon, 28 Feb 2022 18:44:15 +0800 Baowen Zheng wrote:
> Add extack message to return exact message to user when adding invalid
> filter with conflict flags for TC action.
> 
> In previous implement we just return EINVAL which is confusing for user.
> 
> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> ---
>  net/sched/act_api.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index ca03e72..eb0d7bd 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1446,6 +1446,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
>  				continue;
>  			if (skip_sw != tc_act_skip_sw(act->tcfa_flags) ||
>  			    skip_hw != tc_act_skip_hw(act->tcfa_flags)) {
> +				NL_SET_ERR_MSG(extack,
> +					       "Conflict occurs for TC action and filter flags");

Good improvement but I think we can reword a little, how about:

"Mismatch between action and filter offload flags" ?

>  				err = -EINVAL;
>  				goto err;
>  			}

RE: [PATCH net-next v1] flow_offload: improve extack msg for user when adding invalid filter

[Baowen Zheng]

On Wednesday, March 2, 2022 9:56 AM, Jakub wrote:
>On Mon, 28 Feb 2022 18:44:15 +0800 Baowen Zheng wrote:
>> Add extack message to return exact message to user when adding invalid
>> filter with conflict flags for TC action.
>>
>> In previous implement we just return EINVAL which is confusing for user.
>>
>> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
>> ---
>>  net/sched/act_api.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c index
>> ca03e72..eb0d7bd 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -1446,6 +1446,8 @@ int tcf_action_init(struct net *net, struct tcf_proto
>*tp, struct nlattr *nla,
>>  				continue;
>>  			if (skip_sw != tc_act_skip_sw(act->tcfa_flags) ||
>>  			    skip_hw != tc_act_skip_hw(act->tcfa_flags)) {
>> +				NL_SET_ERR_MSG(extack,
>> +					       "Conflict occurs for TC action and
>filter flags");
>
>Good improvement but I think we can reword a little, how about:
>
>"Mismatch between action and filter offload flags" ?
Thanks Jakub for your suggestion, to be honest I did not find a best way to make the msg more understandable. 
I think your description is more clear than mine. I will change as your advice.
>
>>  				err = -EINVAL;
>>  				goto err;
>>  			}