diff mbox series

[ovs-dev,V1,1/2] lib/tc: Handle error parsing action in nl_parse_single_action

Message ID 1520859527-9134-2-git-send-email-roid@mellanox.com
State Accepted
Headers show
Series TC offload dump fix and add frag | expand

Commit Message

Roi Dayan March 12, 2018, 12:58 p.m. UTC 
Raise the error up instead of ignoring it.
Before this commit beside an error an incorrect rule was also printed.

Signed-off-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Paul Blakey <paulb@mellanox.com>
---
 lib/tc.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

Simon Horman March 21, 2018, 10:55 a.m. UTC | #1 
On Mon, Mar 12, 2018 at 02:58:46PM +0200, Roi Dayan wrote:
> Raise the error up instead of ignoring it.
> Before this commit beside an error an incorrect rule was also printed.
> 
> Signed-off-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Paul Blakey <paulb@mellanox.com>

Thanks, applied to master and branch-2.9.

It looks like a backport to branch-2.8 is also appropriate,
could you prepare that?

It also looks like it would be good to exercise this in the test-suite,
is there any plan to do that?

> ---
>  lib/tc.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/tc.c b/lib/tc.c
> index 914465a..b49bbe8 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -809,6 +809,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower)
>      struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)];
>      struct ovs_flow_stats *stats = &flower->stats;
>      const struct gnet_stats_basic *bs;
> +    int err = 0;
>  
>      if (!nl_parse_nested(action, act_policy, action_attrs,
>                           ARRAY_SIZE(act_policy))) {
> @@ -821,20 +822,24 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower)
>      act_cookie = action_attrs[TCA_ACT_COOKIE];
>  
>      if (!strcmp(act_kind, "gact")) {
> -        nl_parse_act_drop(act_options, flower);
> +        err = nl_parse_act_drop(act_options, flower);
>      } else if (!strcmp(act_kind, "mirred")) {
> -        nl_parse_act_mirred(act_options, flower);
> +        err = nl_parse_act_mirred(act_options, flower);
>      } else if (!strcmp(act_kind, "vlan")) {
> -        nl_parse_act_vlan(act_options, flower);
> +        err = nl_parse_act_vlan(act_options, flower);
>      } else if (!strcmp(act_kind, "tunnel_key")) {
> -        nl_parse_act_tunnel_key(act_options, flower);
> +        err = nl_parse_act_tunnel_key(act_options, flower);
>      } else if (!strcmp(act_kind, "pedit")) {
> -        nl_parse_act_pedit(act_options, flower);
> +        err = nl_parse_act_pedit(act_options, flower);
>      } else if (!strcmp(act_kind, "csum")) {
>          nl_parse_act_csum(act_options, flower);
>      } else {
>          VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind);
> -        return EINVAL;
> +        err = EINVAL;
> +    }
> +
> +    if (err) {
> +        return err;
>      }
>  
>      if (act_cookie) {
> -- 
> 2.7.0
>
Roi Dayan March 22, 2018, 10:39 a.m. UTC | #2 
On 21/03/2018 12:55, Simon Horman wrote:
> On Mon, Mar 12, 2018 at 02:58:46PM +0200, Roi Dayan wrote:
>> Raise the error up instead of ignoring it.
>> Before this commit beside an error an incorrect rule was also printed.
>>
>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>> Reviewed-by: Paul Blakey <paulb@mellanox.com>
> 
> Thanks, applied to master and branch-2.9.
> 
> It looks like a backport to branch-2.8 is also appropriate,
> could you prepare that?

sure.

> 
> It also looks like it would be good to exercise this in the test-suite,
> is there any plan to do that?

didn't think of it. i'll check it.

> 
>> ---
>>   lib/tc.c | 17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/tc.c b/lib/tc.c
>> index 914465a..b49bbe8 100644
>> --- a/lib/tc.c
>> +++ b/lib/tc.c
>> @@ -809,6 +809,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower)
>>       struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)];
>>       struct ovs_flow_stats *stats = &flower->stats;
>>       const struct gnet_stats_basic *bs;
>> +    int err = 0;
>>   
>>       if (!nl_parse_nested(action, act_policy, action_attrs,
>>                            ARRAY_SIZE(act_policy))) {
>> @@ -821,20 +822,24 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower)
>>       act_cookie = action_attrs[TCA_ACT_COOKIE];
>>   
>>       if (!strcmp(act_kind, "gact")) {
>> -        nl_parse_act_drop(act_options, flower);
>> +        err = nl_parse_act_drop(act_options, flower);
>>       } else if (!strcmp(act_kind, "mirred")) {
>> -        nl_parse_act_mirred(act_options, flower);
>> +        err = nl_parse_act_mirred(act_options, flower);
>>       } else if (!strcmp(act_kind, "vlan")) {
>> -        nl_parse_act_vlan(act_options, flower);
>> +        err = nl_parse_act_vlan(act_options, flower);
>>       } else if (!strcmp(act_kind, "tunnel_key")) {
>> -        nl_parse_act_tunnel_key(act_options, flower);
>> +        err = nl_parse_act_tunnel_key(act_options, flower);
>>       } else if (!strcmp(act_kind, "pedit")) {
>> -        nl_parse_act_pedit(act_options, flower);
>> +        err = nl_parse_act_pedit(act_options, flower);
>>       } else if (!strcmp(act_kind, "csum")) {
>>           nl_parse_act_csum(act_options, flower);
>>       } else {
>>           VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind);
>> -        return EINVAL;
>> +        err = EINVAL;
>> +    }
>> +
>> +    if (err) {
>> +        return err;
>>       }
>>   
>>       if (act_cookie) {
>> -- 
>> 2.7.0
>>
Roi Dayan March 25, 2018, 9:19 a.m. UTC | #3 
On 22/03/2018 12:39, Roi Dayan wrote:
> 
> 
> On 21/03/2018 12:55, Simon Horman wrote:
>> On Mon, Mar 12, 2018 at 02:58:46PM +0200, Roi Dayan wrote:
>>> Raise the error up instead of ignoring it.
>>> Before this commit beside an error an incorrect rule was also printed.
>>>
>>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>>> Reviewed-by: Paul Blakey <paulb@mellanox.com>
>>
>> Thanks, applied to master and branch-2.9.
>>
>> It looks like a backport to branch-2.8 is also appropriate,
>> could you prepare that?
> 
> sure.
> 
>>
>> It also looks like it would be good to exercise this in the test-suite,
>> is there any plan to do that?
> 
> didn't think of it. i'll check it.
> 

Hi Simon,

I was thinking about this and the current test will catch this.
Before the commit if there was an error parsing the action then it was
shown as drop. the current test will not find the expected forward
action and will fail.

Thanks,
Roi


>>
>>> ---
>>>   lib/tc.c | 17 +++++++++++------
>>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/tc.c b/lib/tc.c
>>> index 914465a..b49bbe8 100644
>>> --- a/lib/tc.c
>>> +++ b/lib/tc.c
>>> @@ -809,6 +809,7 @@ nl_parse_single_action(struct nlattr *action, 
>>> struct tc_flower *flower)
>>>       struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)];
>>>       struct ovs_flow_stats *stats = &flower->stats;
>>>       const struct gnet_stats_basic *bs;
>>> +    int err = 0;
>>>       if (!nl_parse_nested(action, act_policy, action_attrs,
>>>                            ARRAY_SIZE(act_policy))) {
>>> @@ -821,20 +822,24 @@ nl_parse_single_action(struct nlattr *action, 
>>> struct tc_flower *flower)
>>>       act_cookie = action_attrs[TCA_ACT_COOKIE];
>>>       if (!strcmp(act_kind, "gact")) {
>>> -        nl_parse_act_drop(act_options, flower);
>>> +        err = nl_parse_act_drop(act_options, flower);
>>>       } else if (!strcmp(act_kind, "mirred")) {
>>> -        nl_parse_act_mirred(act_options, flower);
>>> +        err = nl_parse_act_mirred(act_options, flower);
>>>       } else if (!strcmp(act_kind, "vlan")) {
>>> -        nl_parse_act_vlan(act_options, flower);
>>> +        err = nl_parse_act_vlan(act_options, flower);
>>>       } else if (!strcmp(act_kind, "tunnel_key")) {
>>> -        nl_parse_act_tunnel_key(act_options, flower);
>>> +        err = nl_parse_act_tunnel_key(act_options, flower);
>>>       } else if (!strcmp(act_kind, "pedit")) {
>>> -        nl_parse_act_pedit(act_options, flower);
>>> +        err = nl_parse_act_pedit(act_options, flower);
>>>       } else if (!strcmp(act_kind, "csum")) {
>>>           nl_parse_act_csum(act_options, flower);
>>>       } else {
>>>           VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", 
>>> act_kind);
>>> -        return EINVAL;
>>> +        err = EINVAL;
>>> +    }
>>> +
>>> +    if (err) {
>>> +        return err;
>>>       }
>>>       if (act_cookie) {
>>> -- 
>>> 2.7.0
>>>
diff mbox series

Patch

diff --git a/lib/tc.c b/lib/tc.c
index 914465a..b49bbe8 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -809,6 +809,7 @@  nl_parse_single_action(struct nlattr *action, struct tc_flower *flower)
     struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)];
     struct ovs_flow_stats *stats = &flower->stats;
     const struct gnet_stats_basic *bs;
