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