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