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

[Roi Dayan]

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 at mellanox.com>
>>> Reviewed-by: Paul Blakey <paulb at 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
>>>