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

[Kieran Bingham]

Hi Laurent,

On 03/03/2020 17:48, Laurent Pinchart wrote:
> 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.

Well that will then become:

ASSERT_ABI_OFFSET(s, m, o)

I still think macroising will simplify things.


-- 
Regards
--
Kieran