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 <amore...@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.
>
>>>
>>>>
>>>>> +])
>>>>> +AT_CLEANUP
>>>>> +
>>>>> AT_SETUP([ovs-ofctl -F nxm -mmm parse-flows])
>>>>> AT_DATA([flows.txt], [[
>>>>> # comment
>>>>>
>>>>
>>>
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
