| Message ID | 20210411213607.9451-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 05b80226010bef5d296ed8331a0582a6e045a4ae |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 4/12/21 3:06 AM, Laurent Pinchart wrote: > The last instance of Thread allocated in the test is never deleted, not I think you meant s/not/nor > are other instances deleted in error paths. Use a std::unique_ptr<> to > ensure deletion. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > test/threads.cpp | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/test/threads.cpp b/test/threads.cpp > index b4b8d913cd2b..e0c50dc90a65 100644 > --- a/test/threads.cpp > +++ b/test/threads.cpp > @@ -7,6 +7,7 @@ > > #include <chrono> > #include <iostream> > +#include <memory> > #include <thread> > > #include "libcamera/internal/thread.h" > @@ -45,23 +46,23 @@ protected: > int run() > { > /* Test Thread() retrieval for the main thread. */ > - Thread *thread = Thread::current(); > - if (!thread) { > + Thread *mainThread = Thread::current(); > + if (!mainThread) { > cout << "Thread::current() failed to main thread" > << endl; > return TestFail; > } > > - if (!thread->isRunning()) { > + if (!mainThread->isRunning()) { > cout << "Main thread is not running" << endl; > return TestFail; > } > > /* Test starting the main thread, the test shall not crash. */ > - thread->start(); > + mainThread->start(); > > /* Test the running state of a custom thread. */ > - thread = new Thread(); > + std::unique_ptr<Thread> thread = std::make_unique<Thread>(); > thread->start(); > > if (!thread->isRunning()) { > @@ -79,10 +80,8 @@ protected: > return TestFail; > } > > - delete thread; > - > /* Test waiting for completion with a timeout. */ > - thread = new DelayThread(chrono::milliseconds(500)); > + thread = std::make_unique<DelayThread>(chrono::milliseconds(500)); > thread->start(); > thread->exit(0); > > @@ -100,10 +99,8 @@ protected: > return TestFail; > } > > - delete thread; > - > /* Test waiting on a thread that isn't running. */ > - thread = new Thread(); > + thread = std::make_unique<Thread>(); > > timeout = !thread->wait(); > if (timeout) {
Hey Laurent, Thank you for the patch. On 12.04.2021 00:36, Laurent Pinchart wrote: >The last instance of Thread allocated in the test is never deleted, not s/not/nor/ ? >are other instances deleted in error paths. Use a std::unique_ptr<> to >ensure deletion. I tested this fix and valgrind does not report any leaked memory from the libcamera tests. But just out of curiosity do you get a similar report in valgrind: ``` basti@nanopct4:~/libcamera$ valgrind --leak-check=full --show-leak-kinds=all ninja -C build test ... libcamera 0.0.0 Configuration Enabled pipelines : ipu3 raspberrypi rkisp1 simple uvcvideo vimc Android support : False GStreamer support : False V4L2 emulation support: False cam application : True qcam application : False Unit tests : True Option werror is: true [default: true] ... ==6151== LEAK SUMMARY: ==6151== definitely lost: 296 bytes in 1 blocks ==6151== indirectly lost: 576 bytes in 2 blocks ==6151== possibly lost: 0 bytes in 0 blocks ==6151== still reachable: 2,725,231 bytes in 15,220 blocks ==6151== suppressed: 0 bytes in 0 blocks ``` Those are all related to our build tool ninja. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Tested-by: Sebastian Fricke <sebastian.fricke@posteo.net> Greetings, Sebastian >--- > test/threads.cpp | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > >diff --git a/test/threads.cpp b/test/threads.cpp >index b4b8d913cd2b..e0c50dc90a65 100644 >--- a/test/threads.cpp >+++ b/test/threads.cpp >@@ -7,6 +7,7 @@ > > #include <chrono> > #include <iostream> >+#include <memory> > #include <thread> > > #include "libcamera/internal/thread.h" >@@ -45,23 +46,23 @@ protected: > int run() > { > /* Test Thread() retrieval for the main thread. */ >- Thread *thread = Thread::current(); >- if (!thread) { >+ Thread *mainThread = Thread::current(); >+ if (!mainThread) { > cout << "Thread::current() failed to main thread" > << endl; > return TestFail; > } > >- if (!thread->isRunning()) { >+ if (!mainThread->isRunning()) { > cout << "Main thread is not running" << endl; > return TestFail; > } > > /* Test starting the main thread, the test shall not crash. */ >- thread->start(); >+ mainThread->start(); > > /* Test the running state of a custom thread. */ >- thread = new Thread(); >+ std::unique_ptr<Thread> thread = std::make_unique<Thread>(); > thread->start(); > > if (!thread->isRunning()) { >@@ -79,10 +80,8 @@ protected: > return TestFail; > } > >- delete thread; >- > /* Test waiting for completion with a timeout. */ >- thread = new DelayThread(chrono::milliseconds(500)); >+ thread = std::make_unique<DelayThread>(chrono::milliseconds(500)); > thread->start(); > thread->exit(0); > >@@ -100,10 +99,8 @@ protected: > return TestFail; > } > >- delete thread; >- > /* Test waiting on a thread that isn't running. */ >- thread = new Thread(); >+ thread = std::make_unique<Thread>(); > > timeout = !thread->wait(); > if (timeout) { >-- >Regards, > >Laurent Pinchart > >_______________________________________________ >libcamera-devel mailing list >libcamera-devel@lists.libcamera.org >https://lists.libcamera.org/listinfo/libcamera-devel
Hi Sebastian, On Mon, Apr 12, 2021 at 06:21:36AM +0200, Sebastian Fricke wrote: > Hey Laurent, > > Thank you for the patch. > > On 12.04.2021 00:36, Laurent Pinchart wrote: > > The last instance of Thread allocated in the test is never deleted, not > > s/not/nor/ ? Oops, indeed. > > are other instances deleted in error paths. Use a std::unique_ptr<> to > > ensure deletion. > > I tested this fix and valgrind does not report any leaked memory from > the libcamera tests. But just out of curiosity do you get a similar > report in valgrind: > ``` > basti@nanopct4:~/libcamera$ valgrind --leak-check=full --show-leak-kinds=all ninja -C build test > ... > libcamera 0.0.0 > > Configuration > Enabled pipelines : ipu3 > raspberrypi > rkisp1 > simple > uvcvideo > vimc > Android support : False > GStreamer support : False > V4L2 emulation support: False > cam application : True > qcam application : False > Unit tests : True > > Option werror is: true [default: true] > ... > ==6151== LEAK SUMMARY: > ==6151== definitely lost: 296 bytes in 1 blocks > ==6151== indirectly lost: 576 bytes in 2 blocks > ==6151== possibly lost: 0 bytes in 0 blocks > ==6151== still reachable: 2,725,231 bytes in 15,220 blocks > ==6151== suppressed: 0 bytes in 0 blocks > ``` > > Those are all related to our build tool ninja. That's checking for leaks in ninja itself, it's not ours. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Tested-by: Sebastian Fricke <sebastian.fricke@posteo.net> > > > --- > > test/threads.cpp | 19 ++++++++----------- > > 1 file changed, 8 insertions(+), 11 deletions(-) > > > > diff --git a/test/threads.cpp b/test/threads.cpp > > index b4b8d913cd2b..e0c50dc90a65 100644 > > --- a/test/threads.cpp > > +++ b/test/threads.cpp > > @@ -7,6 +7,7 @@ > > > > #include <chrono> > > #include <iostream> > > +#include <memory> > > #include <thread> > > > > #include "libcamera/internal/thread.h" > > @@ -45,23 +46,23 @@ protected: > > int run() > > { > > /* Test Thread() retrieval for the main thread. */ > > - Thread *thread = Thread::current(); > > - if (!thread) { > > + Thread *mainThread = Thread::current(); > > + if (!mainThread) { > > cout << "Thread::current() failed to main thread" > > << endl; > > return TestFail; > > } > > > > - if (!thread->isRunning()) { > > + if (!mainThread->isRunning()) { > > cout << "Main thread is not running" << endl; > > return TestFail; > > } > > > > /* Test starting the main thread, the test shall not crash. */ > > - thread->start(); > > + mainThread->start(); > > > > /* Test the running state of a custom thread. */ > > - thread = new Thread(); > > + std::unique_ptr<Thread> thread = std::make_unique<Thread>(); > > thread->start(); > > > > if (!thread->isRunning()) { > > @@ -79,10 +80,8 @@ protected: > > return TestFail; > > } > > > > - delete thread; > > - > > /* Test waiting for completion with a timeout. */ > > - thread = new DelayThread(chrono::milliseconds(500)); > > + thread = std::make_unique<DelayThread>(chrono::milliseconds(500)); > > thread->start(); > > thread->exit(0); > > > > @@ -100,10 +99,8 @@ protected: > > return TestFail; > > } > > > > - delete thread; > > - > > /* Test waiting on a thread that isn't running. */ > > - thread = new Thread(); > > + thread = std::make_unique<Thread>(); > > > > timeout = !thread->wait(); > > if (timeout) {
diff --git a/test/threads.cpp b/test/threads.cpp index b4b8d913cd2b..e0c50dc90a65 100644 --- a/test/threads.cpp +++ b/test/threads.cpp @@ -7,6 +7,7 @@ #include <chrono> #include <iostream> +#include <memory> #include <thread> #include "libcamera/internal/thread.h" @@ -45,23 +46,23 @@ protected: int run() { /* Test Thread() retrieval for the main thread. */ - Thread *thread = Thread::current(); - if (!thread) { + Thread *mainThread = Thread::current(); + if (!mainThread) { cout << "Thread::current() failed to main thread" << endl; return TestFail; } - if (!thread->isRunning()) { + if (!mainThread->isRunning()) { cout << "Main thread is not running" << endl; return TestFail; } /* Test starting the main thread, the test shall not crash. */ - thread->start(); + mainThread->start(); /* Test the running state of a custom thread. */ - thread = new Thread(); + std::unique_ptr<Thread> thread = std::make_unique<Thread>(); thread->start(); if (!thread->isRunning()) { @@ -79,10 +80,8 @@ protected: return TestFail; } - delete thread; - /* Test waiting for completion with a timeout. */ - thread = new DelayThread(chrono::milliseconds(500)); + thread = std::make_unique<DelayThread>(chrono::milliseconds(500)); thread->start(); thread->exit(0); @@ -100,10 +99,8 @@ protected: return TestFail; } - delete thread; - /* Test waiting on a thread that isn't running. */ - thread = new Thread(); + thread = std::make_unique<Thread>(); timeout = !thread->wait(); if (timeout) {
The last instance of Thread allocated in the test is never deleted, not are other instances deleted in error paths. Use a std::unique_ptr<> to ensure deletion. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- test/threads.cpp | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)