On 4/21/26 12:14 PM, Mairtin O'Loingsigh via dev wrote:
> Enable the creation and management of transit switch ports, when
> created, these ports are available across multiple AZs and may share
> port binding depending on configuration.
>
> Commands to add, set address and delete port
> ovn-ic-nbctl tsp-add ts0 ts0-p0 chassis=chassis
> ovn-ic-nbctl tsp-set-addr ts0-p0 "00:11:22:11:22:34
> 192.168.10.11/24"
> ovn-ic-nbctl tsp-del ts0-p0
>
> Signed-off-by: Mairtin O'Loingsigh <[email protected]>
> ---
Hi Mairtin,
Thanks for the patch! I'm a bit confused about how this should work,
please see my comment in the ovn-ic-nbctl section below.
There are some more other things listed inline.
> ic/ovn-ic.c | 223 +++++++++++++++++++++++++++++++++++++--
> lib/ovn-util.c | 44 ++++++++
> lib/ovn-util.h | 4 +
> utilities/ovn-ic-nbctl.c | 201 ++++++++++++++++++++++++++++++++++-
> utilities/ovn-nbctl.c | 52 +--------
> 5 files changed, 468 insertions(+), 56 deletions(-)
>
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index ba9490658..f0312e71d 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -631,6 +631,33 @@ find_tr_in_nb(struct ic_context *ctx, char *tr_name)
> return NULL;
> }
>
> +static const struct nbrec_logical_router *
> +find_router_by_port(struct ic_context *ctx, const char *port_name)
> +{
> + const struct nbrec_logical_router_port *key =
> + nbrec_logical_router_port_index_init_row(ctx->nbrec_lrp_by_name);
> + nbrec_logical_router_port_index_set_name(key, port_name);
> +
> + const struct nbrec_logical_router *lr;
> + const struct nbrec_logical_router_port *lrp;
> + bool found = false;
> + NBREC_LOGICAL_ROUTER_FOR_EACH (lr, ctx->ovnnb_idl) {
We're not using the key, we're doing a full table traversal instead.
You'd need _FOR_EACH_EQUAL().
> + for (size_t i = 0; i < lr->n_ports; i++) {
> + lrp = lr->ports[i];
> + if (!strcmp(lrp->name, port_name)) {
> + found = true;
> + break;
We only break the inner loop, lr will be overwritten in the next
iteration of the outer loop. We need to (destroy the index and) return
here.
> + }
> + }
> + }
> +
> + nbrec_logical_router_port_index_destroy_row(key);
> + if (found) {
> + return lr;
> + }
> + return NULL;
> +}
> +
> static const struct sbrec_port_binding *
> find_sb_pb_by_name(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> const char *name)
> @@ -733,6 +760,29 @@ get_lp_address_for_sb_pb(struct ic_context *ctx,
> return peer->n_mac ? *peer->mac : NULL;
> }
>
> +static const char *
> +get_lp_address_for_tr_pb(struct ic_context *ctx,
> + struct icnbrec_transit_switch_port *tsp)
> +{
> + const struct nbrec_logical_switch_port *nb_lsp;
> +
> + nb_lsp = get_lsp_by_ts_port_name(ctx, tsp->name);
Can this ever return NULL?
> + if (!strcmp(nb_lsp->type, "switch")) {
> + /* Switches always have implicit "unknown" address, and IC-SB port
> + * binding can only have one address specified. */
> + return "unknown";
> + }
> +
> + if (!tsp->option) {
> + return NULL;
> + }
> +
> + const struct sbrec_port_binding *peer =
> + find_sb_pb_by_name(ctx->sbrec_port_binding_by_name, tsp->option);
> +
In other places in ovn-ic.c we assume find_sb_pb_by_name() can return
NULL. We should check here too I guess.
> + return peer->n_mac ? *peer->mac : NULL;
> +}
> +
> static const struct sbrec_chassis *
> find_sb_chassis(struct ic_context *ctx, const char *name)
> {
> @@ -793,8 +843,8 @@ get_router_uuid_by_sb_pb(struct ic_context *ctx,
> }
>
> static void
> -update_isb_pb_external_ids(struct ic_context *ctx,
> - const struct sbrec_port_binding *sb_pb,
> +update_isb_pb_external_ids(struct ic_context *ctx OVS_UNUSED,
> + const struct sbrec_port_binding *sb_pb OVS_UNUSED,
Both ctx and sb_pb are actually used, we shouldn't add OVS_UNUSED.
> const struct icsbrec_port_binding *isb_pb)
> {
> struct uuid lr_uuid;
> @@ -864,6 +914,52 @@ sync_local_port(struct ic_context *ctx,
> /* Sync back tunnel key from ISB to NB */
> sync_lsp_tnl_key(lsp, isb_pb->tunnel_key);
> }
Nit: we need an empty line here.
> +/* For each local port:
> + * - Sync from ISB to NB.
> + * - Sync from ISB to SB.
> + */
> +static void
> +sync_switch_port(struct ic_context *ctx OVS_UNUSED,
Same comment about OVS_UNUSED.
> + struct icnbrec_transit_switch_port *tsp,
> + const struct icsbrec_port_binding *isb_pb,
> + const struct nbrec_logical_switch_port *lsp,
> + const struct sbrec_port_binding *sb_pb)
> +{
> + const char *address = get_lp_address_for_tr_pb(ctx, tsp);
> + if (!address) {
> + VLOG_DBG("Can't get router/switch port address for logical"
> + " switch port %s", lsp->name);
Nit: I'd put the space before "switch" on the previous line.
> + if (isb_pb->address[0]) {
> + icsbrec_port_binding_set_address(isb_pb, "");
> + }
> + } else {
> + if (strcmp(address, isb_pb->address)) {
> + icsbrec_port_binding_set_address(isb_pb, address);
> + }
> + }
> +
> + /* Sync gateway from SB to ISB */
> + const struct sbrec_port_binding *crp = find_crp_for_sb_pb(ctx, sb_pb);
> + if (crp && crp->chassis) {
> + if (strcmp(crp->chassis->name, isb_pb->gateway)) {
> + icsbrec_port_binding_set_gateway(isb_pb, crp->chassis->name);
> + }
> + } else if (!strcmp(tsp->type, "switch") && sb_pb->chassis) {
> + if (strcmp(sb_pb->chassis->name, isb_pb->gateway)) {
> + icsbrec_port_binding_set_gateway(isb_pb, sb_pb->chassis->name);
> + }
> + } else {
> + if (isb_pb->gateway[0]) {
> + icsbrec_port_binding_set_gateway(isb_pb, "");
> + }
> + }
> +
> + /* Sync external_ids:router-id to ISB */
> + update_isb_pb_external_ids(ctx, sb_pb, isb_pb);
> +
> + /* Sync back tunnel key from ISB to NB */
> + sync_lsp_tnl_key(lsp, isb_pb->tunnel_key);
> +}
>
> /* For each remote port:
> * - Sync from ISB to NB
> @@ -991,6 +1087,21 @@ allocate_port_key(struct hmap *pb_tnlids)
> 1, (1u << 15) - 1, &hint);
> }
>
> +static void
> +set_isb_pb_peer(struct ic_context *ctx,
> + const struct icnbrec_transit_switch_port *tsp,
> + const struct icsbrec_port_binding *isb_pb)
> +{
> + const struct nbrec_logical_router *lr;
> + lr = find_router_by_port(ctx, tsp->option);
> + if (lr) {
> + char *uuid_s = xasprintf(UUID_FMT, UUID_ARGS(&lr->header_.uuid));
> + icsbrec_port_binding_update_external_ids_setkey(isb_pb, "router-id",
> + uuid_s);
> + free(uuid_s);
> + }
> +}
> +
> static const struct icsbrec_port_binding *
> create_isb_pb(struct ic_context *ctx, const char *logical_port,
> const struct icsbrec_availability_zone *az, const char
> *ts_name,
> @@ -1010,6 +1121,7 @@ create_isb_pb(struct ic_context *ctx, const char
> *logical_port,
> icsbrec_port_binding_set_tunnel_key(isb_pb, pb_tnl_key);
> icsbrec_port_binding_set_nb_ic_uuid(isb_pb, nb_ic_uuid, 1);
> icsbrec_port_binding_set_type(isb_pb, type);
> +
> return isb_pb;
> }
>
> @@ -1028,7 +1140,7 @@ get_lrp_by_lrp_name(struct ic_context *ctx, const char
> *lrp_name)
> }
>
> static bool
> -trp_is_remote(struct ic_context *ctx, const char *chassis_name)
> +chassis_is_remote(struct ic_context *ctx, const char *chassis_name)
> {
> if (chassis_name) {
> const struct sbrec_chassis *chassis =
> @@ -1057,11 +1169,41 @@ lrp_create(struct ic_context *ctx, const struct
> nbrec_logical_router *lr,
> return lrp;
> }
>
> +static struct nbrec_logical_switch_port *
> +lsp_create(struct ic_context *ctx, const struct nbrec_logical_switch *lr,
Maybe s/lr/ls/ ?
> + const struct icsbrec_port_binding *isb_pb,
> + const struct icnbrec_transit_switch_port *tsp)
> +{
> + bool router_port = !strcmp(tsp->type, "router");
> + const char *type = router_port ? "router" : "localnet";
> + const char *option_name = router_port ? "router-port" : "network_name";
> + struct smap options = SMAP_CONST1(&options, option_name, tsp->option);
> +
> + struct nbrec_logical_switch_port *lsp =
> + nbrec_logical_switch_port_insert(ctx->ovnnb_txn);
> + nbrec_logical_switch_port_set_name(lsp, tsp->name);
> +
> + nbrec_logical_switch_port_update_options_setkey(lsp, "interconn-ts",
> + tsp->name);
> + nbrec_logical_switch_port_set_type(lsp, type);
> +
> + nbrec_logical_switch_port_set_options(lsp, &options);
Doesn't this overwrite "options:interconn-ts" we set just above?
> +
> + nbrec_logical_switch_update_ports_addvalue(lr, lsp);
> +
> + char *uuid_s = xasprintf(UUID_FMT, UUID_ARGS(&lr->header_.uuid));
> + icsbrec_port_binding_update_external_ids_setkey(isb_pb, "router-id",
> + uuid_s);
Hmm, "router-id"? This is a port binding corresponding to a switch
port, and (as mentioned above) "lr" is actually a switch.
> + free(uuid_s);
> + return lsp;
> +}
> +
> static void
> sync_ts_isb_pb(struct ic_context *ctx, const struct sbrec_port_binding
> *sb_pb,
> const struct icsbrec_port_binding *isb_pb)
> {
> const char *address = get_lp_address_for_sb_pb(ctx, sb_pb);
> +
> if (address) {
> icsbrec_port_binding_set_address(isb_pb, address);
> }
> @@ -1118,7 +1260,6 @@ port_binding_run(struct ic_context *ctx)
> }
> icsbrec_port_binding_index_destroy_row(isb_pb_key);
>
> - const struct sbrec_port_binding *sb_pb;
> const struct icnbrec_transit_switch *ts;
> ICNBREC_TRANSIT_SWITCH_FOR_EACH (ts, ctx->ovninb_idl) {
> const struct nbrec_logical_switch *ls = find_ts_in_nb(ctx, ts->name);
> @@ -1126,9 +1267,20 @@ port_binding_run(struct ic_context *ctx)
> VLOG_DBG("Transit switch %s not found in NB.", ts->name);
> continue;
> }
> + struct shash nb_ports = SHASH_INITIALIZER(&nb_ports);
> + struct shash old_nb_ports = SHASH_INITIALIZER(&old_nb_ports);
> struct shash local_pbs = SHASH_INITIALIZER(&local_pbs);
> struct shash remote_pbs = SHASH_INITIALIZER(&remote_pbs);
>
> + for (size_t i = 0; i < ls->n_ports; i++) {
> + const struct nbrec_logical_switch_port *lsp = ls->ports[i];
> + if (smap_get_def(&lsp->options, "interconn-ts", NULL)) {
This can be smap_get().
> + shash_add(&nb_ports, lsp->name, lsp);
> + } else {
> + shash_add(&old_nb_ports, lsp->name, lsp);
> + }
> + }
> +
> isb_pb_key = icsbrec_port_binding_index_init_row(
> ctx->icsbrec_port_binding_by_ts);
> icsbrec_port_binding_index_set_transit_switch(isb_pb_key, ts->name);
> @@ -1145,9 +1297,59 @@ port_binding_run(struct ic_context *ctx)
> }
> icsbrec_port_binding_index_destroy_row(isb_pb_key);
>
> + for (int i = 0; i < ts->n_ports; i++) {
size_t
> + struct icnbrec_transit_switch_port *tsp = ts->ports[i];
> + if (!chassis_is_remote(ctx, tsp->chassis)) {
> + isb_pb = shash_find_and_delete(&local_pbs, tsp->name);
> + if (!isb_pb) {
> + isb_pb = create_isb_pb(ctx, tsp->name, ctx->runned_az,
> + ts->name, &ts->header_.uuid,
> + "transit-switch-port",
> &pb_tnlids);
isb_pb can be NULL here, if we failed allocating keys.
> + set_isb_pb_peer(ctx, tsp, isb_pb);
> + }
> +
> + if (isb_pb->type) {
> + icsbrec_port_binding_set_type(isb_pb,
> + "transit-switch-port");
> + }
> +
> + if (isb_pb->nb_ic_uuid) {
> + icsbrec_port_binding_set_nb_ic_uuid(isb_pb,
> + &ts->header_.uuid,
> 1);
> + }
Didn't we just set ty[e/nb_ic_uuid in create_isb_pb() above (if this is
a new PB)? Should we check first whether there's a difference?
> + const struct nbrec_logical_switch_port *lsp =
> + shash_find_and_delete(&nb_ports, tsp->name);
> + if (!lsp) {
> + lsp = lsp_create(ctx, ls, isb_pb, tsp);
> + }
> +
> + const struct sbrec_port_binding *sb_pb;
> + sb_pb = find_lsp_in_sb(ctx, lsp);
> + if (sb_pb) {
> + sync_switch_port(ctx, tsp, isb_pb, lsp, sb_pb);
> + }
> + } else {
> + /* Remote port sync */
> + isb_pb = shash_find_and_delete(&remote_pbs, tsp->name);
> + if (isb_pb) {
> + const struct nbrec_logical_switch_port *lsp =
> + shash_find_and_delete(&nb_ports, tsp->name);
> + if (!lsp) {
> + lsp = lsp_create(ctx, ls, isb_pb, tsp);
> + }
> + const struct sbrec_port_binding *sb_pb;
> + sb_pb = find_lsp_in_sb(ctx, lsp);
> + if (sb_pb) {
> + sync_remote_port(ctx, isb_pb, lsp, sb_pb);
> + }
> + }
> + }
> + }
Nit: I'd add an empty line.
> + /* Support legacy way of adding transit switch ports*/
Comments should end with dot. Also missing space.
> + const struct sbrec_port_binding *sb_pb;
> const struct nbrec_logical_switch_port *lsp;
> - for (int i = 0; i < ls->n_ports; i++) {
size_t
> - lsp = ls->ports[i];
> + SHASH_FOR_EACH (node, &old_nb_ports) {
> + lsp = node->data;
>
> if (!strcmp(lsp->type, "router")
> || !strcmp(lsp->type, "switch")) {
> @@ -1193,6 +1395,11 @@ port_binding_run(struct ic_context *ctx)
> }
> }
>
> + SHASH_FOR_EACH (node, &nb_ports) {
> + nbrec_logical_switch_port_delete(node->data);
> + nbrec_logical_switch_update_ports_delvalue(ls, node->data);
> + }
> +
> /* Delete extra port-binding from ISB */
> SHASH_FOR_EACH (node, &local_pbs) {
> icsbrec_port_binding_delete(node->data);
> @@ -1203,8 +1410,10 @@ port_binding_run(struct ic_context *ctx)
> create_nb_lsp(ctx, node->data, ls);
> }
>
> + shash_destroy(&nb_ports);
> shash_destroy(&local_pbs);
> shash_destroy(&remote_pbs);
> + shash_destroy(&old_nb_ports);
> }
>
> SHASH_FOR_EACH (node, &switch_all_local_pbs) {
> @@ -1250,7 +1459,7 @@ port_binding_run(struct ic_context *ctx)
> for (size_t i = 0; i < tr->n_ports; i++) {
> const struct icnbrec_transit_router_port *trp = tr->ports[i];
>
> - if (trp_is_remote(ctx, trp->chassis)) {
> + if (chassis_is_remote(ctx, trp->chassis)) {
> isb_pb = shash_find_and_delete(&remote_pbs, trp->name);
> } else {
> isb_pb = shash_find_and_delete(&local_pbs, trp->name);
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index fb02825ac..d41ab366f 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -1869,3 +1869,47 @@ eth_addr_parse_masked(const char *s, struct eth_addr
> *ea, unsigned int *plen)
> *ea = eth_addr_zero;
> return false;
> }
> +
> +bool
> +sp_contains_duplicate_ip(struct lport_addresses *laddrs1,
> + struct lport_addresses *laddrs2,
> + char *port_name,
> + char **error_str)
> +{
> + for (size_t i = 0; i < laddrs1->n_ipv4_addrs; i++) {
> + for (size_t j = 0; j < laddrs2->n_ipv4_addrs; j++) {
> + if (laddrs1->ipv4_addrs[i].addr == laddrs2->ipv4_addrs[j].addr) {
> + if (error_str) {
> + *error_str = xasprintf("duplicate IPv4 address '%s' "
> + "found on logical switch "
> + "port '%s'",
> + laddrs1->ipv4_addrs[i].addr_s,
> + port_name);
> + }
> + return true;
> + }
> + }
> + }
> +
> + for (size_t i = 0; i < laddrs1->n_ipv6_addrs; i++) {
> + for (size_t j = 0; j < laddrs2->n_ipv6_addrs; j++) {
> + if (IN6_ARE_ADDR_EQUAL(&laddrs1->ipv6_addrs[i].addr,
> + &laddrs2->ipv6_addrs[j].addr)) {
> + if (error_str) {
> + *error_str = xasprintf("duplicate IPv6 address "
> + "'%s' found on logical "
> + "switch port '%s'",
> + laddrs1->ipv6_addrs[i].addr_s,
> + port_name);
> + }
> + return true;
> + }
> + }
> + }
> +
> + if (error_str) {
> + *error_str = NULL;
> + }
> +
> + return false;
> +}
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index bcb344de4..df78c7342 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -790,6 +790,10 @@ char *normalize_ipv6_addr_str(const char *orig_addr);
>
> char *normalize_addr_str(const char *orig_addr);
>
> +bool sp_contains_duplicate_ip(struct lport_addresses *laddrs1,
> + struct lport_addresses *laddrs2, char *name,
> + char **error_str);
> +
> #define NEIGH_REDISTRIBUTE_MODES \
> NEIGH_REDISTRIBUTE_MODE(FDB, 0) \
> NEIGH_REDISTRIBUTE_MODE(IP, 1)
> diff --git a/utilities/ovn-ic-nbctl.c b/utilities/ovn-ic-nbctl.c
> index 50e975283..1ba11ddf9 100644
> --- a/utilities/ovn-ic-nbctl.c
> +++ b/utilities/ovn-ic-nbctl.c
> @@ -336,6 +336,9 @@ Transit switch commands:\n\
> ts-add SWITCH create a transit switch named SWITCH\n\
> ts-del SWITCH delete SWITCH\n\
> ts-list print all transit switches\n\
> + tsp-add SWITCH PORT add a transit switch PORT\n\
> + tsp-set-addr PORT NETWORK add a transit switch PORT\n\
set a transit switch PORT address?
For LSPs (in ovn-nbctl) we support multiple addresses, a transit switch
port (in the IC DB) should be able to map one to one to a NB switch
port, so should we support multiple addresses?
Also, why NETWORK?
> + tsp-del PORT delete a transit switch PORT\n\
> \n\
> Transit router commands:\n\
> tr-add ROUTER create a transit router named ROUTER\n\
> @@ -617,6 +620,38 @@ trp_by_name_or_uuid(struct ctl_context *ctx, const char
> *id, bool must_exist,
> return NULL;
> }
>
> +static char *
> +tsp_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist,
> + const struct icnbrec_transit_switch_port **tsp_p)
> +{
> + const struct icnbrec_transit_switch_port *tsp = NULL;
> + *tsp_p = NULL;
> + struct uuid tsp_uuid;
> + bool is_uuid = uuid_from_string(&tsp_uuid, id);
> + if (is_uuid) {
> + tsp = icnbrec_transit_switch_port_get_for_uuid(ctx->idl, &tsp_uuid);
> + }
> +
> + if (!tsp) {
> + const struct icnbrec_transit_switch_port *iter;
> +
> + ICNBREC_TRANSIT_SWITCH_PORT_FOR_EACH (iter, ctx->idl) {
> + if (!strcmp(iter->name, id)) {
> + tsp = iter;
> + break;
> + }
> + }
> + }
> +
> + if (!tsp && must_exist) {
> + return xasprintf("%s: switch port %s not found", id,
> + is_uuid ? "UUID" : "name");
> + }
> +
> + *tsp_p = tsp;
> + return NULL;
> +}
> +
> static void
> ic_nbctl_tr_del(struct ctl_context *ctx)
> {
> @@ -664,6 +699,34 @@ ic_nbctl_trp_del(struct ctl_context *ctx)
> icnbrec_transit_router_port_delete(trp);
> }
>
> +static void
> +ic_nbctl_tsp_del(struct ctl_context *ctx)
> +{
> + bool must_exist = !shash_find(&ctx->options, "--if-exists");
> + const char *tsp_name = ctx->argv[1];
> + const struct icnbrec_transit_switch_port *tsp = NULL;
> +
> + ctx->error = tsp_by_name_or_uuid(ctx, tsp_name, must_exist, &tsp);
> + if (ctx->error) {
> + return;
> + }
> +
> + if (!tsp) {
> + return;
> + }
> +
> + const struct icnbrec_transit_switch *ts = NULL;
> + char *ts_uuid = uuid_to_string(&tsp->ts_uuid);
> + ctx->error = ts_by_name_or_uuid(ctx, ts_uuid, true, &ts);
> + free(ts_uuid);
> + if (ctx->error) {
> + return;
> + }
> +
> + icnbrec_transit_switch_update_ports_delvalue(ts, tsp);
> + icnbrec_transit_switch_port_delete(tsp);
> +}
> +
> static void
> ic_nbctl_tr_list(struct ctl_context *ctx)
> {
> @@ -802,6 +865,137 @@ ic_nbctl_trp_add(struct ctl_context *ctx)
> icnbrec_transit_router_update_ports_addvalue(tr, trp);
> }
>
> +static void
> +ic_nbctl_tsp_add(struct ctl_context *ctx)
> +{
> + bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> + const char *ts_name = ctx->argv[1];
> + const char *tsp_name = ctx->argv[2];
> + const struct icnbrec_transit_switch *ts;
> +
> + ctx->error = ts_by_name_or_uuid(ctx, ts_name, true, &ts);
> + if (ctx->error) {
> + return;
> + }
> +
> + const struct icnbrec_transit_switch_port *tsp;
> + ctx->error = tsp_by_name_or_uuid(ctx, tsp_name, false, &tsp);
> + if (ctx->error) {
> + return;
> + }
> +
> + if (tsp) {
> + if (!may_exist) {
> + ctl_error(ctx, "%s: a port with this name already exists",
> + tsp_name);
> + return;
> + }
> + return;
> + }
> +
> + tsp = icnbrec_transit_switch_port_insert(ctx->txn);
> + icnbrec_transit_switch_port_set_name(tsp, tsp_name);
> + icnbrec_transit_switch_port_set_ts_uuid(tsp, ts->header_.uuid);
> +
> + int n_settings = ctx->argc - 3;
> + char **settings = (char **) &ctx->argv[3];
> + for (int i = 0; i < n_settings; i++) {
size_t
> + ctx->error = ctl_set_column("Transit_Switch_Port", &tsp->header_,
> + settings[i], ctx->symtab);
> + if (ctx->error) {
> + return;
> + }
> + }
> +
> + icnbrec_transit_switch_update_ports_addvalue(ts, tsp);
> +}
> +
> +static char *
> +tsp_contains_duplicates(const struct icnbrec_transit_switch *ts,
> + const struct icnbrec_transit_switch_port *tsp,
> + const char *address)
> +{
> + char *sub_error = NULL;
> + struct lport_addresses laddrs;
> + if (!extract_lsp_addresses(address, &laddrs)) {
> + return NULL;
> + }
> +
> + for (size_t i = 0; i < ts->n_ports; i++) {
> + struct icnbrec_transit_switch_port *tsp_test = ts->ports[i];
> + if (tsp_test == tsp) {
> + continue;
> + }
> + for (size_t j = 0; j < tsp_test->n_addresses; j++) {
> + struct lport_addresses laddrs_test;
> + char *addr = tsp_test->addresses[j];
> + if (extract_lsp_addresses(addr, &laddrs_test)) {
> + bool has_duplicate =
> + sp_contains_duplicate_ip(&laddrs, &laddrs_test,
> + tsp_test->name, &sub_error);
> + destroy_lport_addresses(&laddrs_test);
> + if (has_duplicate) {
> + goto err_out;
> + }
> + }
> + }
> + }
> +
> +err_out: ;
> + char *error = NULL;
> + if (sub_error) {
> + error = xasprintf("Error on switch %s: %s", ts->name, sub_error);
> + free(sub_error);
> + }
> + destroy_lport_addresses(&laddrs);
> + return error;
> +}
> +
> +static void
> +ic_nbctl_tsp_set_addr(struct ctl_context *ctx)
> +{
> + const char *tsp_name = ctx->argv[1];
> +
> + const struct icnbrec_transit_switch_port *tsp;
> + ctx->error = tsp_by_name_or_uuid(ctx, tsp_name, true, &tsp);
> + if (ctx->error) {
> + return;
> + }
> +
> + int i;
> + for (i = 2; i < ctx->argc; i++) {
> + char ipv6_s[IPV6_SCAN_LEN + 1];
> + struct eth_addr ea;
> + ovs_be32 ip;
> +
> + if (strcmp(ctx->argv[i], "unknown") && strcmp(ctx->argv[i],
> "dynamic")
> + && strcmp(ctx->argv[i], "router")
> + && !ovs_scan(ctx->argv[i], ETH_ADDR_SCAN_FMT,
> + ETH_ADDR_SCAN_ARGS(ea))
> + && !ovs_scan(ctx->argv[i], "dynamic "IPV6_SCAN_FMT, ipv6_s)
> + && !ovs_scan(ctx->argv[i], "dynamic "IP_SCAN_FMT,
> + IP_SCAN_ARGS(&ip))) {
> + ctl_error(ctx, "%s: Invalid address format. See ovn-nb(5). "
> + "Hint: An Ethernet address must be "
> + "listed before an IP address, together as a single "
> + "argument.", ctx->argv[i]);
> + return;
> + }
> +
> + const struct icnbrec_transit_switch *ts;
> + ts = icnbrec_transit_switch_get_for_uuid(ctx->idl, &tsp->ts_uuid);
> + ctx->error = tsp_contains_duplicates(ts, tsp, ctx->argv[i]);
> + if (ctx->error) {
> + ctl_error(ctx, "%s", ctx->error);
This is a use after free (ctx->error is first freed by ctl_error()
internally). Can't we just remove this line and let the db-ctl-base
engine handle the ctx->error?
> + return;
> + }
> + }
> +
> + icnbrec_transit_switch_port_set_addresses(tsp,
> + (const char **) ctx->argv + 2,
> + ctx->argc - 2);
> +}
> +
> static void
> verify_connections(struct ctl_context *ctx)
> {
> @@ -1317,7 +1511,13 @@ static const struct ctl_command_syntax
> ic_nbctl_commands[] = {
> { "ts-add", 1, 1, "SWITCH", NULL, ic_nbctl_ts_add, NULL, "--may-exist",
> RW },
> { "ts-del", 1, 1, "SWITCH", NULL, ic_nbctl_ts_del, NULL, "--if-exists",
> RW },
> { "ts-list", 0, 0, "", NULL, ic_nbctl_ts_list, NULL, "", RO },
> + { "tsp-add", 2, INT_MAX, "SWITCH PORT COLUMN[:KEY]=VALUE]...",
> + NULL, ic_nbctl_tsp_add, NULL, "--may-exist", RW },
Maybe I misunderstand the goal of the patch series but if I do this in a
sandbox:
$ ovn-ic-nbctl ts-add ts -- tsp-add ts tsp1
$ ovn-ic-sbctl show
availability-zone az-1
$ ovn-ic-sbctl list port_binding
$ ovn-nbctl show
switch 1479b061-3cd3-42e5-b8f6-1c54021fd1ed (ts)
Shouldn't we have IC-SB/SB port bindings and NB logical switch ports for
tsp1 too?
> + { "tsp-set-addr", 2, INT_MAX, "PORT [ADDRESS]...",
> + NULL, ic_nbctl_tsp_set_addr, NULL, "", RW },
>
> + { "tsp-del", 1, 1, "PORT", NULL, ic_nbctl_tsp_del, NULL, "--if-exists",
> + RW },
> /* transit router commands. */
> { "tr-add", 1, 1, "ROUTER", NULL, ic_nbctl_tr_add, NULL, "--may-exist",
> RW },
> @@ -1329,7 +1529,6 @@ static const struct ctl_command_syntax
> ic_nbctl_commands[] = {
> NULL, ic_nbctl_trp_add, NULL, "--may-exist", RW },
> { "trp-del", 1, 1, "PORT", NULL, ic_nbctl_trp_del, NULL, "--if-exists",
> RW },
> -
> /* Connection commands. */
> {"get-connection", 0, 0, "", pre_connection, cmd_get_connection, NULL,
> "",
> RO},
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 0ef207272..56cfdf5b4 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -1482,50 +1482,6 @@ nbctl_pre_lsp_set_addresses(struct ctl_context *ctx)
> &nbrec_logical_switch_port_col_dynamic_addresses);
> }
>
> -static bool
> -lsp_contains_duplicate_ip(struct lport_addresses *laddrs1,
> - struct lport_addresses *laddrs2,
> - const struct nbrec_logical_switch_port *lsp_test,
> - char **error_str)
> -{
> - for (size_t i = 0; i < laddrs1->n_ipv4_addrs; i++) {
> - for (size_t j = 0; j < laddrs2->n_ipv4_addrs; j++) {
> - if (laddrs1->ipv4_addrs[i].addr == laddrs2->ipv4_addrs[j].addr) {
> - if (error_str) {
> - *error_str = xasprintf("duplicate IPv4 address '%s' "
> - "found on logical switch "
> - "port '%s'",
> - laddrs1->ipv4_addrs[i].addr_s,
> - lsp_test->name);
> - }
> - return true;
> - }
> - }
> - }
> -
> - for (size_t i = 0; i < laddrs1->n_ipv6_addrs; i++) {
> - for (size_t j = 0; j < laddrs2->n_ipv6_addrs; j++) {
> - if (IN6_ARE_ADDR_EQUAL(&laddrs1->ipv6_addrs[i].addr,
> - &laddrs2->ipv6_addrs[j].addr)) {
> - if (error_str) {
> - *error_str = xasprintf("duplicate IPv6 address "
> - "'%s' found on logical "
> - "switch port '%s'",
> - laddrs1->ipv6_addrs[i].addr_s,
> - lsp_test->name);
> - }
> - return true;
> - }
> - }
> - }
> -
> - if (error_str) {
> - *error_str = NULL;
> - }
> -
> - return false;
> -}
> -
> static char *
> lsp_contains_duplicates(const struct nbrec_logical_switch *ls,
> const struct nbrec_logical_switch_port *lsp,
> @@ -1550,8 +1506,8 @@ lsp_contains_duplicates(const struct
> nbrec_logical_switch *ls,
> }
> if (extract_lsp_addresses(addr, &laddrs_test)) {
> bool has_duplicate =
> - lsp_contains_duplicate_ip(&laddrs, &laddrs_test,
> - lsp_test, &sub_error);
> + sp_contains_duplicate_ip(&laddrs, &laddrs_test,
> + lsp_test->name, &sub_error);
> destroy_lport_addresses(&laddrs_test);
> if (has_duplicate) {
> goto err_out;
> @@ -8835,8 +8791,8 @@ lsp_health_check_parse_target_address(
> goto cleanup;
> }
>
> - if (lsp_contains_duplicate_ip(&target_address,
> - &lsp_address, lsp, NULL)) {
> + if (sp_contains_duplicate_ip(&target_address,
> + &lsp_address, lsp->name, NULL)) {
> ip_found_on_port = true;
> }
>
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
