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

[Laurent Pinchart]

Hi Kieran,

On Thu, Mar 05, 2020 at 09:50:57AM +0000, Kieran Bingham wrote:
> On 03/03/2020 17:48, Laurent Pinchart wrote:
> > 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.
> 
> Well that will then become:
> 
> ASSERT_ABI_OFFSET(s, m, o)
> 
> I still think macroising will simplify things.

I have no objection. As part of this patch, or on top of it ?

-- 
Regards,

Laurent Pinchart