[libcamera-devel] [PATCH] test: v4l2_device: Provide compile time assertions

[Kieran Bingham]

Hi Laurent,

On 21/01/2019 12:15, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Mon, Jan 21, 2019 at 10:28:26AM +0000, Kieran Bingham wrote:
>> The V4L2Capability structure is inherited from struct v4l2_capability only
>> to provide an interface. It must not extend the type or data.
>>
>> Enforce this with a static assertion with sizeof() comparisons.
>>
>> There is no need here for a specific test binary which will always return
>> TEST_PASS when compiled, as this test failure will be caught at compile time.
>> In light of this - the static compile time assertion is added to the
>> V4L2DeviceTest base class.
>>
>> Should there be a large number of static assertions required, they could be
>> moved to their own unit for clarity.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  test/v4l2_device/v4l2_device_test.cpp | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
>> index 362553712caa..97876f8d65db 100644
>> --- a/test/v4l2_device/v4l2_device_test.cpp
>> +++ b/test/v4l2_device/v4l2_device_test.cpp
>> @@ -5,6 +5,7 @@
>>   * libcamera V4L2 API tests
>>   */
>>  
>> +#include <assert.h>
>>  #include <iostream>
>>  #include <sys/stat.h>
>>  
>> @@ -41,3 +42,8 @@ void V4L2DeviceTest::cleanup()
>>  {
>>  	delete dev_;
>>  };
>> +
>> +/* Static compile time assertion tests */
>> +
>> +static_assert(sizeof(struct v4l2_capability) == sizeof(struct V4L2Capability),
>> +	      "V4L2Capability must match v4l2_capability size");
> 
> I'm still not sure if we really need this, as this is more a check of
> our C++ understanding than of our implementation :-) If you insist, I
> prefer this check to be in a test case than in libcamera where it
> originally was, so I won't push back too hard. The error message may
> need some rewording though, as it sounds like it's something we can fix.

It's not a check of our understanding - it's a contract written in the
code - that the type V4L2Capability is used directly as a
v4l2_capability and as such must remain compatible.


If this assertion fires, then it should be something to be fixed (or
that developer takes responsibility for removing this check and all
guarantees I make are removed)

I'm not expecting that the compiler might break the sizes arbitrarily -
I'm ensuring that if a developer comes along and changes the size of the
derived structure (s)he will get a warning, and be told off politely for
changing the size of the V4L2Capability.


> By the way, do we really require the two to be equal ? V4L2Capability
> could include more members (I don't see a real use case for that now,
> but that's imaginable), in which case it would be laid ou with
> v4l2_capability first followed by other members, and still work fine.

Yes, I agree - and originally I had planned for that to be possible.
for instance - to cache the appropriate capabilities variable (i.e. if
the device capabilities are to be referenced instead)

I plan to add a function which I believe will be inlined to handle that
instead, but it states the point.

However, if this happens - it could get messy - if the base class (the
kernel structure) members are overridden by the derived class people
might not get the behaviour they expect. (ioctl writes to the base class
struct, user reads from derived override)

Anyway - the point of this assert it to state the intentions of the
definition, and provide a warning if anyone breaks that assumption - not
to assert that two objects which we know are currently the same size -
"are the same size".

-- 
Regards
--
Kieran