+    int err = 0;
 
     if (!nl_parse_nested(action, act_policy, action_attrs,
                          ARRAY_SIZE(act_policy))) {
@@ -821,20 +822,24 @@  nl_parse_single_action(struct nlattr *action, struct tc_flower *flower)
     act_cookie = action_attrs[TCA_ACT_COOKIE];
 
     if (!strcmp(act_kind, "gact")) {
-        nl_parse_act_drop(act_options, flower);
+        err = nl_parse_act_drop(act_options, flower);
     } else if (!strcmp(act_kind, "mirred")) {
-        nl_parse_act_mirred(act_options, flower);
+        err = nl_parse_act_mirred(act_options, flower);
     } else if (!strcmp(act_kind, "vlan")) {
-        nl_parse_act_vlan(act_options, flower);
+        err = nl_parse_act_vlan(act_options, flower);
     } else if (!strcmp(act_kind, "tunnel_key")) {
-        nl_parse_act_tunnel_key(act_options, flower);
+        err = nl_parse_act_tunnel_key(act_options, flower);
     } else if (!strcmp(act_kind, "pedit")) {
-        nl_parse_act_pedit(act_options, flower);
+        err = nl_parse_act_pedit(act_options, flower);
     } else if (!strcmp(act_kind, "csum")) {
         nl_parse_act_csum(act_options, flower);
     } else {
         VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind);
-        return EINVAL;
+        err = EINVAL;
+    }
+
+    if (err) {
+        return err;
     }
 
     if (act_cookie) {