[ovs-dev] ovn-controller: Cleanup memory in binding_evaluate_port_binding_changes
diff mbox series

Message ID 1560254960-18363-1-git-send-email-dceara@redhat.com
State Superseded
Headers show
Series
  • [ovs-dev] ovn-controller: Cleanup memory in binding_evaluate_port_binding_changes
Related show

Commit Message

Dumitru Ceara June 11, 2019, 12:09 p.m. UTC
The 'lport_to_iface' and 'egress_ifaces' hashtables were not cleaned up
in case a port binding was found that would require a recompute.

Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048822.html
Reported-by: Daniel Alvarez Sanchez <dalvarez@redhat.com>
Fixes: 9d0b504abdee ("ovn-controller: runtime_data change handler for SB port-binding")
CC: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 ovn/controller/binding.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Han Zhou June 11, 2019, 2:48 p.m. UTC | #1
On Tue, Jun 11, 2019 at 5:09 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> The 'lport_to_iface' and 'egress_ifaces' hashtables were not cleaned up
> in case a port binding was found that would require a recompute.
>
> Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048822.html
> Reported-by: Daniel Alvarez Sanchez <dalvarez@redhat.com>
> Fixes: 9d0b504abdee ("ovn-controller: runtime_data change handler for SB
port-binding")
> CC: Han Zhou <hzhou8@ebay.com>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
>  ovn/controller/binding.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index b62b3da..87d0b6d 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -695,6 +695,8 @@ binding_evaluate_port_binding_changes(
>          return true;
>      }
>
> +    bool changed = false;
> +
>      const struct sbrec_port_binding *binding_rec;
>      struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface);
>      struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces);
> @@ -718,10 +720,14 @@ binding_evaluate_port_binding_changes(
>              || is_our_chassis(chassis_rec, binding_rec,
>                                active_tunnels, &lport_to_iface,
local_lports)
>              || strcmp(binding_rec->type, "")) {
> -            return true;
> +            changed = true;
> +            break;
>          }
>      }
> -    return false;
> +
> +    shash_destroy(&lport_to_iface);
> +    sset_destroy(&egress_ifaces);
> +    return changed;
>  }
>
>  /* Returns true if the database is all cleaned up, false if more work is
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks for the fix! This was my bad.
The leak should be there regardless of recompute or not, so the commit
message may be updated.

Acked-by: Han Zhou <hzhou8@ebay.com>
Dumitru Ceara June 11, 2019, 2:57 p.m. UTC | #2
On Tue, Jun 11, 2019 at 4:48 PM Han Zhou <zhouhan@gmail.com> wrote:
>
>
>
> On Tue, Jun 11, 2019 at 5:09 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> > The 'lport_to_iface' and 'egress_ifaces' hashtables were not cleaned up
> > in case a port binding was found that would require a recompute.
> >
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048822.html
> > Reported-by: Daniel Alvarez Sanchez <dalvarez@redhat.com>
> > Fixes: 9d0b504abdee ("ovn-controller: runtime_data change handler for SB port-binding")
> > CC: Han Zhou <hzhou8@ebay.com>
> > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> > ---
> >  ovn/controller/binding.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> > index b62b3da..87d0b6d 100644
> > --- a/ovn/controller/binding.c
> > +++ b/ovn/controller/binding.c
> > @@ -695,6 +695,8 @@ binding_evaluate_port_binding_changes(
> >          return true;
> >      }
> >
> > +    bool changed = false;
> > +
> >      const struct sbrec_port_binding *binding_rec;
> >      struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface);
> >      struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces);
> > @@ -718,10 +720,14 @@ binding_evaluate_port_binding_changes(
> >              || is_our_chassis(chassis_rec, binding_rec,
> >                                active_tunnels, &lport_to_iface, local_lports)
> >              || strcmp(binding_rec->type, "")) {
> > -            return true;
> > +            changed = true;
> > +            break;
> >          }
> >      }
> > -    return false;
> > +
> > +    shash_destroy(&lport_to_iface);
> > +    sset_destroy(&egress_ifaces);
> > +    return changed;
> >  }
> >
> >  /* Returns true if the database is all cleaned up, false if more work is
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> Thanks for the fix! This was my bad.
> The leak should be there regardless of recompute or not, so the commit message may be updated.
>
> Acked-by: Han Zhou <hzhou8@ebay.com>
>

Thanks for the review. You're right, the commit log was wrong, i sent
an amended version:
https://patchwork.ozlabs.org/patch/1113901/

Cheers,
Dumitru

Patch
diff mbox series

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index b62b3da..87d0b6d 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -695,6 +695,8 @@  binding_evaluate_port_binding_changes(
         return true;
     }
 
+    bool changed = false;
+
     const struct sbrec_port_binding *binding_rec;
     struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface);
     struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces);
@@ -718,10 +720,14 @@  binding_evaluate_port_binding_changes(
             || is_our_chassis(chassis_rec, binding_rec,
                               active_tunnels, &lport_to_iface, local_lports)
             || strcmp(binding_rec->type, "")) {
-            return true;
+            changed = true;
+            break;
         }
     }
-    return false;
+
+    shash_destroy(&lport_to_iface);
+    sset_destroy(&egress_ifaces);
+    return changed;
 }
 
 /* Returns true if the database is all cleaned up, false if more work is