On 4/11/24 21:57, Ihar Hrachyshka wrote:
This allows callers to avoid cleanup of the record in case the function
fails.

Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
---
  northd/northd.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 4cea669cf..6a8ace52f 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -4322,6 +4322,9 @@ ls_port_init(struct ovn_port *op, struct ovsdb_idl_txn 
*ovnsb_txn,
      }
      /* Assign new tunnel ids where needed. */
      if (!ovn_port_allocate_key(sbrec_chassis_table, op)) {
+        if (!sb) {
+            sbrec_port_binding_delete(op->sb);
+        }

Given how the code is structured, I think this could be simplified a bit. The code above this point is:

if (sb) {
    /* We don't care about this case */
} else {
    op->sb = sbrec_port_binding_insert(ovnsb_txn);
    sbrec_port_binding_set_logical_port(op->sb, op->key);
}

Could the else block be restructured as follows?

if (sb) {
    /* We don't care about this case */
} else {
    if (!ovn_port_allocate_key(sbrec_chassis_table, op)) {
        return false;
    }
    op->sb = sbrec_port_binding_insert(ovnsb_txn);
    sbrec_port_binding_set_logical_port(op->sb, op->key);
}

This way, we wait to insert the SB port binding until after we successfully allocate the tunnel key. We don't insert it and then immediately delete it this way.

          return false;
      }
      ovn_port_update_sbrec(ovnsb_txn, sbrec_chassis_by_name,
@@ -4345,9 +4348,6 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct 
hmap *ls_ports,
      if (!ls_port_init(op, ovnsb_txn, od, sb,
                        sbrec_mirror_table, sbrec_chassis_table,
                        sbrec_chassis_by_name, sbrec_chassis_by_hostname)) {
-        if (op->sb) {
-            sbrec_port_binding_delete(op->sb);
-        }
          ovn_port_destroy(ls_ports, op);
          return NULL;
      }
@@ -4551,8 +4551,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
                                  ni->sbrec_chassis_table,
                                  ni->sbrec_chassis_by_name,
                                  ni->sbrec_chassis_by_hostname)) {
-                if (op->sb) {
-                    sbrec_port_binding_delete(op->sb);
+                if (sb) {
+                    sbrec_port_binding_delete(sb);
                  }
                  ovn_port_destroy(&nd->ls_ports, op);
                  goto fail;

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to