GNOME Bugzilla – Bug 783418
audioconvert: Fix memory leak in test code failure path
Last modified: 2018-01-19 18:52:54 UTC
As I know, gst_structure_to_string function returned allocated memory. So, It should be free using g_free function.
Created attachment 353163 [details] [review] Fix memory leak in test code
Created attachment 353167 [details] [review] Fix memory leak in test code
In the error/failure case everything else will be leaked as well, since g_error() aborts. If you found this with a static code analzyer you should probably tell it that g_error() never returns.
thanks for your comment. that's my mistake. I didn't use static code analyzer. I just use grep command for "FIXME" or "TODO". As you mentioned g_error(), I modify GST_DEBUG instead it.
Created attachment 353174 [details] [review] Fix memory leak in test code
Comment on attachment 353174 [details] [review] Fix memory leak in test code I am not sure if this makes sense. The g_error() is supposed to make the unit test fail. A GST_DEBUG would no longer make the unit test fail in this case. You would need to explain why you think the test should not fail in this case.
Created attachment 353178 [details] [review] Fix memory leak in test code Actually, when I try to fix some code, I refer to test code in the tests folder each plugin. In this case, I just grepping gst_stucture_to_string in plugins-base. But in the audioconvert test code, usage of gst_structure_to_string is different. I know that g_error aborts, but IMHO we need to modify like another example code for consistency for gstreamer newbie like me.
Created attachment 353179 [details] [review] Fix memory leak in test code
Ok, I have pushed another solution (in your name, hope you don't mind) that is simpler: commit 4876fe04f589189db697b5a2e669901a9eb614ee Author: Jimmy Ohn <yongjin.ohn@lge.com> Date: Mon Jun 5 14:08:33 2017 +0900 tests: audioconvert: Fix memory leak in failure path Don't set a bad example by leaking things, even if calling g_error() will make the process abort. https://bugzilla.gnome.org/show_bug.cgi?id=783418 But in general these are unit tests and we are not going to clean up stuff when they fail/abort, so this commit is more of an exception really :)