Re: [ovs-dev] [PATCH ovn v6 4/6] northd: Delete pb if tunnel is not allocated.

[Ihar Hrachyshka]

On Tue, Apr 23, 2024 at 4:59 PM Mark Michelson  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 
> > ---
> >   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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v6 4/6] northd: Delete pb if tunnel is not allocated.

[Mark Michelson]

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 
---
  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

[ovs-dev] [PATCH ovn v6 4/6] northd: Delete pb if tunnel is not allocated.

[Ihar Hrachyshka]

This allows callers to avoid cleanup of the record in case the function
fails.

Signed-off-by: Ihar Hrachyshka 
---
 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);
+}
 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;
-- 
2.41.0

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