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