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

[Kieran Bingham]

Hi Laurent,

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);


-- 
Regards
--
Kieran