<div dir="ltr"><div>Please see the attachment for the updated patch.</div><div>Indeed we have seen other memory leaks in unit-test, </div><div><a href="https://code.google.com/p/chromium/issues/detail?id=470252">https://code.google.com/p/chromium/issues/detail?id=470252<br></a></div><div>could you please take a look at it if possible?</div><div>Thanks.</div><div><br></div><div>---</div><div> plugins/huawei/tests/test-modem-helpers-huawei.c | 23 +++++++++++++++--------</div><div> 1 file changed, 15 insertions(+), 8 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..4b73bb3 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><div>@@ -1122,14 +1127,16 @@ test_time (void)</div><div> &iso8601,</div><div> NULL,</div><div> &error);</div><div>-</div><div>- g_assert (ret == time_tests[i].ret);</div><div>- g_assert (ret == (error ? FALSE : TRUE));</div><div>+ if (ret)</div><div>+ g_assert_no_error (error);</div><div>+ else {</div><div>+ g_assert (ret == time_tests[i].ret);</div><div>+ g_assert (ret == (error ? FALSE : TRUE));</div><div>+ g_error_free (error);</div><div>+ }</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>--</div><div>2.2.0.rc0.207.ga3a616c</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 25, 2015 at 10:10 AM, 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">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>
<span class="HOEnZb"><font color="#888888">> --<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>
</font></span></blockquote></div><br></div>