[libcamera-devel] [PATCH 06/31] libcamera: ipa: Test control structure size with static_assert

[Laurent Pinchart]

Hi Kieran,

On Mon, Mar 02, 2020 at 10:49:01PM +0000, Kieran Bingham wrote:
> On 29/02/2020 16:42, Laurent Pinchart wrote:
> > The control-related structures ipa_controls_header,
> > ipa_control_value_entry and ipa_control_range_entry define the IPA
> > protocol and are thus part of the ABI. To avoid breaking it
> > inadvertently, use static_assert() to check the size of the structures.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Some suggestions but otherwise...
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > ---
> >  src/libcamera/ipa_controls.cpp | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp
> > index dd3ff9a0d467..25f38bab3dd9 100644
> > --- a/src/libcamera/ipa_controls.cpp
> > +++ b/src/libcamera/ipa_controls.cpp
> > @@ -153,6 +153,9 @@
> >   * Reserved for future extensions
> >   */
> >  
> > +static_assert(sizeof(ipa_controls_header) == 32,
> > +	      "Invalid size for struct ipa_control_header");
> > +
> 
> I think this could be better worded to be more helpful in the event that
> it is changed:
> 
> "Invalid ABI size change for struct ...."
> 
> >  /**
> >   * \struct ipa_control_value_entry
> >   * \brief Description of a serialized ControlValue entry
> > @@ -167,6 +170,9 @@
> >   * value data (shall be a multiple of 8 bytes).
> >   */
> >  
> > +static_assert(sizeof(ipa_control_value_entry) == 16,
> > +	      "Invalid size for struct ipa_control_value_entry");
> > +
> >  /**
> >   * \struct ipa_control_range_entry
> >   * \brief Description of a serialized ControlRange entry
> > @@ -180,3 +186,6 @@
> >   * \var ipa_control_range_entry::padding
> >   * Padding bytes (shall be set to 0)
> >   */
> > +
> > +static_assert(sizeof(ipa_control_range_entry) == 16,
> > +	      "Invalid size for struct ipa_control_range_entry");
> > 
> 
> 
> If we're going to do lots of validating of structure sizes with a
> repeatable message, perhaps it warrants a macro:
> 
> ASSERT_ABI_SIZE(struct ipa_controls_header, 32);
> ASSERT_ABI_SIZE(struct ipa_control_value_entry, 16);
> ASSERT_ABI_SIZE(struct ipa_control_range_entry, 16);

It's a good idea, but we will also likely need to statically assert that
the offset of indidivual fields doesn't change, so I think more thoughts
are required on how to create such macros.

-- 
Regards,

Laurent Pinchart