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. > > >> + struct ovn_port *op = >> + ovn_port_find(is_lrp ? lr_ports : ls_ports, >> pb->logical_port); >> >> if (sbrec_port_binding_is_new(pb)) { >> /* Most likely the PB was created by northd and this is the >> @@ -5011,7 +4994,7 @@ northd_handle_sb_port_binding_changes( >> if (!op) { >> VLOG_WARN_RL(&rl, "A port-binding for %s is created but >> the " >> "%s is not found.", pb->logical_port, >> - is_router_port ? "LRP" : "LSP"); >> + is_lrp ? "LRP" : "LSP"); >> return false; >> } >> op->sb = pb; >> @@ -5043,7 +5026,7 @@ northd_handle_sb_port_binding_changes( >> if (!op) { >> VLOG_WARN_RL(&rl, "A port-binding for %s is updated but >> the " >> "%s is not found.", pb->logical_port, >> - is_router_port ? "LRP" : "LSP"); >> + is_lrp ? "LRP" : "LSP"); >> return false; >> } >> if (op->sb != pb) { >> -- >> 2.51.0 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
