Hi William, please try this patch as a substitute for yours. It should
ensure that pointers to nln_notifiers are to the beginning of the
structs instead of to the middle, meaning that valgrind does not
consider them "possible" leaks.
diff --git a/lib/netlink-notifier.c b/lib/netlink-notifier.c
index 0867952..3de6e15 100644
--- a/lib/netlink-notifier.c
+++ b/lib/netlink-notifier.c
@@ -46,9 +46,9 @@ struct nln {
};
struct nln_notifier {
+ struct ovs_list node;
struct nln *nln; /* Parent nln. */
- struct ovs_list node;
int multicast_group; /* Multicast group we listen on. */
nln_notify_func *cb;
void *aux;
On Sat, Jun 25, 2016 at 12:30:46PM -0700, William Tu wrote:
> Hi Cascardo,
>
> Thanks for your feedback.
> I did a couple of more tests and I think it should be valgrind's false
> positive. Even for testcase 1 (TESTSUITEFLAGS='1'), my valgrind
> complains about this case as "possible lost."
>
> On the other hand, I do check and make sure that we only called the
> name_table_init() once. Although we never free the 'name_notifier',
> since it's a static variable, valgrind should reports "still
> reachable" instead of "possible lost" when ovs-vswitchd exits.
>
> Regards,
> William
>
>
> On Mon, Jun 20, 2016 at 12:08 PM, Thadeu Lima de Souza Cascardo
> <[email protected]> wrote:
> > On Mon, Jun 20, 2016 at 07:32:52AM -0700, William Tu wrote:
> >> Testcase 2050, ovn -- 3 HVs, 1 LS, 3 lports/HV, reports possible leak:
> >> nln_notifier_create (netlink-notifier.c:131)
> >> name_table_init (route-table.c:319)
> >> route_table_init (route-table.c:110)
> >> dp_initialize (dpif.c:126)
> >> dp_unregister_provider (dpif.c:218)
> >> dpif_dummy_override (dpif-netdev.c:4309)
> >> dpif_dummy_register (dpif-netdev.c:4329)
> >> dummy_enable (dummy.c:46)
> >> parse_options (ovs-vswitchd.c:205)
> >> main (ovs-vswitchd.c:76)
> >> 'name_notifier' could be overwritten without being free'd.
> >>
> >> Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/138910851
> >> Signed-off-by: William Tu <[email protected]>
> >> ---
> >> lib/route-table.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/lib/route-table.c b/lib/route-table.c
> >> index 58e7f62..cf01c34 100644
> >> --- a/lib/route-table.c
> >> +++ b/lib/route-table.c
> >> @@ -316,6 +316,7 @@ route_table_fallback_lookup(const struct in6_addr
> >> *ip6_dst OVS_UNUSED,
> >> static void
> >> name_table_init(void)
> >> {
> >> + free(name_notifier);
> >> name_notifier = rtnetlink_notifier_create(name_table_change, NULL);
> >> }
> >
> > That doesn't seem right. I could not reproduce the valgrind problem by
> > running
> > TESTSUITEFLAGS=2050 make check-valgrind.
> >
> > But this has several problems. Fist, it's not clear code. Second, if
> > name_notifier was not NULL, it could release memory still in use, and cause
> > other potential bugs and leaks as well.
> >
> > Third: it's not even necessary. This looks much more like a false positive
> > from
> > valgrind. Unless we are calling name_table_init twice, which I can't see
> > how.
> > Can you look into the real bug here, maybe WARN whenever name_table_init
> > when
> > name_notifier is not NULL?
> >
> > If this is really a false positive and you really want to get rid of it, you
> > could just do:
> >
> > static void
> > name_table_init(void)
> > {
> > - name_notifier = rtnetlink_notifier_create(name_table_change, NULL);
> > + if (name_notifier == NULL) {
> > + name_notifier = rtnetlink_notifier_create(name_table_change,
> > NULL);
> > + }
> > }
> >
> > Cascardo.
> >
> >>
> >> --
> >> 2.5.0
> >>
> >> _______________________________________________
> >> dev mailing list
> >> [email protected]
> >> http://openvswitch.org/mailman/listinfo/dev
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
