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 <casca...@redhat.com> 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 <u9012...@gmail.com> >> --- >> 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 >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
