[PATCH] huawei plugin: fix memory leaks in tests.
Yunlian Jiang
yunlian at chromium.org
Wed Mar 25 13:51:32 PDT 2015
Please see the updated patch.
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/modemmanager-devel/attachments/20150325/88ba6995/attachment-0001.html>
More information about the ModemManager-devel
mailing list