On Tue, Apr 23, 2024 at 4:59 PM Mark Michelson <[email protected]> wrote:
> 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 <[email protected]> > > --- > > 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. > We would need to call ovn_port_allocate_key() for `if (sb)` branch too then (it's needed in both cases.) But I think what we can do to simplify this code a bit is reversing the order as follows: ``` if (sb) { ... } if (!ovn_port_allocate_key(sbrec_chassis_table, op)) { return false; } if (!sb) { op->sb = sbrec_port_binding_insert(ovnsb_txn); sbrec_port_binding_set_logical_port(op->sb, op->key); } ... ``` I will test it and post an update. Thanks for considering the series, Mark! > > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
