[ovs-dev] [PATCH ovn] ovn-nbctl: dump next-hop for router policies

[Mark Michelson]

Acked-by: Mark Michelson <mmichels at redhat.com>

See below for one tiny nit that doesn't affect the ACK-edness of the patch.

On 4/12/21 5:12 PM, Lorenzo Bianconi wrote:
> Commit 35b00c7e79 ('northd: Add ECMP support to router policies.')
> introduced 'nexthops' column in the Logical_Router_Policy table to
> support ECMP for router policies however print_routing_policy() still
> dumps legacy 'nexthop' column as policy next hop.
> Fix the issue dumping 'nexthops' columns.
> 
> Fixes: 35b00c7e79 ('northd: Add ECMP support to router policies.')
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> ---
>   tests/ovn-northd.at   |  9 ++++++++-
>   utilities/ovn-nbctl.c | 14 +++++++++-----
>   2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 96476497d..4a12b7fd6 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2485,6 +2485,13 @@ check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
>   
>   check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102
>   
> +ovn-nbctl lr-policy-list lr0 > policy-list
> +AT_CAPTURE_FILE([policy-list])
> +AT_CHECK([cat policy-list], [0], [dnl
> +Routing Policies
> +        10                                ip4.src == 10.0.0.3         reroute             172.168.0.101, 172.168.0.102
> +])
> +
>   ovn-sbctl dump-flows lr0 > lr0flows3
>   AT_CAPTURE_FILE([lr0flows3])
>   
> @@ -2950,4 +2957,4 @@ wait_row_count FDB 0
>   ovn-sbctl list FDB
>   
>   AT_CLEANUP
> -])
> \ No newline at end of file
> +])

This should probably be undone.

> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 184058356..042c21002 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -3865,11 +3865,15 @@ static void
>   print_routing_policy(const struct nbrec_logical_router_policy *policy,
>                        struct ds *s)
>   {
> -    if (policy->nexthop != NULL) {
> -        char *next_hop = normalize_prefix_str(policy->nexthop);
> -        ds_put_format(s, "%10"PRId64" %50s %15s %25s", policy->priority,
> -                      policy->match, policy->action, next_hop);
> -        free(next_hop);
> +    if (policy->n_nexthops) {
> +        ds_put_format(s, "%10"PRId64" %50s %15s", policy->priority,
> +                      policy->match, policy->action);
> +        for (int i = 0; i < policy->n_nexthops; i++) {
> +            char *next_hop = normalize_prefix_str(policy->nexthops[i]);
> +            char *fmt = i ? ", %s" : " %25s";
> +            ds_put_format(s, fmt, next_hop);
> +            free(next_hop);
> +        }
>       } else {
>           ds_put_format(s, "%10"PRId64" %50s %15s", policy->priority,
>                         policy->match, policy->action);
>