| Message ID | 20221202173147.3032702-6-odivlad@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | OVN IC bugfixes & proposals/questions | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On 12/2/22 18:31, Vladislav Odintsov wrote: > 1. Remove excess nbrec_logical_router variable. > 2. Remove excess call to add_static_to_routes_ad(). > 3. Remove double route_table check in ic_route_fin(). Nit: s/route_table/nexthop/ > 4. Move variable declarations out of loop. > > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > --- > ic/ovn-ic.c | 31 ++++++++++--------------------- > 1 file changed, 10 insertions(+), 21 deletions(-) > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index 3e02b4c98..59468545d 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -895,8 +895,7 @@ ic_route_find(struct hmap *routes, const struct in6_addr *prefix, > r->plen == plen && > ipv6_addr_equals(&r->nexthop, nexthop) && > !strcmp(r->origin, origin) && > - !strcmp(r->route_table ? r->route_table : "", route_table) && > - ipv6_addr_equals(&r->nexthop, nexthop)) { > + !strcmp(r->route_table ? r->route_table : "", route_table)) { > return r; > } > } > @@ -1109,17 +1108,8 @@ add_static_to_routes_ad( > struct hmap *routes_ad, > const struct nbrec_logical_router_static_route *nb_route, > const struct lport_addresses *nexthop_addresses, > - const struct smap *nb_options, const char *route_table) > + const struct smap *nb_options) > { > - if (strcmp(route_table, nb_route->route_table)) { > - if (VLOG_IS_DBG_ENABLED()) { > - VLOG_DBG("Skip advertising route %s -> %s as its route table %s !=" > - " %s of TS port", nb_route->ip_prefix, nb_route->nexthop, > - nb_route->route_table, route_table); > - } > - return; > - } > - > struct in6_addr prefix, nexthop; > unsigned int plen; > if (!parse_route(nb_route->ip_prefix, nb_route->nexthop, > @@ -1541,13 +1531,13 @@ build_ts_routes_to_adv(struct ic_context *ctx, > { > const struct nbrec_logical_router *lr = ic_lr->lr; > > + const struct nbrec_logical_router_static_route *nb_route; I'm not sure I agree with this one. Why not keep it inside the for loop below. We don't use 'nb_route' afterwards AFAICT. > + struct uuid id; > + > /* Check static routes of the LR */ > for (int i = 0; i < lr->n_static_routes; i++) { > - const struct nbrec_logical_router_static_route *nb_route > - = lr->static_routes[i]; > - struct uuid isb_uuid; > - if (smap_get_uuid(&nb_route->external_ids, "ic-learned-route", > - &isb_uuid)) { > + nb_route = lr->static_routes[i]; > + if (smap_get_uuid(&nb_route->external_ids, "ic-learned-route", &id)) { > /* It is a learned route */ > if (!add_to_routes_learned(&ic_lr->routes_learned, nb_route)) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > @@ -1557,10 +1547,10 @@ build_ts_routes_to_adv(struct ic_context *ctx, > nbrec_logical_router_update_static_routes_delvalue(lr, > nb_route); > } > - } else { > + } else if (!strcmp(ts_route_table, nb_route->route_table)) { > /* It may be a route to be advertised */ > add_static_to_routes_ad(routes_ad, nb_route, ts_port_addrs, > - &nb_global->options, ts_route_table); > + &nb_global->options); > } > } > > @@ -1593,7 +1583,6 @@ advertise_lr_routes(struct ic_context *ctx, > const struct icsbrec_port_binding *isb_pb; > const char *lrp_name, *ts_name, *route_table; > struct lport_addresses ts_port_addrs; > - const struct nbrec_logical_router *lr = ic_lr->lr; > const struct icnbrec_transit_switch *key; > > struct hmap routes_ad = HMAP_INITIALIZER(&routes_ad); > @@ -1611,7 +1600,7 @@ advertise_lr_routes(struct ic_context *ctx, > VLOG_INFO_RL(&rl, "Route sync ignores port %s on ts %s for router" > " %s because the addresses are invalid.", > isb_pb->logical_port, isb_pb->transit_switch, > - lr->name); > + ic_lr->lr->name); > continue; > } > lrp_name = get_lrp_name_by_ts_port_name(ctx, isb_pb->logical_port);
Regards, Vladislav Odintsov > On 5 Dec 2022, at 19:37, Dumitru Ceara <dceara@redhat.com> wrote: > > On 12/2/22 18:31, Vladislav Odintsov wrote: >> 1. Remove excess nbrec_logical_router variable. >> 2. Remove excess call to add_static_to_routes_ad(). >> 3. Remove double route_table check in ic_route_fin(). > > Nit: s/route_table/nexthop/ > >> 4. Move variable declarations out of loop. >> >> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >> --- >> ic/ovn-ic.c | 31 ++++++++++--------------------- >> 1 file changed, 10 insertions(+), 21 deletions(-) >> >> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c >> index 3e02b4c98..59468545d 100644 >> --- a/ic/ovn-ic.c >> +++ b/ic/ovn-ic.c >> @@ -895,8 +895,7 @@ ic_route_find(struct hmap *routes, const struct in6_addr *prefix, >> r->plen == plen && >> ipv6_addr_equals(&r->nexthop, nexthop) && >> !strcmp(r->origin, origin) && >> - !strcmp(r->route_table ? r->route_table : "", route_table) && >> - ipv6_addr_equals(&r->nexthop, nexthop)) { >> + !strcmp(r->route_table ? r->route_table : "", route_table)) { >> return r; >> } >> } >> @@ -1109,17 +1108,8 @@ add_static_to_routes_ad( >> struct hmap *routes_ad, >> const struct nbrec_logical_router_static_route *nb_route, >> const struct lport_addresses *nexthop_addresses, >> - const struct smap *nb_options, const char *route_table) >> + const struct smap *nb_options) >> { >> - if (strcmp(route_table, nb_route->route_table)) { >> - if (VLOG_IS_DBG_ENABLED()) { >> - VLOG_DBG("Skip advertising route %s -> %s as its route table %s !=" >> - " %s of TS port", nb_route->ip_prefix, nb_route->nexthop, >> - nb_route->route_table, route_table); >> - } >> - return; >> - } >> - >> struct in6_addr prefix, nexthop; >> unsigned int plen; >> if (!parse_route(nb_route->ip_prefix, nb_route->nexthop, >> @@ -1541,13 +1531,13 @@ build_ts_routes_to_adv(struct ic_context *ctx, >> { >> const struct nbrec_logical_router *lr = ic_lr->lr; >> >> + const struct nbrec_logical_router_static_route *nb_route; > > I'm not sure I agree with this one. Why not keep it inside the for loop > below. We don't use 'nb_route' afterwards AFAICT. Agree. I’ll revert this change in v2. Also I’ll revert uuid variable movement. > >> + struct uuid id; >> + >> /* Check static routes of the LR */ >> for (int i = 0; i < lr->n_static_routes; i++) { >> - const struct nbrec_logical_router_static_route *nb_route >> - = lr->static_routes[i]; >> - struct uuid isb_uuid; >> - if (smap_get_uuid(&nb_route->external_ids, "ic-learned-route", >> - &isb_uuid)) { >> + nb_route = lr->static_routes[i]; >> + if (smap_get_uuid(&nb_route->external_ids, "ic-learned-route", &id)) { >> /* It is a learned route */ >> if (!add_to_routes_learned(&ic_lr->routes_learned, nb_route)) { >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >> @@ -1557,10 +1547,10 @@ build_ts_routes_to_adv(struct ic_context *ctx, >> nbrec_logical_router_update_static_routes_delvalue(lr, >> nb_route); >> } >> - } else { >> + } else if (!strcmp(ts_route_table, nb_route->route_table)) { >> /* It may be a route to be advertised */ >> add_static_to_routes_ad(routes_ad, nb_route, ts_port_addrs, >> - &nb_global->options, ts_route_table); >> + &nb_global->options); >> } >> } >> >> @@ -1593,7 +1583,6 @@ advertise_lr_routes(struct ic_context *ctx, >> const struct icsbrec_port_binding *isb_pb; >> const char *lrp_name, *ts_name, *route_table; >> struct lport_addresses ts_port_addrs; >> - const struct nbrec_logical_router *lr = ic_lr->lr; >> const struct icnbrec_transit_switch *key; >> >> struct hmap routes_ad = HMAP_INITIALIZER(&routes_ad); >> @@ -1611,7 +1600,7 @@ advertise_lr_routes(struct ic_context *ctx, >> VLOG_INFO_RL(&rl, "Route sync ignores port %s on ts %s for router" >> " %s because the addresses are invalid.", >> isb_pb->logical_port, isb_pb->transit_switch, >> - lr->name); >> + ic_lr->lr->name); >> continue; >> } >> lrp_name = get_lrp_name_by_ts_port_name(ctx, isb_pb->logical_port); >
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c index 3e02b4c98..59468545d 100644 --- a/ic/ovn-ic.c +++ b/ic/ovn-ic.c @@ -895,8 +895,7 @@ ic_route_find(struct hmap *routes, const struct in6_addr *prefix, r->plen == plen && ipv6_addr_equals(&r->nexthop, nexthop) && !strcmp(r->origin, origin) && - !strcmp(r->route_table ? r->route_table : "", route_table) && - ipv6_addr_equals(&r->nexthop, nexthop)) { + !strcmp(r->route_table ? r->route_table : "", route_table)) { return r; } } @@ -1109,17 +1108,8 @@ add_static_to_routes_ad( struct hmap *routes_ad, const struct nbrec_logical_router_static_route *nb_route, const struct lport_addresses *nexthop_addresses, - const struct smap *nb_options, const char *route_table) + const struct smap *nb_options) { - if (strcmp(route_table, nb_route->route_table)) { - if (VLOG_IS_DBG_ENABLED()) { - VLOG_DBG("Skip advertising route %s -> %s as its route table %s !=" - " %s of TS port", nb_route->ip_prefix, nb_route->nexthop, - nb_route->route_table, route_table); - } - return; - } - struct in6_addr prefix, nexthop; unsigned int plen; if (!parse_route(nb_route->ip_prefix, nb_route->nexthop, @@ -1541,13 +1531,13 @@ build_ts_routes_to_adv(struct ic_context *ctx, { const struct nbrec_logical_router *lr = ic_lr->lr; + const struct nbrec_logical_router_static_route *nb_route; + struct uuid id; + /* Check static routes of the LR */ for (int i = 0; i < lr->n_static_routes; i++) { - const struct nbrec_logical_router_static_route *nb_route - = lr->static_routes[i]; - struct uuid isb_uuid; - if (smap_get_uuid(&nb_route->external_ids, "ic-learned-route", - &isb_uuid)) { + nb_route = lr->static_routes[i]; + if (smap_get_uuid(&nb_route->external_ids, "ic-learned-route", &id)) { /* It is a learned route */ if (!add_to_routes_learned(&ic_lr->routes_learned, nb_route)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); @@ -1557,10 +1547,10 @@ build_ts_routes_to_adv(struct ic_context *ctx, nbrec_logical_router_update_static_routes_delvalue(lr, nb_route); } - } else { + } else if (!strcmp(ts_route_table, nb_route->route_table)) { /* It may be a route to be advertised */ add_static_to_routes_ad(routes_ad, nb_route, ts_port_addrs, - &nb_global->options, ts_route_table); + &nb_global->options); } } @@ -1593,7 +1583,6 @@ advertise_lr_routes(struct ic_context *ctx, const struct icsbrec_port_binding *isb_pb; const char *lrp_name, *ts_name, *route_table; struct lport_addresses ts_port_addrs; - const struct nbrec_logical_router *lr = ic_lr->lr; const struct icnbrec_transit_switch *key; struct hmap routes_ad = HMAP_INITIALIZER(&routes_ad); @@ -1611,7 +1600,7 @@ advertise_lr_routes(struct ic_context *ctx, VLOG_INFO_RL(&rl, "Route sync ignores port %s on ts %s for router" " %s because the addresses are invalid.", isb_pb->logical_port, isb_pb->transit_switch, - lr->name); + ic_lr->lr->name); continue; } lrp_name = get_lrp_name_by_ts_port_name(ctx, isb_pb->logical_port);
1. Remove excess nbrec_logical_router variable. 2. Remove excess call to add_static_to_routes_ad(). 3. Remove double route_table check in ic_route_fin(). 4. Move variable declarations out of loop. Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- ic/ovn-ic.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-)