| Message ID | 20200229164254.23604-7-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Some suggestions but otherwise... Reviewed-by: Kieran Bingham <kieran.bingham@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);
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@ideasonboard.com> > > Some suggestions but otherwise... > > Reviewed-by: Kieran Bingham <kieran.bingham@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.
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@ideasonboard.com> >> >> Some suggestions but otherwise... >> >> Reviewed-by: Kieran Bingham <kieran.bingham@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.
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@ideasonboard.com> > >> > >> Some suggestions but otherwise... > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@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 ?
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@ideasonboard.com> >>>> >>>> Some suggestions but otherwise... >>>> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@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).
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"); + /** * \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");
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@ideasonboard.com> --- src/libcamera/ipa_controls.cpp | 9 +++++++++ 1 file changed, 9 insertions(+)