[PATCH] huawei plugin: fix memory leaks in tests.
Aleksander Morgado
aleksander at aleksander.es
Fri Mar 27 12:32:12 PDT 2015
On Wed, Mar 25, 2015 at 9:51 PM, Yunlian Jiang <yunlian at chromium.org> wrote:
> Please see the updated patch.
> Thanks!
> ---
In git master now, thanks.
> plugins/huawei/tests/test-modem-helpers-huawei.c | 14 +++++++++-----
> plugins/icera/tests/test-modem-helpers-icera.c | 2 ++
> plugins/mbm/tests/test-modem-helpers-mbm.c | 2 ++
> 3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/plugins/huawei/tests/test-modem-helpers-huawei.c
> b/plugins/huawei/tests/test-modem-helpers-huawei.c
> index 9e92eee..770e8e8 100644
> --- a/plugins/huawei/tests/test-modem-helpers-huawei.c
> +++ b/plugins/huawei/tests/test-modem-helpers-huawei.c
> @@ -420,6 +420,7 @@ test_prefmode_response (void)
> found = mm_huawei_parse_prefmode_response
> (prefmode_response_tests[i].str,
> combinations,
> &error);
> + g_assert_no_error (error);
> g_assert (found != NULL);
> g_assert_cmpuint (found->allowed, ==,
> prefmode_response_tests[i].allowed);
> g_assert_cmpuint (found->preferred, ==,
> prefmode_response_tests[i].preferred);
> @@ -672,6 +673,7 @@ test_syscfg_response (void)
> combinations,
> &error);
>
> + g_assert_no_error (error);
> g_assert (found != NULL);
> g_assert_cmpuint (found->allowed, ==,
> syscfg_response_tests[i].allowed);
> g_assert_cmpuint (found->preferred, ==,
> syscfg_response_tests[i].preferred);
> @@ -993,6 +995,7 @@ test_syscfgex_response (void)
> combinations,
> &error);
>
> + g_assert_no_error (error);
> g_assert (found != NULL);
> g_assert_cmpuint (found->allowed, ==,
> syscfgex_response_tests[i].allowed);
> g_assert_cmpuint (found->preferred, ==,
> syscfgex_response_tests[i].preferred);
> @@ -1085,8 +1088,10 @@ test_nwtime (void)
> g_assert (nwtime_tests[i].leap_seconds ==
> mm_network_timezone_get_leap_seconds (tz));
> }
>
> - if (iso8601)
> - g_free (iso8601);
> + g_free (iso8601);
> +
> + if (tz)
> + g_object_unref (tz);
> }
> }
>
> @@ -1125,11 +1130,10 @@ test_time (void)
>
> g_assert (ret == time_tests[i].ret);
> g_assert (ret == (error ? FALSE : TRUE));
> + g_clear_error (&error);
>
> g_assert_cmpstr (time_tests[i].iso8601, ==, iso8601);
> -
> - if (iso8601)
> - g_free (iso8601);
> + g_free (iso8601);
> }
> }
>
> diff --git a/plugins/icera/tests/test-modem-helpers-icera.c
> b/plugins/icera/tests/test-modem-helpers-icera.c
> index e7d1645..2e7b26f 100644
> --- a/plugins/icera/tests/test-modem-helpers-icera.c
> +++ b/plugins/icera/tests/test-modem-helpers-icera.c
> @@ -141,6 +141,7 @@ test_ipdpaddr (void)
> g_assert_cmpint (dnslen, ==, 1);
> g_assert_cmpstr (dns[0], ==, ipdpaddr_tests[i].ipv4_dns1);
> g_assert_cmpstr (dns[1], ==, ipdpaddr_tests[i].ipv4_dns2);
> + g_object_unref (ipv4);
> } else
> g_assert (ipv4 == NULL);
>
> @@ -166,6 +167,7 @@ test_ipdpaddr (void)
> dnslen = g_strv_length ((gchar **) dns);
> g_assert_cmpint (dnslen, ==, 1);
> g_assert_cmpstr (dns[0], ==, ipdpaddr_tests[i].ipv6_dns1);
> + g_object_unref (ipv6);
> } else
> g_assert (ipv6 == NULL);
> }
> diff --git a/plugins/mbm/tests/test-modem-helpers-mbm.c
> b/plugins/mbm/tests/test-modem-helpers-mbm.c
> index 2e6dd1a..0c48894 100644
> --- a/plugins/mbm/tests/test-modem-helpers-mbm.c
> +++ b/plugins/mbm/tests/test-modem-helpers-mbm.c
> @@ -96,6 +96,7 @@ test_e2ipcfg (void)
> g_assert_cmpint (dnslen, ==, 1);
> g_assert_cmpstr (dns[0], ==, tests[i].ipv4_dns1);
> g_assert_cmpstr (dns[1], ==, tests[i].ipv4_dns2);
> + g_object_unref (ipv4);
> } else
> g_assert (ipv4 == NULL);
>
> @@ -122,6 +123,7 @@ test_e2ipcfg (void)
> g_assert_cmpint (dnslen, ==, 1);
> g_assert_cmpstr (dns[0], ==, tests[i].ipv6_dns1);
> g_assert_cmpstr (dns[1], ==, tests[i].ipv6_dns2);
> + g_object_unref (ipv6);
> } else
> g_assert (ipv6 == NULL);
> }
> --
> 2.2.0.rc0.207.ga3a616c
>
>
> On Wed, Mar 25, 2015 at 12:12 PM, Aleksander Morgado
> <aleksander at aleksander.es> wrote:
>>
>> On Wed, Mar 25, 2015 at 6:43 PM, Yunlian Jiang <yunlian at chromium.org>
>> wrote:
>> >
>> > Please see the attachment for the updated patch.
>> > Indeed we have seen other memory leaks in unit-test,
>> > https://code.google.com/p/chromium/issues/detail?id=470252
>> > could you please take a look at it if possible?
>>
>> In plugins/icera/test/test-modem-helpers-icera.c, test_ipdpaddr()
>> method, when mm_icera_parse_ipdpaddr_response() is called and it
>> returns either 'ipv4' or 'ipv6' variables, those don't get properly
>> released. Change would be:
>>
>> /* IPv4 */
>> if (ipdpaddr_tests[i].ipv4_addr) {
>> ...
>> g_object_unref (ipv4);
>> }
>>
>> And same for ipv6.
>>
>> Could you update that and re-check the memleak tests?
>>
>> >
>> > ---
>> > plugins/huawei/tests/test-modem-helpers-huawei.c | 23
>> > +++++++++++++++--------
>> > 1 file changed, 15 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/plugins/huawei/tests/test-modem-helpers-huawei.c
>> > b/plugins/huawei/tests/test-modem-helpers-huawei.c
>> > index 9e92eee..4b73bb3 100644
>> > --- a/plugins/huawei/tests/test-modem-helpers-huawei.c
>> > +++ b/plugins/huawei/tests/test-modem-helpers-huawei.c
>> > @@ -420,6 +420,7 @@ test_prefmode_response (void)
>> > found = mm_huawei_parse_prefmode_response
>> > (prefmode_response_tests[i].str,
>> > combinations,
>> > &error);
>> > + g_assert_no_error (error);
>> > g_assert (found != NULL);
>> > g_assert_cmpuint (found->allowed, ==,
>> > prefmode_response_tests[i].allowed);
>> > g_assert_cmpuint (found->preferred, ==,
>> > prefmode_response_tests[i].preferred);
>> > @@ -672,6 +673,7 @@ test_syscfg_response (void)
>> > combinations,
>> > &error);
>> >
>> > + g_assert_no_error (error);
>> > g_assert (found != NULL);
>> > g_assert_cmpuint (found->allowed, ==,
>> > syscfg_response_tests[i].allowed);
>> > g_assert_cmpuint (found->preferred, ==,
>> > syscfg_response_tests[i].preferred);
>> > @@ -993,6 +995,7 @@ test_syscfgex_response (void)
>> > combinations,
>> > &error);
>> >
>> > + g_assert_no_error (error);
>> > g_assert (found != NULL);
>> > g_assert_cmpuint (found->allowed, ==,
>> > syscfgex_response_tests[i].allowed);
>> > g_assert_cmpuint (found->preferred, ==,
>> > syscfgex_response_tests[i].preferred);
>> > @@ -1085,8 +1088,10 @@ test_nwtime (void)
>> > g_assert (nwtime_tests[i].leap_seconds ==
>> > mm_network_timezone_get_leap_seconds (tz));
>> > }
>> >
>> > - if (iso8601)
>> > - g_free (iso8601);
>> > + g_free (iso8601);
>> > +
>> > + if (tz)
>> > + g_object_unref (tz);
>> > }
>> > }
>> >
>> > @@ -1122,14 +1127,16 @@ test_time (void)
>> > &iso8601,
>> > NULL,
>> > &error);
>> > -
>> > - g_assert (ret == time_tests[i].ret);
>> > - g_assert (ret == (error ? FALSE : TRUE));
>> > + if (ret)
>> > + g_assert_no_error (error);
>> > + else {
>> > + g_assert (ret == time_tests[i].ret);
>> > + g_assert (ret == (error ? FALSE : TRUE));
>> > + g_error_free (error);
>> > + }
>> >
>>
>> I was wrong; error vs ret was being checked in the following line:
>> g_assert (ret == (error ? FALSE : TRUE));
>>
>> So there's no need for the extra checks, just the memleak should get
>> fixed:
>> if (error)
>> g_error_free (error);
>>
>> Could you update the patch?
>>
>>
>> > g_assert_cmpstr (time_tests[i].iso8601, ==, iso8601);
>> > -
>> > - if (iso8601)
>> > - g_free (iso8601);
>> > + g_free (iso8601);
>> > }
>> > }
>> >
>> > --
>> > 2.2.0.rc0.207.ga3a616c
>> >
>> > On Wed, Mar 25, 2015 at 10:10 AM, Aleksander Morgado
>> > <aleksander at aleksander.es> wrote:
>> >>
>> >> On Wed, Mar 25, 2015 at 5:30 PM, Yunlian Jiang <yunlian at chromium.org>
>> >> wrote:
>> >> > ---
>> >> > plugins/huawei/tests/test-modem-helpers-huawei.c | 5 +++++
>> >> > 1 file changed, 5 insertions(+)
>> >> >
>> >> > diff --git a/plugins/huawei/tests/test-modem-helpers-huawei.c
>> >> > b/plugins/huawei/tests/test-modem-helpers-huawei.c
>> >> > index 9e92eee..ef1118a 100644
>> >> > --- a/plugins/huawei/tests/test-modem-helpers-huawei.c
>> >> > +++ b/plugins/huawei/tests/test-modem-helpers-huawei.c
>> >> > @@ -1087,6 +1087,9 @@ test_nwtime (void)
>> >> >
>> >> > if (iso8601)
>> >> > g_free (iso8601);
>> >>
>> >> I know this is not part of your patch, but there's no need to check
>> >> for NULL before doing g_free(). I'll gladly accept a patch revising
>> >> this whole file and changing those.
>> >>
>> >> > +
>> >> > + if (tz)
>> >> > + g_free (tz);
>> >>
>> >> tz is actually a MMNetworkTimezone, which is a GObject. The proper fix
>> >> would be:
>> >> if (tz)
>> >> g_object_unref (tz);
>> >>
>> >> > }
>> >> > }
>> >> >
>> >> > @@ -1128,6 +1131,8 @@ test_time (void)
>> >> >
>> >> > g_assert_cmpstr (time_tests[i].iso8601, ==, iso8601);
>> >> >
>> >> > + g_clear_error (&error);
>> >> > +
>> >>
>> >> Looks like the test never checks if error is set.
>> >>
>> >> I'd update it so that:
>> >>
>> >> if (ret)
>> >> g_assert_no_error (error);
>> >> else {
>> >> g_assert_error (error, ....);
>> >> g_error_free (error);
>> >> }
>> >>
>> >> Could you also review the remaining tests to check if error is being
>> >> checked or not?
>> >>
>> >>
>> >> > if (iso8601)
>> >> > g_free (iso8601);
>> >>
>> >> Same as above, this wasn't part of your patch, but I would gladly
>> >> accept a patch changing that to just do a g_free().
>> >>
>> >> > }
>> >> > --
>> >> > 2.2.0.rc0.207.ga3a616c
>> >> >
>> >> >
>> >> > _______________________________________________
>> >> > ModemManager-devel mailing list
>> >> > ModemManager-devel at lists.freedesktop.org
>> >> > http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
>> >> >
>> >>
>> >>
>> >>
>> >> --
>> >> Aleksander
>> >> https://aleksander.es
>> >
>> >
>>
>>
>>
>> --
>> Aleksander
>> https://aleksander.es
>
>
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list