[ovs-dev,v3,2/3] northd: Refactor chassisresident port checking.

From: Numan Siddique <numans@ovn.org>

The implementation of util functions "is_cr_port()" and "is_l3dgw_port()"
are confusing and not very intuitive.  This patch adds some
documentation.  It also renames the struct ovn_port member 'l3dgw_port'
to 'primary_port'.  If struct ovn_port->primary_port is set, then it
means 'ovn_port' instance is a chassis resident port and it is derived
from the primary port. An upcoming patch creates a chassisresident port
even for a logical switch port of type "patch" and hence renamed to
avoid confusion.

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/northd.c | 48 +++++++++++++++++++++++++++++++++++-------------
 northd/northd.h | 10 ++++------
 2 files changed, 39 insertions(+), 19 deletions(-)

Thank you very much for clarifying things in the code. This is *much* 
needed.

Acked-by: Mark Michelson <mmichels@redhat.com>

On 5/21/24 16:17, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> The implementation of util functions "is_cr_port()" and "is_l3dgw_port()"
> are confusing and not very intuitive.  This patch adds some
> documentation.  It also renames the struct ovn_port member 'l3dgw_port'
> to 'primary_port'.  If struct ovn_port->primary_port is set, then it
> means 'ovn_port' instance is a chassis resident port and it is derived
> from the primary port. An upcoming patch creates a chassisresident port
> even for a logical switch port of type "patch" and hence renamed to
> avoid confusion.
> 
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>   northd/northd.c | 48 +++++++++++++++++++++++++++++++++++-------------
>   northd/northd.h | 10 ++++------
>   2 files changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index 2d0946218a..6393d688f6 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -1077,16 +1077,36 @@ struct ovn_port_routable_addresses {
>   
>   static bool lsp_can_be_inc_processed(const struct nbrec_logical_switch_port *);
>   
> +/* This function returns true if 'op' is a gateway router port.
> + * False otherwise.
> + * For 'op' to be a gateway router port.
> + *  1. op->nbrp->gateway_chassis or op->nbrp->ha_chassis_group should
> + *     be configured.
> + *  2. op->cr_port should not be NULL.  If op->nbrp->gateway_chassis or
> + *     op->nbrp->ha_chassis_group is set by the user, northd WILL create
> + *     a chassis resident port in the SB port binding.
> + *     See join_logical_ports().
> + */
>   static bool
>   is_l3dgw_port(const struct ovn_port *op)
>   {
> -    return op->cr_port;
> +    return op->cr_port && op->nbrp &&
> +           (op->nbrp->n_gateway_chassis || op->nbrp->ha_chassis_group);
>   }
>   
> +/* This function returns true if 'op' is a chassis resident
> + * derived port. False otherwise.
> + * There are 2 ways to check if 'op' is chassis resident port.
> + *  1. op->sb->type is "chassisresident"
> + *  2. op->primary_port is not NULL.  If op->primary_port is set,
> + *     it means 'op' is derived from the ovn_port op->primary_port.
> + *
> + * This function uses the (2) method as it doesn't involve strcmp().
> + */
>   static bool
>   is_cr_port(const struct ovn_port *op)
>   {
> -    return op->l3dgw_port;
> +    return op->primary_port;
>   }
>   
>   static void
> @@ -1171,7 +1191,7 @@ ovn_port_create(struct hmap *ports, const char *key,
>       op->key = xstrdup(key);
>       op->sb = sb;
>       ovn_port_set_nb(op, nbsp, nbrp);
> -    op->l3dgw_port = op->cr_port = NULL;
> +    op->primary_port = op->cr_port = NULL;
>       hmap_insert(ports, &op->key_node, hash_string(op->key, 0));
>   
>       op->lflow_ref = lflow_ref_create();
> @@ -1228,7 +1248,7 @@ ovn_port_destroy(struct hmap *ports, struct ovn_port *port)
>           /* Don't remove port->list. The node should be removed from such lists
>            * before calling this function. */
>           hmap_remove(ports, &port->key_node);
> -        if (port->od && !port->l3dgw_port) {
> +        if (port->od && !port->primary_port) {
>               hmap_remove(&port->od->ports, &port->dp_node);
>           }
>           ovn_port_destroy_orphan(port);
> @@ -1398,7 +1418,7 @@ lsp_force_fdb_lookup(const struct ovn_port *op)
>   static struct ovn_port *
>   ovn_port_get_peer(const struct hmap *lr_ports, struct ovn_port *op)
>   {
> -    if (!op->nbsp || !lsp_is_router(op->nbsp) || op->l3dgw_port) {
> +    if (!op->nbsp || !lsp_is_router(op->nbsp) || is_cr_port(op)) {
>           return NULL;
>       }
>   
> @@ -2278,7 +2298,7 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table,
>                                             NULL, nbrp, NULL);
>                       ovs_list_push_back(nb_only, &crp->list);
>                   }
> -                crp->l3dgw_port = op;
> +                crp->primary_port = op;
>                   op->cr_port = crp;
>                   crp->od = od;
>                   free(redirect_name);
> @@ -2299,7 +2319,7 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table,
>        * to their peers. */
>       struct ovn_port *op;
>       HMAP_FOR_EACH (op, key_node, ports) {
> -        if (op->nbsp && lsp_is_router(op->nbsp) && !op->l3dgw_port) {
> +        if (op->nbsp && lsp_is_router(op->nbsp) && !op->primary_port) {
>               struct ovn_port *peer = ovn_port_get_peer(ports, op);
>               if (!peer || !peer->nbrp) {
>                   continue;
> @@ -2353,7 +2373,7 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table,
>                   op->enable_router_port_acl = smap_get_bool(
>                       &op->nbsp->options, "enable_router_port_acl", false);
>               }
> -        } else if (op->nbrp && op->nbrp->peer && !op->l3dgw_port) {
> +        } else if (op->nbrp && op->nbrp->peer && !is_cr_port(op)) {
>               struct ovn_port *peer = ovn_port_find(ports, op->nbrp->peer);
>               if (peer) {
>                   if (peer->nbrp) {
> @@ -3889,7 +3909,7 @@ sync_pb_for_lrp(struct ovn_port *op,
>   
>           bool always_redirect =
>               !lr_stateful_rec->lrnat_rec->has_distributed_nat &&
> -            !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> +            !l3dgw_port_has_associated_vtep_lports(op->primary_port);
>   
>           const char *redirect_type = smap_get(&op->nbrp->options,
>                                               "redirect-type");
> @@ -4384,7 +4404,7 @@ ls_port_reinit(struct ovn_port *op, struct ovsdb_idl_txn *ovnsb_txn,
>       ovn_port_cleanup(op);
>       op->sb = sb;
>       ovn_port_set_nb(op, nbsp, NULL);
> -    op->l3dgw_port = op->cr_port = NULL;
> +    op->primary_port = op->cr_port = NULL;
>       return ls_port_init(op, ovnsb_txn, od, sb,
>                           sbrec_mirror_table, sbrec_chassis_table,
>                           sbrec_chassis_by_name, sbrec_chassis_by_hostname);
> @@ -13692,7 +13712,7 @@ build_dhcpv6_reply_flows_for_lrouter_port(
>           struct lflow_ref *lflow_ref)
>   {
>       ovs_assert(op->nbrp);
> -    if (op->l3dgw_port) {
> +    if (is_cr_port(op)) {
>           return;
>       }
>       for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> @@ -17809,9 +17829,11 @@ build_ha_chassis_group_ref_chassis(struct hmapx *lr_groups,
>    * table indicating which chassis the distributed port is bond to. */
>   static void
>   handle_cr_port_binding_changes(const struct sbrec_port_binding *sb,
> -                struct ovn_port *orp)
> +                               struct ovn_port *orp)
>   {
> -    const struct nbrec_logical_router_port *nbrec_lrp = orp->l3dgw_port->nbrp;
> +    ovs_assert(orp->primary_port);
> +    const struct nbrec_logical_router_port *nbrec_lrp
> +        = orp->primary_port->nbrp;
>   
>       if (sb->chassis) {
>           nbrec_logical_router_port_update_status_setkey(nbrec_lrp,
> diff --git a/northd/northd.h b/northd/northd.h
> index 940926945e..146139bebc 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -586,13 +586,11 @@ struct ovn_port {
>       /* Logical port multicast data. */
>       struct mcast_port_info mcast_info;
>   
> -    /* At most one of l3dgw_port and cr_port can be not NULL. */
> +    /* At most one of primary_port and cr_port can be not NULL. */
>   
> -    /* This is set to a distributed gateway port if and only if this ovn_port
> -     * is "derived" from it. Otherwise this is set to NULL. The derived
> -     * ovn_port represents the instance of distributed gateway port on the
> -     * gateway chassis.*/
> -    struct ovn_port *l3dgw_port;
> +    /* If this ovn_port is a derived port, then 'primary_port' points to the
> +     * port from which this ovn_port is derived. */
> +    struct ovn_port *primary_port;
>   
>       /* This is set to the "derived" chassis-redirect port of this port if and
>        * only if this port is a distributed gateway port. Otherwise this is set