Hi Ben,
Thanks, this indeed fixes the "possible" leaks.
Regards,
William
On Fri, Jul 1, 2016 at 8:24 PM, Ben Pfaff <b...@ovn.org> wrote:
> 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
>> <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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
