| Message ID | 20200414070642.22366-4-email@uajain.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Tue, Apr 14, 2020 at 07:06:59AM +0000, Umang Jain wrote: > Failing the test guards against negative index (-ENOENT in this case) > being passed to V4L2BufferCache::put(). Can this happens in practice ? > Pointed out by Coverity DefectId=279090 > > Signed-off-by: Umang Jain <email@uajain.com> > --- > test/v4l2_videodevice/buffer_cache.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp > index d730e75..1870249 100644 > --- a/test/v4l2_videodevice/buffer_cache.cpp > +++ b/test/v4l2_videodevice/buffer_cache.cpp > @@ -90,6 +90,11 @@ public: > /* Pick a hot buffer at random and store its index. */ > int hotBuffer = dist(generator_); > int hotIndex = cache->get(*buffers[hotBuffer].get()); > + if (hotIndex < 0) { > + std::cout << "Failed lookup from cache" << std::endl; > + return TestFail; > + } > + > cache->put(hotIndex); > > /*
Hi Laurent, On Wed, Apr 15, 2020 at 00:56, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Umang, > > Thank you for the patch. > > On Tue, Apr 14, 2020 at 07:06:59AM +0000, Umang Jain wrote: >> Failing the test guards against negative index (-ENOENT in this >> case) >> being passed to V4L2BufferCache::put(). > > Can this happens in practice ? Well, V4L2BufferCache::put() accepts only unsigned int argument, so I would say, it won't happen in practice. Still the patches seems correct to me but commit message should be tweaked, is that right?
Hi Laurent, Umang On 15/04/2020 04:09, Umang Jain wrote: > > Hi Laurent, > > On Wed, Apr 15, 2020 at 00:56, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> Hi Umang, Thank you for the patch. On Tue, Apr 14, 2020 at 07:06:59AM >> +0000, Umang Jain wrote: >> >> Failing the test guards against negative index (-ENOENT in this >> case) being passed to V4L2BufferCache::put(). >> >> Can this happens in practice ? This is code which /tests/ library code. Testing things that shouldn't necessarily happen in practice is part of that right? So that when someone changes V4L2BufferCache and breaks something - the tests should fire a failure? In other words, shouldn't test be written defensively? I don't think this can happen in practice with the *current* implementation, but that's the point - the implementation could change. If we didn't ever expect to change implementations - then we don't need to keep any tests around at all :-) > Well, V4L2BufferCache::put() accepts only unsigned int argument, so I > would say, it won't happen in practice. > Still the patches seems correct to me but commit message should be > tweaked, is that right? So in this instance, the ASSERT() will fire, and bail out on V4L2BufferCache::put() ... but the unexpected fault (which should have failed the test) is in the get() ... ? My point is - I would add this check in .. Laurent - are you suggesting we shouldn't? > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Umang, On Wed, Apr 15, 2020 at 03:09:57AM +0000, Umang Jain wrote: > On Wed, Apr 15, 2020 at 00:56, Laurent Pinchart wrote: > > On Tue, Apr 14, 2020 at 07:06:59AM +0000, Umang Jain wrote: > >> Failing the test guards against negative index (-ENOENT in this case) > >> being passed to V4L2BufferCache::put(). > > > > Can this happens in practice ? > > Well, V4L2BufferCache::put() accepts only unsigned int argument, so I > would say, it won't happen in practice. > Still the patches seems correct to me but commit message should be > tweaked, is that right? What I mean is, given how we use V4L2BufferCache in this test, can get() return a negative value here ?
Hi Kieran, On Wed, Apr 15, 2020 at 01:56:05PM +0100, Kieran Bingham wrote: > On 15/04/2020 04:09, Umang Jain wrote: > > On Wed, Apr 15, 2020 at 00:56, Laurent Pinchart wrote: > >> Hi Umang, Thank you for the patch. On Tue, Apr 14, 2020 at 07:06:59AM > >> +0000, Umang Jain wrote: > >> > >> Failing the test guards against negative index (-ENOENT in this > >> case) being passed to V4L2BufferCache::put(). > >> > >> Can this happens in practice ? > > This is code which /tests/ library code. > > Testing things that shouldn't necessarily happen in practice is part of > that right? So that when someone changes V4L2BufferCache and breaks > something - the tests should fire a failure? But is this particular test testing that, or is it already caught by a different test ? Each of the test cases cover a part of the API, I was wondering if for this particular test such a check was needed. The answer may be yes, I haven't read the code. > In other words, shouldn't test be written defensively? > > I don't think this can happen in practice with the *current* > implementation, but that's the point - the implementation could change. > > If we didn't ever expect to change implementations - then we don't need > to keep any tests around at all :-) > > > Well, V4L2BufferCache::put() accepts only unsigned int argument, so I > > would say, it won't happen in practice. > > Still the patches seems correct to me but commit message should be > > tweaked, is that right? > > So in this instance, the ASSERT() will fire, and bail out on > V4L2BufferCache::put() ... but the unexpected fault (which should have > failed the test) is in the get() ... ? > > My point is - I would add this check in .. > Laurent - are you suggesting we shouldn't?
diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp index d730e75..1870249 100644 --- a/test/v4l2_videodevice/buffer_cache.cpp +++ b/test/v4l2_videodevice/buffer_cache.cpp @@ -90,6 +90,11 @@ public: /* Pick a hot buffer at random and store its index. */ int hotBuffer = dist(generator_); int hotIndex = cache->get(*buffers[hotBuffer].get()); + if (hotIndex < 0) { + std::cout << "Failed lookup from cache" << std::endl; + return TestFail; + } + cache->put(hotIndex); /*
Failing the test guards against negative index (-ENOENT in this case) being passed to V4L2BufferCache::put(). Pointed out by Coverity DefectId=279090 Signed-off-by: Umang Jain <email@uajain.com> --- test/v4l2_videodevice/buffer_cache.cpp | 5 +++++ 1 file changed, 5 insertions(+)