[PATCH] huawei plugin: fix memory leaks in tests.

[Yunlian Jiang]

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?
Thanks.

---
 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);
+        }

         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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/modemmanager-devel/attachments/20150325/4145de58/attachment.html>