| 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 | expand |
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>
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
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
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(-)