On 9/29/25 8:26 AM, Ales Musil via dev wrote: > On Thu, Sep 25, 2025 at 11:51 PM Jacob Tanenbaum <[email protected]> > wrote: > >> I have one small possible nit, ignore it if it misuses a function >> > > Hi Jacob, > > thank you for the review. > > >> >> On Thu, Sep 25, 2025 at 3:05 PM Ales Musil via dev < >> [email protected]> wrote: >> >>> That function is misleading as the type isn't guaranteed to be >>> always LRP. The only place where this function was used knew about >>> this hidden logic and did a lookup the lookup twice anyway. Let's >>> use the type from datapath instead, this will give us guaranteed >>> answer. >>> >>> Signed-off-by: Ales Musil <[email protected]> >>> --- >>> v2: Prevent the double lookup for struct ovn_port. >>> --- >>> lib/ovn-util.c | 28 ---------------------------- >>> lib/ovn-util.h | 1 - >>> northd/northd.c | 29 ++++++----------------------- >>> 3 files changed, 6 insertions(+), 52 deletions(-) >>> >>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c >>> index 6c7629e6d..7b77f838f 100644 >>> --- a/lib/ovn-util.c >>> +++ b/lib/ovn-util.c >>> @@ -1283,34 +1283,6 @@ get_lport_type_str(enum en_lport_type lport_type) >>> OVS_NOT_REACHED(); >>> } >>> >>> -bool >>> -is_pb_router_type(const struct sbrec_port_binding *pb) >>> -{ >>> - enum en_lport_type lport_type = get_lport_type(pb); >>> - >>> - switch (lport_type) { >>> - case LP_PATCH: >>> - case LP_CHASSISREDIRECT: >>> - case LP_L3GATEWAY: >>> - case LP_L2GATEWAY: >>> - case LP_REMOTE: >>> - return true; >>> - >>> - case LP_VIF: >>> - case LP_CONTAINER: >>> - case LP_MIRROR: >>> - case LP_VIRTUAL: >>> - case LP_LOCALNET: >>> - case LP_LOCALPORT: >>> - case LP_VTEP: >>> - case LP_EXTERNAL: >>> - case LP_UNKNOWN: >>> - return false; >>> - } >>> - >>> - return false; >>> -} >>> - >>> void >>> sorted_array_apply_diff(const struct sorted_array *a1, >>> const struct sorted_array *a2, >>> diff --git a/lib/ovn-util.h b/lib/ovn-util.h >>> index 67fc3882d..4c686aac9 100644 >>> --- a/lib/ovn-util.h >>> +++ b/lib/ovn-util.h >>> @@ -433,7 +433,6 @@ enum en_lport_type { >>> >>> enum en_lport_type get_lport_type(const struct sbrec_port_binding *); >>> char *get_lport_type_str(enum en_lport_type lport_type); >>> -bool is_pb_router_type(const struct sbrec_port_binding *); >>> >>> /* A wrapper that holds sorted arrays of strings. */ >>> struct sorted_array { >>> diff --git a/northd/northd.c b/northd/northd.c >>> index fe5199a86..ea47e3edc 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -4982,27 +4982,10 @@ northd_handle_sb_port_binding_changes( >>> const struct sbrec_port_binding *pb; >>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >>> SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, >>> sbrec_port_binding_table) { >>> - bool is_router_port = is_pb_router_type(pb); >>> - struct ovn_port *op = NULL; >>> - >>> - if (is_router_port) { >>> - /* A router port binding 'pb' can belong to >>> - * - a logical switch port of type router or >>> - * - a logical router port. >>> - * >>> - * So, first search in lr_ports hmap. If not found, search >>> in >>> - * ls_ports hmap. >>> - * */ >>> - op = ovn_port_find(lr_ports, pb->logical_port); >>> - } >>> - >>> - if (!op) { >>> - op = ovn_port_find(ls_ports, pb->logical_port); >>> - >>> - if (op) { >>> - is_router_port = false; >>> - } >>> - } >>> + bool is_lrp = >>> + !strcmp(datapath_get_nb_type(pb->datapath), >>> "logical-router"); >>> >> >> instead of using the string "logical-router" could you use >> `ovn_datapath_type_to_string(DP_ROUTER)`? >> > > It might be a bit excessive, but won't hurt anything. I'll change that in > v3/merge depending on what comes first. > >
Hi Ales, Jacob, Mark, Numan, Thanks for the patch and reviews! I went ahead and made the suggested change and applied this patch to main. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
