I have one small possible nit, ignore it if it misuses a function
On Thu, Sep 25, 2025 at 3:05 PM Ales Musil via dev <ovs-dev@openvswitch.org>
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 <amu...@redhat.com>
> ---
> 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)`?
> + 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
