[ovs-dev] [PATCH v2] ofp_actions: fix set_mpls_tc formatting

[Adrian Moreno]

On 5/17/21 1:42 PM, Ilya Maximets wrote:
> On 5/17/21 1:28 PM, Adrian Moreno wrote:
>>
>>
>> On 5/14/21 6:50 PM, Ilya Maximets wrote:
>>> On 5/14/21 5:14 PM, Adrian Moreno wrote:
>>>>
>>>>
>>>> On 5/14/21 4:34 PM, Ilya Maximets wrote:
>>>>> On 5/7/21 3:02 PM, Adrian Moreno wrote:
>>>>>> Apart from a cut-and-paste typo, the man page claims that mpls_labels
>>>>>> can be provided in hexadecimal format but that's currently not the case.
>>>>>>
>>>>>> Fix ofctl formatting as well as a few missing spaces in the manpage
>>>>>>
>>>>>> Signed-off-by: Adrian Moreno <amorenoz at redhat.com>
>>>>>> ---
>>>>>>  lib/ofp-actions.c  | 10 ++++++++--
>>>>>>  tests/ovs-ofctl.at | 22 ++++++++++++++++++++++
>>>>>>  2 files changed, 30 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
>>>>>> index 0342a228b..610ccca04 100644
>>>>>> --- a/lib/ofp-actions.c
>>>>>> +++ b/lib/ofp-actions.c
>>>>>> @@ -3775,13 +3775,19 @@ encode_SET_MPLS_LABEL(const struct ofpact_mpls_label *label,
>>>>>>  static char * OVS_WARN_UNUSED_RESULT
>>>>>>  parse_SET_MPLS_LABEL(char *arg, const struct ofpact_parse_params *pp)
>>>>>>  {
>>>>>> +    char* error;
>>>>>> +    uint32_t label;
>>>>>
>>>>> It's better to reverse order of these and move below the definition
>>>>> of 'mpls_label' or right to the point of usage.
>>>>
>>>> Sure
>>>>
>>>>>
>>>>>>      struct ofpact_mpls_label *mpls_label
>>>>>>          = ofpact_put_SET_MPLS_LABEL(pp->ofpacts);
>>>>>>      if (*arg == '\0') {
>>>>>>          return xstrdup("set_mpls_label: expected label.");
>>>>>>      }
>>>>>>  
>>>>>> -    mpls_label->label = htonl(atoi(arg));
>>>>>> +    error = str_to_u32(arg, &label);
>>>>>> +    if (error) {
>>>>>> +        return error;
>>>>>> +    }
>>>>>> +    mpls_label->label = htonl(label);
>>>>>>      return NULL;
>>>>>>  }
>>>>>>  
>>>>>> @@ -3850,7 +3856,7 @@ static void
>>>>>>  format_SET_MPLS_TC(const struct ofpact_mpls_tc *a,
>>>>>>                     const struct ofpact_format_params *fp)
>>>>>>  {
>>>>>> -    ds_put_format(fp->s, "%sset_mpls_ttl(%s%"PRIu8"%s)%s",
>>>>>> +    ds_put_format(fp->s, "%sset_mpls_tc(%s%"PRIu8"%s)%s",
>>>>>>                    colors.paren, colors.end, a->tc,
>>>>>>                    colors.paren, colors.end);
>>>>>>  }
>>>>>> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
>>>>>> index 5ddca67e7..010eac8f1 100644
>>>>>> --- a/tests/ovs-ofctl.at
>>>>>> +++ b/tests/ovs-ofctl.at
>>>>>> @@ -588,6 +588,28 @@ NXT_FLOW_MOD: ADD ip actions=ct(commit,exec(load:0x1->NXM_NX_CT_LABEL[[0..63]],l
>>>>>>  ])
>>>>>>  AT_CLEANUP
>>>>>>  
>>>>>> +AT_SETUP([ovs-ofctl -F nxm parse-flows mpls actions])
>>>>>> +AT_DATA([flows.txt], [
>>>>>> +# comment
>>>>>> +actions=set_mpls_label(10)
>>>>>> +actions=set_mpls_label(0x10)
>>>>>> +actions=set_mpls_tc(4)
>>>>>> +actions=set_mpls_ttl(200)
>>>>>> +])
>>>>>> +AT_CHECK([ovs-ofctl -F nxm parse-flows flows.txt], [0], [stdout])
>>>>>> +# The substitution for fec0:0: is because some libcs (e.g. MUSL)
>>>>>> +# abbreviate a single zero and others (e.g. glibc) don't.
>>>>>> +AT_CHECK([[sed 's/ (xid=0x[0-9a-fA-F]*)//
>>>>>> +s/fec0:0:/fec0::/g' stdout]], [0], [dnl
>>>>>> +usable protocols: OpenFlow10,NXM
>>>>>> +chosen protocol: NXM-table_id
>>>>>> +NXT_FLOW_MOD: ADD actions=set_mpls_label(10)
>>>>>> +NXT_FLOW_MOD: ADD actions=set_mpls_label(16)
>>>>>> +NXT_FLOW_MOD: ADD actions=set_mpls_tc(4)
>>>>>> +NXT_FLOW_MOD: ADD actions=set_mpls_ttl(200)
>>>>>
>>>>> It's probably better to add these checks to existing tests, e.g.
>>>>> 'ovs-ofctl parse-flows (NXM)'.  It might also make sense to use
>>>>> larger values to test the type size.
>>>>
>>>> I did not find other test that only has:
>>>>> "usable protocols: OpenFlow10,NXM"
>>>> so I wanted to avoid changing the usable protocols of existing tests.
>>>
>>> 'OXM,NXM' is wider than 'OpenFlow10,NXM' so tests with usable protocols
>>> 'OXM,NXM'+ will not be changed by adding these new test cases.
>>>
>>
>> I did try to add this tests to testcase:
>> ovs-ofctl.at:399: testing ovs-ofctl parse-flows (NXM)
>>
>> and got:
>>
>> ./ovs-ofctl.at:460: sed 's/ (xid=0x[0-9a-fA-F]*)//' stdout
>> --- -	2021-05-17 13:15:08.193868197 +0200
>> +++ /home/amorenoz/code/ovs/tests/testsuite.dir/at-groups/424/stdout	2021-05-17
>> 13:15:08.191478252 +0200
>> @@ -1,4 +1,4 @@
>> -usable protocols: OXM,NXM+table_id
>> +usable protocols: NXM+table_id
>>  chosen protocol: NXM+table_id
>>  NXT_FLOW_MOD: ADD table:255 tcp,tp_src=123 actions=FLOOD
>>  NXT_FLOW_MOD: ADD table:255 in_port=LOCAL,dl_vlan=9,dl_src=00:0a:e4:25:6b:b0
>> actions=drop
>> 424. ovs-ofctl.at:399: 424. ovs-ofctl parse-flows (NXM) (ovs-ofctl.at:399):
>> FAILED (ovs-ofctl.at:460)
>>
> 
> Just add a match on mpls to these test cases.
> Having no match forces OVS to use OF10 + OVS extensions.
> Consistency rules for OF11+ requires having an mpls header, i.e. match on
> an mpls or previous action that adds an mpls header.  With mpls match,
> higher versions on OF could be used therefore OXM.
> 

Thanks for the pointer Ilya.


>>
>>>>
>>>>>
>>>>>> +])
>>>>>> +AT_CLEANUP
>>>>>> +
>>>>>>  AT_SETUP([ovs-ofctl -F nxm -mmm parse-flows])
>>>>>>  AT_DATA([flows.txt], [[
>>>>>>  # comment
>>>>>>
>>>>>
>>>>
>>>
>>
> 

-- 
Adrián Moreno