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) >> >>> >>>> +]) >>>> +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
