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

[Lorenzo Bianconi]

> Acked-by: Mark Michelson <mmichels at redhat.com>
> 
> See below for one tiny nit that doesn't affect the ACK-edness of the patch.

ack, I will fix it in v2.

Regards,
Lorenzo

> 
> 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);
> > 
>