[ovs-dev] [PATCH v2] ofp_actions: fix set_mpls_tc formatting

Ilya Maximets i.maximets at ovn.org
Fri May 14 14:34:24 UTC 2021


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 <amorenoz at 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.

>      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.

> +])
> +AT_CLEANUP
> +
>  AT_SETUP([ovs-ofctl -F nxm -mmm parse-flows])
>  AT_DATA([flows.txt], [[
>  # comment
> 



More information about the dev mailing list