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 <[email protected]> >>>>>> --- >>>>>> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
