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

[Kieran Bingham]

On 05/03/2020 10:37, Laurent Pinchart wrote:
> 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 ?

Depends on whether this series gets respun or not. I don't think there's
much major coming out of the review, but there are a few small things.

It can change on top if you prefer, and indeed the _OFFSET could be on
top anyway, unless you want to trap all the current offsets now.

I wonder if there's a way to automate this too ... (which can certainly
be later).


-- 
Regards
--
Kieran