| Message ID | 20190121102826.16873-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Nice idea for a test :-) On 2019-01-21 10:28:26 +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@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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"); > -- > 2.17.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On 21/01/2019 10:36, Niklas Söderlund wrote: > Hi Kieran, > > Nice idea for a test :-) > > On 2019-01-21 10:28:26 +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@ideasonboard.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Thanks, I hope this alleviates some of your earlier concerns about the method of implementing the interface to interact kernel structures ... It's to define what functions can operate on a structure - certainly not to to extend that structure in any way. -- Kieran > >> --- >> 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"); >> -- >> 2.17.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
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@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. 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.
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@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".
Hi Kieran, On Mon, Jan 21, 2019 at 12:39:53PM +0000, Kieran Bingham wrote: > On 21/01/2019 12:15, Laurent Pinchart wrote: > > 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@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. But that's guaranteed by C++ (isn't it ?), and we don't have tests for all contracts from the C++ standard :-) > 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) I don't see how that could happen, as C++ should guarantee that the object will be layed out in memory with the base class first (again, doesn't it ?). > 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". My question was if we should test if the compiler breaks assumptions that come from the C++ standard :-)
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");
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@ideasonboard.com> --- test/v4l2_device/v4l2_device_test.cpp | 6 ++++++ 1 file changed, 6 insertions(+)